init and extra checking fixups

PATCH In review Steve Grubb 4 participants 6 messages Sep 26, 2009 – Oct 2, 2009

Subsystems (2)

Bluetooth drivers Library code

lore

Share & tools

lore

Steve Grubb

author

2009-09-26 16:37 UTC #0 lore raw

PATCH 1/1 init and extra checking fixups

Hello,

I was doing some code reviews of the 4.54 release and found a couple things that should be fixed up. This patch is looking at things that may not have been initialized or is doing value checks that are unnecessary.

In audio/avdtp.c, the avdtp_setconf_cmd function has an exit jump for failed. There are several calls to the goto that do not set err to something, meaning that the stack contents are what's used. I set err =0 in the beginning so that we no longer use the stack contents, but it seems like there should be some general error code that says we failed for an unspecified reason.

In audio/control.c, the function control_cb takes a variable cond and first thing it or's G_IO_IN with it and uses the result for a test in an if statement. I suspect it should be a '&' operator.

In audio/unix.c, the function a2dp_discovery_complete at line 659 has a variable ca2dp. There is a check at line 661 to see that its not NULL. It can never be NULL because its position in the unix_client structure will never let it be NULL. So, this check should be dropped.

In compat/hidd.c, there is a function do_connect with a variable, name. It could conceivably be uninitialized when used in a strcmp at line 473.

In lib/sdp.c, the function sdp_get_lang_attr has a variable pCode that is being checked for non-NULL at line 2029. It gets its value from curr_data which has to be non-NULL to enter the loop. So, checking pCode does nothing. It should be dropped.

Same file, in the function sdp_service_attr_req there is a variable rsp_concat_buf. If the allocations for reqbuf or rspbuf fail, it goes to end where it will attempt to free rsp_concat_buf.data which is whatever the stack contents are. This same issue pops up again in the function, sdp_service_search_attr_req a little farther down.

NOT FIXED - In src/sdpd-service.c, the function create_ext_inquiry_response has a variable uuid that is used at line 223 in an equality test without having been initialized. No idea what the right code for that is.

In tools/hcitool.c, around line 687, handle could conceivably be used without being initialized. I set it to 0.

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

audio/avdtp.c +1 -1

diff -urp bluez-4.54.orig/audio/avdtp.c bluez-4.54/audio/avdtp.c
@@ -1248,7 +1248,7 @@ static gboolean avdtp_setconf_cmd(struct
 	struct conf_rej rej;
 	struct avdtp_local_sep *sep;
 	struct avdtp_stream *stream;
-	uint8_t err, category = 0x00;
+	uint8_t err = 0, category = 0x00;
 	struct audio_device *dev;
 	bdaddr_t src, dst;
 	GSList *l;

audio/control.c +1 -1

diff -urp bluez-4.54.orig/audio/control.c bluez-4.54/audio/control.c
@@ -472,7 +472,7 @@ static gboolean control_cb(GIOChannel *c
 	struct avrcp_header *avrcp;
 	int ret, packet_size, operand_count, sock;
 
-	if (!(cond | G_IO_IN))
+	if (!(cond & G_IO_IN))
 		goto failed;
 
 	sock = g_io_channel_unix_get_fd(control->io);

audio/gsta2dpsink.c +1 -2

diff -urp bluez-4.54.orig/audio/gsta2dpsink.c bluez-4.54/audio/gsta2dpsink.c
@@ -143,8 +143,7 @@ remove_element_and_fail:
 	return NULL;
 
 cleanup_and_fail:
-	if (element != NULL)
-		g_object_unref(G_OBJECT(element));
+	g_object_unref(G_OBJECT(element));
 
 	return NULL;
 }

audio/unix.c +1 -2

diff -urp bluez-4.54.orig/audio/unix.c bluez-4.54/audio/unix.c
@@ -658,8 +658,7 @@ static void a2dp_discovery_complete(stru
 			struct unix_client *c = cl->data;
 			struct a2dp_data *ca2dp = &c->d.a2dp;
 
-			if (ca2dp && ca2dp->session == session &&
-					c->seid == seid) {
+			if (ca2dp->session == session && c->seid == seid) {
 				lock = c->lock;
 				break;
 			}

compat/hidd.c +1 -0

diff -urp bluez-4.54.orig/compat/hidd.c bluez-4.54/compat/hidd.c
@@ -453,6 +453,7 @@ static void do_connect(int ctl, bdaddr_t
 	int csk, isk, err;
 
 	memset(&req, 0, sizeof(req));
+	name[0] = 0;
 
 	err = get_sdp_device_info(src, dst, &req);
 	if (err < 0 && fakehid)

lib/sdp.c +3 -1

diff -urp bluez-4.54.orig/lib/sdp.c bluez-4.54/lib/sdp.c
@@ -2026,7 +2026,7 @@ int sdp_get_lang_attr(const sdp_record_t
 		sdp_data_t *pCode = curr_data;
 		sdp_data_t *pEncoding = pCode->next;
 		sdp_data_t *pOffset = pEncoding->next;
-		if (pCode && pEncoding && pOffset) {
+		if (pEncoding && pOffset) {
 			lang = malloc(sizeof(sdp_lang_attr_t));
 			lang->code_ISO639 = pCode->val.uint16;
 			lang->encoding = pEncoding->val.uint16;

@@ -3473,6 +3473,7 @@ sdp_record_t *sdp_service_attr_req(sdp_s
 	rspbuf = malloc(SDP_RSP_BUFFER_SIZE);
 	if (!reqbuf || !rspbuf) {
 		errno = ENOMEM;
+		rsp_concat_buf.data = NULL;
 		goto end;
 	}
 	memset((char *) &rsp_concat_buf, 0, sizeof(sdp_buf_t));

@@ -4293,6 +4294,7 @@ int sdp_service_search_attr_req(sdp_sess
 	if (!reqbuf || !rspbuf) {
 		errno = ENOMEM;
 		status = -1;
+		rsp_concat_buf.data = NULL;
 		goto end;
 	}

tools/hcitool.c +1 -1

diff -urp bluez-4.54.orig/tools/hcitool.c bluez-4.54/tools/hcitool.c
@@ -505,7 +505,7 @@ static void cmd_scan(int dev_id, int arg
 	uint8_t lap[3] = { 0x33, 0x8b, 0x9e };
 	int num_rsp, length, flags;
 	uint8_t cls[3], features[8];
-	uint16_t handle;
+	uint16_t handle = 0;
 	char addr[18], name[249], oui[9], *comp, *tmp;
 	struct hci_version version;
 	struct hci_dev_info di;

David Sainty

2009-09-27 00:00 UTC #1 lore raw

Quoted 4 lines · Steve Grubb

In tools/hcitool.c, around line 687, handle could conceivably be used without being initialized. I set it to 0.

Modern gcc is great at telling you when uninitialised variables might be used. But it can't do that job if you just initialise the variable at the start. The error code patch probably falls into the same category.

At the very least for this specific case, handle should be reset at the start of the loop it's used in, not just at the start of the function.

But it probably makes sense to tidy the code up a bit instead. cc is a boolean that is true iff handle is valid, so maybe some of those (handle > 0) tests should go away and be replaced by (cc).

Though this code would be a bit simpler if it didn't try to survive and continue after malloc() failures, which seems a bit excessive. An error and exit seems more reasonable from hcitool.

Quoted 13 lines

diff -urp bluez-4.54.orig/tools/hcitool.c bluez-4.54/tools/hcitool.c

tools/hcitool.c +1 -1

@@ -505,7 +505,7 @@ static void cmd_scan(int dev_id, int arg
 	uint8_t lap[3] = { 0x33, 0x8b, 0x9e };
 	int num_rsp, length, flags;
 	uint8_t cls[3], features[8];
-	uint16_t handle;
+	uint16_t handle = 0;
 	char addr[18], name[249], oui[9], *comp, *tmp;
 	struct hci_version version;
 	struct hci_dev_info di;

Luiz Augusto von Dentz

maintainer

2009-09-27 03:01 UTC #2 lore raw

In reply to Steve Grubb

Hi Steve,

audio/control.c 2009-09-26 11:24:04.000000000 -0400 static gboolean control_cb(GIOChannel *c · line 476

Quoted 10 lines · Steve Grubb

diff -urp bluez-4.54.orig/audio/control.c bluez-4.54/audio/control.c
audio/control.c 2009-09-26 11:24:04.000000000 -0400 renamed from audio/control.c 2009-09-26 08:43:56.000000000 -0400 +1 -1
@@ -472,7 +472,7 @@ static gboolean control_cb(GIOChannel *c
        struct avrcp_header *avrcp;
        int ret, packet_size, operand_count, sock;
 
-       if (!(cond | G_IO_IN))
+       if (!(cond & G_IO_IN))
                goto failed;

I don't think this is actually right, if we change it to AND operation it would mean we won't process any other error condition if it happens together with G_IO_IN, but still the OR seems wrong here, normally we use the following check which I believe is more appropriate:

	if (cond & (G_IO_HUP | G_IO_ERR))
		goto failed;

Signature · 2 lines

Luiz Augusto von Dentz
Engenheiro de Computação

Johan Hedberg

2009-09-27 08:41 UTC #3 lore raw

In reply to Steve Grubb

Hi Steve,

Quoted 10 lines · Steve Grubb

In audio/avdtp.c, the avdtp_setconf_cmd function has an exit jump for failed. There are several calls to the goto that do not set err to something, meaning that the stack contents are what's used. I set err =0 in the beginning so that we no longer use the stack contents, but it seems like there should be some general error code that says we failed for an unspecified reason.
diff -urp bluez-4.54.orig/audio/avdtp.c bluez-4.54/audio/avdtp.c

audio/avdtp.c static gboolean avdtp_setconf_cmd(struct · line 1254

Quoted 8 lines

audio/avdtp.c +1 -1

@@ -1248,7 +1248,7 @@ static gboolean avdtp_setconf_cmd(struct
 	struct conf_rej rej;
 	struct avdtp_local_sep *sep;
 	struct avdtp_stream *stream;
-	uint8_t err, category = 0x00;
+	uint8_t err = 0, category = 0x00;
 	struct audio_device *dev;
 	bdaddr_t src, dst;
 	GSList *l;

The right error for the two places is AVDTP_BAD_STATE. Setting that error before jumping to failed also means that we avoid the initialization upon declaration for the err variable.

David and Luiz commented already on a few other issues which would be good to fix too. Furthermore, don't be afraid to split your patches into smaller logical chunks. That way some of them could have been already applied and pushed upstream. And please leave out the Signed-off-by line since we don't use it in bluez.

Johan

Johan Hedberg

2009-10-02 09:30 UTC #4 lore raw

In reply to Steve Grubb

Hi,

Quoted 33 lines · Steve Grubb

I was doing some code reviews of the 4.54 release and found a couple things that should be fixed up. This patch is looking at things that may not have been initialized or is doing value checks that are unnecessary.
In audio/avdtp.c, the avdtp_setconf_cmd function has an exit jump for failed. There are several calls to the goto that do not set err to something, meaning that the stack contents are what's used. I set err =0 in the beginning so that we no longer use the stack contents, but it seems like there should be some general error code that says we failed for an unspecified reason.
In audio/control.c, the function control_cb takes a variable cond and first thing it or's G_IO_IN with it and uses the result for a test in an if statement. I suspect it should be a '&' operator.
In audio/unix.c, the function a2dp_discovery_complete at line 659 has a variable ca2dp. There is a check at line 661 to see that its not NULL. It can never be NULL because its position in the unix_client structure will never let it be NULL. So, this check should be dropped.
In compat/hidd.c, there is a function do_connect with a variable, name. It could conceivably be uninitialized when used in a strcmp at line 473.
In lib/sdp.c, the function sdp_get_lang_attr has a variable pCode that is being checked for non-NULL at line 2029. It gets its value from curr_data which has to be non-NULL to enter the loop. So, checking pCode does nothing. It should be dropped.
Same file, in the function sdp_service_attr_req there is a variable rsp_concat_buf. If the allocations for reqbuf or rspbuf fail, it goes to end where it will attempt to free rsp_concat_buf.data which is whatever the stack contents are. This same issue pops up again in the function, sdp_service_search_attr_req a little farther down.

Quoted 8 lines

NOT FIXED - In src/sdpd-service.c, the function create_ext_inquiry_response has a variable uuid that is used at line 223 in an equality test without having been initialized. No idea what the right code for that is.
In tools/hcitool.c, around line 687, handle could conceivably be used without being initialized. I set it to 0.
Signed-off-by Steve Grubb <[email protected]>

I went ahead and split up this patch myself and also did the few necessary fixups to it. The changes are now upstream.

Johan

Steve Grubb

author

2009-10-02 15:26 UTC #5 lore raw

Quoted 2 lines · Johan Hedberg

I went ahead and split up this patch myself and also did the few necessary fixups to it. The changes are now upstream.

Thanks for fixing up all 3 patches. I apologize for not being able to get to it this week.

-Steve