False negative in following pointer for core.NullDereference

[whisperity]

Bugzilla Link	34193
Version	trunk
OS	Linux
Attachments	Exploded graph for core.NullDereference of the example code
CC	@devincoughlin,@AnnaZaks,@Xazax-hun

Extended Description

Consider the following code:

int a() { return nullptr; } int main() { int ptr = a(); int x = *ptr; return x; }

While this is clearly a null dereference, ClangSA does not seem to properly follow this pointer in the analyzer, or some inner heuristics swallow this case.

[Xazax-hun]

We should investigate weakening the inline defensive check heuristics.

We'll have to be careful about the pattern:

int *a() { if (! (precondition the analyzer can't reason about)) return nullptr;

// do other stuff }

I think we don't want defensive precondition checking in the callee to all of a sudden cause null pointer false positives in the caller. But hopefully we can weaken the heuristic to catch more issues.

This is tracked internally as rdar://problem/29759433.

Weakening sounds good. Do you have something in mind? E.g.: if the source of the value is not protected by any condition within the function? I am wondering what is the canonical place to do brainstorming. The bug tracker? The mailing list? Phabricator patches?

This heuristic also feels somewhat specialized. Maybe it is worth to think about also generalizing it for other checks that do not deal with the zero literal (or a value at all).

[llvmbot]

Yes that is presently the case.

The function ReturnVisitor::getEndPath will just mark the Bug Report as invalid. Maybe having some counter suppression could help here?

[whisperity]

We'll have to be careful about the pattern:

int *a() { if (! (precondition the analyzer can't reason about)) return nullptr;

// do other stuff }

If I understand it correctly, if the a function is this:

int* a() { if (X) // X can be reasoned about return nullptr;

// stuff return valid_ptr; }

the analyser still throws away this case, because it's a "cross-function nullptr return" as comment #​4 said?

Because in this case, we can lose true positives too.

[0f73b9cf-134f-41af-a8b1-14d9f305ee95]

To add a historical perspective here, even though the existing heuristic is probably not narrow enough and can be improved, it suppresses real false positives. The analyzer generally tries to prefer false negatives over false positives since dealing with false positives is generally a pain for any static analysis users.

[llvmbot]

Agreed, opening the net wider will catch more issues but will increase the false positives. Supplemental test data would be helpful to justify the need for disabling/enabling this behavior.

Some Technical info: The BugReporter checks the validity of each bug (by analyzing the exploded graph). While doing this it rejects all cross function null dereference warnings (see BugReporterVisitor.cpp). There are no semantic checks on the validity of the bug report; but rather this blindly throws away all issues that fall under this category. Some intelligence place here could make npd checks a lot more effective.

[devincoughlin]

We should investigate weakening the inline defensive check heuristics.

We'll have to be careful about the pattern:

int *a() { if (! (precondition the analyzer can't reason about)) return nullptr;

// do other stuff }

I think we don't want defensive precondition checking in the callee to all of a sudden cause null pointer false positives in the caller. But hopefully we can weaken the heuristic to catch more issues.

This is tracked internally as rdar://problem/29759433.

[llvmbot]

Hi Whisperity,

You can change this behavior by adding the flag:

-suppress-null-return-paths=false

Although keep in mind, this might increase the number of False Positives.

-Nikhil

[whisperity]

assigned to @tkremenek