| Message ID | 20220307140437.Bluez.v1.1.Ieb7448d3d951876e1f412452fcfd27cdc7bd015b@changeid |
|---|---|
| State | New |
| Headers | show |
| Series | [Bluez,v1] audio: fix crash in a2dp_discover | 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=620823 ---Test result--- Test Summary: CheckPatch PASS 1.54 seconds GitLint PASS 1.09 seconds Prep - Setup ELL PASS 52.28 seconds Build - Prep PASS 0.91 seconds Build - Configure PASS 10.71 seconds Build - Make PASS 1460.20 seconds Make Check PASS 13.01 seconds Make Check w/Valgrind PASS 538.68 seconds Make Distcheck PASS 282.48 seconds Build w/ext ELL - Configure PASS 10.73 seconds Build w/ext ELL - Make PASS 1428.08 seconds Incremental Build with patchesPASS 0.00 seconds --- Regards, Linux Bluetooth
Hi Howard, On Sun, Mar 6, 2022 at 10:06 PM Howard Chung <howardchung@google.com> wrote: > > From: Yun-Hao Chung <howardchung@chromium.org> > > Sample stack trace: > 0x0000567c394e4c6b (bluetoothd - a2dp.c: 270) setup_cb_free > 0x0000567c394e4a94 (bluetoothd - a2dp.c: 2884) a2dp_discover > 0x0000567c394e3c03 (bluetoothd - sink.c: 275) sink_setup_stream > 0x0000567c394e3d4f (bluetoothd - sink.c: 299) sink_connect > 0x0000567c39535183 (bluetoothd - service.c: 294) btd_service_connect > 0x0000567c39539f68 (bluetoothd - device.c: 2006) connect_next > 0x0000567c3954086d (bluetoothd - device.c: 2060) service_state_changed > 0x0000567c39534efb (bluetoothd - service.c: 111) change_state > 0x0000567c3953559c (bluetoothd - service.c: 0) > btd_service_connecting_complete > 0x0000567c39534a5c (bluetoothd - profile.c: 1641) record_cb > 0x0000567c395197cd (bluetoothd - sdp-client.c: 298) connect_watch > 0x00007b14bc8034f6 (libglib-2.0.so.0 - gmain.c: 3337) > g_main_context_dispatch > 0x00007b14bc803801 (libglib-2.0.so.0 - gmain.c: 4131) > g_main_context_iterate > 0x00007b14bc803a7d (libglib-2.0.so.0 - gmain.c: 4329) g_main_loop_run > 0x0000567c39566af1 (bluetoothd - mainloop-glib.c: 79) mainloop_run > 0x0000567c39566ddb (bluetoothd - mainloop-notify.c: 201) > mainloop_run_with_signal > 0x0000567c3954bf4c (bluetoothd - main.c: 1222) main > 0x00007b14bc579797 (libc.so.6 - libc-start.c: 332) __libc_start_main > 0x0000567c394df449 (bluetoothd) _start > 0x00007ffd70145737 > > This could be triggered from a2dp_discover -> avdtp_discover -> > send_request -> send_req -> l2cap_connect (return error) -> > avdtp_set_state (to disconnect state)-> channel_remove -> channel_free > -> finalize_setup_errno (discover cb is freed) -> error handling all > the way back to a2dp_discover -> a2dp_discover (discover cb is freed > again, crashed!). > > The fix is to add setup_ref/setup_unref around a2dp_discover to ensure > setup alive, and check if setup->chan to see if channel_free has been > called or not. > > Reviewed-by: Archie Pusaka <apusaka@chromium.org> > --- > Hi maintainers, > > The fix is tested by forcing session->io to NULL in send_req in the code > and verifing the crash doesn't happen. > > profiles/audio/a2dp.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c > index 9fbcd35f7..39e1e9624 100644 > --- a/profiles/audio/a2dp.c > +++ b/profiles/audio/a2dp.c > @@ -2878,14 +2878,22 @@ unsigned int a2dp_discover(struct avdtp *session, a2dp_discover_cb_t cb, > if (!setup) > return 0; > > + setup_ref(setup); > cb_data = setup_cb_new(setup); > cb_data->discover_cb = cb; > cb_data->user_data = user_data; > > - if (avdtp_discover(session, discover_cb, setup) == 0) > + if (avdtp_discover(session, discover_cb, setup) == 0) { > + setup_unref(setup); > return cb_data->id; > + } > > - setup_cb_free(cb_data); > + /* Check if the channel is still there before freeing setup_cb, since it > + * could be freed by channel_free(). > + */ > + if (setup->chan) > + setup_cb_free(cb_data); > + setup_unref(setup); > return 0; This sounds a little too complicated and I'm afraid we may run into other problems if we start taking extra references, so how about we don't attach the cb_data to begin with: https://gist.github.com/Vudentz/7f1a3eeea7decdb17b67d288b1dff77e > } > > -- > 2.35.1.616.g0bdcbb4464-goog >
diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c index 9fbcd35f7..39e1e9624 100644 --- a/profiles/audio/a2dp.c +++ b/profiles/audio/a2dp.c @@ -2878,14 +2878,22 @@ unsigned int a2dp_discover(struct avdtp *session, a2dp_discover_cb_t cb, if (!setup) return 0; + setup_ref(setup); cb_data = setup_cb_new(setup); cb_data->discover_cb = cb; cb_data->user_data = user_data; - if (avdtp_discover(session, discover_cb, setup) == 0) + if (avdtp_discover(session, discover_cb, setup) == 0) { + setup_unref(setup); return cb_data->id; + } - setup_cb_free(cb_data); + /* Check if the channel is still there before freeing setup_cb, since it + * could be freed by channel_free(). + */ + if (setup->chan) + setup_cb_free(cb_data); + setup_unref(setup); return 0; }