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

[via cfe-dev]

Thanks for the clarification Artem!

All clear I think :)

 

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

 

Regards, Balazs

 

From: Artem Dergachev <noqnoqneo at gmail.com> 
Sent: 2021. szeptember 20., hétfő 22:16
To: Deep Majumder <deep.majumder2019 at gmail.com>; Benics Balázs <benicsbalazs at gmail.com>
Cc: cfe-dev <cfe-dev at lists.llvm.org>
Subject: Re: [cfe-dev] [analyzer] Questions about the null dereference checker

 





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





_______________________________________________
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

 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20210921/2105bfbf/attachment-0001.html>