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
--- 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;
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;
Hi,
On Fri, Sep 25, 2009 at 5:36 PM, Steve Grubb <[email protected]> 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 <[email protected]>
>
>
> 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
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