Re: [Bluez PATCH v3] audio/a2dp - fix crash during recovering process

[Luiz Augusto von Dentz <luiz.dentz@gmail.com>]

Hi,

On Fri, Jan 10, 2020 at 12:13 AM Yun-hao Chung <howardchung@google.com> wrote:
>
> Hi Luiz,
>
> Thanks for the comment.
> I did the qualification test with and without the patch on both Chrome
> OS and Linux with bluez-5.52.
> The results are all the same.
> The tests I’ve run are as follows.
>
> A2DP/SRC/AS/BV-01-I
> A2DP/SRC/CC/BV-09-I
> A2DP/SRC/CC/BV-10-I
> A2DP/SRC/REL/BV-01-I
> A2DP/SRC/REL/BV-02-I
> A2DP/SRC/SET/BV-01-I
> A2DP/SRC/SET/BV-02-I
> A2DP/SRC/SET/BV-03-I
> A2DP/SRC/SET/BV-04-I
> A2DP/SRC/SET/BV-05-I
> A2DP/SRC/SET/BV-06-I
> IOPT/CL/A2DP-SRC/SFC/BV-01-I
> AVDTP/SRC/ACP/SIG/SMG/BV-06-C
> AVDTP/SRC/ACP/SIG/SMG/BV-08-C
> AVDTP/SRC/ACP/SIG/SMG/BV-10-C
> AVDTP/SRC/ACP/SIG/SMG/BV-16-C
> AVDTP/SRC/ACP/SIG/SMG/BV-18-C
> AVDTP/SRC/ACP/SIG/SMG/BV-20-C
> AVDTP/SRC/ACP/SIG/SMG/BV-24-C
> AVDTP/SRC/ACP/SIG/SMG/BI-05-C
> AVDTP/SRC/ACP/SIG/SMG/BI-08-C
> AVDTP/SRC/ACP/SIG/SMG/BI-17-C
> AVDTP/SRC/ACP/SIG/SMG/BI-20-C
> AVDTP/SRC/ACP/SIG/SMG/BI-23-C
> AVDTP/SRC/ACP/SIG/SMG/BI-28-C*
> AVDTP/SRC/ACP/TRA/BTR/BI-01-C
> AVDTP/SRC/INT/SIG/SMG/BV-05-C
> AVDTP/SRC/INT/SIG/SMG/BV-07-C
> AVDTP/SRC/INT/SIG/SMG/BV-09-C
> AVDTP/SRC/INT/SIG/SMG/BV-15-C
> AVDTP/SRC/INT/SIG/SMG/BV-17-C
> AVDTP/SRC/INT/SIG/SMG/BV-19-C
> AVDTP/SRC/INT/SIG/SMG/BV-23-C
> AVDTP/SRC/INT/SIG/SMG/BV-31-C
> AVDTP/SRC/INT/SIG/SMG/BI-30-C
> AVDTP/SRC/INT/TRA/BTR/BV-01-C
>
> Except for AVDTP/SRC/ACP/SIG/SMG/BI-28-C, the other tests had been passed.
> Please let me know if I need more verification.
>
> Thanks,
> Howard
>
> On Fri, Jan 10, 2020 at 4:09 PM Yun-hao Chung <howardchung@google.com> wrote:
> >
> > ---------- Forwarded message ---------
> > From: howardchung@google.com <howardchung@google.com>
> > Date: Fri, Jan 10, 2020 at 3:57 PM
> > Subject: [Bluez PATCH v3] audio/a2dp - fix crash during recovering process
> > To: <linux-bluetooth@vger.kernel.org>, <luiz.von.dentz@intel.com>
> > Cc: <chromeos-bluetooth-upstreaming@chromium.org>, howardchung
> > <howardchung@google.com>
> >
> >
> > From: howardchung <howardchung@google.com>
> >
> > The crash with stack trace:
> >
> > (libc-2.27.so -raise.c:51 )                     raise
> > (libc-2.27.so -abort.c:79 )                     abort
> > (libc-2.27.so -libc_fatal.c:181 )               __libc_message
> > (libc-2.27.so -malloc.c:5350 )                  malloc_printerr
> > (libc-2.27.so -malloc.c:4157 )                  _int_free
> > (libglib-2.0.so.0.5200.3 -gslist.c:878 )        g_slist_free_full
> > (bluetoothd -a2dp.c:165 )                       setup_unref
> > (bluetoothd -a2dp.c:2184 )                      a2dp_cancel
> > (bluetoothd -sink.c:317 )                       sink_unregister
> > (bluetoothd -service.c:176 )                    service_remove
> > (bluetoothd -device.c:4678 )                    device_remove
> > (bluetoothd -adapter.c:6573 )                   adapter_remove
> > (bluetoothd -adapter.c:8832 )                   index_removed
> > (bluetoothd -queue.c:220 )                      queue_foreach
> > (bluetoothd -mgmt.c:304 )                       can_read_data
> > (bluetoothd -io-glib.c:170 )                    watch_callback
> > (libglib-2.0.so.0.5200.3 -gmain.c:3234 )        g_main_context_dispatch
> > (libglib-2.0.so.0.5200.3 -gmain.c:3972 )        g_main_context_iterate
> > (libglib-2.0.so.0.5200.3 -gmain.c:4168 )        g_main_loop_run
> > (bluetoothd -main.c:798 )                       main
> > (libc-2.27.so -libc-start.c:308 )               __libc_start_main
> > (bluetoothd + 0x0000b089 )                      _start
> > (bluetoothd + 0x0000b05f )                      _init
> >
> > triggered when 'usb disconnect' happened during AVDTP_SET_CONFIGURATION
> > request is sent but haven't received the response.
> > In this situation, the recovering process goes into sink.c:sink_free and
> > then a2dp.c:a2dp_cancel, avdtp.c:cancel_request, avdtp.c:connection_lost,
> > avdtp.c:release_stream.
> >
> > During recovering, the reference count of setup and avdtp decrease more
> > than it increase, which ends up causing the crash.
> >
> > The reference count of setup decreases one more time since
> > a2dp.c:setconf_cfm(called by cfm->set_configuration in
> > avdtp.c:cancel_request) was called in the 'error mode', which didn't
> > reference the setup, but in a2dp.c:abort_cfm(called by cfm->abort in
> > avdtp.c:release_stream), the reference count decreased by 1.
> >
> > In this case, abort_cfm shouldn't be called as we already know
> > setconf_cfm didn't send any request. Setting avdtp_sep_state to
> > AVDTP_STATE_ABORTING should avoid this issue.
> >
> > The reference count of avdtp decrease one more time since
> > both sink.c:sink_free and sink.c:sink_set_state(called from
> > avdtp.c:connection_lost -> avdtp.c:avdtp_set_state) unreference avdtp
> > for the session. The changes in sink.c should avoid the issue.
> >
> > ---
> > How to test:
> >         The crash can be simulated by the following procedure.
> >         1. injecting sleep(10) right before calling a2dp_config in
> >            sink.c:select_complete.
> >         2. connect with a bluetooth headset
> >         3. run 'rmmod btusb' after ~5 seconds(before the connection
> >            complete)
> > The procedure can reproduce the crash with ~50% probability.
> > Even if the bluetoothd didn't crash or it crashed with different
> > signature, the reference count can end up with some invalid number.
> >
> > After the patch applies, there is no crash after running the test above 10
> > times in a row.
> >
> > Changes in v3:
> > - Update the title
> > - Remove the signed-off-by section
> >
> > Changes in v2:
> > - Fixed typo in commit message
> >
> >  profiles/audio/avdtp.c | 3 +++
> >  profiles/audio/sink.c  | 4 +++-
> >  2 files changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/profiles/audio/avdtp.c b/profiles/audio/avdtp.c
> > index 51ead684a..620a76c90 100644
> > --- a/profiles/audio/avdtp.c
> > +++ b/profiles/audio/avdtp.c
> > @@ -3484,6 +3484,7 @@ int avdtp_abort(struct avdtp *session, struct
> > avdtp_stream *stream)
> >  {
> >         struct seid_req req;
> >         int ret;
> > +       struct avdtp_local_sep *sep = stream->lsep;
> >
> >         if (!stream && session->discover) {
> >                 /* Don't call cb since it being aborted */
> > @@ -3498,6 +3499,8 @@ int avdtp_abort(struct avdtp *session, struct
> > avdtp_stream *stream)
> >         if (stream->lsep->state == AVDTP_STATE_ABORTING)
> >                 return -EINVAL;
> >
> > +       avdtp_sep_set_state(session, sep, AVDTP_STATE_ABORTING);
> > +
> >         if (session->req && stream == session->req->stream)
> >                 return cancel_request(session, ECANCELED);
> >
> > diff --git a/profiles/audio/sink.c b/profiles/audio/sink.c
> > index 7cac21034..726e2f562 100644
> > --- a/profiles/audio/sink.c
> > +++ b/profiles/audio/sink.c
> > @@ -309,8 +309,10 @@ static void sink_free(struct btd_service *service)
> >                 avdtp_stream_remove_cb(sink->session, sink->stream,
> >                                         sink->cb_id);
> >
> > -       if (sink->session)
> > +       if (sink->session) {
> >                 avdtp_unref(sink->session);
> > +               sink->session = NULL;
> > +       }
> >
> >         if (sink->connect_id > 0) {
> >                 btd_service_connecting_complete(sink->service, -ECANCELED);
> > --
> > 2.25.0.rc1.283.g88dfdc4193-goog


Applied, thanks.

-- 
Luiz Augusto von Dentz