[PATCH BlueZ 1/6] unit/test-avctp: Use tester framework

[PATCH BlueZ 1/6] unit/test-avctp: Use tester framework

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

Our tester framework actually produces much better output.
---
 unit/test-avctp.c | 63 ++++++++++++++++++++-----------------------------------
 1 file changed, 23 insertions(+), 40 deletions(-)

diff --git a/unit/test-avctp.c b/unit/test-avctp.c
index 8f7d5ad..ce52222 100644
--- a/unit/test-avctp.c
+++ b/unit/test-avctp.c
@@ -36,6 +36,7 @@
 #include <glib.h>
 
 #include "src/shared/util.h"
+#include "src/shared/tester.h"
 #include "src/log.h"
 
 #include "android/avctp.h"
@@ -52,7 +53,6 @@ struct test_data {
 };
 
 struct context {
-	GMainLoop *main_loop;
 	struct avctp *session;
 	guint source;
 	guint process;
@@ -79,14 +79,14 @@ struct context {
 		data.test_name = g_strdup(name);			\
 		data.pdu_list = g_malloc(sizeof(pdus));			\
 		memcpy(data.pdu_list, pdus, sizeof(pdus));		\
-		g_test_add_data_func(name, &data, function);		\
+		tester_add(name, &data, NULL, function, NULL);		\
 	} while (0)
 
 static void test_debug(const char *str, void *user_data)
 {
 	const char *prefix = user_data;
 
-	g_print("%s%s\n", prefix, str);
+	tester_debug("%s%s", prefix, str);
 }
 
 static void test_free(gconstpointer user_data)
@@ -97,6 +97,17 @@ static void test_free(gconstpointer user_data)
 	g_free(data->pdu_list);
 }
 
+static void destroy_context(struct context *context)
+{
+	if (context->source > 0)
+		g_source_remove(context->source);
+
+	avctp_shutdown(context->session);
+
+	test_free(context->data);
+	g_free(context);
+}
+
 static gboolean context_quit(gpointer user_data)
 {
 	struct context *context = user_data;
@@ -104,7 +115,9 @@ static gboolean context_quit(gpointer user_data)
 	if (context->process > 0)
 		g_source_remove(context->process);
 
-	g_main_loop_quit(context->main_loop);
+	destroy_context(context);
+
+	tester_test_passed();
 
 	return FALSE;
 }
@@ -119,8 +132,7 @@ static gboolean send_pdu(gpointer user_data)
 
 	len = write(context->fd, pdu->data, pdu->size);
 
-	if (g_test_verbose())
-		util_hexdump('<', pdu->data, len, test_debug, "AVCTP: ");
+	util_hexdump('<', pdu->data, len, test_debug, "AVCTP: ");
 
 	g_assert_cmpint(len, ==, pdu->size);
 
@@ -161,8 +173,7 @@ static gboolean test_handler(GIOChannel *channel, GIOCondition cond,
 
 	g_assert(len > 0);
 
-	if (g_test_verbose())
-		util_hexdump('>', buf, len, test_debug, "AVCTP: ");
+	util_hexdump('>', buf, len, test_debug, "AVCTP: ");
 
 	g_assert_cmpint(len, ==, pdu->size);
 
@@ -179,9 +190,6 @@ static struct context *create_context(uint16_t version, gconstpointer data)
 	GIOChannel *channel;
 	int err, sv[2];
 
-	context->main_loop = g_main_loop_new(NULL, FALSE);
-	g_assert(context->main_loop);
-
 	err = socketpair(AF_UNIX, SOCK_SEQPACKET | SOCK_CLOEXEC, 0, sv);
 	g_assert(err == 0);
 
@@ -207,26 +215,6 @@ static struct context *create_context(uint16_t version, gconstpointer data)
 	return context;
 }
 
-static void destroy_context(struct context *context)
-{
-	if (context->source > 0)
-		g_source_remove(context->source);
-
-	avctp_shutdown(context->session);
-
-	g_main_loop_unref(context->main_loop);
-
-	test_free(context->data);
-	g_free(context);
-}
-
-static void execute_context(struct context *context)
-{
-	g_main_loop_run(context->main_loop);
-
-	destroy_context(context);
-}
-
 static ssize_t handler(struct avctp *session,
 					uint8_t transaction, uint8_t *code,
 					uint8_t *subunit, uint8_t *operands,
@@ -266,8 +254,6 @@ static void test_client(gconstpointer data)
 
 	avctp_send_vendor_req(context->session, AVC_CTYPE_CONTROL, 0, NULL,
 						0, handler_response, context);
-
-	execute_context(context);
 }
 
 static void test_server(gconstpointer data)
@@ -284,23 +270,20 @@ static void test_server(gconstpointer data)
 	}
 
 	g_idle_add(send_pdu, context);
-
-	execute_context(context);
 }
 
 static void test_dummy(gconstpointer data)
 {
 	struct context *context = create_context(0x0100, data);
 
-	destroy_context(context);
+	context_quit(context);
 }
 
 int main(int argc, char *argv[])
 {
-	g_test_init(&argc, &argv, NULL);
+	tester_init(&argc, &argv);
 
-	if (g_test_verbose())
-		__btd_log_init("*", 0);
+	__btd_log_init("*", 0);
 
 	/* Connection Channel Management tests */
 
@@ -335,5 +318,5 @@ int main(int argc, char *argv[])
 				raw_pdu(0x00, 0xff, 0xff, 0x00, 0x00, 0x00),
 				raw_pdu(0x03, 0xff, 0xff));
 
-	return g_test_run();
+	return tester_run();
 }
-- 
2.1.0

[PATCH BlueZ 2/6] android/avctp: Fix crashes caused by re-entrant calls

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

This make sure that while processing a PDU the session callbacks are
not able to destroy the session causing crashes.
---
 android/avctp.c | 74 ++++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 50 insertions(+), 24 deletions(-)

diff --git a/android/avctp.c b/android/avctp.c
index 758dbd4..0d92a81 100644
--- a/android/avctp.c
+++ b/android/avctp.c
@@ -162,6 +162,7 @@ struct key_pressed {
 };
 
 struct avctp {
+	unsigned int ref;
 	int uinput;
 
 	unsigned int passthrough_id;
@@ -708,6 +709,47 @@ static gboolean process_queue(void *user_data)
 
 }
 
+static struct avctp *avctp_ref(struct avctp *session)
+{
+	__sync_fetch_and_add(&session->ref, 1);
+
+	DBG("%p: ref=%d", session, session->ref);
+
+	return session;
+}
+
+static void avctp_unref(struct avctp *session)
+{
+	if (__sync_sub_and_fetch(&session->ref, 1))
+		return;
+
+	DBG("%p: ref=%d", session, session->ref);
+
+	if (session->browsing)
+		avctp_channel_destroy(session->browsing);
+
+	if (session->control)
+		avctp_channel_destroy(session->control);
+
+	if (session->destroy)
+		session->destroy(session->data);
+
+	g_free(session->handler);
+
+	if (session->key.timer > 0)
+		g_source_remove(session->key.timer);
+
+	if (session->uinput >= 0) {
+		DBG("AVCTP: closing uinput");
+
+		ioctl(session->uinput, UI_DEV_DESTROY);
+		close(session->uinput);
+		session->uinput = -1;
+	}
+
+	g_free(session);
+}
+
 static void control_response(struct avctp_channel *control,
 					struct avctp_header *avctp,
 					struct avc_header *avc,
@@ -740,6 +782,8 @@ static void control_response(struct avctp_channel *control,
 		if (p->transaction != avctp->transaction)
 			continue;
 
+		avctp_ref(control->session);
+
 		if (req->func && req->func(control->session, avc->code,
 						avc->subunit_type,
 						operands, operand_count,
@@ -749,6 +793,8 @@ static void control_response(struct avctp_channel *control,
 		control->processed = g_slist_remove(control->processed, p);
 		pending_destroy(p, NULL);
 
+		avctp_unref(control->session);
+
 		return;
 	}
 }
@@ -784,6 +830,8 @@ static void browsing_response(struct avctp_channel *browsing,
 		if (p->transaction != avctp->transaction)
 			continue;
 
+		avctp_ref(browsing->session);
+
 		if (req->func && req->func(browsing->session, operands,
 						operand_count, req->user_data))
 			return;
@@ -1563,7 +1611,7 @@ struct avctp *avctp_new(int fd, size_t imtu, size_t omtu, uint16_t version)
 	control->watch = g_io_add_watch(session->control->io, cond,
 						(GIOFunc) session_cb, session);
 
-	return session;
+	return avctp_ref(session);
 }
 
 int avctp_connect_browsing(struct avctp *session, int fd, size_t imtu,
@@ -1599,27 +1647,5 @@ void avctp_shutdown(struct avctp *session)
 	if (!session)
 		return;
 
-	if (session->browsing)
-		avctp_channel_destroy(session->browsing);
-
-	if (session->control)
-		avctp_channel_destroy(session->control);
-
-	if (session->destroy)
-		session->destroy(session->data);
-
-	g_free(session->handler);
-
-	if (session->key.timer > 0)
-		g_source_remove(session->key.timer);
-
-	if (session->uinput >= 0) {
-		DBG("AVCTP: closing uinput");
-
-		ioctl(session->uinput, UI_DEV_DESTROY);
-		close(session->uinput);
-		session->uinput = -1;
-	}
-
-	g_free(session);
+	avctp_unref(session);
 }
-- 
2.1.0

[PATCH BlueZ 3/6] unit/test-avdtp: Use tester framework

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

Our tester framework actually produces much better output.
---
 unit/test-avdtp.c | 104 +++++++++++++++++++++---------------------------------
 1 file changed, 40 insertions(+), 64 deletions(-)

diff --git a/unit/test-avdtp.c b/unit/test-avdtp.c
index 805f08d..58628a7 100644
--- a/unit/test-avdtp.c
+++ b/unit/test-avdtp.c
@@ -37,7 +37,9 @@
 
 #include "src/shared/util.h"
 #include "src/shared/queue.h"
+#include "src/shared/tester.h"
 #include "src/log.h"
+
 #include "android/avdtp.h"
 
 #define MAX_SEID 0x3E
@@ -80,11 +82,10 @@ struct test_data {
 		data.test_name = g_strdup(name);			\
 		data.pdu_list = g_malloc(sizeof(pdus));			\
 		memcpy(data.pdu_list, pdus, sizeof(pdus));		\
-		g_test_add_data_func(name, &data, function);		\
+		tester_add(name, &data, NULL, function, NULL);		\
 	} while (0)
 
 struct context {
-	GMainLoop *main_loop;
 	struct avdtp *session;
 	struct avdtp_local_sep *sep;
 	struct avdtp_stream *stream;
@@ -103,7 +104,7 @@ static void test_debug(const char *str, void *user_data)
 {
 	const char *prefix = user_data;
 
-	g_print("%s%s\n", prefix, str);
+	tester_debug("%s%s", prefix, str);
 }
 
 static void test_free(gconstpointer user_data)
@@ -114,6 +115,26 @@ static void test_free(gconstpointer user_data)
 	g_free(data->pdu_list);
 }
 
+static void unregister_sep(void *data)
+{
+	struct avdtp_local_sep *sep = data;
+
+	/* Removed from the queue by caller */
+	avdtp_unregister_sep(NULL, sep);
+}
+
+static void destroy_context(struct context *context)
+{
+	if (context->source > 0)
+		g_source_remove(context->source);
+	avdtp_unref(context->session);
+
+	test_free(context->data);
+	queue_destroy(context->lseps, unregister_sep);
+
+	g_free(context);
+}
+
 static gboolean context_quit(gpointer user_data)
 {
 	struct context *context = user_data;
@@ -121,7 +142,9 @@ static gboolean context_quit(gpointer user_data)
 	if (context->process > 0)
 		g_source_remove(context->process);
 
-	g_main_loop_quit(context->main_loop);
+	destroy_context(context);
+
+	tester_test_passed();
 
 	return FALSE;
 }
@@ -136,8 +159,7 @@ static gboolean send_pdu(gpointer user_data)
 
 	len = write(context->fd, pdu->data, pdu->size);
 
-	if (g_test_verbose())
-		util_hexdump('<', pdu->data, len, test_debug, "AVDTP: ");
+	util_hexdump('<', pdu->data, len, test_debug, "AVDTP: ");
 
 	g_assert_cmpint(len, ==, pdu->size);
 
@@ -194,8 +216,7 @@ static gboolean test_handler(GIOChannel *channel, GIOCondition cond,
 
 	g_assert(len > 0);
 
-	if (g_test_verbose())
-		util_hexdump('>', buf, len, test_debug, "AVDTP: ");
+	util_hexdump('>', buf, len, test_debug, "AVDTP: ");
 
 	g_assert_cmpint(len, ==, pdu->size);
 
@@ -227,9 +248,6 @@ static struct context *context_new(uint16_t version, uint16_t imtu,
 	GIOChannel *channel;
 	int err, sv[2];
 
-	context->main_loop = g_main_loop_new(NULL, FALSE);
-	g_assert(context->main_loop);
-
 	err = socketpair(AF_UNIX, SOCK_SEQPACKET | SOCK_CLOEXEC, 0, sv);
 	g_assert(err == 0);
 
@@ -264,35 +282,6 @@ static struct context *create_context(uint16_t version, gconstpointer data)
 	return context_new(version, 672, 672, data);
 }
 
-static void unregister_sep(void *data)
-{
-	struct avdtp_local_sep *sep = data;
-
-	/* Removed from the queue by caller */
-	avdtp_unregister_sep(NULL, sep);
-}
-
-static void destroy_context(struct context *context)
-{
-	if (context->source > 0)
-		g_source_remove(context->source);
-	avdtp_unref(context->session);
-
-	g_main_loop_unref(context->main_loop);
-
-	test_free(context->data);
-	queue_destroy(context->lseps, unregister_sep);
-
-	g_free(context);
-}
-
-static void execute_context(struct context *context)
-{
-	g_main_loop_run(context->main_loop);
-
-	destroy_context(context);
-}
-
 static gboolean sep_getcap_ind(struct avdtp *session,
 					struct avdtp_local_sep *sep,
 					GSList **caps, uint8_t *err,
@@ -424,6 +413,11 @@ static void sep_setconf_cfm(struct avdtp *session, struct avdtp_local_sep *sep,
 	struct context *context = user_data;
 	int ret;
 
+	if (g_str_equal(context->data->test_name, "/TP/SIG/SMG/BV-09-C")) {
+		context_quit(context);
+		return;
+	}
+
 	if (g_str_equal(context->data->test_name, "/TP/SIG/SMG/BI-07-C")) {
 		g_assert(err != NULL);
 		g_assert_cmpint(avdtp_error_error_code(err), ==, 0x13);
@@ -531,8 +525,6 @@ static void test_server(gconstpointer data)
 	g_assert(sep);
 
 	g_idle_add(send_pdu, context);
-
-	execute_context(context);
 }
 
 static void test_server_1_3(gconstpointer data)
@@ -546,8 +538,6 @@ static void test_server_1_3(gconstpointer data)
 	g_assert(sep);
 
 	g_idle_add(send_pdu, context);
-
-	execute_context(context);
 }
 
 static void test_server_1_3_sink(gconstpointer data)
@@ -561,8 +551,6 @@ static void test_server_1_3_sink(gconstpointer data)
 	g_assert(sep);
 
 	g_idle_add(send_pdu, context);
-
-	execute_context(context);
 }
 
 static void test_server_0_sep(gconstpointer data)
@@ -570,8 +558,6 @@ static void test_server_0_sep(gconstpointer data)
 	struct context *context = create_context(0x0100, data);
 
 	g_idle_add(send_pdu, context);
-
-	execute_context(context);
 }
 
 static void test_server_seid(gconstpointer data)
@@ -595,7 +581,7 @@ static void test_server_seid(gconstpointer data)
 						context);
 	g_assert(!sep);
 
-	destroy_context(context);
+	context_quit(context);
 }
 
 static void test_server_seid_duplicate(gconstpointer data)
@@ -628,8 +614,6 @@ static void test_server_seid_duplicate(gconstpointer data)
 	/* Check SEID ids with DISCOVER */
 
 	g_idle_add(send_pdu, context);
-
-	execute_context(context);
 }
 
 static gboolean sep_getcap_ind_frg(struct avdtp *session,
@@ -684,8 +668,6 @@ static void test_server_frg(gconstpointer data)
 	g_assert(sep);
 
 	g_idle_add(send_pdu, context);
-
-	execute_context(context);
 }
 
 static void discover_cb(struct avdtp *session, GSList *seps,
@@ -767,8 +749,6 @@ static void test_client(gconstpointer data)
 	context->sep = sep;
 
 	avdtp_discover(context->session, discover_cb, context);
-
-	execute_context(context);
 }
 
 static void test_client_1_3(gconstpointer data)
@@ -783,8 +763,6 @@ static void test_client_1_3(gconstpointer data)
 	context->sep = sep;
 
 	avdtp_discover(context->session, discover_cb, context);
-
-	execute_context(context);
 }
 
 static void test_client_frg(gconstpointer data)
@@ -799,16 +777,13 @@ static void test_client_frg(gconstpointer data)
 	context->sep = sep;
 
 	avdtp_discover(context->session, discover_cb, context);
-
-	execute_context(context);
 }
 
 int main(int argc, char *argv[])
 {
-	g_test_init(&argc, &argv, NULL);
+	tester_init(&argc, &argv);
 
-	if (g_test_verbose())
-		__btd_log_init("*", 0);
+	__btd_log_init("*", 0);
 
 	/*
 	 * Stream Management Service
@@ -843,7 +818,8 @@ int main(int argc, char *argv[])
 			raw_pdu(0x42, 0x02, 0x01, 0x00, 0x07, 0x06, 0x00, 0x00,
 				0xff, 0xff, 0x02, 0x40),
 			raw_pdu(0x50, 0x03, 0x04, 0x04, 0x01, 0x00, 0x07, 0x06,
-				0x00, 0x00, 0x21, 0x02, 0x02, 0x20));
+				0x00, 0x00, 0x21, 0x02, 0x02, 0x20),
+			raw_pdu(0x52, 0x03));
 	define_test("/TP/SIG/SMG/BV-10-C", test_server,
 			raw_pdu(0x00, 0x01),
 			raw_pdu(0x02, 0x01, 0x04, 0x00),
@@ -1413,5 +1389,5 @@ int main(int argc, char *argv[])
 			raw_pdu(0x50, 0x0d, 0x04, 0x00, 0x00),
 			raw_pdu(0x52, 0x0d));
 
-	return g_test_run();
+	return tester_run();
 }
-- 
2.1.0

[PATCH BlueZ 4/6] android/avdtp: Fix crashes caused by re-entrant calls

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

This make sure that while processing a PDU the session callbacks are
not able to destroy the session causing crashes.
---
 android/avdtp.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/android/avdtp.c b/android/avdtp.c
index e4fd2b7..2252782 100644
--- a/android/avdtp.c
+++ b/android/avdtp.c
@@ -784,6 +784,10 @@ static void handle_unanswered_req(struct avdtp *session,
 	struct avdtp_local_sep *lsep;
 	struct avdtp_error err;
 
+	if (!session->req->timeout)
+		/* Request is in process */
+		return;
+
 	if (session->req->signal_id == AVDTP_ABORT) {
 		/* Avoid freeing the Abort request here */
 		DBG("handle_unanswered_req: Abort req, returning");
@@ -2017,6 +2021,9 @@ static gboolean session_cb(GIOChannel *chan, GIOCondition cond,
 		break;
 	}
 
+	/* Take a reference to protect against callback destroying session */
+	avdtp_ref(session);
+
 	if (session->in.message_type == AVDTP_MSG_TYPE_COMMAND) {
 		if (!avdtp_parse_cmd(session, session->in.transaction,
 					session->in.signal_id,
@@ -2031,21 +2038,25 @@ static gboolean session_cb(GIOChannel *chan, GIOCondition cond,
 			goto next;
 		}
 
+		avdtp_unref(session);
 		return TRUE;
 	}
 
 	if (session->req == NULL) {
 		error("No pending request, ignoring message");
+		avdtp_unref(session);
 		return TRUE;
 	}
 
 	if (header->transaction != session->req->transaction) {
 		error("Transaction label doesn't match");
+		avdtp_unref(session);
 		return TRUE;
 	}
 
 	if (session->in.signal_id != session->req->signal_id) {
 		error("Response signal doesn't match");
+		avdtp_unref(session);
 		return TRUE;
 	}
 
@@ -2085,7 +2096,10 @@ next:
 	pending_req_free(session->req);
 	session->req = NULL;
 
-	process_queue(session);
+	if (session->ref > 1)
+		process_queue(session);
+
+	avdtp_unref(session);
 
 	return TRUE;
 
-- 
2.1.0

[PATCH BlueZ 5/6] android/avdtp: Fix test /TP/SIG/SMG/BI-19-C

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

Test /TP/SIG/SMG/BI-19-C was causing the following backtrace:

Invalid write of size 4
   at 0x415653: avdtp_parse_rej (avdtp.c:2801)
   by 0x415653: session_cb (avdtp.c:2067)
   by 0x4E7E7FA: g_main_context_dispatch (in /usr/lib64/libglib-2.0.so.0.4200.2)
   by 0x4E7EB97: ??? (in /usr/lib64/libglib-2.0.so.0.4200.2)
   by 0x4E7EEC1: g_main_loop_run (in /usr/lib64/libglib-2.0.so.0.4200.2)
   by 0x4189D1: tester_run (tester.c:831)
   by 0x40C8C7: main (test-avdtp.c:1392)
 Address 0x5c75b60 is 96 bytes inside a block of size 104 free'd
   at 0x4C2ACE9: free (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
   by 0x4E8450E: g_free (in /usr/lib64/libglib-2.0.so.0.4200.2)
   by 0x4101F0: stream_free (avdtp.c:697)
   by 0x4E9C7FC: g_slist_foreach (in /usr/lib64/libglib-2.0.so.0.4200.2)
   by 0x4E9C81A: g_slist_free_full (in /usr/lib64/libglib-2.0.so.0.4200.2)
   by 0x4106FF: avdtp_free (avdtp.c:974)
   by 0x4106FF: avdtp_unref (avdtp.c:1042)
   by 0x40CE18: destroy_context (test-avdtp.c:130)
   by 0x40CE18: context_quit (test-avdtp.c:145)
   by 0x40E38F: sep_start_cfm (test-avdtp.c:481)
   by 0x415652: avdtp_parse_rej (avdtp.c:2799)
   by 0x415652: session_cb (avdtp.c:2067)
   by 0x4E7E7FA: g_main_context_dispatch (in /usr/lib64/libglib-2.0.so.0.4200.2)
   by 0x4E7EB97: ??? (in /usr/lib64/libglib-2.0.so.0.4200.2)
   by 0x4E7EEC1: g_main_loop_run (in /usr/lib64/libglib-2.0.so.0.4200.2)
---
 android/avdtp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/android/avdtp.c b/android/avdtp.c
index 2252782..7dcccd0 100644
--- a/android/avdtp.c
+++ b/android/avdtp.c
@@ -2808,9 +2808,9 @@ static gboolean avdtp_parse_rej(struct avdtp *session,
 		error("START request rejected: %s (%d)",
 				avdtp_strerror(&err), err.err.error_code);
 		if (sep && sep->cfm && sep->cfm->start) {
+			stream->starting = FALSE;
 			sep->cfm->start(session, sep, stream, &err,
 					sep->user_data);
-			stream->starting = FALSE;
 		}
 		return TRUE;
 	case AVDTP_SUSPEND:
-- 
2.1.0

[PATCH BlueZ 6/6] android/avdtp: Fix test /TP/SIG/SMG/BV-09-C

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

Test /TP/SIG/SMG/BV-09-C frees the session after set_configuration is
called causing the following backtrace:

Invalid read of size 8
   at 0x412B81: avdtp_sep_set_state (avdtp.c:846)
   by 0x413F3F: avdtp_set_configuration_resp (avdtp.c:2515)
   by 0x413F3F: avdtp_parse_resp (avdtp.c:2656)
   by 0x4159B7: session_cb (avdtp.c:2057)
   by 0x4E7E7FA: g_main_context_dispatch (in /usr/lib64/libglib-2.0.so.0.4200.2)
   by 0x4E7EB97: ??? (in /usr/lib64/libglib-2.0.so.0.4200.2)
   by 0x4E7EEC1: g_main_loop_run (in /usr/lib64/libglib-2.0.so.0.4200.2)
   by 0x4189D1: tester_run (tester.c:831)
   by 0x40C8C7: main (test-avdtp.c:1392)
 Address 0x58572b8 is 8 bytes inside a block of size 64 free'd
   at 0x4C2ACE9: free (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
   by 0x4E8450E: g_free (in /usr/lib64/libglib-2.0.so.0.4200.2)
   by 0x4164B8: avdtp_unregister_sep (avdtp.c:3417)
   by 0x40CEA5: unregister_sep (test-avdtp.c:123)
   by 0x41704F: queue_remove_all (queue.c:387)
   by 0x4170B4: queue_destroy (queue.c:76)
   by 0x40CE54: destroy_context (test-avdtp.c:133)
   by 0x40CE54: context_quit (test-avdtp.c:145)
   by 0x40E74F: sep_setconf_cfm (test-avdtp.c:417)
   by 0x413F27: avdtp_set_configuration_resp (avdtp.c:2512)
   by 0x413F27: avdtp_parse_resp (avdtp.c:2656)
   by 0x4159B7: session_cb (avdtp.c:2057)
   by 0x4E7E7FA: g_main_context_dispatch (in /usr/lib64/libglib-2.0.so.0.4200.2)
   by 0x4E7EB97: ??? (in /usr/lib64/libglib-2.0.so.0.4200.2)
---
 android/avdtp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/android/avdtp.c b/android/avdtp.c
index 7dcccd0..7e61280 100644
--- a/android/avdtp.c
+++ b/android/avdtp.c
@@ -2522,12 +2522,12 @@ static gboolean avdtp_set_configuration_resp(struct avdtp *session,
 {
 	struct avdtp_local_sep *sep = stream->lsep;
 
+	avdtp_sep_set_state(session, sep, AVDTP_STATE_CONFIGURED);
+
 	if (sep->cfm && sep->cfm->set_configuration)
 		sep->cfm->set_configuration(session, sep, stream, NULL,
 						sep->user_data);
 
-	avdtp_sep_set_state(session, sep, AVDTP_STATE_CONFIGURED);
-
 	return TRUE;
 }
 
-- 
2.1.0

Re: [PATCH BlueZ 2/6] android/avctp: Fix crashes caused by re-entrant calls

[Szymon Janc]

Hi Luiz,

On Thursday 26 of March 2015 16:20:21 Luiz Augusto von Dentz wrote:
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> 
> This make sure that while processing a PDU the session callbacks are
> not able to destroy the session causing crashes.
> ---
>  android/avctp.c | 74
> ++++++++++++++++++++++++++++++++++++++------------------- 1 file changed,
> 50 insertions(+), 24 deletions(-)
> 
> diff --git a/android/avctp.c b/android/avctp.c
> index 758dbd4..0d92a81 100644
> --- a/android/avctp.c
> +++ b/android/avctp.c
> @@ -162,6 +162,7 @@ struct key_pressed {
>  };
> 
>  struct avctp {
> +	unsigned int ref;
>  	int uinput;
> 
>  	unsigned int passthrough_id;
> @@ -708,6 +709,47 @@ static gboolean process_queue(void *user_data)
> 
>  }
> 
> +static struct avctp *avctp_ref(struct avctp *session)
> +{
> +	__sync_fetch_and_add(&session->ref, 1);
> +
> +	DBG("%p: ref=%d", session, session->ref);
> +
> +	return session;
> +}
> +
> +static void avctp_unref(struct avctp *session)
> +{
> +	if (__sync_sub_and_fetch(&session->ref, 1))
> +		return;
> +
> +	DBG("%p: ref=%d", session, session->ref);

I'd print this debug before subtracting so that one can track unrefs.

> +
> +	if (session->browsing)
> +		avctp_channel_destroy(session->browsing);
> +
> +	if (session->control)
> +		avctp_channel_destroy(session->control);
> +
> +	if (session->destroy)
> +		session->destroy(session->data);
> +
> +	g_free(session->handler);
> +
> +	if (session->key.timer > 0)
> +		g_source_remove(session->key.timer);
> +
> +	if (session->uinput >= 0) {
> +		DBG("AVCTP: closing uinput");
> +
> +		ioctl(session->uinput, UI_DEV_DESTROY);
> +		close(session->uinput);
> +		session->uinput = -1;
> +	}
> +
> +	g_free(session);
> +}
> +
>  static void control_response(struct avctp_channel *control,
>  					struct avctp_header *avctp,
>  					struct avc_header *avc,
> @@ -740,6 +782,8 @@ static void control_response(struct avctp_channel
> *control, if (p->transaction != avctp->transaction)
>  			continue;
> 
> +		avctp_ref(control->session);
> +
>  		if (req->func && req->func(control->session, avc->code,
>  						avc->subunit_type,
>  						operands, operand_count,
> @@ -749,6 +793,8 @@ static void control_response(struct avctp_channel
> *control, control->processed = g_slist_remove(control->processed, p);
>  		pending_destroy(p, NULL);
> 
> +		avctp_unref(control->session);
> +
>  		return;
>  	}
>  }
> @@ -784,6 +830,8 @@ static void browsing_response(struct avctp_channel
> *browsing, if (p->transaction != avctp->transaction)
>  			continue;
> 
> +		avctp_ref(browsing->session);
> +
>  		if (req->func && req->func(browsing->session, operands,
>  						operand_count, req->user_data))
>  			return;
> @@ -1563,7 +1611,7 @@ struct avctp *avctp_new(int fd, size_t imtu, size_t
> omtu, uint16_t version) control->watch =
> g_io_add_watch(session->control->io, cond,
>  						(GIOFunc) session_cb, session);
> 
> -	return session;
> +	return avctp_ref(session);
>  }
> 
>  int avctp_connect_browsing(struct avctp *session, int fd, size_t imtu,
> @@ -1599,27 +1647,5 @@ void avctp_shutdown(struct avctp *session)
>  	if (!session)
>  		return;
> 
> -	if (session->browsing)
> -		avctp_channel_destroy(session->browsing);
> -
> -	if (session->control)
> -		avctp_channel_destroy(session->control);
> -
> -	if (session->destroy)
> -		session->destroy(session->data);
> -
> -	g_free(session->handler);
> -
> -	if (session->key.timer > 0)
> -		g_source_remove(session->key.timer);
> -
> -	if (session->uinput >= 0) {
> -		DBG("AVCTP: closing uinput");
> -
> -		ioctl(session->uinput, UI_DEV_DESTROY);
> -		close(session->uinput);
> -		session->uinput = -1;
> -	}
> -
> -	g_free(session);
> +	avctp_unref(session);
>  }

-- 
BR
Szymon Janc