Fixes issues reported by Smatch static checker

Fixes issues reported by Smatch static checker

[Gustavo F. Padovan]

Issues found by smatch static check: http://smatch.sourceforge.net/

There are still 3 critical issues:

src/storage.c +773 write_trust(29) error: we previously assumed 'match' could be null.
lib/sdp.c +2052 sdp_get_lang_attr(16) warn: variable dereferenced before check 'pEncoding'
lib/sdp.c +2067 sdp_get_lang_attr(31) error: we previously assumed 'pOffset' could be null.

[PATCH 1/3] sbc: Fix redundant null check on calling free()

[Gustavo F. Padovan]

Issues found by smatch static check: http://smatch.sourceforge.net/
---
 sbc/sbc.c    |    3 +--
 sbc/sbcdec.c |    9 +++------
 2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/sbc/sbc.c b/sbc/sbc.c
index edd152f..86399dd 100644
--- a/sbc/sbc.c
+++ b/sbc/sbc.c
@@ -1138,8 +1138,7 @@ void sbc_finish(sbc_t *sbc)
 	if (!sbc)
 		return;
 
-	if (sbc->priv_alloc_base)
-		free(sbc->priv_alloc_base);
+	free(sbc->priv_alloc_base);
 
 	memset(sbc, 0, sizeof(sbc_t));
 }
diff --git a/sbc/sbcdec.c b/sbc/sbcdec.c
index 77b8aed..7008e88 100644
--- a/sbc/sbcdec.c
+++ b/sbc/sbcdec.c
@@ -259,15 +259,13 @@ int main(int argc, char *argv[])
 			break;
 
 		case 'd':
-			if (output)
-				free(output);
+			free(output);
 			output = strdup(optarg);
 			tofile = 0;
 			break;
 
 		case 'f' :
-			if (output)
-				free(output);
+			free(output);
 			output = strdup(optarg);
 			tofile = 1;
 			break;
@@ -289,8 +287,7 @@ int main(int argc, char *argv[])
 	for (i = 0; i < argc; i++)
 		decode(argv[i], output ? output : "/dev/dsp", tofile);
 
-	if (output)
-		free(output);
+	free(output);
 
 	return 0;
 }
-- 
1.7.1

[PATCH 2/3] Fix redundant null check on calling free()

[Gustavo F. Padovan]

Issues found by smatch static check: http://smatch.sourceforge.net/
---
 input/device.c     |    6 +--
 lib/sdp.c          |   89 +++++++++++++++++----------------------------------
 src/sdp-xml.c      |    8 +---
 src/sdpd-request.c |   12 ++-----
 src/storage.c      |    6 +--
 test/ipctest.c     |    6 +--
 6 files changed, 42 insertions(+), 85 deletions(-)

diff --git a/input/device.c b/input/device.c
index 8daf8b2..f06b229 100644
--- a/input/device.c
+++ b/input/device.c
@@ -596,8 +596,7 @@ failed:
 	close(req->ctrl_sock);
 
 cleanup:
-	if (req->rd_data)
-		free(req->rd_data);
+	free(req->rd_data);
 
 	g_free(req);
 }
@@ -662,8 +661,7 @@ static int hidp_add_connection(const struct input_device *idev,
 	err = ioctl_connadd(req);
 
 cleanup:
-	if (req->rd_data)
-		free(req->rd_data);
+	free(req->rd_data);
 	g_free(req);
 
 	return err;
diff --git a/lib/sdp.c b/lib/sdp.c
index ca3b4c4..79f4ae5 100644
--- a/lib/sdp.c
+++ b/lib/sdp.c
@@ -2915,11 +2915,8 @@ int sdp_device_record_register_binary(sdp_session_t *session, bdaddr_t *device,
 	}
 
 end:
-	if (req)
-		free(req);
-
-	if (rsp)
-		free(rsp);
+	free(req);
+	free(rsp);
 
 	return status;
 }
@@ -3025,11 +3022,8 @@ int sdp_device_record_unregister_binary(sdp_session_t *session, bdaddr_t *device
 		status = -1;
 	}
 end:
-	if (reqbuf)
-		free(reqbuf);
-
-	if (rspbuf)
-		free(rspbuf);
+	free(reqbuf);
+	free(rspbuf);
 
 	return status;
 }
@@ -3133,10 +3127,8 @@ int sdp_device_record_update(sdp_session_t *session, bdaddr_t *device, const sdp
 		status = -1;
 	}
 end:
-	if (reqbuf)
-		free(reqbuf);
-	if (rspbuf)
-		free(rspbuf);
+	free(reqbuf);
+	free(rspbuf);
 	return status;
 }
 
@@ -3472,10 +3464,8 @@ int sdp_service_search_req(sdp_session_t *session, const sdp_list_t *search,
 	} while (cstate);
 
 end:
-	if (reqbuf)
-		free(reqbuf);
-	if (rspbuf)
-		free(rspbuf);
+	free(reqbuf);
+	free(rspbuf);
 
 	return status;
 }
@@ -3649,12 +3639,9 @@ sdp_record_t *sdp_service_attr_req(sdp_session_t *session, uint32_t handle,
 	}
 
 end:
-	if (reqbuf)
-		free(reqbuf);
-	if (rsp_concat_buf.data)
-		free(rsp_concat_buf.data);
-	if (rspbuf)
-		free(rspbuf);
+	free(reqbuf);
+	free(rsp_concat_buf.data);
+	free(rspbuf);
 	return rec;
 }
 
@@ -3777,9 +3764,8 @@ int sdp_service_search_async(sdp_session_t *session, const sdp_list_t *search, u
 
 	t = session->priv;
 
-	/* check if the buffer is already allocated */
-	if (t->rsp_concat_buf.data)
-		free(t->rsp_concat_buf.data);
+	/* clean possible allocated buffer */
+	free(t->rsp_concat_buf.data);
 	memset(&t->rsp_concat_buf, 0, sizeof(sdp_buf_t));
 
 	if (!t->reqbuf) {
@@ -3825,10 +3811,8 @@ int sdp_service_search_async(sdp_session_t *session, const sdp_list_t *search, u
 	return 0;
 end:
 
-	if (t->reqbuf) {
-		free(t->reqbuf);
-		t->reqbuf = NULL;
-	}
+	free(t->reqbuf);
+	t->reqbuf = NULL;
 
 	return -1;
 }
@@ -3881,9 +3865,8 @@ int sdp_service_attr_async(sdp_session_t *session, uint32_t handle, sdp_attrreq_
 
 	t = session->priv;
 
-	/* check if the buffer is already allocated */
-	if (t->rsp_concat_buf.data)
-		free(t->rsp_concat_buf.data);
+	/* clean possible allocated buffer */
+	free(t->rsp_concat_buf.data);
 	memset(&t->rsp_concat_buf, 0, sizeof(sdp_buf_t));
 
 	if (!t->reqbuf) {
@@ -3939,10 +3922,8 @@ int sdp_service_attr_async(sdp_session_t *session, uint32_t handle, sdp_attrreq_
 	return 0;
 end:
 
-	if (t->reqbuf) {
-		free(t->reqbuf);
-		t->reqbuf = NULL;
-	}
+	free(t->reqbuf);
+	t->reqbuf = NULL;
 
 	return -1;
 }
@@ -3996,9 +3977,8 @@ int sdp_service_search_attr_async(sdp_session_t *session, const sdp_list_t *sear
 
 	t = session->priv;
 
-	/* check if the buffer is already allocated */
-	if (t->rsp_concat_buf.data)
-		free(t->rsp_concat_buf.data);
+	/* clean possible allocated buffer */
+	free(t->rsp_concat_buf.data);
 	memset(&t->rsp_concat_buf, 0, sizeof(sdp_buf_t));
 
 	if (!t->reqbuf) {
@@ -4058,10 +4038,8 @@ int sdp_service_search_attr_async(sdp_session_t *session, const sdp_list_t *sear
 	return 0;
 end:
 
-	if (t->reqbuf) {
-		free(t->reqbuf);
-		t->reqbuf = NULL;
-	}
+	free(t->reqbuf);
+	t->reqbuf = NULL;
 
 	return -1;
 }
@@ -4286,8 +4264,7 @@ end:
 			t->cb(pdu_id, status, pdata, size, t->udata);
 	}
 
-	if (rspbuf)
-		free(rspbuf);
+	free(rspbuf);
 
 	return err;
 }
@@ -4521,12 +4498,9 @@ int sdp_service_search_attr_req(sdp_session_t *session, const sdp_list_t *search
 		}
 	}
 end:
-	if (rsp_concat_buf.data)
-		free(rsp_concat_buf.data);
-	if (reqbuf)
-		free(reqbuf);
-	if (rspbuf)
-		free(rspbuf);
+	free(rsp_concat_buf.data);
+	free(reqbuf);
+	free(rspbuf);
 	return status;
 }
 
@@ -4557,11 +4531,9 @@ int sdp_close(sdp_session_t *session)
 	t = session->priv;
 
 	if (t) {
-		if (t->reqbuf)
-			free(t->reqbuf);
+		free(t->reqbuf);
 
-		if (t->rsp_concat_buf.data)
-			free(t->rsp_concat_buf.data);
+		free(t->rsp_concat_buf.data);
 
 		free(t);
 	}
@@ -4668,8 +4640,7 @@ fail:
 	err = errno;
 	if (session->sock >= 0)
 		close(session->sock);
-	if (session->priv)
-		free(session->priv);
+	free(session->priv);
 	free(session);
 	errno = err;
 
diff --git a/src/sdp-xml.c b/src/sdp-xml.c
index fbe608c..4b0520d 100644
--- a/src/sdp-xml.c
+++ b/src/sdp-xml.c
@@ -728,12 +728,8 @@ void sdp_xml_data_free(struct sdp_xml_data *elem)
 	if (elem->data)
 		sdp_data_free(elem->data);
 
-	if (elem->name)
-		free(elem->name);
-
-	if (elem->text)
-
-		free(elem->text);
+	free(elem->name);
+	free(elem->text);
 	free(elem);
 }
 
diff --git a/src/sdpd-request.c b/src/sdpd-request.c
index 8c88d6e..d56ffc2 100644
--- a/src/sdpd-request.c
+++ b/src/sdpd-request.c
@@ -526,8 +526,7 @@ static int service_search_req(sdp_req_t *req, sdp_buf_t *buf)
 	}
 
 done:
-	if (cstate)
-		free(cstate);
+	free(cstate);
 	if (pattern)
 		sdp_list_free(pattern, free);
 
@@ -745,8 +744,7 @@ static int service_attr_req(sdp_req_t *req, sdp_buf_t *buf)
 	buf->buf_size += sizeof(uint16_t);
 
 done:
-	if (cstate)
-		free(cstate);
+	free(cstate);
 	if (seq)
 		sdp_list_free(seq, free);
 	if (status)
@@ -929,10 +927,8 @@ static int service_search_attr_req(sdp_req_t *req, sdp_buf_t *buf)
 	}
 
 done:
-	if (cstate)
-		free(cstate);
-	if (tmpbuf.data)
-		free(tmpbuf.data);
+	free(cstate);
+	free(tmpbuf.data);
 	if (pattern)
 		sdp_list_free(pattern, free);
 	if (seq)
diff --git a/src/storage.c b/src/storage.c
index ed1e5ec..07c1ac2 100644
--- a/src/storage.c
+++ b/src/storage.c
@@ -763,8 +763,7 @@ int write_trust(const char *src, const char *addr, const char *service,
 	/* If the old setting is the same as the requested one, we're done */
 	if (trusted == trust) {
 		g_slist_free(services);
-		if (str)
-			free(str);
+		free(str);
 		return 0;
 	}
 
@@ -784,8 +783,7 @@ int write_trust(const char *src, const char *addr, const char *service,
 
 	g_slist_free(services);
 
-	if (str)
-		free(str);
+	free(str);
 
 	return ret;
 }
diff --git a/test/ipctest.c b/test/ipctest.c
index 1a7543b..9fdfac4 100644
--- a/test/ipctest.c
+++ b/test/ipctest.c
@@ -1029,8 +1029,7 @@ static gboolean input_cb(GIOChannel *gin, GIOCondition condition, gpointer data)
 		if (sscanf(line, "%*s %as", &address) != 1)
 			DBG("set with bdaddr BDADDR");
 
-		if (u->address)
-			free(u->address);
+		free(u->address);
 
 		u->address = address;
 		DBG("bdaddr %s", u->address);
@@ -1049,8 +1048,7 @@ static gboolean input_cb(GIOChannel *gin, GIOCondition condition, gpointer data)
 			DBG("set with profile [hsp|a2dp]");
 		}
 
-		if (profile)
-			free(profile);
+		free(profile);
 		DBG("profile %s", u->transport == BT_CAPABILITIES_TRANSPORT_SCO ?
 			"hsp" : "a2dp");
 	}
-- 
1.7.1

[PATCH 3/3] sdptool: Fix 2 possible NULL dereference

[Gustavo F. Padovan]

Issues found by smatch static check:
http://smatch.sourceforge.net/
---
 tools/sdptool.c |   19 +++++++++----------
 1 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/tools/sdptool.c b/tools/sdptool.c
index 6fb2e90..89a497a 100644
--- a/tools/sdptool.c
+++ b/tools/sdptool.c
@@ -513,14 +513,16 @@ static void sdp_data_printf(sdp_data_t *sdpdata, struct attrib_context *context,
  */
 static void print_tree_attr_func(void *value, void *userData)
 {
-	sdp_data_t *sdpdata = NULL;
+	sdp_data_t *sdpdata = value;
 	uint16_t attrId;
 	struct service_context *service = (struct service_context *) userData;
 	struct attrib_context context;
 	struct attrib_def *attrDef = NULL;
 	int i;
 
-	sdpdata = (sdp_data_t *)value;
+	if (!sdpdata)
+		return;
+
 	attrId = sdpdata->attrId;
 	/* Search amongst the generic attributes */
 	for (i = 0; i < attrib_max; i++)
@@ -549,10 +551,7 @@ static void print_tree_attr_func(void *value, void *userData)
 	context.attrib = attrDef;
 	context.member_index = 0;
 	/* Parse attribute members */
-	if (sdpdata)
-		sdp_data_printf(sdpdata, &context, 2);
-	else
-		printf("  NULL value\n");
+	sdp_data_printf(sdpdata, &context, 2);
 	/* Update service */
 	service->service = context.service;
 }
@@ -723,6 +722,9 @@ static void print_raw_attr_func(void *value, void *userData)
 	struct attrib_def *def = NULL;
 	int i;
 
+	if (!data)
+		return;
+
 	/* Search amongst the generic attributes */
 	for (i = 0; i < attrib_max; i++)
 		if (attrib_names[i].num == data->attrId) {
@@ -735,10 +737,7 @@ static void print_raw_attr_func(void *value, void *userData)
 	else
 		printf("\tAttribute 0x%04x\n", data->attrId);
 
-	if (data)
-		print_raw_data(data, 2);
-	else
-		printf("  NULL value\n");
+	print_raw_data(data, 2);
 }
 
 static void print_raw_attr(sdp_record_t *rec)
-- 
1.7.1

Re: [PATCH 3/3] sdptool: Fix 2 possible NULL dereference

[Johan Hedberg]

Hi Gustavo,

On Sat, Jun 05, 2010, Gustavo F. Padovan wrote:
> Issues found by smatch static check:
> http://smatch.sourceforge.net/
> ---
>  tools/sdptool.c |   19 +++++++++----------
>  1 files changed, 9 insertions(+), 10 deletions(-)

This patch is now also upstream. Thanks.

Johan

Re: [PATCH 2/3] Fix redundant null check on calling free()

[Johan Hedberg]

Hi Gustavo,

On Sat, Jun 05, 2010, Gustavo F. Padovan wrote:
> Issues found by smatch static check: http://smatch.sourceforge.net/
> ---
>  input/device.c     |    6 +--
>  lib/sdp.c          |   89 +++++++++++++++++----------------------------------
>  src/sdp-xml.c      |    8 +---
>  src/sdpd-request.c |   12 ++-----
>  src/storage.c      |    6 +--
>  test/ipctest.c     |    6 +--
>  6 files changed, 42 insertions(+), 85 deletions(-)

Both NULL-check removal patches have been pushed upstream.

Johan

Re: [PATCH 1/3] sbc: Fix redundant null check on calling free()

[Johan Hedberg]

Hi Gustavo,

On Sat, Jun 05, 2010, Gustavo F. Padovan wrote:
> Issues found by smatch static check: http://smatch.sourceforge.net/
> ---
>  sbc/sbc.c    |    3 +--
>  sbc/sbcdec.c |    9 +++------
>  2 files changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/sbc/sbc.c b/sbc/sbc.c
> index edd152f..86399dd 100644
> --- a/sbc/sbc.c
> +++ b/sbc/sbc.c
> @@ -1138,8 +1138,7 @@ void sbc_finish(sbc_t *sbc)
>  	if (!sbc)
>  		return;
>  
> -	if (sbc->priv_alloc_base)
> -		free(sbc->priv_alloc_base);
> +	free(sbc->priv_alloc_base);

I'm a bit hesitant at removing these NULL checks since older libc
versions might need them. Any comments from Marcel?

Johan

Re: [PATCH 1/3] sbc: Fix redundant null check on calling free()

[Marcel Holtmann]

Hi Johan,

> > Issues found by smatch static check: http://smatch.sourceforge.net/
> > ---
> >  sbc/sbc.c    |    3 +--
> >  sbc/sbcdec.c |    9 +++------
> >  2 files changed, 4 insertions(+), 8 deletions(-)
> > 
> > diff --git a/sbc/sbc.c b/sbc/sbc.c
> > index edd152f..86399dd 100644
> > --- a/sbc/sbc.c
> > +++ b/sbc/sbc.c
> > @@ -1138,8 +1138,7 @@ void sbc_finish(sbc_t *sbc)
> >  	if (!sbc)
> >  		return;
> >  
> > -	if (sbc->priv_alloc_base)
> > -		free(sbc->priv_alloc_base);
> > +	free(sbc->priv_alloc_base);
> 
> I'm a bit hesitant at removing these NULL checks since older libc
> versions might need them. Any comments from Marcel?

this is just fine. If you have such an old libc version that doesn't do
it, then you have a problem anyway. I am also not sure that this is a
problem with any libc version at all.

The only argument that might hold this is if you wanna compile this for
a non GNU platform. I don't even know if that is possible since I
actually don't care about non Linux platforms.

Regards

Marcel