Clang Static Analyzer `core.NullDereference` false positive for `*e = (0 == e);`

[Geoffrey1014]

I got a false positive error when compiling the following "minimal, complete and verifiable example (MCVE)" program:

# include <stdio.h>

void a(int *e) {
//   printf("NPD_FLAG: %d\n", e[0]);
    *e = (0 == e);
}
int main() {
    int d[4];
    for (int c =0 ; c < 4; c++)
        ;
    a(d);
    return 0;
}

Compiling the above code with Clang (trunk) with --analyze --analyzer-output text in https://godbolt.org/z/nfrbd4P5h results in :

<source>:5:8: warning: Dereference of null pointer (loaded from variable 'e') [core.NullDereference]
    *e = (0 == e);
     ~ ^
<source>:5:11: note: Assuming 'e' is equal to null
    *e = (0 == e);
          ^~~~~~
<source>:5:8: note: Dereference of null pointer (loaded from variable 'e')
    *e = (0 == e);
     ~ ^
1 warning generated.
Compiler returned: 0

But the false positive error will disappear, if I change the "c < 4" to "c < 3" or comment lines 9-10 as the following two:

# include <stdio.h>

void a(int *e) {
//   printf("NPD_FLAG: %d\n", e[0]);
    *e = (0 == e);
}
int main() {
    int d[4];
    for (int c =0 ; c < 3; c++)
        ;
    a(d);
    return 0;
}

# include <stdio.h>

void a(int *e) {
//   printf("NPD_FLAG: %d\n", e[0]);
    *e = (0 == e);
}
int main() {
    int d[4];
    //for (int c =0 ; c < 4; c++)
    //    ;
    a(d);
    return 0;
}

[llvmbot]

@llvm/issue-subscribers-clang-static-analyzer

[martong]

TLDR, a is being analyzed as a top-level function and from main via the call expression a(d). You get the warning message from the top-level analysis. When a is analysed as a top-level function, the engine does not consider the possible call sites, and in this sense it assumes that e can be a nullptr, and that is correct. Since a is visible it might be called from another translation unit, e.g. with a(nullptr).

...

Details: Let's discuss the top-level analysis of main. If the widen-loops option is not set then we model only 4 iterations of loops and then the analyzer stops on that path by putting a sink node (ExprEngine.cpp: ExprEngine::processCFGBlockEntrance).

So, when the loop limit is 3 then the call expression of a(d) is analyzed, however, it is not being analyzed if the limit is 4. When a function is being analyzed as part of a call expression then it is no longer analyzed as a top level function. (We do a topological sort of the Call Graph and start with the analysis of main.) Thus, in the case of the loop limit 3 , a is not analyzed as a top-level function. You get the warning from the top-level analysis of a.

You can see which functions and in what order are analyzed top-level here (look for ANALYZE (Path, Inline_Regular)): https://godbolt.org/z/xKxTajdvx

Now what you can do about it? You can set -Xclang -analyzer-config -Xclang widen-loops=true to enable loop widening (or you can change the maximum allowed block visit count), this way a will be analyzed from main and will not be analyzed as a top-level. On a long term, I think we should have a better loop widening (implemented with dataflow analysis) and then we could confidently enable loop widening by default.

[haoNoQ]

I think @martong's explanation is perfect. I'd like to make a high-level comment about the static analyzer's strategy and why such warning is generally desired.

The static analyzer successfully proves, by looking at function a() in isolation, that either there's a null dereference in that function, or there's dead code. Indeed, if the pointer can't be null, why check for null? And if it can be null, why dereference unconditionally?

This is what the word "Assuming..." in the note text means. It means that the static analyzer has no reason to believe that it knows which branch is taken, so it tries to explore both possibilities, because otherwise the code already doesn't make sense.

So basically the static analyzer operates under the assumption that there's no dead code in the program. An MCVE program can have dead code, therefore it doesn't satisfy this assumption.

In practice it can go both ways. Some users care about eliminating dead code, some users don't. We have found that generally users are happy to discover that their code is dead this way. It contributes nicely to every programmer's constant struggle against complexity of their systems.

But, yeah, you always have to read the complete path notes, not just the warning itself. The intended message is not "This program has null dereference". Instead it says "This program has null dereference assuming...", where the assumed condition may or may not be feasible, but one way or another the code doesn't make sense. The actual fix may either go towards dereference, or towards the null check, or anywhere in between, that's why we have all these notes, to highlight every operation that contributes to the defective path.

[Geoffrey1014]

@haoNoQ @martong Thanks a lot for your detailed explaination !