sound/oss/cs46xx.c: fix a check after use

Hi Adrian,

sound/oss/cs46xx.c line 3096

Quoted 13 lines

This patch fixes a check after use found by the Coverity checker. (...)
static void amp_hercules(struct cs_card *card, int change) {

sound/oss/cs46xx.c renamed from sound/oss/cs46xx.c.old +2 -1

-	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/[email protected]

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

Thanks,
Jean Delvare

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

Hi Adrian,

Quoted 5 lines

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

Agreed.

Quoted 4 lines

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.

Quoted 5 lines

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.

Quoted 8 lines

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,

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.

Quoted 6 lines

Thanks,
Jean Delvare

cu
Adrian

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.

Quoted 5 lines

This patch fixes a check after use found by the Coverity checker. (...)
static void amp_hercules(struct cs_card *card, int change) {

sound/oss/cs46xx.c line 3096

Quoted 9 lines

sound/oss/cs46xx.c renamed from sound/oss/cs46xx.c.old +2 -1

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

Quoted 5 lines

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.

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.

Quoted 9 lines

sound/oss/cs46xx.c line 3096

Quoted 9 lines

sound/oss/cs46xx.c renamed from sound/oss/cs46xx.c.old +2 -1

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

Quoted 8 lines

Quoted 5 lines

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 :)

Quoted 9 lines

Quoted 2 lines

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.

Quoted 10 lines

Quoted 9 lines

Quoted 2 lines

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 :)

Quoted 2 lines

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;

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

can be seen as equivalent to

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

Quoted 5 lines · Jean Delvare

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

can be seen as equivalent to

Quoted 2 lines

   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.

Quoted 5 lines · Jean Delvare

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

can be seen as equivalent to

Quoted 2 lines

    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.

struct test {
int code;
};

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

   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

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

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

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?

#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);
}

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

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

nullmem[1].code = 1
test_check_first(&nullmem[1]) = 1
test_check_last(&nullmem[1]) = 1

Quoted 3 lines · Paulo Marques

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:

Quoted 42 lines

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

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

Quoted 4 lines · Jakub Jelinek

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):

Quoted 2 lines

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.

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.

Quoted 22 lines · Jean Delvare

Quoted 10 lines

Quoted 9 lines

Quoted 2 lines

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 :)

Quoted 2 lines

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

Quoted 2 lines

    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;