Fun with NULL pointers, part 2

Posted Jul 21, 2009 21:31 UTC (Tue) by spender (guest, #23067)
Parent article: Fun with NULL pointers, part 2

"One assumes that the relevant developers understand this now, but there may be a lot of places in the kernel where necessary pointer checks were removed from the code."

I wouldn't be so sure: those mails you link to are from 2008; we had gone through the whole "NULL pointer dereferences can be exploitable" thing back in 2007 when I released my first exploit.

But I hope you're right ;)

-Brad

to post comments

Fun with NULL pointers, part 2

Posted Jul 21, 2009 21:50 UTC (Tue) by ebiederm (subscriber, #35028) [Link] (13 responses)

No. BUG_ON(ptr == NULL) is generally a bad idea because it is fixing
a problem in the wrong place. Leading to maintenance problems etc.

In some common places like sysfs where the are a lot of users and mistakes are common because of the volume of use of the interface it makes sense.

In general it is better to simply make it impossible for a NULL pointer
to reach someplace. Or to handle that NULL pointer dereference.

Checking for the impossible just makes the code harder to understand.

Fun with NULL pointers, part 2

Posted Jul 21, 2009 21:53 UTC (Tue) by eparis (guest, #33060) [Link] (7 responses)

Clearly you never make mistakes, but when Herbert Xu does, I think it proves that any of us mere mortals can. Writing code defensively is clearly a good idea and one I think all of us should embrace.

Fun with NULL pointers, part 2

Posted Jul 21, 2009 22:11 UTC (Tue) by ebiederm (subscriber, #35028) [Link] (6 responses)

As best I can tell Herbert badly resolved a merge between my changes
to tun.c and his changes to tun.c, and simply had not realized how much
I had changed tun.c.

As for writing code defensively. Doing that routinely doubles your number of bugs.

You have the wrong input should never have been generated in the first place.

You have the wrong BUG_ON in the function.

Bugs per line of code are roughly constant. Therefore you want a design
that uses fewer lines of code.

What made all of this interesting is the fact someone found a way around the defensive measure of mmap_min_addr.

Eric

Fun with NULL pointers, part 2

Posted Jul 22, 2009 7:01 UTC (Wed) by PaXTeam (guest, #24616) [Link]

> What made all of this interesting is the fact someone found a way around the defensive measure of mmap_min_addr.

It's not at all interesting because any feature which is *designed* to be circumventible will be, well, circumvented. From day one it was obvious and publicly discussed that exploit writers would go after the needed capability (that assuming they don't have better bugs to exploit not needing it ;).

Fun with NULL pointers, part 2

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

You have the wrong input should never have been generated in the first place.
You have the wrong BUG_ON in the function.
Bugs per line of code are roughly constant. Therefore you want a design that uses fewer lines of code.

Adding the null pointer check does not cause the first bug you mention, although it may help to reveal it. The second item is a coding style issue, not a bug (and your logic is circular; you try to show that adding the BUG_ON is a bad idea, so you cannot assume that in your argument). The third assertion needs some evidence, and even if true cannot be used to show that adding any particular line of code introduces a bug, any more than you could use it to justify removing one particular line of code from the middle of your program. (It is sometimes the case that adding overzealous error checking introduces a bug, but that needs to be shown in each individual case, and you haven't shown it here.)

Are you an idiot or just play one or TV?

Posted Jul 23, 2009 5:35 UTC (Thu) by khim (subscriber, #9252) [Link] (2 responses)

It's not even funny: you are discussing one particular function in it's current incarnation where Eric discusses the whole approach.

Adding the null pointer check does not cause the first bug you mention, although it may help to reveal it.

But it can crash the perfectly working system tomorrow after some kind of refactoring.

The second item is a coding style issue, not a bug (and your logic is circular; you try to show that adding the BUG_ON is a bad idea, so you cannot assume that in your argument).

Today human mistakes are promoted to "code style issue"? News to me...

The third assertion needs some evidence, and even if true cannot be used to show that adding any particular line of code introduces a bug

There were a lot of investigations. Number of bugs per line of code does not change too much when you switch languages, paradigms, etc. It reduces if you do a lot of rafactoring (like kernel developers do) and srinks when you are using different autodetection tools (which are they using too), this change is pretty limited. It'll be somewhat strange to see that BUG_ON is somehow 100 times less buggy then the rest of the code.

any more than you could use it to justify removing one particular line of code from the middle of your program.

Yup. Justification is the same as for the rest of code: if you function will continue to work - why have this line in first place? Redundant comments can be kept around, but actual line of code? Please - a lot of stuff kernel developers are doing is this exact "removal lines of code from the middle of your program".

(It is sometimes the case that adding overzealous error checking introduces a bug, but that needs to be shown in each individual case, and you haven't shown it here.)

And now sound almost adequately. If sometimes overzealous error checking introduces a bug then it's good idea to keep it out of program: asserts, coverity, etc. These things can detect bug but they can not introduce new bug (false positive is not a bug as it's not compiled in actual kernel).

Defensive programming makes sense when you suspect other code has lower quality (userspace, drivers for obscure devices, etc), but to try to protect code from code of equal quality with so-called "defensive programming" is to increase number of bugs!

Are you an idiot or just play one or TV?

Posted Jul 23, 2009 11:15 UTC (Thu) by epa (subscriber, #39769) [Link] (1 responses)

Why yes, I was talking about this particular case. Doing a null pointer check earlier (whether with BUG_ON(x==NULL) or some other test) would have avoided a local root exploit in this one case. That suggests that the whole approach is not completely useless, even if in other cases it doesn't help.

I am not arguing for 'defensive programming' as sometimes practised, where you cruft up the code with workarounds to silently return or do something random if a caller is buggy. That tends to hide bugs and thus cause buggier software in the long run. (Although even this kind of defensive programming can occasionally be useful, for example in safety-critical systems where the code must keep running no matter what.)

There were a lot of investigations. Number of bugs per line of code does not change too much when you switch languages, paradigms, etc.

That is quite true but you cannot reason as follows: on average, more lines of code means more bugs, therefore an average 101-line program is buggier than an average 100-line program, therefore adding *this particular line of code* to *this particular* program is likely to cause a bug! Of course it depends on the individual case! Some kinds of code such as assertions (runtime checking) and type annotation (compile-time checking) exist only to catch bugs, and it doesn't make sense to say that adding them will tend to make code buggier just based on a general rule about lines of code.

An incorrect BUG_ON(x==NULL) certainly can introduce a bug into a program, but then an incorrect anything can introduce a bug.

+1

Posted Jul 23, 2009 14:57 UTC (Thu) by xoddam (subscriber, #2322) [Link]

+1, for sanity.

Fun with NULL pointers, part 2

Posted Jul 23, 2009 12:27 UTC (Thu) by ortalo (guest, #4654) [Link]

Don't you think a static checker could benefit from BUG_ON()-like information (or ASSERT() or whatever you want to name it) by using the given check/assumption for code analysis.
In this case, the additional line is more an annotation (like those many static checkers introduce) than source code.
As a developper you may legitimately want that such annotations be placed elsewhere than in the middle of code, but then where would you like to place them?

Side note: I really do not buy the constant number_of_fault/loc assumption. But well... that's another debate; and anyway IMHO your argument with respect to code readability and maintanability is totally valid.

Fun with NULL pointers, part 2

Posted Jul 21, 2009 23:32 UTC (Tue) by johill (subscriber, #25196) [Link] (3 responses)

Besides, depending on the platform the compiler could elide the BUG_ON(ptr == NULL) anyway, no? It's often just

if (unlikely(ptr == NULL))
    out_of_line_bug();

or something like that?

Fun with NULL pointers, part 2

Posted Jul 22, 2009 4:12 UTC (Wed) by gmaxwell (guest, #30048) [Link]

Yes, if the compiler can prove that its impossible for the condition to be true it can and will omit the check. Sadly because the compiler's visibility is so narrow it usually can't do this except in the most obvious of situations.

Fun with NULL pointers, part 2

Posted Jul 22, 2009 9:53 UTC (Wed) by mb (subscriber, #50428) [Link] (1 responses)

Well, for most architectures the BUG() functions are __attribute__((noreturn)) or something equivalent. So the compiler cannot assume that the execution flow reaches to after the if (!ptr) check at all, if ptr is NULL. So it should not optimize it out in that case.

Fun with NULL pointers, part 2

Posted Jul 22, 2009 11:23 UTC (Wed) by johill (subscriber, #25196) [Link]

Really? I thought the out_of_line_bug() in my example would have that attribute, but not the BUG() itself. But powerpc for example is different though, it just inserts a trap (twi or such).

Fun with NULL pointers, part 2

Posted Aug 11, 2009 18:50 UTC (Tue) by bdonlan (guest, #51264) [Link]

BUG_ON doesn't fix a problem - it complains about one. It's a bit like
assert(!condition) in userspace - if you actually hit it, the program's
already doomed. The Linux kernel tries to keep going a bit, just because it's
difficult to get debug information after a panic, but this is _not_ fixing a
problem, and a tripped BUG_ON is _by definition_ a bug.

As such, the only disadvantage to extra BUG_ONs is the run-time overhead from
them.