[PATCH] misc fixups

[PATCH] misc fixups

[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 <sgrubb@redhat.com>


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,
-			sizeof(a2dp->sbc_capabilities));
+			sizeof(req->codec));
 
 	req->codec.transport = BT_CAPABILITIES_TRANSPORT_A2DP;
 	req->codec.length = sizeof(a2dp->sbc_capabilities);
diff -urp bluez-4.54.orig/cups/main.c bluez-4.54/cups/main.c
--- bluez-4.54.orig/cups/main.c	2009-09-25 11:33:47.000000000 -0400
+++ bluez-4.54/cups/main.c	2009-09-25 14:48:46.000000000 -0400
@@ -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;

Re: [PATCH] misc fixups

[Luiz Augusto von Dentz]

Hi,

On Fri, Sep 25, 2009 at 5:36 PM, Steve Grubb <sgrubb@redhat.com> wrote:
> 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 <sgrubb@redhat.com>
>
>
> 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,
> -                       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.

-- 
Luiz Augusto von Dentz
Engenheiro de Computação

Re: [PATCH] misc fixups

[Steve Grubb]

On Saturday 26 September 2009 06:29:14 pm you wrote:
> > 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,
> > -                       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


diff -urp bluez-4.54.orig/cups/main.c bluez-4.54/cups/main.c
--- bluez-4.54.orig/cups/main.c	2009-09-25 11:33:47.000000000 -0400
+++ bluez-4.54/cups/main.c	2009-09-25 14:48:46.000000000 -0400
@@ -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;

Re: [PATCH] misc fixups

[Johan Hedberg]

Hi,

On Mon, Sep 28, 2009, Steve Grubb wrote:
> On Saturday 26 September 2009 06:29:14 pm you wrote:
> > > 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,
> > > -                       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.

This one is now also pushed upstream.

Johan