Re: Do not misuse Coverity please (Was: sound/oss/cs46xx.c: fix acheck after use)

[Andrew Morton]

Jean Delvare <khali@xxxxxxxxxxxx> wrote:
>
> > This patch fixes a check after use found by the Coverity checker.
>  > (...)
>  >  static void amp_hercules(struct cs_card *card, int change)
>  >  {
>  > -	int old=card->amplifier;
>  > +	int old;
>  >  	if(!card)
>  >  	{
>  >  		CS_DBGOUT(CS_ERROR, 2, printk(KERN_INFO 
>  >  			"cs46xx: amp_hercules() called before initialized.\n"));
>  >  		return;
>  >  	}
>  > +	old = card->amplifier;
> 
>  I see that you are fixing many bugs like this one today, all reported by
>  Coverity. In all cases (as far as I could see at least) you are moving
>  the dereference after the check. Of course it prevents any NULL pointer
>  dereference, and will make Coverity happy. However, I doubt that this is
>  always the correct solution.
> 
>  Think about it. If the pointer could be NULL, then it's unlikely that
>  the bug would have gone unnoticed so far (unless the code is very
>  recent). Coverity found 3 such bugs in one i2c driver [1], and the
>  correct solution was to NOT check for NULL because it just couldn't
>  happen.

No, there is a third case: the pointer can be NULL, but the compiler
happened to move the dereference down to after the check.

If the optimiser is later changed, or if someone tries to compile the code
with -O0, it will oops.

-
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/