From: Howard Chung <hidden> Date: 2021-07-12 12:39:06
From: Yun-Hao Chung <redacted>
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 <redacted>
---
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(-)
From: Luiz Augusto von Dentz <luiz.dentz@gmail.com> Date: 2021-07-12 20:41:05
Hi Howard,
On Mon, Jul 12, 2021 at 5:39 AM Howard Chung [off-list ref] wrote:
quoted hunk
From: Yun-Hao Chung <redacted>
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 <redacted>
---
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(-)
@@ -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.