Bug: Unexpected dereference claim on --analyze

Observed output:

It complains because it sees that you are trying to check the pointer
for NULLness, but it can't prove that the branch has a check for it.

Joerg

The analyzer tries to flag the issues which most likely are bugs. (If we restrict to only warning about definite bugs, it would miss a bunch of real bugs.)

In your example, you only get the warning when the function contains the check: (t->root != 0). The reasoning here is that the programmer added this check because there is a possibility of t->root to be 0. If t->root might be 0, you probably only want to dereference it if it's not 0. I suspect that the dereference checker has special heuristic which helps it to identify this case.

Cheers,
Anna.

Klocwork has a similar heuristic, and it proved obnoxious in practice.
~75%-90% of the time, the real problem in the code was that someone
was checking against 0, but the pointer was actually guaranteed not to
be 0, so the check was redundant. Unless the analyzer has other
evidence that the pointer may actually be 0, it should treat
"dereference null" and "redundant check" as similar likelihood and
include both in the warning, or omit the warning.

FWIW,
Jeffrey

Klocwork has a similar heuristic, and it proved obnoxious in practice.
~75%-90% of the time, the real problem in the code was that someone
was checking against 0, but the pointer was actually guaranteed not to
be 0, so the check was redundant. Unless the analyzer has other
evidence that the pointer may actually be 0, it should treat
"dereference null" and "redundant check" as similar likelihood and
include both in the warning, or omit the warning.

After looking at the code, I realize that this is probably a general analyzer heuristic, not specific to NULL dereferencing.

Thanks for the feedback. I think the best solution would be to mention the possible redundant check in the diagnostic. Please, file a bug report!

Anna.

Hi Jeffrey,

Excellent points. I think the heuristic is still valuable, however. Either the pointer could be null, and thus result in a null dereference, or the logic is redundant, and thus the invariants of the code aren’t well documented or well understood. I agree that the latter is less severe than the other. In such cases, I’d argue that the runtime check should be an assert() or the code should use attribute((nonnull)) to document the assumptions in the code. The analyzer cues off of either of these, and so do other programmers.

Do you think it would be reasonable to still emit a warning here, but categorize it differently in the places where we are less certain that the issue is a null dereference?

Cheers,
Ted

This is a place where taint analysis would also be potentially useful. If the pointer value is influenced by a tainted source, it probably should be treated as being a more severe candidate for a real null dereference, even in the cases where we can’t fully prove that a null dereference occurred.

Klocwork has a similar heuristic, and it proved obnoxious in
practice. ~75%-90% of the time, the real problem in the code was
that someone was checking against 0, but the pointer was actually
guaranteed not to be 0, so the check was redundant. Unless the
analyzer has other evidence that the pointer may actually be 0, it
should treat "dereference null" and "redundant check" as similar
likelihood and include both in the warning, or omit the warning.

Excellent points. I think the heuristic is still valuable,
however. Either the pointer could be null, and thus result in a null
dereference, or the logic is redundant, and thus the invariants of
the code aren't well documented or well understood. [...] Do you
think it would be reasonable to still emit a warning here, but
categorize it differently in the places where we are less certain
that the issue is a null dereference?

Seeing that this little code piece made for such a discussion, I am
tempted to post a slightly bigger testcase, to put this into light.

In this one, the warning only occurs with -DWITH_HACK, and that seems
right to me, following Annas statement

The analyzer tries to flag the issues which most likely are bugs.

and one can most likely argue that the stab-through cast is a danger
zone.
Or maybe clang can be made to understand this trick?

Yes, sorry, I wasn't as clear as I should have been. The warning was
obnoxious only because it asserted that a possibly-null pointer was
definitely being dereferenced, when it meant that either a
possibly-null pointer was being dereferenced OR there was a redundant
check, and it always hid the location of the possibly-redundant check.
I would value getting the "redundant check or null dereference"
warning in my own code: I just want the analyzer to be honest about
what it's finding and not make overly-certain claims based on dodgy
assumptions.

Jeffrey

Makes perfect sense.