Fun with NULL pointers, part 2

Posted Jul 21, 2009 21:53 UTC (Tue) by eparis (guest, #33060)
In reply to: Fun with NULL pointers, part 2 by ebiederm
Parent article: Fun with NULL pointers, part 2

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.

to post comments

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.