[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 <***@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))
{

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



-------------------------------------------------------------------------------
Achtung: diese Newsgruppe ist eine unidirektional gegatete Mailingliste.
Antworten nur per Mail an die im Reply-To-Header angegebene Adresse.
Fragen zum Gateway -> ***@inka.de.
-------------------------------------------------------------------------------

[Adrian Bunk]

Hi Adrian,

Post by Adrian Bunk
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.
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 ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/



-------------------------------------------------------------------------------
Achtung: diese Newsgruppe ist eine unidirektional gegatete Mailingliste.
Antworten nur per Mail an die im Reply-To-Header angegebene Adresse.
Fragen zum Gateway -> ***@inka.de.
-------------------------------------------------------------------------------

[Jean Delvare]

Hi Adrian,

Post by Adrian Bunk
1. NULL is impossible, the check is superfluous
2. this was an actual bug

Agreed.

Post by Adrian Bunk
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.

Post by Adrian Bunk
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.

Post by Adrian Bunk
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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/



-------------------------------------------------------------------------------
Achtung: diese Newsgruppe ist eine unidirektional gegatete Mailingliste.
Antworten nur per Mail an die im Reply-To-Header angegebene Adresse.
Fragen zum Gateway -> ***@inka.de.
-------------------------------------------------------------------------------

[Russell King]

Post by Jean Delvare
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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/



-------------------------------------------------------------------------------
Achtung: diese Newsgruppe ist eine unidirektional gegatete Mailingliste.
Antworten nur per Mail an die im Reply-To-Header angegebene Adresse.
Fragen zum Gateway -> ***@inka.de.
-------------------------------------------------------------------------------

[Matthias-Christian Ott]

Hi Adrian,

Post by Adrian Bunk
1. NULL is impossible, the check is superfluous
2. this was an actual bug

Agreed.

Post by Adrian Bunk
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.

Post by Adrian Bunk
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.

Post by Adrian Bunk
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

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



-------------------------------------------------------------------------------
Achtung: diese Newsgruppe ist eine unidirektional gegatete Mailingliste.
Antworten nur per Mail an die im Reply-To-Header angegebene Adresse.
Fragen zum Gateway -> ***@inka.de.
-------------------------------------------------------------------------------

[L. A. Walsh]

Post by Adrian Bunk
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

Post by Adrian Bunk

Thanks,
Jean Delvare

cu
Adrian

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



-------------------------------------------------------------------------------
Achtung: diese Newsgruppe ist eine unidirektional gegatete Mailingliste.
Antworten nur per Mail an die im Reply-To-Header angegebene Adresse.
Fragen zum Gateway -> ***@inka.de.
-------------------------------------------------------------------------------

[Daniel Barkalow]

Post by L. A. Walsh
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*

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



-------------------------------------------------------------------------------
Achtung: diese Newsgruppe ist eine unidirektional gegatete Mailingliste.
Antworten nur per Mail an die im Reply-To-Header angegebene Adresse.
Fragen zum Gateway -> ***@inka.de.
-------------------------------------------------------------------------------

[Jean Delvare]

Hi Adrian,

Post by Adrian Bunk
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/***@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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/



-------------------------------------------------------------------------------
Achtung: diese Newsgruppe ist eine unidirektional gegatete Mailingliste.
Antworten nur per Mail an die im Reply-To-Header angegebene Adresse.
Fragen zum Gateway -> ***@inka.de.
-------------------------------------------------------------------------------

[Andrew Morton]

Post by Adrian Bunk
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 ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/



-------------------------------------------------------------------------------
Achtung: diese Newsgruppe ist eine unidirektional gegatete Mailingliste.
Antworten nur per Mail an die im Reply-To-Header angegebene Adresse.
Fragen zum Gateway -> ***@inka.de.
-------------------------------------------------------------------------------

[Jean Delvare]

Hi Andrew, all,

Post by Andrew Morton

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.

Post by Andrew Morton
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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/



-------------------------------------------------------------------------------
Achtung: diese Newsgruppe ist eine unidirektional gegatete Mailingliste.
Antworten nur per Mail an die im Reply-To-Header angegebene Adresse.
Fragen zum Gateway -> ***@inka.de.
-------------------------------------------------------------------------------

[Chris Friesen]

Post by Jean Delvare
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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/



-------------------------------------------------------------------------------
Achtung: diese Newsgruppe ist eine unidirektional gegatete Mailingliste.
Antworten nur per Mail an die im Reply-To-Header angegebene Adresse.
Fragen zum Gateway -> ***@inka.de.
-------------------------------------------------------------------------------

[Daniel Jacobowitz]

Post by Andrew Morton

Post by Adrian Bunk
- 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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/



-------------------------------------------------------------------------------
Achtung: diese Newsgruppe ist eine unidirektional gegatete Mailingliste.
Antworten nur per Mail an die im Reply-To-Header angegebene Adresse.
Fragen zum Gateway -> ***@inka.de.
-------------------------------------------------------------------------------