Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220708062548.31167-1-wangyouwan@uniontech.com>
In-Reply-To: <20220708062548.31167-1-wangyouwan@uniontech.com>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Fri, 8 Jul 2022 11:06:21 -0700
Message-ID: <CABBYNZJT8QpA2sWMOxE2pnwuQsRarenWMc5GLbcpROiW7Aqe=A@mail.gmail.com>
Subject: Re: [PATCH] obexd: fix crashed after cancel the on-going transfer
To:     Youwan Wang <wangyouwan@uniontech.com>
Cc:     "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Youwan,

On Thu, Jul 7, 2022 at 11:30 PM Youwan Wang <wangyouwan@uniontech.com> wrote:
>
> Pointer is reused after release.
> After the transfer pointer is released,
> remove the response function in pending_pkg
> to avoid the 'p->rsp_func' is reused
>
> step:
> [obex]# connect 28:33:34:1E:96:98
> Attempting to connect to 28:33:34:1E:96:98
> [NEW] Session /org/bluez/obex/client/session2 [default]
> [NEW] ObjectPush /org/bluez/obex/client/session2
> Connection successful
> [28:33:34:1E:96:98]# send /home/uos/Desktop/systemd.zip
> Attempting to send /home/uos/Desktop/systemd.zip
> [NEW] Transfer /org/bluez/obex/client/session2/transfer2
> Transfer /org/bluez/obex/client/session2/transfer2
>         Status: queued
>         Name: systemd.zip
>         Size: 33466053
>         Filename: /home/uos/Desktop/systemd.zip
>         Session: /org/bluez/obex/client/session2
> [CHG] Transfer /org/bluez/obex/client/session2/transfer2
> [CHG] Transfer /org/bluez/obex/client/session2/transfer2
> [CHG] Transfer /org/bluez/obex/client/session2/transfer2
> [CHG] Transfer /org/bluez/obex/client/session2/transfer2
> [CHG] Transfer /org/bluez/obex/client/session2/transfer2
> [CHG] Transfer /org/bluez/obex/client/session2/transfer2
> [CHG] Transfer /org/bluez/obex/client/session2/transfer2
> [CHG] Transfer /org/bluez/obex/client/session2/transfer2
> [CHG] Transfer /org/bluez/obex/client/session2/transfer2
> [CHG] Transfer /org/bluez/obex/client/session2/transfer2
> [CHG] Transfer /org/bluez/obex/client/session2/transfer2
> [CHG] Transfer /org/bluez/obex/client/session2/transfer2
> er2 33:34:1E:96:98]# cancel /org/bluez/obex/client/sessi
> Attempting to cancel transfer /org/bluez/obex/client/s
> Cancel successful
>
> valgrind trace:
> ==11431== Invalid read of size 4
> ==11431==    at 0x12B442: transfer_response ()
> ==11431==    by 0x127764: req_timeout ()
> ==11431==    by 0x49B8922: ??? ( )
> ==11431==    by 0x49B7E97: g_main_context_dispatch ()
> ==11431==    by 0x49B8287: ??? (in )
> ==11431==    by 0x49B8581: g_main_loop_run ()
> ==11431==    by 0x121834: main (main.c:322)
> ==11431==  Address 0x7344fa0 is 16 bytes inside a block of size
> ==11431==    at 0x48369AB: free ()
> ==11431==    by 0x12B459: transfer_response ()
> ==11431==    by 0x127B3D: cancel_complete ()
> ==11431==    by 0x49B7E97: g_main_context_dispatch ()
> ==11431==    by 0x49B8287: ??? ()
> ==11431==    by 0x49B8581: g_main_loop_run ()
> ==11431==    by 0x121834: main (main.c:322)
> ==11431==  Block was alloc'd at
> ==11431==    at 0x4837B65: calloc ()
> ==11431==    by 0x49BD9D8: g_malloc0 ()
> ==11431==    by 0x12AB89: transfer_new ()
> ==11431==    by 0x12B732: g_obex_put_req_pkt ()
> ==11431==    by 0x12B732: g_obex_put_req_pkt ()
> ==11431==    by 0x146982: transfer_start_put ()
> ==11431==    by 0x146982: obc_transfer_start ()
> ==11431==    by 0x13C5A7: session_process_transfer ()
> ==11431==    by 0x13D248: session_process_queue ()
> ==11431==    by 0x13D248: session_process_queue ()
> ==11431==    by 0x13D2AF: session_process ()
> ==11431==    by 0x49B7E97: g_main_context_dispatch ()
> ==11431==    by 0x49B8287: ??? (i)
> ==11431==    by 0x49B8581: g_main_loop_run ()
> ==11431==    by 0x121834: main ()
> ==11431==
> ==11431== (action on error) vgdb me ...
> ---
>  gobex/gobex-transfer.c | 2 ++
>  gobex/gobex.c          | 6 ++++++
>  gobex/gobex.h          | 1 +
>  3 files changed, 9 insertions(+)
>
> diff --git a/gobex/gobex-transfer.c b/gobex/gobex-transfer.c
> index c94d018b2..38c23785c 100644
> --- a/gobex/gobex-transfer.c
> +++ b/gobex/gobex-transfer.c
> @@ -64,6 +64,8 @@ static void transfer_free(struct transfer *transfer)
>                 g_obex_remove_request_function(transfer->obex,
>                                                         transfer->abort_id);
>
> +       g_obex_remove_responsefunc(transfer->obex);
> +
>         g_obex_unref(transfer->obex);
>         g_free(transfer);
>  }
> diff --git a/gobex/gobex.c b/gobex/gobex.c
> index bc4d52551..54ce484f2 100644
> --- a/gobex/gobex.c
> +++ b/gobex/gobex.c
> @@ -533,6 +533,12 @@ void g_obex_drop_tx_queue(GObex *obex)
>                 pending_pkt_free(p);
>  }
>
> +void g_obex_remove_responsefunc(GObex *obex)
> +{
> +       if (obex->pending_req)
> +               obex->pending_req->rsp_func = NULL;
> +}

Can we stop treating the symptom and really fix the cause,
transfer_free does already call g_obex_cancel_req which normally
should take care of removing any timeout handling which seems to be
the cause of the crash here:

https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/gobex/gobex.c#n841

Btw, Ive already pointed out that there are test cases for it and if
we are not capturing the exact scenario you are describing here would
you please add a test case with the exact response that is causing
such a crash:

https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/unit/test-gobex-transfer.c#n480

>  static gboolean g_obex_send_internal(GObex *obex, struct pending_pkt *p,
>                                                                 GError **err)
>  {
> diff --git a/gobex/gobex.h b/gobex/gobex.h
> index f16e4426c..1f7ae513a 100644
> --- a/gobex/gobex.h
> +++ b/gobex/gobex.h
> @@ -51,6 +51,7 @@ void g_obex_suspend(GObex *obex);
>  void g_obex_resume(GObex *obex);
>  gboolean g_obex_srm_active(GObex *obex);
>  void g_obex_drop_tx_queue(GObex *obex);
> +void g_obex_remove_responsefunc(GObex *obex);
>
>  GObex *g_obex_new(GIOChannel *io, GObexTransportType transport_type,
>                                                 gssize rx_mtu, gssize tx_mtu);
> --
> 2.20.1
>
>
>


-- 
Luiz Augusto von Dentz