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@xxxxxxx>
> 
> ---
>  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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/