[2.6 patch] sound/oss/cs46xx.c: fix a check after use

[2.6 patch] sound/oss/cs46xx.c: fix a check after use

[Adrian Bunk]

This patch fixes a check after use found by the Coverity checker.

Signed-off-by: Adrian Bunk <bunk@fs.tum.de>

--- linux-2.6.12-rc1-mm1-full/sound/oss/cs46xx.c.old	2005-03-23 04:48:53.000000000 +0100
+++ linux-2.6.12-rc1-mm1-full/sound/oss/cs46xx.c	2005-03-23 04:49:31.000000000 +0100
@@ -3086,13 +3086,14 @@
  
 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;
 	card->amplifier+=change;
 	if( (card->amplifier && !old) && !(hercules_egpio_disable))
 	{

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

[Jean Delvare]

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.

Considering the very important flow of patches you are sending these
days, I have to admit I am quite suspicious that you don't really
investigate all issues individually as you should, but merely want to
fix as many bugs as possible in a short amount of time. This is not,
IMVHO, what needs to be done. Of course, if you actually investigated
all these bugs and are certain that none of these checks can be removed
because pointers can actually be NULL in regular cases, then please
accept my humble apologizes and keep up with the good work.

Thanks,
-- 
Jean Delvare

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

[Adrian Bunk]

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

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

[Jean Delvare]

Hi Adrian,

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

Agreed.

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

The fact that it isn't a bug does not imply that the patch doesn't harm.
Tricking the reader into thinking that something can happen when it in
fact cannot does possibly harm in the long run.

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

True, except that e.g. the sg_mmap() function of scsi/sg hardly falls
into this category. Same for fbcon_init() in video/console/fbcon. You
don't seem to treat core code any differently than esoteric drivers.

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

This is where my opinion strongly differs.

The very fact that these "check after use" cases were not fixed yet
means that they are not hit frequently, if ever, regardless of how
popular the driver is. This means that we have (relatively) plenty of
time to fix them. At least Coverity or a similar tool will keep
reminding us to take a look and decide what must be done after we
carefully checked the code. Your approach will not let us do that.
Mass-posting these patches here is likely to make them end in Andrew's
tree soon and to Linus' right after that is nobody objects, right?

If you can make sure that none of these patches ever reaches Linus' tree
without their respective driver maintainer having confirmed that they
were the right thing to do, then fine with me, but it doesn't look like
the way things will go. I think that you'd be better just telling the
maintainers about the problem without providing an arbitrary patch, so
that they will actually look into the problem with their advanced
knowledge of the driver, rather than merely ack'ing that the patch looks
OK, which I think is what will happen with your method. (I'd love to be
proven wrong though.)

Thanks,
-- 
Jean Delvare

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

[Russell King]

On Mon, Mar 28, 2005 at 12:34:01AM +0200, Jean Delvare wrote:
> I think that you'd be better just telling the
> maintainers about the problem without providing an arbitrary patch, so
> that they will actually look into the problem with their advanced
> knowledge of the driver

FWIW, I agree with Jean.

Applying these patches without adequate review is like applying __iomem
or __user fixes where they've been generated with the aim of shutting up
sparse, rather than considering whether the warning is actually valid.
Once the warning is gone, the real problem is lost and never gets looked
at.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

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

[Matthias-Christian Ott]

Jean Delvare schrieb:

>Hi Adrian,
>
>  
>
>>There are two cases:
>>1. NULL is impossible, the check is superfluous
>>2. this was an actual bug
>>    
>>
>
>Agreed.
>
>  
>
>>In the first case, my patch doesn't do any harm (a superfluous isn't
>>a real bug).
>>    
>>
>
>The fact that it isn't a bug does not imply that the patch doesn't harm.
>Tricking the reader into thinking that something can happen when it in
>fact cannot does possibly harm in the long run.
>
>  
>
>>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.
>>    
>>
>
>True, except that e.g. the sg_mmap() function of scsi/sg hardly falls
>into this category. Same for fbcon_init() in video/console/fbcon. You
>don't seem to treat core code any differently than esoteric drivers.
>
>  
>
>>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.
>>    
>>
>
>This is where my opinion strongly differs.
>
>The very fact that these "check after use" cases were not fixed yet
>means that they are not hit frequently, if ever, regardless of how
>popular the driver is. This means that we have (relatively) plenty of
>time to fix them. At least Coverity or a similar tool will keep
>reminding us to take a look and decide what must be done after we
>carefully checked the code. Your approach will not let us do that.
>Mass-posting these patches here is likely to make them end in Andrew's
>tree soon and to Linus' right after that is nobody objects, right?
>
>If you can make sure that none of these patches ever reaches Linus' tree
>without their respective driver maintainer having confirmed that they
>were the right thing to do, then fine with me, but it doesn't look like
>the way things will go. I think that you'd be better just telling the
>maintainers about the problem without providing an arbitrary patch, so
>that they will actually look into the problem with their advanced
>knowledge of the driver, rather than merely ack'ing that the patch looks
>OK, which I think is what will happen with your method. (I'd love to be
>proven wrong though.)
>
>Thanks,
>  
>
Hi!
I fully disagree with you opinion.

   1. Adrian sent this patches to the LKML _and_ to their maintainers.
      So the patches can be proved by 2 instances and not Adrian has to
      do this (of course he has to filter non-sense patches (I   think
      he did so)).
   2. I don't know in which case NULL pointers are usefull, but it seems
      they're.
   3. Not only Maintainers have knowledge about the driver and can
      decide whether a patch is useful or not (this hierarchical
      structure is outdated (it's adapeted from the middle ages and a
      reason why many developers (like me) don't want to waste their
      time on fixing little Kernel bugs (in most cases you have to
      resend your patch several times to get it appllied))). This only
      elongates the development time and is not necessary because as
      mentioned above it's sent to the LKML and the maintainers have
      subscribed the LKML and can give their comment here and not in a
      private discussion.

Because of this I recommend everybody to use such tools and sent such 
patches, increasing the quality of the source and fixing problems like 
the problems Adrians patches fix, to the LKML.

Matthias-Christian Ott

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

[L. A. Walsh]

Adrian Bunk wrote:

>On Sun, Mar 27, 2005 at 11:21:58PM +0200, Jean Delvare wrote:
>  
>
>
>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.
>  
>
---
    I'd agree in [al]most any part of the kernel.  Unless it
is extremely time critical code, subroutines should expect
possible garbage from their callers.

    Just because it may be perfect today doesn't mean someone down
the line won't call the routine with less than perfect parameters.

    It used to be called "defensive" programming.

    However, in this case, if the author is _certain_ the
pointer can never be NULL, than an "ASSERT(card!=NULL);" might
be appropriate, where ASSERT is a macro that normally compiles
in the check, but could compile to "nothing" for embedded or
kernels that aren't being developed in.

-linda

>  
>
>>Thanks,
>>Jean Delvare
>>    
>>
>
>cu
>Adrian
>
>  
>

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

[Daniel Barkalow]

On Mon, 28 Mar 2005, L. A. Walsh wrote:

>     However, in this case, if the author is _certain_ the
> pointer can never be NULL, than an "ASSERT(card!=NULL);" might
> be appropriate, where ASSERT is a macro that normally compiles
> in the check, but could compile to "nothing" for embedded or
> kernels that aren't being developed in.

If the kernel dereferences a NULL pointer, it produces an extensive bug
report. The automatic oops is about the most useful thing to do if a
pointer is unexpectedly NULL.

	-Daniel
*This .sig left intentionally blank*

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

[Andrew Morton]

Jean Delvare <khali@linux-fr.org> 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.

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

[Jean Delvare]

Hi Andrew, all,

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

Wow. Great point. I completely missed that possibility. In fact I didn't
know that the compiler could possibly alter the order of the
instructions. For one thing, I thought it was simply not allowed to. For
another, I didn't know that it had been made so aware that it could
actually figure out how to do this kind of things. What a mess. Let's
just hope that the gcc folks know their business :)

I'll try to remember this next time I debug something. Do not assume
that instructions are run in the order seen in the source. Irk.

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

Interesting. Then wouldn't it be worth attempting such compilations at
times, and see if we get additional oops? Doesn't gcc have a flag to
specifically forbid this family of optimizations?

Thanks,
--
Jean Delvare

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

[Chris Friesen]

Jean Delvare wrote:

> Wow. Great point. I completely missed that possibility. In fact I didn't
> know that the compiler could possibly alter the order of the
> instructions. For one thing, I thought it was simply not allowed to. For
> another, I didn't know that it had been made so aware that it could
> actually figure out how to do this kind of things. What a mess. Let's
> just hope that the gcc folks know their business :)
> 
> I'll try to remember this next time I debug something. Do not assume
> that instructions are run in the order seen in the source. Irk.

It gets better, in that the cpus themselves can reorder instructions. 
This becomes interesting when dealing with memory being shared between 
multiple cpus on SMP machines.  Either you need to use existing locking 
primitives which enforce ordering or else you need to use explicit 
cpu-level ordering instructions to ensure that data gets written/read in 
the expected order.  (See "mb" and friends in the kernel code.)

Then you get into potential caching issues with memory mapped at 
different addresses on cpus with VIVT caches, and that introduces more 
issues.

Computers are perfectly predictable, as long as you understand exactly 
what you've told them to do...

Chris

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

[Daniel Jacobowitz]

On Mon, Mar 28, 2005 at 10:23:48PM -0800, Andrew Morton wrote:
> >  > -	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;

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

The thing GCC is most likely to do with this code is discard the NULL
check entirely and leave only the oops; the "if (!card)" can not be
reached without passing through "card->amplifier", and a pointer which
is dereferenced can not be NULL in a valid program.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

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

[Kyle Moffett]

On Mar 29, 2005, at 09:22, Daniel Jacobowitz wrote:
> The thing GCC is most likely to do with this code is discard the NULL
> check entirely and leave only the oops; the "if (!card)" can not be
> reached without passing through "card->amplifier", and a pointer which
> is dereferenced can not be NULL in a valid program.

Not true!  It is perfectly legal on a large number of platforms to
explicitly mmap something at the address 0, at which point dereferencing
a null pointer is exactly like dereferencing any other pointer on the
heap.  Doing so is not recommended except in a few special cases for
emulators and such, but it works to some extent.

Cheers,
Kyle Moffett

-----BEGIN GEEK CODE BLOCK-----
Version: 3.12
GCM/CS/IT/U d- s++: a18 C++++>$ UB/L/X/*++++(+)>$ P+++(++++)>$
L++++(+++) E W++(+) N+++(++) o? K? w--- O? M++ V? PS+() PE+(-) Y+
PGP+++ t+(+++) 5 X R? tv-(--) b++++(++) DI+ D+ G e->++++$ h!*()>++$ r  
!y?(-)
------END GEEK CODE BLOCK------

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

[Horst von Brand]

"Jean Delvare" <khali@linux-fr.org> said:

[Sttributions missing, sorry]

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

> Wow. Great point. I completely missed that possibility. In fact I didn't
> know that the compiler could possibly alter the order of the
> instructions. For one thing, I thought it was simply not allowed to. For
> another, I didn't know that it had been made so aware that it could
> actually figure out how to do this kind of things. What a mess. Let's
> just hope that the gcc folks know their business :)

The compiler is most definitely /not/ allowed to change the results the
code gives.
-- 
Dr. Horst H. von Brand                   User #22616 counter.li.org
Departamento de Informatica                     Fono: +56 32 654431
Universidad Tecnica Federico Santa Maria              +56 32 654239
Casilla 110-V, Valparaiso, Chile                Fax:  +56 32 797513

Re: Do not misuse Coverity please

[Jean Delvare]

Hi Horst,

> > > No, there is a third case: the pointer can be NULL, but the compiler
> > > happened to move the dereference down to after the check.
>
> > Wow. Great point. I completely missed that possibility. In fact I didn't
> > know that the compiler could possibly alter the order of the
> > instructions. For one thing, I thought it was simply not allowed to. For
> > another, I didn't know that it had been made so aware that it could
> > actually figure out how to do this kind of things. What a mess. Let's
> > just hope that the gcc folks know their business :)
>
> The compiler is most definitely /not/ allowed to change the results the
> code gives.

I think that Andrew's point was that the compiler could change the order
of the instructions *when this doesn't change the result*, not just in
the general case, of course. In our example, The instructions:

    v = p->field;
    if (!p) return;

can be seen as equivalent to

    if (!p) return;
    v = p->field;

by the compiler, which might actually optimize the first into the second
(and quite rightly so, as it actually is faster in the null case). The
fact that doing so prevents an oops is unknown to the compiler, so it
just wouldn't care.

Now I don't know what gcc actually does or not, but Andrew's point
makes perfect sense to me in the theory, and effectively voids my own
argument if gcc performs this kind of optimizations. (The prefered
approach to fix these bugs is a different issue though.)

--
Jean Delvare

Re: Do not misuse Coverity please

[Horst von Brand]

"Jean Delvare" <khali@linux-fr.org> said:
> > > > No, there is a third case: the pointer can be NULL, but the compiler
> > > > happened to move the dereference down to after the check.

> > > Wow. Great point. I completely missed that possibility. In fact I didn't
> > > know that the compiler could possibly alter the order of the
> > > instructions. For one thing, I thought it was simply not allowed to. For
> > > another, I didn't know that it had been made so aware that it could
> > > actually figure out how to do this kind of things. What a mess. Let's
> > > just hope that the gcc folks know their business :)

> > The compiler is most definitely /not/ allowed to change the results the
> > code gives.

> I think that Andrew's point was that the compiler could change the order
> of the instructions *when this doesn't change the result*, not just in
> the general case, of course. In our example, The instructions:
> 
>     v = p->field;
>     if (!p) return;
> 
> can be seen as equivalent to
> 
>     if (!p) return;
>     v = p->field;

They are not. If p == NULL, the first gives an exception (SIGSEGV), the
second one doesn't. Just as you can't "optimize" by switching:

    x = b / a;   
    if (a == 0) return;
-- 
Dr. Horst H. von Brand                   User #22616 counter.li.org
Departamento de Informatica                     Fono: +56 32 654431
Universidad Tecnica Federico Santa Maria              +56 32 654239
Casilla 110-V, Valparaiso, Chile                Fax:  +56 32 797513

Re: Do not misuse Coverity please

[Shankar Unni]

Jean Delvare wrote:

>     v = p->field;
>     if (!p) return;
> 
> can be seen as equivalent to
> 
>     if (!p) return;
>     v = p->field;

Heck, no.

You're missing the side-effect of a null pointer dereference crash (for 
p->field) (even though v is unused before the return). The optimizer is 
not allowed to make exceptions go away as a result of the hoisting.

Re: Do not misuse Coverity please

[Olivier Galibert]

On Wed, Mar 30, 2005 at 10:29:43AM -0800, Shankar Unni wrote:
> Jean Delvare wrote:
> 
> >    v = p->field;
> >    if (!p) return;
> >
> >can be seen as equivalent to
> >
> >    if (!p) return;
> >    v = p->field;
> 
> Heck, no.
> 
> You're missing the side-effect of a null pointer dereference crash (for 
> p->field) (even though v is unused before the return). The optimizer is 
> not allowed to make exceptions go away as a result of the hoisting.

Actually it is.  Dereferencing a null pointer is either undefined or
implementation-dependant in the standard (don't remember which), and
as such the compiler can do whatever it wants, be it starting nethack
or not doing the dereference in the first place.

The principle of least surprise makes doing such an "optimisation" not
so smart in practice.  A compiler capable of detecting that situation
would be better off spitting a warning in red blinking letter.

  OG.

Re: Do not misuse Coverity please

[Paulo Marques]

Shankar Unni wrote:
> Jean Delvare wrote:
> 
>>     v = p->field;
>>     if (!p) return;
>>
>> can be seen as equivalent to
>>
>>     if (!p) return;
>>     v = p->field;
> 
> 
> Heck, no.
> 
> You're missing the side-effect of a null pointer dereference crash (for 
> p->field) (even though v is unused before the return). The optimizer is 
> not allowed to make exceptions go away as a result of the hoisting.

I just had to try this out :)

Using gcc 3.3.2 this code sample:

> struct test {
>   int code;
> };
> 
> int test_func(struct test *a)
> {
>   int ret;
>   if (!a) return -1;
>   ret = a->code;
>   return ret;
> }

is compiled into:

>    0:   8b 54 24 04             mov    0x4(%esp,1),%edx
>    4:   83 c8 ff                or     $0xffffffff,%eax
>    7:   85 d2                   test   %edx,%edx
>    9:   74 02                   je     d <test_func+0xd>
>    b:   8b 02                   mov    (%edx),%eax
>    d:   c3                      ret

whereas this one:

> int test_func(struct test *a)
> {
>   int ret;
>   ret = a->code;
>   if (!a) return -1;
>   return ret;
> }

is simply compiled into:

>    0:   8b 44 24 04             mov    0x4(%esp,1),%eax
>    4:   8b 00                   mov    (%eax),%eax
>    6:   c3                      ret

It seems that gcc is smart enough to know that after we've dereferenced 
a pointer, if it was NULL, it doesn't matter any more. So it just 
assumes that if execution reaches that "if" statement then the pointer 
can not be NULL at all.

So the 2 versions aren't equivalent, and gcc doesn't treat them as such 
either.

Just a minor nitpick, though: wouldn't it be possible for an application 
to catch the SIGSEGV and let the code proceed, making invalid the 
assumption made by gcc?

-- 
Paulo Marques - www.grupopie.com

All that is necessary for the triumph of evil is that good men do nothing.
Edmund Burke (1729 - 1797)

Big GCC bug!!! [Was: Re: Do not misuse Coverity please]

[Kyle Moffett]

On Mar 30, 2005, at 14:14, Paulo Marques wrote:
> Just a minor nitpick, though: wouldn't it be possible for an
> application to catch the SIGSEGV and let the code proceed,
> making invalid the assumption made by gcc?

Uhh, it's even worse than that.  Have a look at the following code:
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
> #include <errno.h>
> #include <sys/types.h>
> #include <sys/mman.h>
>
> struct test {
>         int code;
> };
> int test_check_first(struct test *a) {
>         int ret;
>         if (!a) return -1;
>         ret = a->code;
>         return ret;
> }
> int test_check_last(struct test *a) {
>         int ret;
>         ret = a->code;
>         if (!a) return -1;
>         return ret;
> }
>
> int main() {
>         int i;
>         struct test *nullmem = mmap(NULL, 4096, PROT_READ|PROT_WRITE,
>                         MAP_ANON|MAP_FIXED|MAP_PRIVATE, -1, 0);
>         if (nullmem == MAP_FAILED) {
>                 fprintf(stderr,"mmap: %s\n",strerror(errno));
>                 exit(1);
>         }
>         for (i = 0; i < 2; i++) {
>                 nullmem[i].code = i;
>                 printf("nullmem[%d].code = %d\n",i,i);
>                 printf("test_check_first(&nullmem[%d]) = %d\n",i,
>                         test_check_first(&nullmem[i]));
>                 printf("test_check_last(&nullmem[%d]) = %d\n",i,
>                         test_check_last(&nullmem[i]));
>         }
>         munmap(nullmem,4096);
>         exit(0);
> }

Without optimization:
> king:~# gcc -o mmapnull mmapnull.c
> king:~# ./mmapnull
> nullmem[0].code = 0
> test_check_first(&nullmem[0]) = -1
> test_check_last(&nullmem[0]) = -1
> nullmem[1].code = 1
> test_check_first(&nullmem[1]) = 1
> test_check_last(&nullmem[1]) = 1

With optimization:
> king:~# gcc -O2 -o mmapnull mmapnull.c
> king:~# ./mmapnull
> nullmem[0].code = 0
> test_check_first(&nullmem[0]) = -1
> test_check_last(&nullmem[0]) = 0
                         BUG ==> ^^^
> nullmem[1].code = 1
> test_check_first(&nullmem[1]) = 1
> test_check_last(&nullmem[1]) = 1

This is on multiple platforms, including PPC Linux, X86 Linux, and
PPC Mac OS X.  All exhibit the exact same behavior and output.  I
think I'll probably go report a GCC bug now :-D

Dereferencing null pointers is relied upon by a number of various
emulators and such, and is "platform-defined" in the standard, so
since Linux allows mmap at NULL, GCC shouldn't optimize that case
any differently.

Cheers,
Kyle Moffett

-----BEGIN GEEK CODE BLOCK-----
Version: 3.12
GCM/CS/IT/U d- s++: a18 C++++>$ UB/L/X/*++++(+)>$ P+++(++++)>$
L++++(+++) E W++(+) N+++(++) o? K? w--- O? M++ V? PS+() PE+(-) Y+
PGP+++ t+(+++) 5 X R? tv-(--) b++++(++) DI+ D+ G e->++++$ h!*()>++$ r  
!y?(-)
------END GEEK CODE BLOCK------

Not a GCC bug (was Re: Big GCC bug!!! [Was: Re: Do not misuse Coverity please])

[Jakub Jelinek]

On Wed, Mar 30, 2005 at 06:11:43PM -0500, Kyle Moffett wrote:
> On Mar 30, 2005, at 14:14, Paulo Marques wrote:
> >Just a minor nitpick, though: wouldn't it be possible for an
> >application to catch the SIGSEGV and let the code proceed,
> >making invalid the assumption made by gcc?
> 
> Uhh, it's even worse than that.  Have a look at the following code:
> >#include <stdio.h>
> >#include <stdlib.h>
> >#include <string.h>
> >#include <errno.h>
> >#include <sys/types.h>
> >#include <sys/mman.h>
> >
> >struct test {
> >        int code;
> >};
> >int test_check_first(struct test *a) {
> >        int ret;
> >        if (!a) return -1;
> >        ret = a->code;
> >        return ret;
> >}
> >int test_check_last(struct test *a) {
> >        int ret;
> >        ret = a->code;
> >        if (!a) return -1;
> >        return ret;
> >}
> >
> >int main() {
> >        int i;
> >        struct test *nullmem = mmap(NULL, 4096, PROT_READ|PROT_WRITE,
> >                        MAP_ANON|MAP_FIXED|MAP_PRIVATE, -1, 0);
> >        if (nullmem == MAP_FAILED) {
> >                fprintf(stderr,"mmap: %s\n",strerror(errno));
> >                exit(1);
> >        }
> >        for (i = 0; i < 2; i++) {
> >                nullmem[i].code = i;
> >                printf("nullmem[%d].code = %d\n",i,i);
> >                printf("test_check_first(&nullmem[%d]) = %d\n",i,
> >                        test_check_first(&nullmem[i]));
> >                printf("test_check_last(&nullmem[%d]) = %d\n",i,
> >                        test_check_last(&nullmem[i]));
> >        }
> >        munmap(nullmem,4096);
> >        exit(0);
> >}

This testcase violates ISO C99 6.3.2.3:
If a null pointer constant is converted to a pointer type, the resulting
pointer, called a null pointer, is guaranteed to compare unequal to a
pointer to any object or function.

But in the testcase above, there is an object whose pointer compares equal
to the null pointer.  So there is nothing wrong with what GCC does.

	Jakub

Re: Not a GCC bug (was Re: Big GCC bug!!! [Was: Re: Do not misuse Coverity please])

[Kyle Moffett]

On Mar 30, 2005, at 18:38, Jakub Jelinek wrote:
> This testcase violates ISO C99 6.3.2.3:
> If a null pointer constant is converted to a pointer type, the 
> resulting
> pointer, called a null pointer, is guaranteed to compare unequal to a
> pointer to any object or function.

Except that the result of dereferencing a null pointer is implementation
defined according to the C99 standard.  My implementation allows me to 
mmap
stuff at NULL, and therefore its compiler should be able to handle that
case.  I would have no problem with either the standard or 
implementation
if it either properly handled the case or didn't allow it in the first
place.

On another note, I've discovered the flag 
"-fno-delete-null-pointer-checks",
which should probably be included in the kernel makefiles to disable 
that
optimization for the kernel.  (Ok, yes, I apologize, this isn't really 
a GCC
bug, the behavior is documented, although it can be quite confusing.  I
suspect it may bite some platform-specific code someday.  It also 
muddies
the waters somewhat with respect to the original note (and the effects 
on
the generated code):

> int x = my_struct->the_x;
> if (!my_struct) return;

Cheers,
Kyle Moffett

-----BEGIN GEEK CODE BLOCK-----
Version: 3.12
GCM/CS/IT/U d- s++: a18 C++++>$ UB/L/X/*++++(+)>$ P+++(++++)>$
L++++(+++) E W++(+) N+++(++) o? K? w--- O? M++ V? PS+() PE+(-) Y+
PGP+++ t+(+++) 5 X R? tv-(--) b++++(++) DI+ D+ G e->++++$ h!*()>++$ r  
!y?(-)
------END GEEK CODE BLOCK------

Re: Not a GCC bug (was Re: Big GCC bug!!! [Was: Re: Do not misuse Coverity please])

[Nick Piggin]

Kyle Moffett wrote:
> On Mar 30, 2005, at 18:38, Jakub Jelinek wrote:
> 
>> This testcase violates ISO C99 6.3.2.3:
>> If a null pointer constant is converted to a pointer type, the resulting
>> pointer, called a null pointer, is guaranteed to compare unequal to a
>> pointer to any object or function.
> 
> 
> Except that the result of dereferencing a null pointer is implementation
> defined according to the C99 standard.  My implementation allows me to mmap
> stuff at NULL, and therefore its compiler should be able to handle that
> case.  I would have no problem with either the standard or implementation
> if it either properly handled the case or didn't allow it in the first
> place.
> 
> On another note, I've discovered the flag 
> "-fno-delete-null-pointer-checks",
> which should probably be included in the kernel makefiles to disable that
> optimization for the kernel.  (Ok, yes, I apologize, this isn't really a 
> GCC
> bug, the behavior is documented, although it can be quite confusing.  I
> suspect it may bite some platform-specific code someday.  It also muddies
> the waters somewhat with respect to the original note (and the effects on
> the generated code):
> 
>> int x = my_struct->the_x;
>> if (!my_struct) return;
> 

Why should this be in the kernel makefiles? If my_struct is NULL,
then the kernel will never reach the if statement.

A warning might be nice though.

Re: Not a GCC bug (was Re: Big GCC bug!!! [Was: Re: Do not misuse Coverity please])

[Kyle Moffett]

On Mar 30, 2005, at 20:12, Nick Piggin wrote:
> Why should this be in the kernel makefiles? If my_struct is NULL,
> then the kernel will never reach the if statement.

Well, I think there is probably some arch code that uses 16-bit
that might use a null pointer, or at least a struct that starts
at the 0 address, which would have problems.  I think it would
be better to avoid that issue just in case, especially since
this optimization does not save anything in the case of properly
written code.

> A warning might be nice though.

If we could turn off the optimization and add a warning, I
would support that.  Even if we could only add the warning, then
at least people would know.


Cheers,
Kyle Moffett

-----BEGIN GEEK CODE BLOCK-----
Version: 3.12
GCM/CS/IT/U d- s++: a18 C++++>$ UB/L/X/*++++(+)>$ P+++(++++)>$
L++++(+++) E W++(+) N+++(++) o? K? w--- O? M++ V? PS+() PE+(-) Y+
PGP+++ t+(+++) 5 X R? tv-(--) b++++(++) DI+ D+ G e->++++$ h!*()>++$ r  
!y?(-)
------END GEEK CODE BLOCK------

Re: Do not misuse Coverity please

[Patrick McFarland]

[-- Attachment #1: Type: text/plain, Size: 1239 bytes --]

On Wednesday 30 March 2005 01:55 pm, Olivier Galibert wrote:
> Actually it is.  Dereferencing a null pointer is either undefined or
> implementation-dependant in the standard (don't remember which), and
> as such the compiler can do whatever it wants, be it starting nethack

Can this be configured to start slashem instead? ;)

> or not doing the dereference in the first place.

What scares me is, "Is there any code in the kernel that does this, or 
something similarly stupid?". I regard the linux kernel as mostly sane, and 
I'd rather not have that illusion of safety broken* any time soon. Sure, its 
great for an app to randomly segfault, but having the kernel oops randomly is 
not much fun.

To quote a sig I once saw on usenet: "Why Linux is better than Windows, Reason 
#325: No needing to reboot every 15 seconds; or every 3 seconds during full 
moons."



* I am by no means an expert on kernel programing

-- 
Patrick "Diablo-D3" McFarland || pmcfarland@downeast.net
"Computer games don't affect kids; I mean if Pac-Man affected us as kids, we'd 
all be running around in darkened rooms, munching magic pills and listening to
repetitive electronic music." -- Kristian Wilson, Nintendo, Inc, 1989

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: Do not misuse Coverity please

[Pavel Machek]

Hi!

> "Jean Delvare" <khali@linux-fr.org> said:
> > > > > No, there is a third case: the pointer can be NULL, but the compiler
> > > > > happened to move the dereference down to after the check.
> 
> > > > Wow. Great point. I completely missed that possibility. In fact I didn't
> > > > know that the compiler could possibly alter the order of the
> > > > instructions. For one thing, I thought it was simply not allowed to. For
> > > > another, I didn't know that it had been made so aware that it could
> > > > actually figure out how to do this kind of things. What a mess. Let's
> > > > just hope that the gcc folks know their business :)
> 
> > > The compiler is most definitely /not/ allowed to change the results the
> > > code gives.
> 
> > I think that Andrew's point was that the compiler could change the order
> > of the instructions *when this doesn't change the result*, not just in
> > the general case, of course. In our example, The instructions:
> > 
> >     v = p->field;
> >     if (!p) return;
> > 
> > can be seen as equivalent to
> > 
> >     if (!p) return;
> >     v = p->field;
> 
> They are not. If p == NULL, the first gives an exception (SIGSEGV), the
> second one doesn't. Just as you can't "optimize" by switching:
> 
>     x = b / a;   
>     if (a == 0) return;

Dereferencing NULL pointer is undefined. It *may* give SIGSEGV. That's
what enables optimization above. You can't rely on dereferencing NULL
to always give SIGSEGV. Sorry.

								Pavel
-- 
Boycott Kodak -- for their patent abuse against Java.

[2.6 patch] sound/oss/cs46xx.c: fix a check after use

[Adrian Bunk]

This patch fixes a check after use found by the Coverity checker.

Signed-off-by: Adrian Bunk <bunk@fs.tum.de>

---

This patch was already sent on:
- 27 Mar 2005

--- linux-2.6.12-rc1-mm1-full/sound/oss/cs46xx.c.old	2005-03-23 04:48:53.000000000 +0100
+++ linux-2.6.12-rc1-mm1-full/sound/oss/cs46xx.c	2005-03-23 04:49:31.000000000 +0100
@@ -3086,13 +3086,14 @@
  
 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;
 	card->amplifier+=change;
 	if( (card->amplifier && !old) && !(hercules_egpio_disable))
 	{

Re: [2.6 patch] sound/oss/cs46xx.c: fix a check after use

[Al Viro]

On Wed, Apr 13, 2005 at 04:17:39AM +0200, Adrian Bunk wrote:
> This patch fixes a check after use found by the Coverity checker.

NAK.  Please, read the surrounding code.  All places that can call
that function have form
	<expression>->amplifier_ctrl(<same expression>,...);
so we _can't_ get NULL first argument.  The check should be removed -
it's not paranoia, it's simple stupidity.