[PATCH BlueZ 1/3] audio/media: Fix notifying settings changed incorrectly

[PATCH BlueZ 1/3] audio/media: Fix notifying settings changed incorrectly

[Luiz Augusto von Dentz]

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

We should notify only the setting that has changed not all of them.
---
 profiles/audio/media.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/profiles/audio/media.c b/profiles/audio/media.c
index eb5ea81..69139a7 100644
--- a/profiles/audio/media.c
+++ b/profiles/audio/media.c
@@ -1480,7 +1480,7 @@ static gboolean set_property(struct media_player *mp, const char *key,
 
 	g_hash_table_replace(mp->settings, g_strdup(key), g_strdup(value));
 
-	settings = list_settings(mp);
+	settings = g_list_prepend(NULL, (char *) key);
 
 	avrcp_player_event(mp->player, AVRCP_EVENT_SETTINGS_CHANGED, settings);
 
-- 
1.8.1.4

[PATCH BlueZ 2/3] audio/AVRCP: Fix invalid response to RegisterNotification

[Luiz Augusto von Dentz]

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

The response to RegisterNotification for event settings changed was
not setting the initial length properly which cause the code to send
malformed/invalid PDUs.
---
 profiles/audio/avrcp.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
index 92d4527..6d1139b 100644
--- a/profiles/audio/avrcp.c
+++ b/profiles/audio/avrcp.c
@@ -1451,20 +1451,25 @@ static uint8_t avrcp_handle_register_notification(struct avrcp *session,
 		len = 1;
 		break;
 	case AVRCP_EVENT_SETTINGS_CHANGED:
+		len = 1;
 		settings = player_list_settings(player);
 
-		pdu->params[++len] = g_list_length(settings);
+		pdu->params[len++] = g_list_length(settings);
 		for (; settings; settings = settings->next) {
 			const char *key = settings->data;
-			uint8_t attr = attr_to_val(key);
+			int attr;
 			int val;
 
+			attr = attr_to_val(key);
+			if (attr < 0)
+				continue;
+
 			val = player_get_setting(player, attr);
 			if (val < 0)
 				continue;
 
-			pdu->params[++len] = attr;
-			pdu->params[++len] = val;
+			pdu->params[len++] = attr;
+			pdu->params[len++] = val;
 		}
 
 		break;
-- 
1.8.1.4

[PATCH BlueZ 3/3] core: Fix crash when a duplicated record is found

[Luiz Augusto von Dentz]

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

 Invalid read of size 8
    at 0x470101: update_bredr_services (device.c:2784)
    by 0x470591: browse_cb (device.c:2975)
    by 0x458B0E: search_completed_cb (sdp-client.c:186)
    by 0x47C154: sdp_process (sdp.c:4343)
    by 0x458954: search_process_cb (sdp-client.c:205)
    by 0x3F31A47A54: g_main_context_dispatch (in /usr/lib64/libglib-2.0.so.0.3400.2)
    by 0x3F31A47D87: ??? (in /usr/lib64/libglib-2.0.so.0.3400.2)
    by 0x3F31A48181: g_main_loop_run (in /usr/lib64/libglib-2.0.so.0.3400.2)
    by 0x40A265: main (main.c:595)
  Address 0x0 is not stack'd, malloc'd or (recently) free'd
---
 src/device.c | 65 +++++++++++++++++++++++++++++++++++-------------------------
 1 file changed, 38 insertions(+), 27 deletions(-)

diff --git a/src/device.c b/src/device.c
index 0f75c60..c324764 100644
--- a/src/device.c
+++ b/src/device.c
@@ -2677,6 +2677,40 @@ static int rec_cmp(const void *a, const void *b)
 	return r1->handle - r2->handle;
 }
 
+static int update_record(struct browse_req *req, const char *uuid,
+							sdp_record_t *rec)
+{
+	GSList *l;
+
+	/* Check for duplicates */
+	if (sdp_list_find(req->records, rec, rec_cmp))
+		return -EALREADY;
+
+	/* Copy record */
+	req->records = sdp_list_append(req->records, sdp_copy_record(rec));
+
+	/* Check if UUID is duplicated */
+	l = g_slist_find_custom(req->device->uuids, uuid, bt_uuid_strcmp);
+	if (l == NULL) {
+		l = g_slist_find_custom(req->profiles_added, uuid,
+							bt_uuid_strcmp);
+		if (l == NULL)
+			return 0;
+		req->profiles_added = g_slist_append(req->profiles_added,
+							g_strdup(uuid));
+		return 0;
+	}
+
+	l = g_slist_find_custom(req->profiles_removed, uuid, bt_uuid_strcmp);
+	if (l == NULL)
+		return 0;
+
+	g_free(l->data);
+	req->profiles_removed = g_slist_delete_link(req->profiles_removed, l);
+
+	return 0;
+}
+
 static void update_bredr_services(struct browse_req *req, sdp_list_t *recs)
 {
 	struct btd_device *device = req->device;
@@ -2712,7 +2746,6 @@ static void update_bredr_services(struct browse_req *req, sdp_list_t *recs)
 		sdp_record_t *rec = (sdp_record_t *) seq->data;
 		sdp_list_t *svcclass = NULL;
 		char *profile_uuid;
-		GSList *l;
 
 		if (!rec)
 			break;
@@ -2754,12 +2787,8 @@ static void update_bredr_services(struct browse_req *req, sdp_list_t *recs)
 							product, version);
 		}
 
-		/* Check for duplicates */
-		if (sdp_list_find(req->records, rec, rec_cmp)) {
-			g_free(profile_uuid);
-			sdp_list_free(svcclass, free);
-			continue;
-		}
+		if (update_record(req, profile_uuid, rec) < 0)
+			goto next;
 
 		if (sdp_key_file)
 			store_sdp_record(sdp_key_file, rec);
@@ -2767,26 +2796,8 @@ static void update_bredr_services(struct browse_req *req, sdp_list_t *recs)
 		if (att_key_file)
 			store_primaries_from_sdp_record(att_key_file, rec);
 
-		/* Copy record */
-		req->records = sdp_list_append(req->records,
-							sdp_copy_record(rec));
-
-		l = g_slist_find_custom(device->uuids, profile_uuid,
-							bt_uuid_strcmp);
-		if (!l)
-			req->profiles_added =
-					g_slist_append(req->profiles_added,
-							profile_uuid);
-		else {
-			l = g_slist_find_custom(req->profiles_removed,
-							profile_uuid,
-							bt_uuid_strcmp);
-			g_free(l->data);
-			req->profiles_removed =
-				g_slist_delete_link(req->profiles_removed, l);
-			g_free(profile_uuid);
-		}
-
+next:
+		g_free(profile_uuid);
 		sdp_list_free(svcclass, free);
 	}
 
-- 
1.8.1.4

Re: [PATCH BlueZ 1/3] audio/media: Fix notifying settings changed incorrectly

[Marcel Holtmann]

Hi Luiz,

> We should notify only the setting that has changed not all of them.
> ---
> profiles/audio/media.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/profiles/audio/media.c b/profiles/audio/media.c
> index eb5ea81..69139a7 100644
> --- a/profiles/audio/media.c
> +++ b/profiles/audio/media.c
> @@ -1480,7 +1480,7 @@ static gboolean set_property(struct media_player *mp, const char *key,
> 
> 	g_hash_table_replace(mp->settings, g_strdup(key), g_strdup(value));
> 
> -	settings = list_settings(mp);
> +	settings = g_list_prepend(NULL, (char *) key);

this is dangerous. Can you guarantee that the memory it points to is not used outside of this function. Seriously, making a comment here if this is safe is a good idea. If it is not safe, then it needs to be fixed.

We are using const function arguments for pointers to clearly indicate that the lifetime of that pointer is really only guaranteed inside the function.

Regards

Marcel