[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

214

Typo :p

230

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().

234–235

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

249

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

214

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

230

Gotcha!

234–235

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()) {
+    if (Parameter->hasAttr<NonNullAttr>())
+      AttrNonNull.set(Parameter->getFunctionScopeIndex());
+  }
+}
+
+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));
       }
for (unsigned idx = 0; idx < NumArgs; ++idx) {
     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 'nonnul' 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 {
+  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);
+    auto StoredVal = State->getSVal(ParameterLoc).getAs<loc::MemRegionVal>();
+    if (!StoredVal)
+      continue;
+
+    // 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

int f2(CFErrorRef* error) {
   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/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}}
+}