[PATCH] Fix for possible null pointer dereference in node.c

[PATCH] Fix for possible null pointer dereference in node.c

[Rickard Strandqvist]

There is otherwise a risk of a possible null pointer dereference.

Was largely found by using a static code analysis program called cppcheck.

Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
---
 drivers/staging/tidspbridge/rmgr/node.c |    4 ++--
 1 fil ändrad, 2 tillägg(+), 2 borttagningar(-)

diff --git a/drivers/staging/tidspbridge/rmgr/node.c b/drivers/staging/tidspbridge/rmgr/node.c
index 9d3044a..94c2cce 100644
--- a/drivers/staging/tidspbridge/rmgr/node.c
+++ b/drivers/staging/tidspbridge/rmgr/node.c
@@ -2361,8 +2361,7 @@ static void delete_node(struct node_object *hnode,
 	struct node_taskargs task_arg_obj;
 #ifdef DSP_DMM_DEBUG
 	struct dmm_object *dmm_mgr;
-	struct proc_object *p_proc_object =
-	    (struct proc_object *)hnode->processor;
+	struct proc_object *p_proc_object;
 #endif
 	int status;
 	if (!hnode)
@@ -2431,6 +2430,7 @@ static void delete_node(struct node_object *hnode,
 							dsp_heap_res_addr,
 							pr_ctxt);
 #ifdef DSP_DMM_DEBUG
+			p_proc_object = (struct proc_object *)hnode->processor;
 			status = dmm_get_handle(p_proc_object, &dmm_mgr);
 			if (dmm_mgr)
 				dmm_mem_map_dump(dmm_mgr);
-- 
1.7.10.4

Re: [PATCH] Fix for possible null pointer dereference in node.c

[Greg Kroah-Hartman]

On Thu, May 15, 2014 at 11:53:53PM +0200, Rickard Strandqvist wrote:
> There is otherwise a risk of a possible null pointer dereference.
> 
> Was largely found by using a static code analysis program called cppcheck.
> 
> Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
> ---
>  drivers/staging/tidspbridge/rmgr/node.c |    4 ++--
>  1 fil ändrad, 2 tillägg(+), 2 borttagningar(-)

Please work a bit on your Subject lines to make it easier to figure out
where the patches belong.

For this one, it should be:
	Subject: staging: tidspridge: fix for possible null pointer dereference in node.c
or:
	Subject: staging: tidspridge: node.c: fix for possible null pointer dereference

That way people can figure out _what_ node.c file you are referring to
and to see if they care about it or not :)

thanks,

greg k-h

Re: [PATCH] Fix for possible null pointer dereference in node.c

[Rickard Strandqvist]

Hi all, and Greg!

Good idea!
I will do it in all the other mail.


Best regards
Rickard Strandqvist


2014-05-16 0:08 GMT+02:00 Greg Kroah-Hartman <gregkh@linuxfoundation.org>:
> On Thu, May 15, 2014 at 11:53:53PM +0200, Rickard Strandqvist wrote:
>> There is otherwise a risk of a possible null pointer dereference.
>>
>> Was largely found by using a static code analysis program called cppcheck.
>>
>> Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
>> ---
>>  drivers/staging/tidspbridge/rmgr/node.c |    4 ++--
>>  1 fil ändrad, 2 tillägg(+), 2 borttagningar(-)
>
> Please work a bit on your Subject lines to make it easier to figure out
> where the patches belong.
>
> For this one, it should be:
>         Subject: staging: tidspridge: fix for possible null pointer dereference in node.c
> or:
>         Subject: staging: tidspridge: node.c: fix for possible null pointer dereference
>
> That way people can figure out _what_ node.c file you are referring to
> and to see if they care about it or not :)
>
> thanks,
>
> greg k-h

Re: [PATCH] Fix for possible null pointer dereference in node.c

[Dan Carpenter]

On Thu, May 15, 2014 at 11:53:53PM +0200, Rickard Strandqvist wrote:
> There is otherwise a risk of a possible null pointer dereference.
> 

None of the callers pass in a NULL hnode so there isn't actually a NULL
dereference here.  You could just remove the check.

regards,
dan carpenter

Re: [PATCH] Fix for possible null pointer dereference in node.c

[Rickard Strandqvist]

[-- Attachment #1: Type: text/plain, Size: 509 bytes --]

Okay, everyone agrees with Dan..?

I have made a new patch, which does not check if hnode is NULL.



Best regards
Rickard Strandqvist


2014-05-16 0:43 GMT+02:00 Dan Carpenter <dan.carpenter@oracle.com>:
> On Thu, May 15, 2014 at 11:53:53PM +0200, Rickard Strandqvist wrote:
>> There is otherwise a risk of a possible null pointer dereference.
>>
>
> None of the callers pass in a NULL hnode so there isn't actually a NULL
> dereference here.  You could just remove the check.
>
> regards,
> dan carpenter
>

[-- Attachment #2: 0001-Fix-for-possible-null-pointer-dereferenc.patch --]
[-- Type: text/x-patch, Size: 1021 bytes --]

From 5986593f49044f1c4faef09b1e4db3ed8ec557d3 Mon Sep 17 00:00:00 2001
From: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
Date: Sun, 11 May 2014 18:51:42 +0200
Subject: [PATCH] staging: tidspridge: node.c: fix for possible null pointer dereference

Removde unnecessary check for null pointer.

Was largely found by using a static code analysis program called cppcheck.

Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
---
 drivers/staging/tidspbridge/rmgr/node.c |    2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/staging/tidspbridge/rmgr/node.c b/drivers/staging/tidspbridge/rmgr/node.c
index 9d3044a..d918f7f 100644
--- a/drivers/staging/tidspbridge/rmgr/node.c
+++ b/drivers/staging/tidspbridge/rmgr/node.c
@@ -2365,8 +2365,6 @@ static void delete_node(struct node_object *hnode,
 	    (struct proc_object *)hnode->processor;
 #endif
 	int status;
-	if (!hnode)
-		goto func_end;
 	hnode_mgr = hnode->node_mgr;
 	if (!hnode_mgr)
 		goto func_end;
-- 
1.7.10.4

Re: [PATCH] Fix for possible null pointer dereference in node.c

[Dan Carpenter]

On Sat, May 17, 2014 at 03:21:56PM +0200, Rickard Strandqvist wrote:
> Okay, everyone agrees with Dan..?
> 

You can audit it yourself, there are only 4 callers.  The
node_allocate() is total garbage so the second delete_node() is a bit
complicated to read but it's not that hard.

> I have made a new patch, which does not check if hnode is NULL.

Send it inline so we can use our normal scripts.

regards,
dan carpenter

Re: [PATCH] Fix for possible null pointer dereference in node.c

[Rickard Strandqvist]

Hi

When I run cppcheck find the about 5,000 errors, of these, I selected
the 100 most serious and has made patches. But my ability to immerse
myself in each failure has been limited unfortunately.

Okay, I send it through the scripts instead.


Best regards
Rickard Strandqvist


2014-05-17 20:56 GMT+02:00 Dan Carpenter <dan.carpenter@oracle.com>:
> On Sat, May 17, 2014 at 03:21:56PM +0200, Rickard Strandqvist wrote:
>> Okay, everyone agrees with Dan..?
>>
>
> You can audit it yourself, there are only 4 callers.  The
> node_allocate() is total garbage so the second delete_node() is a bit
> complicated to read but it's not that hard.
>
>> I have made a new patch, which does not check if hnode is NULL.
>
> Send it inline so we can use our normal scripts.
>
> regards,
> dan carpenter
>

Re: [PATCH] Fix for possible null pointer dereference in node.c

[Dan Carpenter]

On Sun, May 18, 2014 at 05:49:52PM +0200, Rickard Strandqvist wrote:
> Hi
> 
> When I run cppcheck find the about 5,000 errors, of these, I selected
> the 100 most serious and has made patches. But my ability to immerse
> myself in each failure has been limited unfortunately.
> 

I am familiar with that feeling.  ;)

With Smatch, I ignore inconsistent NULL checking like this when we know
that the parameter is always non-NULL.  It was just too much hassle to
deal with.

These days I tend to report more bugs instead of fixing them myself.  I
run on linux-next and complain to the original author as soon as the
buggy code is merged.  If the code is old, then my experience is that no
one will fix it.

Also 5000 inconsistent NULL checks seems like too high of a figure.  For
my inconsistent NULL checks I get 215 of these warnings:

	sound/i2c/other/ak4xxx-adda.c:808 build_adc_controls()
	error: we previously assumed 'ak->adc_info' could be null (see line 789)

And 75 of these:

	fs/efs/inode.c:298 efs_map_block()
	warn: variable dereferenced before check 'bh' (see line 292)

regards,
dan carpenter

Re: [PATCH] Fix for possible null pointer dereference in node.c

[Rickard Strandqvist]

Hi

The approximately 5,000 errors were not only null pointers, but
uninitialized variables, etc. also a mixture of error and warnings.
Same I did not consider a reall error. Then a bunch of warnings for
printf("%d", unsigned int)  and vice versa, which I have not even
bothered to check even.

Regarding the possible use of NULL pointer that I started with, so
reacts cppcheck that there is a check of NULL in some places, but not
others.
As examples:

drivers/staging/tidspbridge/rmgr/node.c : 2365] ->
[drivers/staging/tidspbridge/rmgr/node.c : 2368] :  (warning) Possible
null pointer dereference :  hnode - otherwise it is redundant to check
it against null.

So suppose a lot of it stems from the old code then.


Best regards
Rickard Strandqvist


2014-05-18 18:22 GMT+02:00 Dan Carpenter <dan.carpenter@oracle.com>:
> On Sun, May 18, 2014 at 05:49:52PM +0200, Rickard Strandqvist wrote:
>> Hi
>>
>> When I run cppcheck find the about 5,000 errors, of these, I selected
>> the 100 most serious and has made patches. But my ability to immerse
>> myself in each failure has been limited unfortunately.
>>
>
> I am familiar with that feeling.  ;)
>
> With Smatch, I ignore inconsistent NULL checking like this when we know
> that the parameter is always non-NULL.  It was just too much hassle to
> deal with.
>
> These days I tend to report more bugs instead of fixing them myself.  I
> run on linux-next and complain to the original author as soon as the
> buggy code is merged.  If the code is old, then my experience is that no
> one will fix it.
>
> Also 5000 inconsistent NULL checks seems like too high of a figure.  For
> my inconsistent NULL checks I get 215 of these warnings:
>
>         sound/i2c/other/ak4xxx-adda.c:808 build_adc_controls()
>         error: we previously assumed 'ak->adc_info' could be null (see line 789)
>
> And 75 of these:
>
>         fs/efs/inode.c:298 efs_map_block()
>         warn: variable dereferenced before check 'bh' (see line 292)
>
> regards,
> dan carpenter
>