[analyzer] Questions about the null dereference checker

Hi,

Let’s examine this code snippet:

void simply_deref_null() {

int *p = 0;

*p ; // no warning?

*p = 42; // warns!

}

Turns out the NullDereference checker treats the two pointer derefs differently.

For simply reading through a null pointer is allowed but storing a value is prohibited.

Why don’t we prohibit reading through null pointers?

The second one seems about right to me. The first one is a bit perplexing. In fact, if p were a unique_ptr, the first one would have emitted a warning on *p.
Warm regards,
Deep

Hi,

Thanks Harald,

I missed that.

On the other hand, I still think that we should be consistent. (Hereby thanks for Deep for the unique_ptr case.)

And, by definition we dereferenced a null pointer, so the abstract machine should halt.

That being said, a slightly modified version of the same code:

https://godbolt.org/z/Eenavva8x

constexpr void foo() {

int *p = 0;

*p = 44;

}

This code won’t compile, and reports an error:

error: constexpr function never produces a constant expression [-Winvalid-constexpr]

note: assignment to dereferenced null pointer is not allowed in a constant expression

So, in this sense, the abstract machine (aka. the analyzer) should recognize this null dereference IMO.

That way it would be consistent with the users expectation, also with the store (lvalue) case and with the unique_ptr case.

Balazs

Again, *p is not a dereference. The result of this expression is still the location, not the value read from that location. You can easily convert this location back to the numeric value of the address: &*p. This is completely normal code with null pointers that does not constitute any UB and in fact has practical uses: for example, you can calculate offset of field x in struct S as (uintptr_t)&(((S *)0)->x), people have macros like this.

On the contrary, *p in (*p + 1) or *p in (x = p) or in your example are actual dereferences because they are contextually converted to rvalues in order to perform the surrounding operations. _This conversion is the dereference, not the operator on its own._ The operator simply converts rvalue to lvalue - it still represents a location.

Operators * and & are modeled in the static analyzer as no-op for the above reason: they really don’t do anything on their own. The implicit cast from lvalue to rvalue is the read and this is what the checker reacts to. This is also why I’m nervous every time I see IgnoringParenImpCasts() in the code: some of these implicit casts are really important and shouldn’t be missed most of the time.

You should read AST dumps of these expressions for more clarity, it really helped me back in the day.

The second example is an example of so-called inlined-defensive-check suppression (2nd type, to be exact). The specific warning in your example would have been a true positive but in most practical examples it wouldn’t be. Inlined-defensive-check suppressions are a partial solution that tries to eliminate infeasible paths that arise from the huge difference between state splits in nested (“inlined”) stack frames and state splits in the top-level stack frame. Namely, a state split in the top-level stack frame is justified by saying that “if both states aren’t possible then one of the branches is dead code”. However, in a nested stack frame that would only mean that the code is dead for the current call site but there may be other call sites on which that code is executed. Fundamentally speaking, this is a very real architectural problem with inlining. We need summaries or call widening to deal with it properly but as of today we only have this partial solution. As an example, void foo(int *p) { if (p) {} *p = 1; // null deref? } is a true positive because if it was impossible for p to be null then the check is dead. However, void bar(int *p) { if (p) {} // defensive check } void foo(int *p) { bar(p); // inline the defensive check *p = 1; // null deref? } is a false positive because there’s no indication in the code that the false-branch can in fact be hit from the call site within foo(). Inlined-defensive-check suppressions of the first type suppress null dereference warnings where the null pointer is tracked back (with the help of trackExpressionValue()) to a state split in a nested stack frame. Inlined-defensive-check suppressions of the second type deal with the fact that it is common for an unrelated defensive check to return null. For example: int *bar(int x) { if (x == 0) // defensive check return nullptr; // the defensive behavior return new int; } void foo(int x) { int *p = bar(x); // inline the defensive check *p = 1; // null deref? } In this case the null dereference would, again, be a false positive because there’s no indication that x can be zero at the current call site. Inlined-defensive-check suppressions of the second type suppress null dereference warnings where the null pointer is tracked back (with the help of trackExpressionValue()) to a ‘return nullptr’ in a nested stack frame. This takes care of this other example. Unfortunately this specific implementation also suppresses your example where there’s no defensive check to begin with. However, in practice such functions don’t exist: there’s no reason for a function to have a return value if it’s always null. So it’s an extremely minor false negative. You can fix it if you want but I honestly think it’ll do more harm than good to mess with this already-flimsy facility. Yes, to add to my other email, the rules for smart pointers are different. Invoking overloaded operator * or operator → on a null smart pointer is an immediate undefined behavior according to the standard even if no actual dereference is being performed after the fact. In particular, this clearly makes a lot of sense for operator * because it would otherwise have to produce a null lvalue reference as its return value. So there’s no consistency in the language rules in the first place.

Thanks for the clarification Artem!

All clear I think :slight_smile:

PS: The defensive checks explanation was really useful. I appreciate that.

Regards, Balazs