| Message ID | 20220615124902.17347-1-wangyouwan@uniontech.com |
|---|---|
| State | New |
| Headers | show |
| Series | obexd: Fix crash of transfer pointer has being freed | expand |
This is automated email and please do not reply to this email! Dear submitter, Thank you for submitting the patches to the linux bluetooth mailing list. This is a CI test results with your patch series: PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=650581 ---Test result--- Test Summary: CheckPatch PASS 1.42 seconds GitLint PASS 0.93 seconds Prep - Setup ELL PASS 42.31 seconds Build - Prep PASS 0.67 seconds Build - Configure PASS 8.38 seconds Build - Make PASS 1244.74 seconds Make Check PASS 11.39 seconds Make Check w/Valgrind PASS 463.73 seconds Make Distcheck PASS 236.82 seconds Build w/ext ELL - Configure PASS 9.33 seconds Build w/ext ELL - Make PASS 1265.84 seconds Incremental Build with patchesPASS 0.00 seconds --- Regards, Linux Bluetooth
Hi Youwan, On Wed, Jun 15, 2022 at 5:54 AM Youwan Wang <wangyouwan@uniontech.com> wrote: > > PC1 -> PC2 in the process of transferring the file, > PC2 terminate the reception. > It is found that PC1 will receive the Rcvd > Forbiden message. > transfer_response func is called and go to failed: > transfer_complete func is called and go to transfer_free > > Invalid read of size 8 > at 0x129C9B: put_get_data (gobex-transfer.c:138) > by 0x128675: get_body (gobex-packet.c:397) > by 0x128675: g_obex_packet_encode (gobex-packet.c:451) > by 0x1264ED: write_data (gobex.c:461) > by 0x1264ED: write_data (gobex.c:431) > by 0x49B7E97: g_main_context_dispatch () > by 0x49B8287: ??? () > by 0x49B8581: g_main_loop_run () > by 0x120834: main (main.c:322) > Address 0x7300028 is 56 bytes inside a block of size 64 free'd > at 0x48369AB: free (vg_replace_malloc.c:530) > by 0x12A404: transfer_response (gobex-transfer.c:245) > by 0x126B02: handle_response (gobex.c:1128) > by 0x127087: incoming_data (gobex.c:1373) > by 0x49B7E97: g_main_context_dispatch () > by 0x49B8287: ??? () > by 0x49B8581: g_main_loop_run () > by 0x120834: main (main.c:322) > Block was alloc'd at > at 0x4837B65: calloc (vg_replace_malloc.c:752) > by 0x49BD9D8: g_malloc0 () > by 0x129A49: transfer_new (gobex-transfer.c:258) > by 0x12A612: g_obex_put_req_pkt (gobex-transfer.c:282) > by 0x12A612: g_obex_put_req_pkt (gobex-transfer.c:271) > by 0x145862: transfer_start_put (transfer.c:880) > by 0x145862: obc_transfer_start (transfer.c:911) > by 0x13B487: session_process_transfer (session.c:836) > by 0x13C128: session_process_queue (session.c:887) > by 0x13C128: session_process_queue (session.c:872) > by 0x13C18F: session_process (session.c:820) > by 0x49B7E97: g_main_context_dispatch (in /usr/lib/x86_64-linux-gnu/) > by 0x49B8287: ??? (in /usr/lib/x86_64-linux-gnu/) > by 0x49B8581: g_main_loop_run (in /usr/lib/x86_64-linux-gnu/) > by 0x120834: main (main.c:322) > --- > gobex/gobex-transfer.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/gobex/gobex-transfer.c b/gobex/gobex-transfer.c > index c94d018b2..8e97fc73e 100644 > --- a/gobex/gobex-transfer.c > +++ b/gobex/gobex-transfer.c > @@ -197,6 +197,14 @@ static void transfer_response(GObex *obex, GError *err, GObexPacket *rsp, > } > > rspcode = g_obex_packet_get_operation(rsp, &final); > + if (rspcode == G_OBEX_RSP_FORBIDDEN) { > + err = g_error_new(G_OBEX_ERROR, rspcode, "%s", > + g_obex_strerror(rspcode)); > + g_obex_debug(G_OBEX_DEBUG_ERROR, "%s", err->message); > + g_error_free(err); > + return; > + } This is not quite right, in fact the if statement below already treat such errors, we might be missing something like final: /* Reset if final so it can no longer be cancelled */ if (final_rsp) obex->pending_req = NULL; This would be a problem on the remote implementation which is indicating that perhaps it is just a temporary failure, anyway then: id = transfer->req_id; transfer->req_id = 0; This resets the req_id so on transfer_free the following code is not run: if (transfer->req_id > 0) g_obex_cancel_req(transfer->obex, transfer->req_id, TRUE); So either we have to restore the transfer->req_id = id or not reset it to 0, so perhaps something like this works better: diff --git a/gobex/gobex-transfer.c b/gobex/gobex-transfer.c index c94d018b2..8fae65d8d 100644 --- a/gobex/gobex-transfer.c +++ b/gobex/gobex-transfer.c @@ -200,6 +200,11 @@ static void transfer_response(GObex *obex, GError *err, GObexPacket *rsp, if (rspcode != G_OBEX_RSP_SUCCESS && rspcode != G_OBEX_RSP_CONTINUE) { err = g_error_new(G_OBEX_ERROR, rspcode, "%s", g_obex_strerror(rspcode)); + /* If not marked as final response but transfer_complete gonna + * be called restore the req_id so it can be properly cancelled + */ + if (!final) + transport->req_id = id; goto failed; } > + > if (rspcode != G_OBEX_RSP_SUCCESS && rspcode != G_OBEX_RSP_CONTINUE) { > err = g_error_new(G_OBEX_ERROR, rspcode, "%s", > g_obex_strerror(rspcode)); > -- > 2.20.1 > > >
Hi Youwan, On Fri, Jun 17, 2022 at 3:58 PM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > Hi Youwan, > > On Wed, Jun 15, 2022 at 5:54 AM Youwan Wang <wangyouwan@uniontech.com> wrote: > > > > PC1 -> PC2 in the process of transferring the file, > > PC2 terminate the reception. > > It is found that PC1 will receive the Rcvd > > Forbiden message. > > transfer_response func is called and go to failed: > > transfer_complete func is called and go to transfer_free > > > > Invalid read of size 8 > > at 0x129C9B: put_get_data (gobex-transfer.c:138) > > by 0x128675: get_body (gobex-packet.c:397) > > by 0x128675: g_obex_packet_encode (gobex-packet.c:451) > > by 0x1264ED: write_data (gobex.c:461) > > by 0x1264ED: write_data (gobex.c:431) > > by 0x49B7E97: g_main_context_dispatch () > > by 0x49B8287: ??? () > > by 0x49B8581: g_main_loop_run () > > by 0x120834: main (main.c:322) > > Address 0x7300028 is 56 bytes inside a block of size 64 free'd > > at 0x48369AB: free (vg_replace_malloc.c:530) > > by 0x12A404: transfer_response (gobex-transfer.c:245) > > by 0x126B02: handle_response (gobex.c:1128) > > by 0x127087: incoming_data (gobex.c:1373) > > by 0x49B7E97: g_main_context_dispatch () > > by 0x49B8287: ??? () > > by 0x49B8581: g_main_loop_run () > > by 0x120834: main (main.c:322) > > Block was alloc'd at > > at 0x4837B65: calloc (vg_replace_malloc.c:752) > > by 0x49BD9D8: g_malloc0 () > > by 0x129A49: transfer_new (gobex-transfer.c:258) > > by 0x12A612: g_obex_put_req_pkt (gobex-transfer.c:282) > > by 0x12A612: g_obex_put_req_pkt (gobex-transfer.c:271) > > by 0x145862: transfer_start_put (transfer.c:880) > > by 0x145862: obc_transfer_start (transfer.c:911) > > by 0x13B487: session_process_transfer (session.c:836) > > by 0x13C128: session_process_queue (session.c:887) > > by 0x13C128: session_process_queue (session.c:872) > > by 0x13C18F: session_process (session.c:820) > > by 0x49B7E97: g_main_context_dispatch (in /usr/lib/x86_64-linux-gnu/) > > by 0x49B8287: ??? (in /usr/lib/x86_64-linux-gnu/) > > by 0x49B8581: g_main_loop_run (in /usr/lib/x86_64-linux-gnu/) > > by 0x120834: main (main.c:322) > > --- > > gobex/gobex-transfer.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/gobex/gobex-transfer.c b/gobex/gobex-transfer.c > > index c94d018b2..8e97fc73e 100644 > > --- a/gobex/gobex-transfer.c > > +++ b/gobex/gobex-transfer.c > > @@ -197,6 +197,14 @@ static void transfer_response(GObex *obex, GError *err, GObexPacket *rsp, > > } > > > > rspcode = g_obex_packet_get_operation(rsp, &final); > > + if (rspcode == G_OBEX_RSP_FORBIDDEN) { > > + err = g_error_new(G_OBEX_ERROR, rspcode, "%s", > > + g_obex_strerror(rspcode)); > > + g_obex_debug(G_OBEX_DEBUG_ERROR, "%s", err->message); > > + g_error_free(err); > > + return; > > + } > > This is not quite right, in fact the if statement below already treat > such errors, we might be missing something like final: > > /* Reset if final so it can no longer be cancelled */ > if (final_rsp) > obex->pending_req = NULL; > > This would be a problem on the remote implementation which is > indicating that perhaps it is just a temporary failure, anyway then: > > id = transfer->req_id; > transfer->req_id = 0; > > This resets the req_id so on transfer_free the following code is not run: > > if (transfer->req_id > 0) > g_obex_cancel_req(transfer->obex, transfer->req_id, TRUE); > > So either we have to restore the transfer->req_id = id or not reset it > to 0, so perhaps something like this works better: > > diff --git a/gobex/gobex-transfer.c b/gobex/gobex-transfer.c > index c94d018b2..8fae65d8d 100644 > --- a/gobex/gobex-transfer.c > +++ b/gobex/gobex-transfer.c > @@ -200,6 +200,11 @@ static void transfer_response(GObex *obex, GError > *err, GObexPacket *rsp, > if (rspcode != G_OBEX_RSP_SUCCESS && rspcode != G_OBEX_RSP_CONTINUE) { > err = g_error_new(G_OBEX_ERROR, rspcode, "%s", > g_obex_strerror(rspcode)); > + /* If not marked as final response but transfer_complete gonna > + * be called restore the req_id so it can be properly cancelled > + */ > + if (!final) > + transport->req_id = id; > goto failed; > } You haven't responded about this one, I believe there are quite a few patches of yours that are trying to circumvent this very issue. > > + > > if (rspcode != G_OBEX_RSP_SUCCESS && rspcode != G_OBEX_RSP_CONTINUE) { > > err = g_error_new(G_OBEX_ERROR, rspcode, "%s", > > g_obex_strerror(rspcode)); > > -- > > 2.20.1 > > > > > > > > > -- > Luiz Augusto von Dentz
diff --git a/gobex/gobex-transfer.c b/gobex/gobex-transfer.c index c94d018b2..8e97fc73e 100644 --- a/gobex/gobex-transfer.c +++ b/gobex/gobex-transfer.c @@ -197,6 +197,14 @@ static void transfer_response(GObex *obex, GError *err, GObexPacket *rsp, } rspcode = g_obex_packet_get_operation(rsp, &final); + if (rspcode == G_OBEX_RSP_FORBIDDEN) { + err = g_error_new(G_OBEX_ERROR, rspcode, "%s", + g_obex_strerror(rspcode)); + g_obex_debug(G_OBEX_DEBUG_ERROR, "%s", err->message); + g_error_free(err); + return; + } + if (rspcode != G_OBEX_RSP_SUCCESS && rspcode != G_OBEX_RSP_CONTINUE) { err = g_error_new(G_OBEX_ERROR, rspcode, "%s", g_obex_strerror(rspcode));
PC1 -> PC2 in the process of transferring the file, PC2 terminate the reception. It is found that PC1 will receive the Rcvd Forbiden message. transfer_response func is called and go to failed: transfer_complete func is called and go to transfer_free Invalid read of size 8 at 0x129C9B: put_get_data (gobex-transfer.c:138) by 0x128675: get_body (gobex-packet.c:397) by 0x128675: g_obex_packet_encode (gobex-packet.c:451) by 0x1264ED: write_data (gobex.c:461) by 0x1264ED: write_data (gobex.c:431) by 0x49B7E97: g_main_context_dispatch () by 0x49B8287: ??? () by 0x49B8581: g_main_loop_run () by 0x120834: main (main.c:322) Address 0x7300028 is 56 bytes inside a block of size 64 free'd at 0x48369AB: free (vg_replace_malloc.c:530) by 0x12A404: transfer_response (gobex-transfer.c:245) by 0x126B02: handle_response (gobex.c:1128) by 0x127087: incoming_data (gobex.c:1373) by 0x49B7E97: g_main_context_dispatch () by 0x49B8287: ??? () by 0x49B8581: g_main_loop_run () by 0x120834: main (main.c:322) Block was alloc'd at at 0x4837B65: calloc (vg_replace_malloc.c:752) by 0x49BD9D8: g_malloc0 () by 0x129A49: transfer_new (gobex-transfer.c:258) by 0x12A612: g_obex_put_req_pkt (gobex-transfer.c:282) by 0x12A612: g_obex_put_req_pkt (gobex-transfer.c:271) by 0x145862: transfer_start_put (transfer.c:880) by 0x145862: obc_transfer_start (transfer.c:911) by 0x13B487: session_process_transfer (session.c:836) by 0x13C128: session_process_queue (session.c:887) by 0x13C128: session_process_queue (session.c:872) by 0x13C18F: session_process (session.c:820) by 0x49B7E97: g_main_context_dispatch (in /usr/lib/x86_64-linux-gnu/) by 0x49B8287: ??? (in /usr/lib/x86_64-linux-gnu/) by 0x49B8581: g_main_loop_run (in /usr/lib/x86_64-linux-gnu/) by 0x120834: main (main.c:322) --- gobex/gobex-transfer.c | 8 ++++++++ 1 file changed, 8 insertions(+)