| Message ID | 20210712203813.Bluez.v1.1.Id7aa1152377161d17b442bf258773d9b6c624ca3@changeid (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [Bluez,v1] a2dp: Fix crash in channel_free while waiting cmd resp | 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=514025
---Test result---
Test Summary:
CheckPatch PASS 0.27 seconds
GitLint FAIL 0.10 seconds
Prep - Setup ELL PASS 39.38 seconds
Build - Prep PASS 0.09 seconds
Build - Configure PASS 6.92 seconds
Build - Make FAIL 130.35 seconds
Make Check FAIL 1.09 seconds
Make Distcheck PASS 202.13 seconds
Build w/ext ELL - Configure PASS 6.99 seconds
Build w/ext ELL - Make FAIL 120.25 seconds
Details
##############################
Test: CheckPatch - PASS
Desc: Run checkpatch.pl script with rule in .checkpatch.conf
##############################
Test: GitLint - FAIL
Desc: Run gitlint with rule in .gitlint
Output:
a2dp: Fix crash in channel_free while waiting cmd resp
14: B3 Line contains hard tab characters (\t): "0x000059f01943e688 (bluetoothd -avdtp.c:3690)"
16: B3 Line contains hard tab characters (\t): "0x000059f01943928a (bluetoothd -a2dp.c:3069)"
18: B3 Line contains hard tab characters (\t): "0x000059f0194377fa (bluetoothd -sink.c:324)"
20: B3 Line contains hard tab characters (\t): "0x000059f01948715a (bluetoothd -service.c:177)"
22: B3 Line contains hard tab characters (\t): "0x000059f01948d77c (bluetoothd -device.c:5346)"
24: B3 Line contains hard tab characters (\t): "0x000059f019476d14 (bluetoothd -adapter.c:7202)"
26: B3 Line contains hard tab characters (\t): "0x000059f019476c3e (bluetoothd -adapter.c:10827)"
28: B3 Line contains hard tab characters (\t): "0x000059f01949d8d7 (bluetoothd -main.c:1114) main"
29: B3 Line contains hard tab characters (\t): "0x0000787b36185d74 (libc.so.6 -libc-start.c:308)"
31: B3 Line contains hard tab characters (\t): "0x000059f019433e39 (bluetoothd + 0x00026e39) _start"
##############################
Test: Prep - Setup ELL - PASS
Desc: Clone, build, and install ELL
##############################
Test: Build - Prep - PASS
Desc: Prepare environment for build
##############################
Test: Build - Configure - PASS
Desc: Configure the BlueZ source tree
##############################
Test: Build - Make - FAIL
Desc: Build the BlueZ source tree
Output:
profiles/audio/a2dp.c: In function ‘channel_free’:
profiles/audio/a2dp.c:1544:42: error: passing argument 3 of ‘finalize_setup_errno’ from incompatible pointer type [-Werror=incompatible-pointer-types]
1544 | finalize_setup_errno(setup, -ENOTCONN, finalize_discover,
| ^~~~~~~~~~~~~~~~~
| |
| void (*)(struct a2dp_setup *)
profiles/audio/a2dp.c:262:18: note: expected ‘GSourceFunc’ {aka ‘int (*)(void *)’} but argument is of type ‘void (*)(struct a2dp_setup *)’
262 | GSourceFunc cb1, ...)
| ~~~~~~~~~~~~^~~
cc1: all warnings being treated as errors
make[1]: *** [Makefile:8531: profiles/audio/bluetoothd-a2dp.o] Error 1
make: *** [Makefile:4134: all] Error 2
##############################
Test: Make Check - FAIL
Desc: Run 'make check'
Output:
profiles/audio/a2dp.c: In function ‘channel_free’:
profiles/audio/a2dp.c:1544:42: error: passing argument 3 of ‘finalize_setup_errno’ from incompatible pointer type [-Werror=incompatible-pointer-types]
1544 | finalize_setup_errno(setup, -ENOTCONN, finalize_discover,
| ^~~~~~~~~~~~~~~~~
| |
| void (*)(struct a2dp_setup *)
profiles/audio/a2dp.c:262:18: note: expected ‘GSourceFunc’ {aka ‘int (*)(void *)’} but argument is of type ‘void (*)(struct a2dp_setup *)’
262 | GSourceFunc cb1, ...)
| ~~~~~~~~~~~~^~~
cc1: all warnings being treated as errors
make[1]: *** [Makefile:8531: profiles/audio/bluetoothd-a2dp.o] Error 1
make: *** [Makefile:10406: check] Error 2
##############################
Test: Make Distcheck - PASS
Desc: Run distcheck to check the distribution
##############################
Test: Build w/ext ELL - Configure - PASS
Desc: Configure BlueZ source with '--enable-external-ell' configuration
##############################
Test: Build w/ext ELL - Make - FAIL
Desc: Build BlueZ source with '--enable-external-ell' configuration
Output:
profiles/audio/a2dp.c: In function ‘channel_free’:
profiles/audio/a2dp.c:1544:42: error: passing argument 3 of ‘finalize_setup_errno’ from incompatible pointer type [-Werror=incompatible-pointer-types]
1544 | finalize_setup_errno(setup, -ENOTCONN, finalize_discover,
| ^~~~~~~~~~~~~~~~~
| |
| void (*)(struct a2dp_setup *)
profiles/audio/a2dp.c:262:18: note: expected ‘GSourceFunc’ {aka ‘int (*)(void *)’} but argument is of type ‘void (*)(struct a2dp_setup *)’
262 | GSourceFunc cb1, ...)
| ~~~~~~~~~~~~^~~
cc1: all warnings being treated as errors
make[1]: *** [Makefile:8531: profiles/audio/bluetoothd-a2dp.o] Error 1
make: *** [Makefile:4134: all] Error 2
---
Regards,
Linux Bluetooth
Hi Howard, On Mon, Jul 12, 2021 at 5:39 AM Howard Chung <howardchung@google.com> wrote: > > From: Yun-Hao Chung <howardchung@chromium.org> > > When channel_free is called and we are waiting for a command response > from the peer, bluez NULL the setup->session but would not free its > setup_cb. Since setup_cb holds a ref of setup, the setup wouldn't be > freed and if service_removed is called after channel_free, a2dp_cancel > tries to abort the ongoing avdtp commands, which accesses the > setup->session and triggers a crash. > > This change finalizes all avdtp commands before assigning setup->session > to NULL in channel_free. > > Crash stack trace: > 0x000059f01943e688 (bluetoothd -avdtp.c:3690) > avdtp_abort > 0x000059f01943928a (bluetoothd -a2dp.c:3069) > a2dp_cancel > 0x000059f0194377fa (bluetoothd -sink.c:324) > sink_unregister > 0x000059f01948715a (bluetoothd -service.c:177) > service_remove > 0x000059f01948d77c (bluetoothd -device.c:5346) > device_remove > 0x000059f019476d14 (bluetoothd -adapter.c:7202) > adapter_remove > 0x000059f019476c3e (bluetoothd -adapter.c:10827) > adapter_cleanup > 0x000059f01949d8d7 (bluetoothd -main.c:1114) main > 0x0000787b36185d74 (libc.so.6 -libc-start.c:308) > __libc_start_main > 0x000059f019433e39 (bluetoothd + 0x00026e39) _start > 0x00007fff2d2c0127 > > Reviewed-by: Archie Pusaka <apusaka@chromium.org> > --- > There are two other options to fix this crash. > 1. add a NULL check in a2dp_cancel before calling avdtp_abort. > 2. call setup_cb_free to every setup_cb in setup->cb in channel_free. > > Since each setup_cb needs setup->session, I think there is no need to > keep the setup_cb after assigning setup->session to NULL. So the first > option is not ideal. If the second option is adopted, there would be > some time that sink/source->connect_id/disconnect_id is not zero, but > there is no corresponding setup_cb. > > Test steps: > Reproduce the crash with the following steps. Verify the crash is > no longer observed after this change. > 1. ignore AVDTP_SET_CONFIGURATION resp by modifying avdtp.c > 2. turn on a paired headset > 3. check the bluetooth.log, while bluez is waiting for > AVDTP_SET_CONFIGURATION resp, stop bluetoothd immediately. > This will trigger: > session_cb (I/O error) -> connection_lost > -> avdtp_set_state (AVDTP_SESSION_STATE_DISCONNECTED) > -> avdtp_state_cb -> channel_remove -> channel_free > then: > adapter_cleanup -> adapter_remove -> device_remove -> service_remove > -> a2dp_sink_remove -> sink_unregister -> sink_free -> a2dp_cancel > 4. check if bluetoothd crash > The above steps can trigger the crash 100%. > > profiles/audio/a2dp.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c > index d31ed845cbe7..f201b98c79d0 100644 > --- a/profiles/audio/a2dp.c > +++ b/profiles/audio/a2dp.c > @@ -1540,9 +1540,14 @@ static void channel_free(void *data) > setup = find_setup_by_session(chan->session); > if (setup) { > setup->chan = NULL; > + /* Finalize pending commands before we NULL setup->session */ > + finalize_setup_errno(setup, -ENOTCONN, finalize_discover, > + finalize_select, > + finalize_config, > + finalize_resume, > + finalize_suspend, NULL); While the analysis seems correct I wonder if wouldn't be a better idea to have a finalize_all so we don't have to iterate the callback list multiple times, other than that the patch looks good and I would prioritize it to have it included asap. > avdtp_unref(setup->session); > setup->session = NULL; > - finalize_setup_errno(setup, -ENOTCONN, NULL); > } > > g_free(chan); > -- > 2.32.0.93.g670b81a890-goog >
diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c index d31ed845cbe7..f201b98c79d0 100644 --- a/profiles/audio/a2dp.c +++ b/profiles/audio/a2dp.c @@ -1540,9 +1540,14 @@ static void channel_free(void *data) setup = find_setup_by_session(chan->session); if (setup) { setup->chan = NULL; + /* Finalize pending commands before we NULL setup->session */ + finalize_setup_errno(setup, -ENOTCONN, finalize_discover, + finalize_select, + finalize_config, + finalize_resume, + finalize_suspend, NULL); avdtp_unref(setup->session); setup->session = NULL; - finalize_setup_errno(setup, -ENOTCONN, NULL); } g_free(chan);