Page MenuHomePhabricator

[analyzer] Do not report CFError null dereference for nonnull params
ClosedPublic

Authored by vsavchenko on Apr 9 2020, 7:23 AM.

Details

Summary

We want to trust user type annotations and stop assuming pointers declared
as nonnull still can be null. This functionality is implemented as part
of NonNullParamChecker because it already checks parameter attributes.
Whenever we start analyzing a new function, we assume that all parameters
with 'nonnull' attribute are indeed non-null.

Depends on D77722.

Diff Detail

Event Timeline

vsavchenko created this revision.Apr 9 2020, 7:23 AM
vsavchenko updated this revision to Diff 256306.Apr 9 2020, 7:47 AM

Add one more test

vsavchenko updated this revision to Diff 256318.Apr 9 2020, 8:22 AM

Add forgotten hunk

Add extra check to be safe

NoQ added a comment.Apr 9 2020, 7:28 PM

We're doing much more here than just fixing the CFError behavior; we're actually making the whole analyzer respect these annotations in top frame.

Let's add checker-inspecific tests. We have a test checker debug.ExprInspection just for that:

// RUN: %clang_analyze_cc1 ... -analyzer-checker=debug.ExprInspection ...
void clang_analyzer_eval(bool);

void foo(void *x) __attribute__((nonnull)) {
  clang_analyzer_eval(x != nullptr); // expected-warning{{TRUE}}
}
clang/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
218

Typo :p

234

As far as i understand, you should only do all this for top-level functions, i.e. the ones from which we've started the analysis. You can skip inlined functions here because a similar assumption is already made for their parameters in checkPreCall.

You can figure out if you're in top frame via Context.inTopFrame().

238–239

Nice! Should we add new tests for ObjC method calls as well then?

253

Once you restrict yourself to top frame, you can save two lines of code and one asterisk by doing .castAs<DefinedOrUnknownSVal>() here instead. Because we'll never assume that we're being fed an undefined value in an out-of-context top-level function. A much stronger assertion can probably be made.

clang/test/Analysis/nonnull.cpp
29

C-style variadic functions are the real problem. Variadic templates are easy; they're just duplicated in the AST as many times as necessary and all the parameter declarations are in place. Default arguments are also easy; the argument expression is still present in the AST even if it's not explicitly written down at the call site. C-style variadic functions are hard because they actually have more arguments than they have parameters.

vsavchenko marked 4 inline comments as done.Apr 10 2020, 12:31 AM
In D77806#1973616, @NoQ wrote:

We're doing much more here than just fixing the CFError behavior; we're actually making the whole analyzer respect these annotations in top frame.

Let's add checker-inspecific tests. We have a test checker debug.ExprInspection just for that:

// RUN: %clang_analyze_cc1 ... -analyzer-checker=debug.ExprInspection ...
void clang_analyzer_eval(bool);

void foo(void *x) __attribute__((nonnull)) {
  clang_analyzer_eval(x != nullptr); // expected-warning{{TRUE}}
}

Cool, I didn't know about that! I'll add a group of tests for user annotations then.

clang/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
218

Whoops! It even got me a full minute or two to spot it now!

234

Gotcha!

238–239

Good catch!

clang/test/Analysis/nonnull.cpp
29

C-style variadic functions are already covered in nonnull.m. I added here C++-specific cases.

Fix review remarks

vsavchenko marked 5 inline comments as done.Apr 10 2020, 2:15 AM
NoQ accepted this revision.Apr 14 2020, 7:17 AM

Looks great. I'll commit this one as well.

This revision is now accepted and ready to land.Apr 14 2020, 7:17 AM
This revision was automatically updated to reflect the committed changes.

@vsavchenko
Seems you could fix this bug https://bugs.llvm.org/show_bug.cgi?id=24876
Could you check it, please?