From d19da25e55da33afaf7184efa27313bd8f2f822d Mon Sep 17 00:00:00 2001
From: borine <32966433+borine@users.noreply.github.com>
Date: Sat, 28 Sep 2024 18:45:35 +0100
Subject: [PATCH] Fix temporary deadlock in A2DP codec selection

If a D-Bus client thread locks the codec_id_mtx mutex while BlueZ is
processing a SetConfiguration request, then that can lead to the main
thread being blocked if a client calls any method that needs to read
the current codec. However, the SetConfiguration request needs the main
thread to be active to respond to the ClearConfiguration call that BlueZ
makes during its own handling of SetConfiguration. So, there is a risk
of deadlock if the client attempts to read the codec ID while
SetConfiguration is in progress. This deadlock is temporary because the
D-Bus calls have a timeout - but BlueZ handles the timeout by closing
the A2DP profile connection.

There does not appear to be any clear reason why SetConfiguration
needs to lock the client_id_mtx mutex, since it does not actually modify
the transport codec_id (a new transport instance is created instead). So
this commit avoids this deadlock simply by removing the codec_id_mtx
mutex lock from ba_transport_select_codec_a2dp().

Fixes #725
---
 src/ba-rfcomm.c    |  4 ++--
 src/ba-transport.c | 56 +++++++++++++++++++++++-----------------------
 test/test-rfcomm.c |  4 +++-
 3 files changed, 33 insertions(+), 31 deletions(-)

diff --git a/src/ba-rfcomm.c b/src/ba-rfcomm.c
index 8313f75ce..a1763b3ab 100644
--- a/src/ba-rfcomm.c
+++ b/src/ba-rfcomm.c
@@ -138,9 +138,9 @@ static void rfcomm_set_hfp_state(struct ba_rfcomm *r, enum hfp_slc_state state)
  * Finalize HFP codec selection - signal other threads. */
 static void rfcomm_finalize_codec_selection(struct ba_rfcomm *r) {
 
-	pthread_mutex_lock(&r->sco->codec_id_mtx);
+	pthread_mutex_lock(&r->sco->codec_select_client_mtx);
 	r->codec_selection_done = true;
-	pthread_mutex_unlock(&r->sco->codec_id_mtx);
+	pthread_mutex_unlock(&r->sco->codec_select_client_mtx);
 
 	pthread_cond_signal(&r->codec_selection_cond);
 
diff --git a/src/ba-transport.c b/src/ba-transport.c
index 2b627be8d..884133923 100644
--- a/src/ba-transport.c
+++ b/src/ba-transport.c
@@ -413,6 +413,7 @@ struct ba_transport *ba_transport_new_a2dp(
 		return NULL;
 
 	t->profile = profile;
+	t->codec_id = sep->config.codec_id;
 
 	pthread_cond_init(&t->a2dp.state_changed_cond, NULL);
 	t->a2dp.state = BLUEZ_A2DP_TRANSPORT_STATE_IDLE;
@@ -440,7 +441,7 @@ struct ba_transport *ba_transport_new_a2dp(
 	t->acquire = transport_acquire_bt_a2dp;
 	t->release = transport_release_bt_a2dp;
 
-	ba_transport_set_codec(t, sep->config.codec_id);
+	a2dp_transport_init(t);
 
 	storage_pcm_data_sync(&t->a2dp.pcm);
 	storage_pcm_data_sync(&t->a2dp.pcm_bc);
@@ -547,6 +548,9 @@ struct ba_transport *ba_transport_new_sco(
 		return NULL;
 
 	t->profile = profile;
+	/* In case of the HSP and HFP without codec selection support,
+	 * there is no other option than the CVSD codec. */
+	t->codec_id = HFP_CODEC_CVSD;
 
 	err |= transport_pcm_init(&t->sco.pcm_spk,
 			is_ag ? BA_TRANSPORT_PCM_MODE_SINK : BA_TRANSPORT_PCM_MODE_SOURCE,
@@ -568,10 +572,6 @@ struct ba_transport *ba_transport_new_sco(
 	t->acquire = transport_acquire_bt_sco;
 	t->release = transport_release_bt_sco;
 
-	/* In case of the HSP and HFP without codec selection support,
-	 * there is no other option than the CVSD codec. */
-	uint32_t codec_id = HFP_CODEC_CVSD;
-
 #if ENABLE_HFP_CODEC_SELECTION
 	/* Only HFP supports codec selection. */
 	if (profile & BA_TRANSPORT_PROFILE_MASK_HFP &&
@@ -580,16 +580,16 @@ struct ba_transport *ba_transport_new_sco(
 			BA_TEST_ESCO_SUPPORT(device->a)) {
 # if ENABLE_MSBC
 		if (config.hfp.codecs.msbc)
-			codec_id = HFP_CODEC_UNDEFINED;
+			t->codec_id = HFP_CODEC_UNDEFINED;
 # endif
 # if ENABLE_LC3_SWB
 		if (config.hfp.codecs.lc3_swb)
-			codec_id = HFP_CODEC_UNDEFINED;
+			t->codec_id = HFP_CODEC_UNDEFINED;
 # endif
 	}
 #endif
 
-	ba_transport_set_codec(t, codec_id);
+	sco_transport_init(t);
 
 	storage_pcm_data_sync(&t->sco.pcm_spk);
 	storage_pcm_data_sync(&t->sco.pcm_mic);
@@ -898,10 +898,10 @@ int ba_transport_select_codec_a2dp(
 		const struct a2dp_sep_config *remote_sep_cfg,
 		const void *configuration) {
 
-	if (!(t->profile & BA_TRANSPORT_PROFILE_MASK_A2DP))
-		return errno = ENOTSUP, -1;
-
-	pthread_mutex_lock(&t->codec_id_mtx);
+#if DEBUG
+	/* Assert that we were called with the codec selection mutex locked. */
+	g_assert_cmpint(pthread_mutex_trylock(&t->codec_select_client_mtx), !=, 0);
+#endif
 
 	/* the same codec with the same configuration already selected */
 	if (remote_sep_cfg->codec_id == t->codec_id &&
@@ -920,19 +920,23 @@ int ba_transport_select_codec_a2dp(
 	if (!bluez_a2dp_set_configuration(t->a2dp.bluez_dbus_sep_path,
 				remote_sep_cfg, configuration, &err)) {
 		error("Couldn't set A2DP configuration: %s", err->message);
-		pthread_mutex_unlock(&t->codec_id_mtx);
 		g_error_free(err);
 		return errno = EIO, -1;
 	}
 
 final:
-	pthread_mutex_unlock(&t->codec_id_mtx);
 	return 0;
 }
 
 int ba_transport_select_codec_sco(
 		struct ba_transport *t,
 		uint8_t codec_id) {
+
+#if DEBUG
+	/* Assert that we were called with the codec selection mutex locked. */
+	g_assert_cmpint(pthread_mutex_trylock(&t->codec_select_client_mtx), !=, 0);
+#endif
+
 	switch (t->profile) {
 	case BA_TRANSPORT_PROFILE_HFP_HF:
 	case BA_TRANSPORT_PROFILE_HFP_AG:
@@ -942,18 +946,16 @@ int ba_transport_select_codec_sco(
 		if (t->sco.rfcomm == NULL)
 			return errno = ENOTSUP, -1;
 
-		/* Lock the mutex because we are about to change the codec ID. The codec
-		 * ID itself will be set by the RFCOMM thread. The RFCOMM thread and the
-		 * current one will be synchronized by the RFCOMM codec selection
-		 * condition variable. */
-		pthread_mutex_lock(&t->codec_id_mtx);
-
 		struct ba_rfcomm * const r = t->sco.rfcomm;
 		enum ba_rfcomm_signal rfcomm_signal;
 
 		/* codec already selected, skip switching */
 		if (t->codec_id == codec_id)
-			goto final;
+			return 0;
+
+		/* The codec ID itself will be set by the RFCOMM thread. The
+		 * RFCOMM thread and the current one will be synchronized by
+		 * the RFCOMM codec selection condition variable. */
 
 		switch (codec_id) {
 		case HFP_CODEC_CVSD:
@@ -983,24 +985,22 @@ int ba_transport_select_codec_sco(
 		ba_rfcomm_send_signal(r, rfcomm_signal);
 
 		while (!r->codec_selection_done)
-			pthread_cond_wait(&r->codec_selection_cond, &t->codec_id_mtx);
+			pthread_cond_wait(&r->codec_selection_cond, &t->codec_select_client_mtx);
 
-		if (t->codec_id != codec_id) {
-			pthread_mutex_unlock(&t->codec_id_mtx);
+		if (t->codec_id != codec_id)
 			return errno = EIO, -1;
-		}
 
-final:
-		pthread_mutex_unlock(&t->codec_id_mtx);
 #else
 		(void)codec_id;
 #endif
 		return 0;
 	case BA_TRANSPORT_PROFILE_HSP_HS:
 	case BA_TRANSPORT_PROFILE_HSP_AG:
-	default:
 		return errno = ENOTSUP, -1;
+	default:
+		g_assert_not_reached();
 	}
+
 }
 
 uint32_t ba_transport_get_codec(
diff --git a/test/test-rfcomm.c b/test/test-rfcomm.c
index 4df1c1239..4374dd9fb 100644
--- a/test/test-rfcomm.c
+++ b/test/test-rfcomm.c
@@ -187,10 +187,12 @@ CK_START_TEST(test_rfcomm_hsp_hs) {
 #if ENABLE_HFP_CODEC_SELECTION
 static void *test_rfcomm_hfp_ag_switch_codecs(void *userdata) {
 	struct ba_transport *sco = userdata;
-	/* the test code rejects first codec selection request for mSBC */
+	pthread_mutex_lock(&sco->codec_select_client_mtx);
 	ck_assert_int_eq(ba_transport_select_codec_sco(sco, HFP_CODEC_CVSD), 0);
+	/* The test code rejects first codec selection request for mSBC. */
 	ck_assert_int_eq(ba_transport_select_codec_sco(sco, HFP_CODEC_MSBC), -1);
 	ck_assert_int_eq(ba_transport_select_codec_sco(sco, HFP_CODEC_MSBC), 0);
+	pthread_mutex_unlock(&sco->codec_select_client_mtx);
 	return NULL;
 }
 #endif