[cfe-dev] [analyzer] Questions about the null dereference checker

[Artem Dergachev via cfe-dev]

On 9/20/21 5:42 AM, Deep Majumder via cfe-dev wrote:
> The second one seems about right to me.

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.

> 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.

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.

> Warm regards,
> Deep
>
> On Mon, 20 Sep, 2021, 6:03 pm via cfe-dev, <cfe-dev at lists.llvm.org 
> <mailto:cfe-dev at lists.llvm.org>> wrote:
>
>     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?
>
>     ----
>
>     By returning a null pointer from a function it suddenly we no
>     longer report an error:
>
>       int *get() { return 0; }
>
>       void foo() {
>
>         int *p = get();
>
>         *p = 42; // no warning?
>
>       }
>
>     According to my investigation the bug actually found and a sink
>     node will be generated in the Exploded graph, but the bug report
>     will be marked as invalid by the ReturnVisitor.
>
>     This behavior could be altered to prevent such suppression from
>     happening by setting the `suppress-null-return-paths` analyzer
>     option to `true`.
>
>     Am I right that this is the intentional behavior and if we want to
>     catch bugs like this, then we should enable the aforementioned option?
>
>     /CC NoQ
>
>     Regards,
>
>     Balazs
>
>     _______________________________________________
>     cfe-dev mailing list
>     cfe-dev at lists.llvm.org <mailto:cfe-dev at lists.llvm.org>
>     https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>     <https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev>
>
>
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20210920/882bdb57/attachment.html>