[PATCH v2] tools/sdptool: Fix NULL pointer dereference

[PATCH v2] tools/sdptool: Fix NULL pointer dereference

[Atul Rai]

This patch fixes NULL pointer dereferences in case malloc fails
and returns NULL.
---
 tools/sdptool.c | 37 +++++++++++++++++++++++++++++++++----
 1 file changed, 33 insertions(+), 4 deletions(-)

diff --git a/tools/sdptool.c b/tools/sdptool.c
index 257964d..02e7f23 100644
--- a/tools/sdptool.c
+++ b/tools/sdptool.c
@@ -902,9 +902,9 @@ static int set_attribseq(sdp_session_t *session, uint32_t handle, uint16_t attri
 	uint32_t range = 0x0000ffff;
 	sdp_record_t *rec;
 	sdp_data_t *pSequenceHolder = NULL;
-	void **dtdArray;
-	void **valueArray;
-	void **allocArray;
+	void **dtdArray = NULL;
+	void **valueArray = NULL;
+	void **allocArray = NULL;
 	uint8_t uuid16 = SDP_UUID16;
 	uint8_t uint32 = SDP_UINT32;
 	uint8_t str8 = SDP_TEXT_STR8;
@@ -922,8 +922,22 @@ static int set_attribseq(sdp_session_t *session, uint32_t handle, uint16_t attri
 
 	/* Create arrays */
 	dtdArray = (void **)malloc(argc * sizeof(void *));
+	if (!dtdArray) {
+		ret = -ENOMEM;
+		goto cleanup;
+	}
+
 	valueArray = (void **)malloc(argc * sizeof(void *));
+	if (!valueArray) {
+		ret = -ENOMEM;
+		goto cleanup;
+	}
+
 	allocArray = (void **)malloc(argc * sizeof(void *));
+	if (!allocArray) {
+		ret = -ENOMEM;
+		goto cleanup;
+	}
 
 	/* Loop on all args, add them in arrays */
 	for (i = 0; i < argc; i++) {
@@ -932,6 +946,11 @@ static int set_attribseq(sdp_session_t *session, uint32_t handle, uint16_t attri
 			/* UUID16 */
 			uint16_t value_int = strtoul((argv[i]) + 3, NULL, 16);
 			uuid_t *value_uuid = (uuid_t *) malloc(sizeof(uuid_t));
+			if (!value_uuid) {
+				ret = -ENOMEM;
+				goto cleanup;
+			}
+
 			allocArray[i] = value_uuid;
 			sdp_uuid16_create(value_uuid, value_int);
 
@@ -941,6 +960,11 @@ static int set_attribseq(sdp_session_t *session, uint32_t handle, uint16_t attri
 		} else if (!strncasecmp(argv[i], "0x", 2)) {
 			/* Int */
 			uint32_t *value_int = (uint32_t *) malloc(sizeof(int));
+			if (!value_int) {
+				ret = -ENOMEM;
+				goto cleanup;
+			}
+
 			allocArray[i] = value_int;
 			*value_int = strtoul((argv[i]) + 2, NULL, 16);
 
@@ -967,9 +991,14 @@ static int set_attribseq(sdp_session_t *session, uint32_t handle, uint16_t attri
 	} else
 		printf("Failed to create pSequenceHolder\n");
 
+cleanup:
+	if (ret == -ENOMEM)
+		printf("Memory allocation failed\n");
+
 	/* Cleanup */
 	for (i = 0; i < argc; i++)
-		free(allocArray[i]);
+		if (allocArray)
+			free(allocArray[i]);
 
 	free(dtdArray);
 	free(valueArray);
-- 
2.1.4

Re: [PATCH v2] tools/sdptool: Fix NULL pointer dereference

[Johan Hedberg]

Hi Atul,

On Tue, Jul 28, 2015, Atul Rai wrote:
> This patch fixes NULL pointer dereferences in case malloc fails
> and returns NULL.
> ---
>  tools/sdptool.c | 37 +++++++++++++++++++++++++++++++++----
>  1 file changed, 33 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/sdptool.c b/tools/sdptool.c
> index 257964d..02e7f23 100644
> --- a/tools/sdptool.c
> +++ b/tools/sdptool.c
> @@ -902,9 +902,9 @@ static int set_attribseq(sdp_session_t *session, uint32_t handle, uint16_t attri
>  	uint32_t range = 0x0000ffff;
>  	sdp_record_t *rec;
>  	sdp_data_t *pSequenceHolder = NULL;
> -	void **dtdArray;
> -	void **valueArray;
> -	void **allocArray;
> +	void **dtdArray = NULL;
> +	void **valueArray = NULL;
> +	void **allocArray = NULL;

This doesn't seem to be related to fixing missing malloc failure checks.
It's also unnecessary since all of these either way get unconditionally
assigned to before reading the values.

>  	/* Create arrays */
>  	dtdArray = (void **)malloc(argc * sizeof(void *));

While you're at it could you (in a separate patch) fix all of these
unnecessary typecasts of malloc return values?

Johan

Re: [PATCH v2] tools/sdptool: Fix NULL pointer dereference

[Szymon Janc]

Hi,

On Tuesday 28 of July 2015 10:40:56 Johan Hedberg wrote:
> Hi Atul,
> 
> On Tue, Jul 28, 2015, Atul Rai wrote:
> > This patch fixes NULL pointer dereferences in case malloc fails
> > and returns NULL.
> > ---
> > 
> >  tools/sdptool.c | 37 +++++++++++++++++++++++++++++++++----
> >  1 file changed, 33 insertions(+), 4 deletions(-)
> > 
> > diff --git a/tools/sdptool.c b/tools/sdptool.c
> > index 257964d..02e7f23 100644
> > --- a/tools/sdptool.c
> > +++ b/tools/sdptool.c
> > @@ -902,9 +902,9 @@ static int set_attribseq(sdp_session_t *session,
> > uint32_t handle, uint16_t attri> 
> >  	uint32_t range = 0x0000ffff;
> >  	sdp_record_t *rec;
> >  	sdp_data_t *pSequenceHolder = NULL;
> > 
> > -	void **dtdArray;
> > -	void **valueArray;
> > -	void **allocArray;
> > +	void **dtdArray = NULL;
> > +	void **valueArray = NULL;
> > +	void **allocArray = NULL;
> 
> This doesn't seem to be related to fixing missing malloc failure checks.
> It's also unnecessary since all of these either way get unconditionally
> assigned to before reading the values.

Those are due to 'goto cleanup' where all pointers are freed.
But we could make this code a bit simpler with:

foo = malloc();
bar = malloc();
if (!foo || !bar)
  goto cleanup;

Then initialization is not needed.

> 
> >  	/* Create arrays */
> >  	dtdArray = (void **)malloc(argc * sizeof(void *));
> 
> While you're at it could you (in a separate patch) fix all of these
> unnecessary typecasts of malloc return values?
> 
> Johan
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth"
> in the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
BR
Szymon Janc

Re: [PATCH v2] tools/sdptool: Fix NULL pointer dereference

[Johan Hedberg]

Hi Szymon,

On Tue, Jul 28, 2015, Szymon Janc wrote:
> > > -	void **dtdArray;
> > > -	void **valueArray;
> > > -	void **allocArray;
> > > +	void **dtdArray = NULL;
> > > +	void **valueArray = NULL;
> > > +	void **allocArray = NULL;
> > 
> > This doesn't seem to be related to fixing missing malloc failure checks.
> > It's also unnecessary since all of these either way get unconditionally
> > assigned to before reading the values.
> 
> Those are due to 'goto cleanup' where all pointers are freed.

Right. I was looking at the existing code and forgot that the patch adds
this label.

> But we could make this code a bit simpler with:
> 
> foo = malloc();
> bar = malloc();
> if (!foo || !bar)
>   goto cleanup;
> 
> Then initialization is not needed.

Agreed.

Johan