Skip to contentSkip to conversation

misc fixups

PATCH In review Steve Grubb 3 participants 4 messages Sep 25, 2009 – Oct 2, 2009 Bluetooth drivers

lore
Share & tools
lore

Steve Grubb

author
#0 lore raw

PATCH 1/1 misc fixups

Hello,

I was doing some code reviews of the 4.54 release and found a couple things that should be fixed up. The first is that in audio/pcm_bluetooth.c, a data structure is being overrun. Because the underlying buffer is 512 bytes, no overflow really occurs. What appears to happen is too much data gets copied.

The other issue is in cups/main.c, error is a stack variable and its address cannot be NULL. So, no need to check its value.

Signed-off-by Steve Grubb <[email protected]>
audio/pcm_bluetooth.c +1 -1
diff -urp bluez-4.54.orig/audio/pcm_bluetooth.c bluez-4.54/audio/pcm_bluetooth.c
@@ -729,7 +729,7 @@ static int bluetooth_a2dp_hw_params(snd_
 	req->h.length = sizeof(*req);
 
 	memcpy(&req->codec, &a2dp->sbc_capabilities,
-			sizeof(a2dp->sbc_capabilities));
+			sizeof(req->codec));
 
 	req->codec.transport = BT_CAPABILITIES_TRANSPORT_A2DP;
 	req->codec.length = sizeof(a2dp->sbc_capabilities);
cups/main.c +3 -3
diff -urp bluez-4.54.orig/cups/main.c bluez-4.54/cups/main.c
@@ -426,7 +426,7 @@ static gboolean list_known_printers(cons
 
 	dbus_message_unref(message);
 
-	if (&error != NULL && dbus_error_is_set(&error))
+	if (dbus_error_is_set(&error))
 		return FALSE;
 
 	dbus_message_iter_init(reply, &reply_iter);

@@ -527,7 +527,7 @@ static gboolean list_printers(void)
 
 	dbus_error_init(&error);
 	hcid_exists = dbus_bus_name_has_owner(conn, "org.bluez", &error);
-	if (&error != NULL && dbus_error_is_set(&error))
+	if (dbus_error_is_set(&error))
 		return TRUE;
 
 	if (!hcid_exists)

@@ -547,7 +547,7 @@ static gboolean list_printers(void)
 
 	dbus_message_unref(message);
 
-	if (&error != NULL && dbus_error_is_set(&error)) {
+	if (dbus_error_is_set(&error)) {
 		dbus_connection_unref(conn);
 		/* No adapter */
 		return TRUE;
maintainer
#1 lore raw

Hi,

Quoted 18 lines · Steve Grubb

Hello,

I was doing some code reviews of the 4.54 release and found a couple things that should be fixed up. The first is that in audio/pcm_bluetooth.c, a data structure is being overrun. Because the underlying buffer is 512 bytes, no overflow really occurs. What appears to happen is too much data gets copied.

The other issue is in  cups/main.c, error is a stack variable and its address cannot be NULL. So, no need to check its value.

Signed-off-by Steve Grubb <[email protected]>

diff -urp bluez-4.54.orig/audio/pcm_bluetooth.c bluez-4.54/audio/pcm_bluetooth.c

audio/pcm_bluetooth.c    2009-09-25 14:35:35.000000000 -0400 static int bluetooth_a2dp_hw_params(snd_ · line 732

Quoted 5 lines
audio/pcm_bluetooth.c    2009-09-25 14:35:35.000000000 -0400 renamed from audio/pcm_bluetooth.c       2009-09-25 11:33:47.000000000 -0400 +1 -1
@@ -729,7 +729,7 @@ static int bluetooth_a2dp_hw_params(snd_
        req->h.length = sizeof(*req);
 
        memcpy(&req->codec, &a2dp->sbc_capabilities,
-                       sizeof(a2dp->sbc_capabilities));
+                       sizeof(req->codec));

Be careful that this structs are different, we really want to copy sbc codec capabilities which is used to configure latter.

Signature · 2 lines
Luiz Augusto von Dentz
Engenheiro de Computação
author
#2 lore raw
Quoted 12 lines · you
Quoted 12 lines

The first is that in audio/pcm_bluetooth.c, a data structure is being overrun. Because the underlying buffer is 512 bytes, no overflow really occurs. What appears to happen is too much data gets copied.

diff -urp bluez-4.54.orig/audio/pcm_bluetooth.c bluez-4.54/audio/pcm_bluetooth.c ---
bluez-4.54.orig/audio/pcm_bluetooth.c 2009-09-25 11:33:47.000000000
-0400 +++ bluez-4.54/audio/pcm_bluetooth.c 2009-09-25
14:35:35.000000000 -0400 @@ -729,7 +729,7 @@ static int bluetooth_a2dp_hw_params(snd_

       req->h.length = sizeof(*req);
Quoted 6 lines

Review

Quoted 3 lines
       memcpy(&req->codec, &a2dp->sbc_capabilities,
(quoted diff) +1 -1
-                       sizeof(a2dp->sbc_capabilities));
+                       sizeof(req->codec));

Be careful that this structs are different, we really want to copy sbc codec capabilities which is used to configure latter.

OK, I see the uint8_t data[0] in codec_capabilities_t which usually means data to follow. Missed that. OK, the revised patch would just drop that.

-Steve

cups/main.c +3 -3
diff -urp bluez-4.54.orig/cups/main.c bluez-4.54/cups/main.c
@@ -426,7 +426,7 @@ static gboolean list_known_printers(cons
 
 	dbus_message_unref(message);
 
-	if (&error != NULL && dbus_error_is_set(&error))
+	if (dbus_error_is_set(&error))
 		return FALSE;
 
 	dbus_message_iter_init(reply, &reply_iter);

@@ -527,7 +527,7 @@ static gboolean list_printers(void)
 
 	dbus_error_init(&error);
 	hcid_exists = dbus_bus_name_has_owner(conn, "org.bluez", &error);
-	if (&error != NULL && dbus_error_is_set(&error))
+	if (dbus_error_is_set(&error))
 		return TRUE;
 
 	if (!hcid_exists)

@@ -547,7 +547,7 @@ static gboolean list_printers(void)
 
 	dbus_message_unref(message);
 
-	if (&error != NULL && dbus_error_is_set(&error)) {
+	if (dbus_error_is_set(&error)) {
 		dbus_connection_unref(conn);
 		/* No adapter */
 		return TRUE;
#3 lore raw

Hi,

Quoted 17 lines · Steve Grubb
Quoted 16 lines · you
Quoted 15 lines

The first is that in audio/pcm_bluetooth.c, a data structure is being overrun. Because the underlying buffer is 512 bytes, no overflow really occurs. What appears to happen is too much data gets copied.

diff -urp bluez-4.54.orig/audio/pcm_bluetooth.c bluez-4.54/audio/pcm_bluetooth.c ---
bluez-4.54.orig/audio/pcm_bluetooth.c 2009-09-25 11:33:47.000000000
-0400 +++ bluez-4.54/audio/pcm_bluetooth.c 2009-09-25
14:35:35.000000000 -0400 @@ -729,7 +729,7 @@ static int bluetooth_a2dp_hw_params(snd_

       req->h.length = sizeof(*req);

       memcpy(&req->codec, &a2dp->sbc_capabilities,
(quoted diff) +1 -1
-                       sizeof(a2dp->sbc_capabilities));
+                       sizeof(req->codec));
Quoted 5 lines
Quoted 2 lines

Be careful that this structs are different, we really want to copy sbc codec capabilities which is used to configure latter.

OK, I see the uint8_t data[0] in codec_capabilities_t which usually means data to follow. Missed that. OK, the revised patch would just drop that.

This one is now also pushed upstream.

Johan