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

From: Adrian Bunk (bunk_at_stusta.de)
Date: 03/27/05

  • Next message: Steven Rostedt: "Re: Can't use SYSFS for "Proprietry" driver modules !!!." 
    Date:	Sun, 27 Mar 2005 23:43:23 +0200
To: Jean Delvare <khali@linux-fr.org>

    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/
  • Next message: Steven Rostedt: "Re: Can't use SYSFS for "Proprietry" driver modules !!!."

    Relevant Pages