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

[vsavchenko]

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.

[vsavchenko]

Add one more test

[vsavchenko]

Add forgotten hunk

[vsavchenko]

Add extra check to be safe

[NoQ]

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]

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.

[vsavchenko]

Fix review remarks

[NoQ]

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

[ASDenysPetrov]

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

clang/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp

 //===--- NonNullParamChecker.cpp - Undefined arguments checker -*- C++ -*--===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This defines NonNullParamChecker, which checks for arguments expected not to
 // be null due to:
 //   - the corresponding parameters being declared to have nonnull attribute
 //   - the corresponding parameters being references; since the call would form
 //     a reference to a null pointer
 //
 //===----------------------------------------------------------------------===//
 
-#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
 #include "clang/AST/Attr.h"
+#include "clang/Analysis/AnyCall.h"
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 #include "llvm/ADT/StringExtras.h"
 
 using namespace clang;
 using namespace ento;
 
 namespace {
 class NonNullParamChecker
-  : public Checker< check::PreCall, EventDispatcher<ImplicitNullDerefEvent> > {
+    : public Checker<check::PreCall, check::BeginFunction,
+                     EventDispatcher<ImplicitNullDerefEvent>> {
   mutable std::unique_ptr<BugType> BTAttrNonNull;
   mutable std::unique_ptr<BugType> BTNullRefArg;
 
 public:
-
   void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
+  void checkBeginFunction(CheckerContext &C) const;
 
   std::unique_ptr<PathSensitiveBugReport>
-  genReportNullAttrNonNull(const ExplodedNode *ErrorN,
-                           const Expr *ArgE,
+  genReportNullAttrNonNull(const ExplodedNode *ErrorN, const Expr *ArgE,
                            unsigned IdxOfArg) const;
   std::unique_ptr<PathSensitiveBugReport>
   genReportReferenceToNullPointer(const ExplodedNode *ErrorN,
                                   const Expr *ArgE) const;
 };
-} // end anonymous namespace
 
-/// \return Bitvector marking non-null attributes.
-static llvm::SmallBitVector getNonNullAttrs(const CallEvent &Call) {
+template <class CallType>
+void setBitsAccordingToFunctionAttributes(const CallType &Call,
+                                          llvm::SmallBitVector &AttrNonNull) {
   const Decl *FD = Call.getDecl();
-  unsigned NumArgs = Call.getNumArgs();
-  llvm::SmallBitVector AttrNonNull(NumArgs);
+
   for (const auto *NonNull : FD->specific_attrs<NonNullAttr>()) {
     if (!NonNull->args_size()) {
-      AttrNonNull.set(0, NumArgs);
+      // Lack of attribute parameters means that all of the parameters are
+      // implicitly marked as non-null.
+      AttrNonNull.set();
       break;
     }
+
     for (const ParamIdx &Idx : NonNull->args()) {
+      // 'nonnull' attribute's parameters are 1-based and should be adjusted to
+      // match actual AST parameter/argument indices.
       unsigned IdxAST = Idx.getASTIndex();
-      if (IdxAST >= NumArgs)
+      if (IdxAST >= AttrNonNull.size())
         continue;
       AttrNonNull.set(IdxAST);
     }
   }
+}
+
+template <class CallType>
+void setBitsAccordingToParameterAttributes(const CallType &Call,
+                                           llvm::SmallBitVector &AttrNonNull) {
+  for (const ParmVarDecl *Parameter : Call.parameters()) {
+    unsigned ParameterIndex = Parameter->getFunctionScopeIndex();
+    if (ParameterIndex == AttrNonNull.size())
+      break;
+
+    if (Parameter->hasAttr<NonNullAttr>())
+      AttrNonNull.set(ParameterIndex);
+  }
+}
+
+template <class CallType>
+llvm::SmallBitVector getNonNullAttrsImpl(const CallType &Call,
+                                         unsigned ExpectedSize) {
+  llvm::SmallBitVector AttrNonNull(ExpectedSize);
+
+  setBitsAccordingToFunctionAttributes(Call, AttrNonNull);
+  setBitsAccordingToParameterAttributes(Call, AttrNonNull);
+
   return AttrNonNull;
 }
 
+/// \return Bitvector marking non-null attributes.
+llvm::SmallBitVector getNonNullAttrs(const CallEvent &Call) {
+  return getNonNullAttrsImpl(Call, Call.getNumArgs());
+}
+
+/// \return Bitvector marking non-null attributes.
+llvm::SmallBitVector getNonNullAttrs(const AnyCall &Call) {
+  return getNonNullAttrsImpl(Call, Call.param_size());
+}
+} // end anonymous namespace
+
 void NonNullParamChecker::checkPreCall(const CallEvent &Call,
                                        CheckerContext &C) const {
   if (!Call.getDecl())
     return;
 
   llvm::SmallBitVector AttrNonNull = getNonNullAttrs(Call);
   unsigned NumArgs = Call.getNumArgs();
 
   ProgramStateRef state = C.getState();
-  ArrayRef<ParmVarDecl*> parms = Call.parameters();
+  ArrayRef<ParmVarDecl *> parms = Call.parameters();
 
   for (unsigned idx = 0; idx < NumArgs; ++idx) {
     // For vararg functions, a corresponding parameter decl may not exist.
     bool HasParam = idx < parms.size();
 
     // Check if the parameter is a reference. We want to report when reference
     // to a null pointer is passed as a parameter.
-    bool haveRefTypeParam =
+    bool HasRefTypeParam =
         HasParam ? parms[idx]->getType()->isReferenceType() : false;
-    bool haveAttrNonNull = AttrNonNull[idx];
+    bool ExpectedToBeNonNull = AttrNonNull.test(idx);
 
-    // Check if the parameter is also marked 'nonnull'.
-    if (!haveAttrNonNull && HasParam)
-      haveAttrNonNull = parms[idx]->hasAttr<NonNullAttr>();
-
-    if (!haveAttrNonNull && !haveRefTypeParam)
+    if (!ExpectedToBeNonNull && !HasRefTypeParam)
       continue;
 
     // If the value is unknown or undefined, we can't perform this check.
     const Expr *ArgE = Call.getArgExpr(idx);
     SVal V = Call.getArgSVal(idx);
     auto DV = V.getAs<DefinedSVal>();
     if (!DV)
       continue;
 
-    assert(!haveRefTypeParam || DV->getAs<Loc>());
+    assert(!HasRefTypeParam || DV->getAs<Loc>());
 
     // Process the case when the argument is not a location.
-    if (haveAttrNonNull && !DV->getAs<Loc>()) {
+    if (ExpectedToBeNonNull && !DV->getAs<Loc>()) {
       // If the argument is a union type, we want to handle a potential
       // transparent_union GCC extension.
       if (!ArgE)
         continue;
 
       QualType T = ArgE->getType();
       const RecordType *UT = T->getAsUnionType();
       if (!UT || !UT->getDecl()->hasAttr<TransparentUnionAttr>())
for (unsigned idx = 0; idx < NumArgs; ++idx) {
     std::tie(stateNotNull, stateNull) = CM.assumeDual(state, *DV);
 
     // Generate an error node.  Check for a null node in case
     // we cache out.
     if (stateNull && !stateNotNull) {
       if (ExplodedNode *errorNode = C.generateErrorNode(stateNull)) {
 
         std::unique_ptr<BugReport> R;
-        if (haveAttrNonNull)
+        if (ExpectedToBeNonNull)
           R = genReportNullAttrNonNull(errorNode, ArgE, idx + 1);
-        else if (haveRefTypeParam)
+        else if (HasRefTypeParam)
           R = genReportReferenceToNullPointer(errorNode, ArgE);
 
         // Highlight the range of the argument that was null.
         R->addRange(Call.getArgSourceRange(idx));
 
         // Emit the bug report.
         C.emitReport(std::move(R));
       }
 
       // Always return.  Either we cached out or we just emitted an error.
       return;
     }
 
     if (stateNull) {
       if (ExplodedNode *N = C.generateSink(stateNull, C.getPredecessor())) {
         ImplicitNullDerefEvent event = {
             V, false, N, &C.getBugReporter(),
-          /*IsDirectDereference=*/haveRefTypeParam};
+            /*IsDirectDereference=*/HasRefTypeParam};
         dispatchEvent(event);
       }
     }
 
     // If a pointer value passed the check we should assume that it is
     // indeed not null from this point forward.
     state = stateNotNull;
   }
 
   // If we reach here all of the arguments passed the nonnull check.
   // If 'state' has been updated generated a new node.
   C.addTransition(state);
 }
 
+/// We want to trust developer annotations and consider all 'nonnull' parameters
+/// as non-null indeed. Each marked parameter will get a corresponding
+/// constraint.
+///
+/// This approach will not only help us to get rid of some false positives, but
+/// remove duplicates and shorten warning traces as well.
+///
+/// \code
+///   void foo(int *x) [[gnu::nonnull]] {
+///     // . . .
+///     *x = 42;    // we don't want to consider this as an error...
+///     // . . .
+///   }
+///
+///   foo(nullptr); // ...and report here instead
+/// \endcode
+void NonNullParamChecker::checkBeginFunction(CheckerContext &Context) const {
+  // Planned assumption makes sense only for top-level functions.
+  // Inlined functions will get similar constraints as part of 'checkPreCall'.
+  if (!Context.inTopFrame())
+    return;
+
+  const LocationContext *LocContext = Context.getLocationContext();
+
+  const Decl *FD = LocContext->getDecl();
+  // AnyCall helps us here to avoid checking for FunctionDecl and ObjCMethodDecl
+  // separately and aggregates interfaces of these classes.
+  auto AbstractCall = AnyCall::forDecl(FD);
+  if (!AbstractCall)
+    return;
+
+  ProgramStateRef State = Context.getState();
+  llvm::SmallBitVector ParameterNonNullMarks = getNonNullAttrs(*AbstractCall);
+
+  for (const ParmVarDecl *Parameter : AbstractCall->parameters()) {
+    // 1. Check parameter if it is annotated as non-null
+    if (!ParameterNonNullMarks.test(Parameter->getFunctionScopeIndex()))
+      continue;
+
+    Loc ParameterLoc = State->getLValue(Parameter, LocContext);
+    // We never consider top-level function parameters undefined.
+    auto StoredVal =
+        State->getSVal(ParameterLoc).castAs<DefinedOrUnknownSVal>();
+
+    // 2. Assume that it is indeed non-null
+    if (ProgramStateRef NewState = State->assume(StoredVal, true)) {
+      State = NewState;
+    }
+  }
+
+  Context.addTransition(State);
+}
+
 std::unique_ptr<PathSensitiveBugReport>
 NonNullParamChecker::genReportNullAttrNonNull(const ExplodedNode *ErrorNode,
                                               const Expr *ArgE,
                                               unsigned IdxOfArg) const {
   // Lazily allocate the BugType object if it hasn't already been
   // created. Ownership is transferred to the BugReporter object once
   // the BugReport is passed to 'EmitWarning'.
   if (!BTAttrNonNull)

clang/test/Analysis/CheckNSError.m

 + (id)errorWithDomain:(NSString *)domain code:(NSInteger)code userInfo:(NSDictionary *)dict;
 @end
 extern NSString * const NSXMLParserErrorDomain ;
 
 @interface A
 - (void)myMethodWhichMayFail:(NSError **)error;
 - (BOOL)myMethodWhichMayFail2:(NSError **)error;
 - (BOOL)myMethodWhichMayFail3:(NSError **_Nonnull)error;
+- (BOOL)myMethodWhichMayFail4:(NSError **)error __attribute__((nonnull));
 @end
 
 @implementation A
 - (void)myMethodWhichMayFail:(NSError **)error {   // expected-warning {{Method accepting NSError** should have a non-void return value to indicate whether or not an error occurred}}
   *error = [NSError errorWithDomain:@"domain" code:1 userInfo:0]; // expected-warning {{Potential null dereference}}
 }
 
 - (BOOL)myMethodWhichMayFail2:(NSError **)error {  // no-warning
   if (error) *error = [NSError errorWithDomain:@"domain" code:1 userInfo:0]; // no-warning
   return 0;
 }
 
 - (BOOL)myMethodWhichMayFail3:(NSError **_Nonnull)error {         // no-warning
   *error = [NSError errorWithDomain:@"domain" code:1 userInfo:0]; // no-warning
   return 0;
 }
+
+- (BOOL)myMethodWhichMayFail4:(NSError **)error {                 // no-warning
+  *error = [NSError errorWithDomain:@"domain" code:1 userInfo:0]; // no-warning
+  return 0;
+}
 @end
 
 struct __CFError {};
 typedef struct __CFError* CFErrorRef;
 
 void foo(CFErrorRef* error) { // expected-warning {{Function accepting CFErrorRef* should have a non-void return value to indicate whether or not an error occurred}}
   *error = 0;  // expected-warning {{Potential null dereference}}
 }
 
 int f1(CFErrorRef* error) {
   if (error) *error = 0; // no-warning
   return 0;
 }
 
 int f2(CFErrorRef* error) {
   if (0 != error) *error = 0; // no-warning
   return 0;
 }
 
 int f3(CFErrorRef* error) {
   if (error != 0) *error = 0; // no-warning
   return 0;
 }
 
+int __attribute__((nonnull)) f4(CFErrorRef *error) {
+  *error = 0; // no-warning
+  return 0;
+}
+
+int __attribute__((nonnull(1))) f5(int *x, CFErrorRef *error) {
+  *error = 0; // expected-warning {{Potential null dereference}}
+  return 0;
+}
 
+int __attribute__((nonnull(2))) f6(int *x, CFErrorRef *error) {
+  *error = 0; // no-warning
+  return 0;
+}

clang/test/Analysis/UserNullabilityAnnotations.m

+// RUN: %clang_analyze_cc1 -verify -Wno-objc-root-class %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=nullability \
+// RUN:   -analyzer-checker=debug.ExprInspection
+
+void clang_analyzer_eval(int);
+
+@interface TestFunctionLevelAnnotations
+- (void)method1:(int *_Nonnull)x;
+- (void)method2:(int *)x __attribute__((nonnull));
+@end
+
+@implementation TestFunctionLevelAnnotations
+- (void)method1:(int *_Nonnull)x {
+  clang_analyzer_eval(x != 0); // expected-warning{{TRUE}}
+}
+
+- (void)method2:(int *)x {
+  clang_analyzer_eval(x != 0); // expected-warning{{TRUE}}
+}
+@end
+
+typedef struct NestedNonnullMember {
+  struct NestedNonnullMember *Child;
+  int *_Nonnull Value;
+} NestedNonnullMember;
+
+NestedNonnullMember *foo();
+
+void f1(NestedNonnullMember *Root) {
+  NestedNonnullMember *Grandson = Root->Child->Child;
+
+  clang_analyzer_eval(Root->Value != 0);         // expected-warning{{TRUE}}
+  clang_analyzer_eval(Grandson->Value != 0);     // expected-warning{{TRUE}}
+  clang_analyzer_eval(foo()->Child->Value != 0); // expected-warning{{TRUE}}
+}

clang/test/Analysis/nonnull.cpp

+// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core -verify %s
+
+void nonnull [[gnu::nonnull]] (int *q);
+
+void f1(int *p) {
+  if (p)
+    return;
+  nonnull(p); //expected-warning{{nonnull}}
+}
+
+void f2(int *p) {
+  if (p)
+    return;
+  auto lambda = [](int *q) __attribute__((nonnull)){};
+  lambda(p); //expected-warning{{nonnull}}
+}
+
+template <class... ARGS>
+void variadicNonnull(ARGS... args) __attribute__((nonnull));
+
+void f3(int a, float b, int *p) {
+  if (p)
+    return;
+  variadicNonnull(a, b, p); //expected-warning{{nonnull}}
+}
+
+int globalVar = 15;
+void moreParamsThanArgs [[gnu::nonnull(2, 4)]] (int a, int *p, int b = 42, int *q = &globalVar);
+
+void f4(int a, int *p) {
+  if (p)
+    return;
+  moreParamsThanArgs(a, p); //expected-warning{{nonnull}}
+}