Fun with NULL pointers, part 2

Posted Jul 21, 2009 21:35 UTC (Tue) by i3839 (guest, #31386)
Parent article: Fun with NULL pointers, part 2

> has often seen the BUG_ON() line removed with a comment like:
>
> If we dereference NULL then the kernel will display basically the same
> information as would a BUG, and it takes the same action. So adding a
> BUG_ON here really doesn't gain us anything.
>
> This reasoning is based on the idea that dereferencing a NULL pointer
> will cause a kernel oops.

For the Smack example, if you read the code it's clear that the BUG_ON check makes no sense. It is in a static function and all callers either check or dereference the pointer. More generically, adding BUG_ONs everywhere for pointers that shouldn't be NULL is the wrong approach: Either make sure that all callers don't pass in a NULL pointer, or if that isn't possible, add a check and return with an error.

For the other example the kernel better make that it notices it when it's NULL, one way or the other.

to post comments

Fun with NULL pointers, part 2

Posted Jul 22, 2009 9:15 UTC (Wed) by epa (subscriber, #39769) [Link] (6 responses)

Either make sure that all callers don't pass in a NULL pointer, or if that isn't possible, add a check and return with an error.

Suppose you take the first option; you check all the code that could call your routine and make sure NULL is never passed. Then there is no need for a runtime check, right? In theory this is true but I know I make mistakes, so even after checking all the code I would still put in the check for this 'impossible condition' to stop a small oversight causing a big problem.

It's the same argument for any assertion you put in your code: since they are checking for something that can never ever happen, why bother with them at all? All programmers make mistakes, but the better ones are aware of this fact and take appropriate measures.

(This is not an argument for sloppy 'defensive programming' as sometimes practised, where you insert code to silently return the wrong answer or do something random if the inputs aren't as you expect. You want a macro like BUG_ON or assert() which loudly announces the problem, not hides it.)

Fun with NULL pointers, part 2

Posted Jul 22, 2009 10:19 UTC (Wed) by i3839 (guest, #31386) [Link] (1 responses)

Making sure that a pointer is never NULL is easy when it is passed to a static function. For public functions it is harder and more checks are needed, but if all callers are limited to one directory it's still not hard.

If you pass along a pointer and say "hey, do something with this", in general that code did something with it before, otherwise the second function could be called directly.

For pointers within structures it's much harder though, then it helps to have clean code with clear lifetime rules. If it's messy enough you can make things more messy by adding NULL checks everywhere. If this adds more unexpected error paths then it's not a good way forwards though. If it's unclear if something can be NULL or not then in the long term it's much better to clean up the code. Like assertions BUG_ON is good while developing a new piece of code to make sure all assumptions hold. When the code gets more into shape and becomes more stable there should be not much need for such checks anymore.

An exception is low level library like code which can be used by many users and which wants to prevent them making the same bloody mistakes. It's also a good way to prevent corruptions from spreading around.

The further away the code giving the input is from the code using it the more checking is necessary.

Fun with NULL pointers, part 2

Posted Jul 22, 2009 11:14 UTC (Wed) by epa (subscriber, #39769) [Link]

Making sure that a pointer is never NULL is easy when it is passed to a static function. For public functions it is harder and more checks are needed, but if all callers are limited to one directory it's still not hard.

It's not hard, but I know that I am able to make mistakes on even the simplest tasks. I also know that I am not quite the world's worst programmer or the most careless, so if I can screw up, others can too. That's why even after you have carefully gone through the code and made sure that 'obviously', NULL can never be passed, it is still a good idea to include a check just in case, to stop a minor oversight becoming a major bug (such as the root exploit discussed in the article).

The best option is for the compiler to statically ensure non-null pointers; tools like Splint let you be certain that getting null is impossible.

If it's unclear if something can be NULL or not then in the long term it's much better to clean up the code.

Absolutely agreed. If it's unclear whether NULL is allowed, then BUG_ON(x==NULL) is not the right way, except as a temporary debugging aid. You need to check the code and make a decision - can NULL ever legally be passed here? If the answer is no, then you should make sure all the callers are behaving properly - but then when you've finished put in the BUG_ON check anyway, because even Linus makes mistakes.

Fun with NULL pointers, part 2

Posted Jul 22, 2009 13:25 UTC (Wed) by dgm (subscriber, #49227) [Link] (3 responses)

Looks like a variation of the "end to end principle" (http://en.wikipedia.org/wiki/End-to-end_principle). No matter that callers check for NULL, the callee has still to check it, again.

Also, as someone has pointed out, NULL is just *one* invalid pointer value (even if a common one). The kernel should better be testing to prevent pointers to user space, except when needed.

I don't know enough Linux internals. Should it be possible to make those tests with some memory protection trickery?

Fun with NULL pointers, part 2

Posted Jul 22, 2009 16:03 UTC (Wed) by brianomahoney (guest, #6206) [Link] (2 responses)

Add more checks of all types, we all make mistakes is pure nonsense, it will make the kernel slow and fat, with no real improvement in reliability. it is the mark of a confused and poor programmer who has no sense of design and debugging kernel code.

This bug happened because the author wrote patently idiotic code, using a pointer and THEN checking it. This code, and all others like it need to be fixed so it actually does what it is intended to do. We do not need the kernel full of ah, well, but checks and other kruft.

The secondary, and very worrying thing is GCC silently dropping the check, we do not need optimizations like that without a STRONG warning, but I guess the motivations of the GCC developers are different here, and they really cannot win, there is constant pressure to improve compiled code and not to introduce more baby-minding warnings.

Perhaps COVERTY can help here, but we need more and better analysis, and focused bug-fixing bu good developers not the injection of confused well meaning tests.

Fun with NULL pointers, part 2

Posted Jul 22, 2009 23:26 UTC (Wed) by i3839 (guest, #31386) [Link]

> This bug happened because the author wrote patently idiotic code, using a pointer and THEN checking it.

My understanding was that two people worked on the same code and that this was a result of a bad merge, though I could be confusing this with another case.

Fun with NULL pointers, part 2

Posted Jul 23, 2009 8:09 UTC (Thu) by dgm (subscriber, #49227) [Link]

Except that the kernel is not designed, it evolves. Linus has pointed it out several times. At most, some features are initially designed, but from there on, it's all evolution.

Maybe you write perfectly designed code that works wonderfully and will never need an update, fix or improvement. I centainly don't. I make mistakes, that's why my code is full of assertions, and I try to manage impossible situations in a sane way, just in case.

With regards to fat and slow... Any check can be made optional, by simply putting it between a pair of #ifdefs. Maybe somebody will apreciate a more rugged, even if slower kernel for certain servers that are exposed to the outside World. An important point would be to measure how much slower that kernel would be.