[PATCH 0/2] test returned value

[PATCH 0/2] test returned value

[Julia Lawall]

Put NULL test on the result of the previous call instead on one of its
arguments.  The complete semantic match that finds these problems is as
follows (http://coccinelle.lip6.fr/):

// <smpl>
@r@
expression *e1;
expression *e2;
identifier f;
statement S1,S2;
position p,p2;
@@

 e1 = f@p(...,e2,...);
(
if (e1 = NULL || ...) S1 else S2
|
if (e1 != NULL || ...) S1 else S2
|
if@p2 (e2 = NULL || ...) S1 else S2
|
if@p2  (e2 != NULL || ...) S1 else S2
)

@ok@
expression e1,e2;
identifier f;
statement S1,S2;
position r.p,r.p2;
@@

 e1 = f@p(...);
(
if@p2 (e2 = NULL || ...) S1 else S2
|
if@p2  (e2 != NULL || ...) S1 else S2
)

@ok1 depends on ok exists@
position r.p;
expression *r.e2;
expression e3;
identifier f,g;
@@

e2->g
... when != e2 = e3
    when != &e2
f@p

@ok2 depends on ok exists@
position r.p;
expression *r.e2;
expression e,e3;
statement S1,S2;
identifier f;
@@

(
if (e2 = NULL || ...) {... return ...;} else S2
|
if (e2 != NULL || ...) S1 else {... return ...;}
)
... when != e2 = e3
    when != &e2
e = f@p(...);

@depends on ok1 || ok2@
expression e1;
identifier f;
position r.p;
@@

* e1 = f@p(...);
// </smpl>

[PATCH 1/2] clk: versatile: test returned value

[Julia Lawall]

Put NULL test on the result of the previous call instead on one of its
arguments.  A simplified version of the semantic match that finds this
problem is as follows (http://coccinelle.lip6.fr/):

// <smpl>
r@
expression *e1;
expression *e2;
identifier f;
statement S1,S2;
@@

e1 = f(...,e2,...);
(
if (e1 = NULL || ...) S1 else S2
|
*if (e2 = NULL || ...) S1 else S2
)
// </smpl>

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
 drivers/clk/versatile/clk-versatile.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/versatile/clk-versatile.c b/drivers/clk/versatile/clk-versatile.c
index a76981e..7a4f863 100644
--- a/drivers/clk/versatile/clk-versatile.c
+++ b/drivers/clk/versatile/clk-versatile.c
@@ -69,7 +69,7 @@ static void __init cm_osc_setup(struct device_node *np,
 		struct device_node *parent;
 
 		parent = of_get_parent(np);
-		if (!np) {
+		if (!parent) {
 			pr_err("no parent on core module clock\n");
 			return;
 		}

[PATCH 2/2] staging: emxx_udc: test returned value

[Julia Lawall]

Put NULL test on the result of the previous call instead on one of its
arguments.  A simplified version of the semantic match that finds this
problem is as follows (http://coccinelle.lip6.fr/):

// <smpl>
r@
expression *e1;
expression *e2;
identifier f;
statement S1,S2;
@@

e1 = f(...,e2,...);
(
if (e1 = NULL || ...) S1 else S2
|
*if (e2 = NULL || ...) S1 else S2
)
// </smpl>

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---
 drivers/staging/emxx_udc/emxx_udc.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/emxx_udc/emxx_udc.c b/drivers/staging/emxx_udc/emxx_udc.c
index fbf82bc..7de1e9e 100644
--- a/drivers/staging/emxx_udc/emxx_udc.c
+++ b/drivers/staging/emxx_udc/emxx_udc.c
@@ -2998,7 +2998,7 @@ static void  nbu2ss_ep_fifo_flush(struct usb_ep *_ep)
 	}
 
 	ep = container_of(_ep, struct nbu2ss_ep, ep);
-	if (!_ep) {
+	if (!ep) {
 		pr_err("udc: %s, bad ep\n", __func__);
 		return;
 	}

Re: [PATCH 2/2] staging: emxx_udc: test returned value

[Dan Carpenter]

This is a clever Coccinelle check.  :)

On Sat, Apr 04, 2015 at 04:59:30PM +0200, Julia Lawall wrote:
> Put NULL test on the result of the previous call instead on one of its
> arguments.  A simplified version of the semantic match that finds this
> problem is as follows (http://coccinelle.lip6.fr/):
> 
> // <smpl>
> r@
> expression *e1;
> expression *e2;
> identifier f;
> statement S1,S2;
> @@
> 
> e1 = f(...,e2,...);
> (
> if (e1 = NULL || ...) S1 else S2
> |
> *if (e2 = NULL || ...) S1 else S2
> )
> // </smpl>
> 
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> ---
>  drivers/staging/emxx_udc/emxx_udc.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/emxx_udc/emxx_udc.c b/drivers/staging/emxx_udc/emxx_udc.c
> index fbf82bc..7de1e9e 100644
> --- a/drivers/staging/emxx_udc/emxx_udc.c
> +++ b/drivers/staging/emxx_udc/emxx_udc.c
> @@ -2998,7 +2998,7 @@ static void  nbu2ss_ep_fifo_flush(struct usb_ep *_ep)
>  	}
>  
>  	ep = container_of(_ep, struct nbu2ss_ep, ep);
> -	if (!_ep) {
> +	if (!ep) {
>  		pr_err("udc: %s, bad ep\n", __func__);
>  		return;

The better thing to do here is to remove the condition.  container_of()
takes an pointer and subtracts an offset so testing the return value
is normally meaningless.

Sometimes (and in this case) the offset is zero.  In that situation it
might make sense to check the return value but it's an ugly thing to
do because if we re-order the nbu2ss_ep struct layout so "ep" isn't the
first member then it breaks.

In this case we already verified that "_ep" is non-NULL so the check
isn't needed.

regards,
dan carpenter

Re: [PATCH 2/2] staging: emxx_udc: test returned value

[Greg Kroah-Hartman]

On Sat, Apr 04, 2015 at 04:59:30PM +0200, Julia Lawall wrote:
> Put NULL test on the result of the previous call instead on one of its
> arguments.  A simplified version of the semantic match that finds this
> problem is as follows (http://coccinelle.lip6.fr/):
> 
> // <smpl>
> r@
> expression *e1;
> expression *e2;
> identifier f;
> statement S1,S2;
> @@
> 
> e1 = f(...,e2,...);
> (
> if (e1 = NULL || ...) S1 else S2
> |
> *if (e2 = NULL || ...) S1 else S2
> )
> // </smpl>
> 
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> ---
>  drivers/staging/emxx_udc/emxx_udc.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/emxx_udc/emxx_udc.c b/drivers/staging/emxx_udc/emxx_udc.c
> index fbf82bc..7de1e9e 100644
> --- a/drivers/staging/emxx_udc/emxx_udc.c
> +++ b/drivers/staging/emxx_udc/emxx_udc.c
> @@ -2998,7 +2998,7 @@ static void  nbu2ss_ep_fifo_flush(struct usb_ep *_ep)
>  	}
>  
>  	ep = container_of(_ep, struct nbu2ss_ep, ep);
> -	if (!_ep) {
> +	if (!ep) {

This is actually even worse, container_of() can't return NULL.  Or if it
does, something is really wrong (it can only happen if the field happens
to be the first field in the structure and the original pointer was
NULL).  So I would say that all tests for container_of (and
functions/macros that are just wrappers around container_of()) can just
be deleted as they will never be triggered.

Not to say that this patch is wrong at all, I'll go apply it, and you
should add it to the lists of tests in the kernel source, but you should
also consider making a test to catch container_of() results.

Hm, now that I know coccinelle, I guess I should be able to do this :)

thanks,

greg k-h

Re: [PATCH 2/2] staging: emxx_udc: test returned value

[Greg Kroah-Hartman]

On Sat, Apr 04, 2015 at 06:59:58PM +0300, Dan Carpenter wrote:
> This is a clever Coccinelle check.  :)
> 
> On Sat, Apr 04, 2015 at 04:59:30PM +0200, Julia Lawall wrote:
> > Put NULL test on the result of the previous call instead on one of its
> > arguments.  A simplified version of the semantic match that finds this
> > problem is as follows (http://coccinelle.lip6.fr/):
> > 
> > // <smpl>
> > r@
> > expression *e1;
> > expression *e2;
> > identifier f;
> > statement S1,S2;
> > @@
> > 
> > e1 = f(...,e2,...);
> > (
> > if (e1 = NULL || ...) S1 else S2
> > |
> > *if (e2 = NULL || ...) S1 else S2
> > )
> > // </smpl>
> > 
> > Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
> > 
> > ---
> >  drivers/staging/emxx_udc/emxx_udc.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/staging/emxx_udc/emxx_udc.c b/drivers/staging/emxx_udc/emxx_udc.c
> > index fbf82bc..7de1e9e 100644
> > --- a/drivers/staging/emxx_udc/emxx_udc.c
> > +++ b/drivers/staging/emxx_udc/emxx_udc.c
> > @@ -2998,7 +2998,7 @@ static void  nbu2ss_ep_fifo_flush(struct usb_ep *_ep)
> >  	}
> >  
> >  	ep = container_of(_ep, struct nbu2ss_ep, ep);
> > -	if (!_ep) {
> > +	if (!ep) {
> >  		pr_err("udc: %s, bad ep\n", __func__);
> >  		return;
> 
> The better thing to do here is to remove the condition.  container_of()
> takes an pointer and subtracts an offset so testing the return value
> is normally meaningless.
> 
> Sometimes (and in this case) the offset is zero.  In that situation it
> might make sense to check the return value but it's an ugly thing to
> do because if we re-order the nbu2ss_ep struct layout so "ep" isn't the
> first member then it breaks.
> 
> In this case we already verified that "_ep" is non-NULL so the check
> isn't needed.

This will teach me to read ahead in an email thread... :)

Re: [PATCH 2/2] staging: emxx_udc: test returned value

[Julia Lawall]

On Sat, 4 Apr 2015, Greg Kroah-Hartman wrote:

> On Sat, Apr 04, 2015 at 04:59:30PM +0200, Julia Lawall wrote:
> > Put NULL test on the result of the previous call instead on one of its
> > arguments.  A simplified version of the semantic match that finds this
> > problem is as follows (http://coccinelle.lip6.fr/):
> > 
> > // <smpl>
> > r@
> > expression *e1;
> > expression *e2;
> > identifier f;
> > statement S1,S2;
> > @@
> > 
> > e1 = f(...,e2,...);
> > (
> > if (e1 = NULL || ...) S1 else S2
> > |
> > *if (e2 = NULL || ...) S1 else S2
> > )
> > // </smpl>
> > 
> > Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
> > 
> > ---
> >  drivers/staging/emxx_udc/emxx_udc.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/staging/emxx_udc/emxx_udc.c b/drivers/staging/emxx_udc/emxx_udc.c
> > index fbf82bc..7de1e9e 100644
> > --- a/drivers/staging/emxx_udc/emxx_udc.c
> > +++ b/drivers/staging/emxx_udc/emxx_udc.c
> > @@ -2998,7 +2998,7 @@ static void  nbu2ss_ep_fifo_flush(struct usb_ep *_ep)
> >  	}
> >  
> >  	ep = container_of(_ep, struct nbu2ss_ep, ep);
> > -	if (!_ep) {
> > +	if (!ep) {
> 
> This is actually even worse, container_of() can't return NULL.  Or if it
> does, something is really wrong (it can only happen if the field happens
> to be the first field in the structure and the original pointer was
> NULL).  So I would say that all tests for container_of (and
> functions/macros that are just wrappers around container_of()) can just
> be deleted as they will never be triggered.

Couldn't one say:

x = NULL;
y = &x->whatever;
z = container_of(y, struct blah, whatever);

and end up with z being NULL?

> Not to say that this patch is wrong at all, I'll go apply it, and you
> should add it to the lists of tests in the kernel source, but you should
> also consider making a test to catch container_of() results.
> 
> Hm, now that I know coccinelle, I guess I should be able to do this :)

Yes indeed :)

julia

Re: [PATCH 2/2] staging: emxx_udc: test returned value

[Greg Kroah-Hartman]

On Sat, Apr 04, 2015 at 06:20:53PM +0200, Julia Lawall wrote:
> 
> 
> On Sat, 4 Apr 2015, Greg Kroah-Hartman wrote:
> 
> > On Sat, Apr 04, 2015 at 04:59:30PM +0200, Julia Lawall wrote:
> > > Put NULL test on the result of the previous call instead on one of its
> > > arguments.  A simplified version of the semantic match that finds this
> > > problem is as follows (http://coccinelle.lip6.fr/):
> > > 
> > > // <smpl>
> > > r@
> > > expression *e1;
> > > expression *e2;
> > > identifier f;
> > > statement S1,S2;
> > > @@
> > > 
> > > e1 = f(...,e2,...);
> > > (
> > > if (e1 = NULL || ...) S1 else S2
> > > |
> > > *if (e2 = NULL || ...) S1 else S2
> > > )
> > > // </smpl>
> > > 
> > > Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
> > > 
> > > ---
> > >  drivers/staging/emxx_udc/emxx_udc.c |    2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/staging/emxx_udc/emxx_udc.c b/drivers/staging/emxx_udc/emxx_udc.c
> > > index fbf82bc..7de1e9e 100644
> > > --- a/drivers/staging/emxx_udc/emxx_udc.c
> > > +++ b/drivers/staging/emxx_udc/emxx_udc.c
> > > @@ -2998,7 +2998,7 @@ static void  nbu2ss_ep_fifo_flush(struct usb_ep *_ep)
> > >  	}
> > >  
> > >  	ep = container_of(_ep, struct nbu2ss_ep, ep);
> > > -	if (!_ep) {
> > > +	if (!ep) {
> > 
> > This is actually even worse, container_of() can't return NULL.  Or if it
> > does, something is really wrong (it can only happen if the field happens
> > to be the first field in the structure and the original pointer was
> > NULL).  So I would say that all tests for container_of (and
> > functions/macros that are just wrappers around container_of()) can just
> > be deleted as they will never be triggered.
> 
> Couldn't one say:
> 
> x = NULL;
> y = &x->whatever;
> z = container_of(y, struct blah, whatever);
> 
> and end up with z being NULL?

Yes, if you were really lucky.  If you are passing a pointer to
container_of() it had better be checked to be NULL before, not after,
the operation, as afterward makes no sense because this is just pointer
math happening.

thanks,

greg k-h

Re: [PATCH 2/2] staging: emxx_udc: test returned value

[Dan Carpenter]

On Sat, Apr 04, 2015 at 06:20:53PM +0200, Julia Lawall wrote:
> Couldn't one say:
> 
> x = NULL;
> y = &x->whatever;
> z = container_of(y, struct blah, whatever);
> 
> and end up with z being NULL?

That is crazy person code.  It looks deliberately wrong.  If we start
merging deliberate mistakes then we're already screwed.

I have a smatch check which warns on container_of() but I should update
it to not complain if it's the first struct member.  I have looked at
quite a few of these warnings and I worry that there are some places
where it relies on container_of() to be a no-op...  That's also crazy
but it's the kind of crazy that people do in real life.  :P

regards,
dan carpenter

Re: [PATCH 2/2] staging: emxx_udc: test returned value

[Julia Lawall]

On Sat, 4 Apr 2015, Dan Carpenter wrote:

> On Sat, Apr 04, 2015 at 06:20:53PM +0200, Julia Lawall wrote:
> > Couldn't one say:
> > 
> > x = NULL;
> > y = &x->whatever;
> > z = container_of(y, struct blah, whatever);
> > 
> > and end up with z being NULL?
> 
> That is crazy person code.  It looks deliberately wrong.  If we start
> merging deliberate mistakes then we're already screwed.
> 
> I have a smatch check which warns on container_of() but I should update
> it to not complain if it's the first struct member.  I have looked at
> quite a few of these warnings and I worry that there are some places
> where it relies on container_of() to be a no-op...  That's also crazy
> but it's the kind of crazy that people do in real life.  :P

OK.  Should I update the patch to remove the test?

I find 57 NULL tests on the result of container_of in the kernel as a 
whole.

julia

Re: [PATCH 2/2] staging: emxx_udc: test returned value

[Dan Carpenter]

On Sat, Apr 04, 2015 at 07:21:51PM +0200, Julia Lawall wrote:
> 
> OK.  Should I update the patch to remove the test?
> 

I thought Greg already applied it.  If you want to send a new patch on
top which removes it that's fine.  Either way someone will eventually.

:)

regards,
dan carpenter

Re: [PATCH 1/2] clk: versatile: test returned value

[Stephen Boyd]

On 04/04/15 07:59, Julia Lawall wrote:
> Put NULL test on the result of the previous call instead on one of its
> arguments.  A simplified version of the semantic match that finds this
> problem is as follows (http://coccinelle.lip6.fr/):
>
> // <smpl>
> r@
> expression *e1;
> expression *e2;
> identifier f;
> statement S1,S2;
> @@
>
> e1 = f(...,e2,...);
> (
> if (e1 = NULL || ...) S1 else S2
> |
> *if (e2 = NULL || ...) S1 else S2
> )
> // </smpl>
>
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
>
> ---

Looks right. Linus?

>  drivers/clk/versatile/clk-versatile.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/clk/versatile/clk-versatile.c b/drivers/clk/versatile/clk-versatile.c
> index a76981e..7a4f863 100644
> --- a/drivers/clk/versatile/clk-versatile.c
> +++ b/drivers/clk/versatile/clk-versatile.c
> @@ -69,7 +69,7 @@ static void __init cm_osc_setup(struct device_node *np,
>  		struct device_node *parent;
>  
>  		parent = of_get_parent(np);
> -		if (!np) {
> +		if (!parent) {
>  			pr_err("no parent on core module clock\n");
>  			return;
>  		}
>


-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 1/2] clk: versatile: test returned value

[Linus Walleij]

On Sat, Apr 4, 2015 at 4:59 PM, Julia Lawall <Julia.Lawall@lip6.fr> wrote:

> Put NULL test on the result of the previous call instead on one of its
> arguments.  A simplified version of the semantic match that finds this
> problem is as follows (http://coccinelle.lip6.fr/):
>
> // <smpl>
> r@
> expression *e1;
> expression *e2;
> identifier f;
> statement S1,S2;
> @@
>
> e1 = f(...,e2,...);
> (
> if (e1 = NULL || ...) S1 else S2
> |
> *if (e2 = NULL || ...) S1 else S2
> )
> // </smpl>
>
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

Yep that's a bug:
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH 1/2] clk: versatile: test returned value

[Stephen Boyd]

On 04/09, Linus Walleij wrote:
> On Sat, Apr 4, 2015 at 4:59 PM, Julia Lawall <Julia.Lawall@lip6.fr> wrote:
> 
> > Put NULL test on the result of the previous call instead on one of its
> > arguments.  A simplified version of the semantic match that finds this
> > problem is as follows (http://coccinelle.lip6.fr/):
> >
> > // <smpl>
> > r@
> > expression *e1;
> > expression *e2;
> > identifier f;
> > statement S1,S2;
> > @@
> >
> > e1 = f(...,e2,...);
> > (
> > if (e1 = NULL || ...) S1 else S2
> > |
> > *if (e2 = NULL || ...) S1 else S2
> > )
> > // </smpl>
> >
> > Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> Yep that's a bug:
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
> 

Thanks. Applied to clk-next.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project