Messages in this thread | | | Date | Sun, 27 Mar 2005 23:43:23 +0200 | From | Adrian Bunk <> | Subject | Re: 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/
| |