2022-06-15 12:54:34

by Youwan Wang

[permalink] [raw]
Subject: [PATCH] obexd: Fix crash of transfer pointer has being freed

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;
+ }
+
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




2022-06-15 14:44:55

by bluez.test.bot

[permalink] [raw]
Subject: RE: obexd: Fix crash of transfer pointer has being freed

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

2022-06-17 23:02:08

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCH] obexd: Fix crash of transfer pointer has being freed

Hi Youwan,

On Wed, Jun 15, 2022 at 5:54 AM Youwan Wang <[email protected]> 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
>
>
>


--
Luiz Augusto von Dentz

2022-06-30 00:10:50

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCH] obexd: Fix crash of transfer pointer has being freed

Hi Youwan,

On Fri, Jun 17, 2022 at 3:58 PM Luiz Augusto von Dentz
<[email protected]> wrote:
>
> Hi Youwan,
>
> On Wed, Jun 15, 2022 at 5:54 AM Youwan Wang <[email protected]> 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



--
Luiz Augusto von Dentz