[clang static analyzer] core.NullDereference false positive with `*p`

[hallo-wereld]

version: clang trunk args: --analyze -Xclang -analyzer-stats -Xclang -analyzer-checker=core,debug.ExprInspection

[core.NullDereference] disappears when the if branch is reachable, however, [core.NullDereference] appears when the if branch is non-reachable. In both cases, the p is not a null pointer.

See it live: https://godbolt.org/z/P46z5eYxr https://godbolt.org/z/df4E8cPoP

Thanks!

[llvmbot]

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

[haoNoQ]

This behaves more or less correctly.

The static analyzer successfully proves that either this program contains null dereference, or it contains dead code. Either of those outcomes warrants a warning from our point of view. In other words, the code clearly doesn't make sense. And there's nothing more you can expect from static analysis, than discovering some code that doesn't make sense. Every stronger claim relies on assumptions that aren't part of the source code, such as "this code is actually executed".

The analyzer never says that the program has a null dereference. The full output (https://godbolt.org/z/vMG5Pz1b4) (which always needs to be seen - either through a user interface like scan-build, or at least by passing --analyzer-output text) contains the clause

<source>:8:9: note: Assuming 'p' is equal to null
    if (p == 0)
        ^~~~~~

so even if the assumption is invalid, the analyzer's claim "Assuming this can happen, there's null dereference" is still technically true (albeit a vacuous truth, due to code being completely dead).

What can we do better in this case? That's a very rare case where we see the program's main() so we could take advantage of that in order to prove that the function is actually never called so it's dead code; there's enough information in the program for that. Even then, we'll probably emit a warning about dead code; what we're already doing is even better than that because we not only identify potential dead code, but also notice that the code wouldn't make sense even if it wasn't dead. Even if the correct fix is probably to remove the dead function, our warning is quite actionable and desired by a good portion of users. And technically there are other ways to call this non-static function, even if it's not reachable from main(), such as dlopen-ing the binary; which would be weird, but so is the dead function! So the analyzer analyzes the function contains_null_check() "out of context" and comes to correct conclusions based on the assumption that there exists a different call site that it doesn't see.

Now, there's also an actual bug in the static analyzer in this case - the warning actually makes two assumptions:

<source>:7:25: note: Assuming 't' is not equal to 0
    clang_analyzer_eval(t == 0);
                        ^~~~~~
<source>:8:9: note: Assuming 'p' is equal to null
    if (p == 0)

which is clearly wrong as these assumptions are never true simultaneously. There's more than enough information in the source code to refute this possibility. This must have something to do with our constraint solver being unable to handle equations with integer-to-pointer casts correctly :(

[haoNoQ]

Just to add to this, @hallo-wereld, most of what I said probably doesn't apply to your original problem; just to the reduced example. If you're just trying to figure out how this thing works, my reply probably covers it (with #61669 being another good read about the limits of mental gymnastics I demonstrated here). But if you're trying to deal with a false positive in real-world code, it's probably better to report it as close to original as possible, without trying to reduce it to a minimal example. This will help us figure out how practical or impractical was the original warning, and it's likely that the underlying root cause of the false positive may be completely different from the reduced example. I usually don't trust myself to reduce the reproducer before I debug it, so you probably shouldn't too!

[hallo-wereld]

That helps me a lot! Thanks!!! @haoNoQ 👍 👍 👍

[0x21af]

But if you're trying to deal with a false positive in real-world code, it's probably better to report it as close to original as possible, without trying to reduce it to a minimal example. This will help us figure out how practical or impractical was the original warning, and it's likely that the underlying root cause of the false positive may be completely different from the reduced example. I usually don't trust myself to reduce the reproducer before I debug it, so you probably shouldn't too!

Hi, @haoNoQ, I've noticed that the reducer.pl in the repo (https://github.com/llvm/llvm-project/blob/main/clang/utils/analyzer/reducer.pl) utilizes the reducer from UC Berkeley. I'm curious, and I want to ask, why wasn't C-Reduce chosen for this task? Could it be because the root cause of a false positive is more prone to alteration after reducing the case with C-Reduce?

[haoNoQ]

Hi, @haoNoQ, I've noticed that the reducer.pl in the repo (https://github.com/llvm/llvm-project/blob/main/clang/utils/analyzer/reducer.pl) utilizes the reducer from UC Berkeley. I'm curious, and I hope you don't mind me asking, why wasn't C-Reduce chosen for this task? Could it be because the root cause of a false positive is more prone to alteration after reducing the case with C-Reduce?

That's probably just because creduce didn't exist back then. They both appear to be around 12 years old.

I don't think this is about chances to reduce false positives to true positives. No matter the tool, if you can't come up with a formal test to verify the output, the chances of reducing incorrectly are so high that I don't recommend them at all.