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: <20220614114914.30497-1-wangyouwan@uniontech.com>
In-Reply-To: <20220614114914.30497-1-wangyouwan@uniontech.com>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Tue, 14 Jun 2022 13:40:49 -0700
Message-ID: <CABBYNZKcqH=pTCgt=nvjUMUhv8GiBAaXxi2PvHtqA_Zar983Lg@mail.gmail.com>
Subject: Re: [PATCH] obexd: Fix transfer has been free during
 transfer_abort_response func
To:     wangyouwan <wangyouwan@uniontech.com>
Cc:     "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi,

On Tue, Jun 14, 2022 at 4:55 AM wangyouwan <wangyouwan@uniontech.com> wrote:
>
> Breakpoint 7, transfer_new (obex=0x5555555f5b50, opcode=2 '\002',
> complete_func=0x555555590c40 <xfer_complete>, user_data=0x5555555f7000)
> at gobex/gobex-transfer.c:254
> 254     gobex/gobex-transfer.c:
> (gdb) c
> Continuing.
>
> Breakpoint 3, transfer_complete (transfer=0x5555555f5f40, err=0x5555555f6d60)
> at gobex/gobex-transfer.c:99
> 99      in gobex/gobex-transfer.c
> (gdb) c
> Continuing.
>
> Breakpoint 4, xfer_complete (obex=0x5555555f5b50, err=0x5555555f6d60,
> user_data=0x5555555f7000)
> at obexd/client/transfer.c:659
> 659     obexd/client/transfer.c:
> (gdb) n
> 661     in obexd/client/transfer.c
> (gdb) n
> 663     in obexd/client/transfer.c
> (gdb) p callback->func
> $17 = (transfer_callback_t) 0x5555555885e0 <transfer_complete>
> (gdb) c
> Continuing.
>
> Breakpoint 3, transfer_complete (transfer=0x5555555f7000, err=0x5555555f6d60,
> user_data=0x5555555f48d0) at obexd/client/session.c:964
> 964     obexd/client/session.c:
> (gdb) c
> Continuing.
>
> Breakpoint 5, transfer_free (transfer=0x5555555f5f40) at gobex/gobex-transfer.c:61
> 61      gobex/gobex-transfer.c:
> (gdb) n
> 63      in gobex/gobex-transfer.c
> (gdb) p transfer->id
> $18 = 1
> (gdb) c
> Continuing.
>
> Breakpoint 2, transfer_abort_response (obex=0x5555555f5b50, err=0x0, rsp=0x5555555f0810,
> user_data=0x5555555f5f40) at gobex/gobex-transfer.c:116
> 116     in gobex/gobex-transfer.c
> (gdb) n
> 118     in gobex/gobex-transfer.c
> (gdb) p transfer->id
> $19 = 1432314080
> (gdb) c
> Continuing.
>
> Breakpoint 3, transfer_complete (transfer=0x5555555f5f40, err=0x5555555f6f00) at gobex/gobex-transfer.c:99
> 99      in gobex/gobex-transfer.c
> (gdb) c
> Continuing.
>
> Breakpoint 4, xfer_complete (obex=0x5555555f5b50, err=0x5555555f6f00, user_data=0x5555555f7000)
> at obexd/client/transfer.c:659
> 659     obexd/client/transfer.c:
> (gdb) n
> 661     in obexd/client/transfer.c
> (gdb) p callback->func
> $20 = (transfer_callback_t) 0x5555555f6420
> (gdb) n
> 663     in obexd/client/transfer.c
> (gdb) n
> 668     in obexd/client/transfer.c
> (gdb) n
> 671     in obexd/client/transfer.c
> (gdb) n
> 672     in obexd/client/transfer.c
> (gdb) n
> 676     in obexd/client/transfer.c
> (gdb) n
> 679     in obexd/client/transfer.c
> (gdb) n
> 680     in obexd/client/transfer.c
> (gdb) n
> 0x00005555555f6420 in ?? ()
> (gdb) n
> Cannot find bounds of current function
> (gdb) c
> Continuing.
>
> Program received signal SIGSEGV, Segmentation fault.
> 0x00005555555f6420 in ?? ()
> (gdb)
> ---
>  gobex/gobex-transfer.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/gobex/gobex-transfer.c b/gobex/gobex-transfer.c
> index c94d018b2..48b1f6962 100644
> --- a/gobex/gobex-transfer.c
> +++ b/gobex/gobex-transfer.c
> @@ -92,7 +92,9 @@ static void transfer_complete(struct transfer *transfer, GError *err)
>                 g_obex_drop_tx_queue(transfer->obex);
>         }
>
> -       transfer->complete_func(transfer->obex, err, transfer->user_data);
> +       if (find_transfer(id) != NULL)
> +               transfer->complete_func(transfer->obex, err, transfer->user_data);

I would rather fix the code calling of the transfer_complete rather
than its side effect if transfer has been freed already, also it might
be better to use valgrind to collect the bracktrace since it should be
able to tell us where the transfer pointer is being freed.

>         /* Check if the complete_func removed the transfer */
>         if (find_transfer(id) == NULL)
>                 return;
> --
> 2.20.1
>
>
>


-- 
Luiz Augusto von Dentz