lkml.org 
[lkml]   [2005]   [Mar]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: Do not misuse Coverity please (Was: sound/oss/cs46xx.c: fix a check after use)
On Sun, Mar 27, 2005 at 11:21:58PM +0200, Jean Delvare wrote:
> Hi Adrian,
>
> > 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. Could be the case for all the bugs you are fixing right now too.
>
> [1] http://linux.bkbits.net:8080/linux-2.5/cset@1.1982.139.27
>
> Coverity and similar tools are a true opportunity for us to find out and
> study suspect parts of our code. Please do not misuse these tools! The
> goal is NOT to make the tools happy next time you run them, but to
> actually fix the problems, once and for all. If you focus too much on
> fixing the problems quickly rather than fixing them cleanly, then we
> forever lose the opportunity to clean our code, because the problems
> will then be hidden. If you look at case [1] above, you'll see that we
> were able to fix more than just what Coverity had reported.
>...

There are two cases:
1. NULL is impossible, the check is superfluous
2. this was an actual bug

In the first case, my patch doesn't do any harm (a superfluous isn't a
real bug).

In the second case, it fixed a bug.
It might be a bug not many people hit because it might be in some error
path of some esoteric driver.

If a maintainer of a well-maintained subsystem like i2c says
"The check is superfluous." that's the perfect solution.

But in less maintained parts of the kernel, even a low possibility that
it fixes a possible bug is IMHO worth making such a riskless patch.

> Thanks,
> Jean Delvare

cu
Adrian

--

"Is there not promise of rain?" Ling Tan asked suddenly out
of the darkness. There had been need of rain for many days.
"Only a promise," Lao Er said.
Pearl S. Buck - Dragon Seed

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

\
 
 \ /
  Last update: 2005-04-06 13:31    [W:0.116 / U:5.620 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site