Should I really worry about fixing null defres if there's no crashes?

Question

Clang's scan-build reports quite a few dereference of null pointers in my project, however, I don't really see any unusual behavior (in 6 years of using it), ie:

Dereference of null pointer (loaded from variable chan)

char *tmp;
CList *chan = NULL; 
/* This is weird because chan is set via do_lookup so why could it be NULL? */
chan = do_lookup(who, me, UNLINK);
if (chan)
tmp = do_lookup2(you,me,0);

prot(get_sec_var(chan->zsets));

                 ^^^^

I know null derefs can cause crashes but is this really a big security concern as some people make it out to be? What should I do in this case?

Why do you properly check with if(chan), but then access it two lines later outside the if()? Are there some braces missing? — Andreas Fester, Oct 02 '12 at 07:05

score 5 · Accepted Answer · answered Oct 02 '12 at 07:04

5

It is Undefined Behavior to dereference a NULL pointer. It can show any behavior, it might crash or not but you MUST fix those!

The truth about Undefined Behavior is that it obeys Murphy's Law

"Anything that can go wrong will go wrong"

answered Oct 02 '12 at 07:04

Alok Save

202,538
53
430
533

+1, although I disagree on your last statement. Afaik, the truth about UB is not defined. But it may be implementation defined ;-) – stefan Oct 06 '12 at 23:38
@stefan: Standard clearly defines [what it is Undefined Behavior and what is Implementation defined or Unspecified behavior.](http://stackoverflow.com/questions/2397984/undefined-unspecified-and-implementation-defined-behavior) – Alok Save Oct 07 '12 at 15:03
Well, maybe, but this still doesn't mean that "anything that can go wrong will go wrong". This is the very definition of "undefined". It can (magically) go "right", since the intended behaviour is contained in all possible (yet undefined) outcome of undefined behaviour. – stefan Oct 07 '12 at 15:13

score 4 · Answer 2 · answered Oct 02 '12 at 07:06

4

It makes no sense checking chan for NULL at one point:

if (chan)
  tmp = do_lookup2(you,me,0);     /* not evaluated if `chan` is NULL */
  prot(get_sec_var(chan->zsets)); /* will be evaluated in any case */

... yet NOT checking it right at the next line.

Don't you have to execute both these statements within if branch?

answered Oct 02 '12 at 07:06

raina77ow

103,633
15
192
229

Why? You have a `chan = do_lookup(who, me, UNLINK);` before this line? – Kiril Kirov Oct 02 '12 at 07:07
1

Don't you think that if `chan` cannot be NULL in any case, `if(chan)` line is, well, redundant? – raina77ow Oct 02 '12 at 07:11

score 4 · Answer 3 · answered Oct 02 '12 at 07:07

Clang is warning you because you check for chan being NULL, and then you unconditionally dereference it in the next line anyway. This cannot possibly be correct. Either do_lookup cannot return NULL, then the check is useless and should be removed. Or it can, then the last line can cause undefined behaviour and MUST be fixed. Als is 100% correct: NULL pointer dereferences are undefined behaviour and are always a potential risk.

Probably you want to enclose your code in a block, so that all of it is governed by the check for NULL, and not just the next line.

score 1 · Answer 4 · 2012-10-02T07:35:54.047

You have to fix these as soon as possible. Or probably sooner. The Standard says that the NULL pointer is a pointer that points to "no valid memory location", so dereferencing it is undefined behaviour. It means that it may work, it may crash, and it may do strange things at other parts of your program, or maybe cause daemons to fly out of your nose.

Fix them. Now.

Here's how: put the dereference statement inside the if - doing otherwise (as you do: checking for NULL then dereferencing anyways) makes no sense.

if (pointer != NULL) {
    something = pointer->field;
}

^^ this is good practice.

score 0 · Answer 5 · answered Oct 02 '12 at 07:27

If you have never experienced problems with this code, it's probably because:

do_lookup(who, me, UNLINK);

always returns a valid pointer.

But what will it happen if this function changes? Or its parameters vary?

You definitely have to check for NULL pointers before dereferencing them.

if (chan)
   prot(get_sec_var(chan->zsets));

If you are absolutely sure that neither do_lookup or its parameters will change in the future (and you can bet the safe execution of your program on it), and the cost of changing all the occurrences of similar functions is excessively high compared to the benefits that you would have in doing so, then:

you may decide to leave your code broken.

Many programmers did that in the past, and many more will do that in the future. Otherwise what would explain the existence of Windows ME?

Ahah what I need is the percentage that confers superhuman **debugging** abilities! — Avio, Oct 02 '12 at 14:11
Some say that debugging is twice as hard as coding, so you could try twice the percentage if you dare. — Daniel Fischer, Oct 02 '12 at 14:24

score 0 · Answer 6 · answered Oct 02 '12 at 08:29

If your program crashes because of a NULL pointer dereference, this can be classified as a Denial of Service (DoS).

If this program is used together with other programs (e.g. they invoke it), the security aspects now start to depend on what those other programs do when this one crashes. The overall effect can be the same DoS or something worse (exploitation, sensitive info leakage, and so on).

If your program does not crash because of a NULL pointer dereference and instead continues running while corrupting itself and possibly the OS and/or other programs within the same address space, you can have a whole spectrum of security issues.

Don't put on the line (or online) broken code, unless you can afford dealing with consequences of potential hacking.

Should I really worry about fixing null defres if there's no crashes?

6 Answers6

"Anything that can go wrong will go wrong"

you may decide to leave your code broken.