archive mirror
 help / color / mirror / Atom feed
From: Howard Chung <>
Cc: Yun-Hao Chung <>,
	Archie Pusaka <>
Subject: [Bluez PATCH v4] a2dp: Fix crash in channel_free while waiting cmd resp
Date: Wed, 14 Jul 2021 17:02:03 +0800	[thread overview]
Message-ID: <20210714170153.Bluez.v4.1.Id7aa1152377161d17b442bf258773d9b6c624ca3@changeid> (raw)

From: Yun-Hao Chung <>

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)
0x000059f01943928a	(bluetoothd -a2dp.c:3069)
0x000059f0194377fa	(bluetoothd -sink.c:324)
0x000059f01948715a	(bluetoothd -service.c:177)
0x000059f01948d77c	(bluetoothd -device.c:5346)
0x000059f019476d14	(bluetoothd -adapter.c:7202)
0x000059f019476c3e	(bluetoothd -adapter.c:10827)
0x000059f01949d8d7	(bluetoothd -main.c:1114)		main
0x0000787b36185d74	( -libc-start.c:308)
0x000059f019433e39	(bluetoothd + 0x00026e39)		_start

Reviewed-by: Archie Pusaka <>
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_state_cb -> channel_remove -> channel_free
   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%.

Changes in v4:
- Fix compiling errors in finalize_all

Changes in v3:
- Remove the duplicated finalize_all

Changes in v2:
- Implement helper function finalize_all
- add setup_ref before finalize_all, in case the setup is freed during

 profiles/audio/a2dp.c | 40 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c
index d31ed845cbe7..86bc02994f75 100644
--- a/profiles/audio/a2dp.c
+++ b/profiles/audio/a2dp.c
@@ -404,6 +404,41 @@ static void finalize_discover(struct a2dp_setup *s)
+static gboolean finalize_all(gpointer data)
+	struct a2dp_setup *s = data;
+	struct avdtp_stream *stream = s->err ? NULL : s->stream;
+	GSList *l;
+	for (l = s->cb; l != NULL; ) {
+		struct a2dp_setup_cb *cb = l->data;
+		l = l->next;
+		if (cb->discover_cb) {
+			cb->discover_cb(s->session, s->seps,
+					error_to_errno(s->err), cb->user_data);
+		} else if (cb->select_cb) {
+			cb->select_cb(s->session, s->sep, s->caps,
+					error_to_errno(s->err), cb->user_data);
+		} else if (cb->suspend_cb) {
+			cb->suspend_cb(s->session,
+					error_to_errno(s->err), cb->user_data);
+		} else if (cb->resume_cb) {
+			cb->resume_cb(s->session,
+					error_to_errno(s->err), cb->user_data);
+		} else if (cb->config_cb) {
+			cb->config_cb(s->session, s->sep, stream,
+					error_to_errno(s->err), cb->user_data);
+		} else
+			warn("setup_cb doesn't have any callback function");
+		setup_cb_free(cb);
+	}
+	return FALSE;
 static struct a2dp_setup *find_setup_by_session(struct avdtp *session)
 	GSList *l;
@@ -1540,9 +1575,12 @@ static void channel_free(void *data)
 	setup = find_setup_by_session(chan->session);
 	if (setup) {
 		setup->chan = NULL;
+		setup_ref(setup);
+		/* Finalize pending commands before we NULL setup->session */
+		finalize_setup_errno(setup, -ENOTCONN, finalize_all, NULL);
 		setup->session = NULL;
-		finalize_setup_errno(setup, -ENOTCONN, NULL);
+		setup_unref(setup);

             reply	other threads:[~2021-07-14  9:02 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-14  9:02 Howard Chung [this message]
2021-07-14  9:36 ` [Bluez,v4] a2dp: Fix crash in channel_free while waiting cmd resp
2021-07-14 18:41   ` Luiz Augusto von Dentz

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210714170153.Bluez.v4.1.Id7aa1152377161d17b442bf258773d9b6c624ca3@changeid \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).