[analyzer] Apply the suppression rules to the nil receiver only if the value particip...

We should only suppress a bug report if the IDCed or null returned nil value is directly related to the value we are warning about. This was
not the case for nil receivers - we would suppress a bug report that had an IDCed nil receiver on the path regardless of how it’s
related to the warning.

1) Thread EnableNullFPSuppression parameter through the visitors to differentiate between tracking the value which
is directly responsible for the bug and other values that visitors are tracking (ex: general tracking of nil receivers).
2) in trackNullOrUndef specifically address the case when a value of the message send is nil due to the receiver being nil.

llvm-svn: 178309

diff --git a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h
index fab70e9..6eb5f25 100644 (file)
--- a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h
+++ b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h
@@ -101,21 +101,22 @@ class FindLastStoreBRVisitor
   SVal V;
   bool Satisfied;
 
-public:
-  /// \brief Convenience method to create a visitor given only the MemRegion.
-  /// Returns NULL if the visitor cannot be created. For example, when the
-  /// corresponding value is unknown.
-  static BugReporterVisitor *createVisitorObject(const ExplodedNode *N,
-                                                 const MemRegion *R);
+  /// If the visitor is tracking the value directly responsible for the
+  /// bug, we are going to employ false positive suppression.
+  bool EnableNullFPSuppression;
 
+public:
   /// Creates a visitor for every VarDecl inside a Stmt and registers it with
   /// the BugReport.
-  static void registerStatementVarDecls(BugReport &BR, const Stmt *S);
+  static void registerStatementVarDecls(BugReport &BR, const Stmt *S,
+                                        bool EnableNullFPSuppression);
 
-  FindLastStoreBRVisitor(KnownSVal V, const MemRegion *R)
+  FindLastStoreBRVisitor(KnownSVal V, const MemRegion *R,
+                         bool InEnableNullFPSuppression)
   : R(R),
     V(V),
-    Satisfied(false) {}
+    Satisfied(false),
+    EnableNullFPSuppression(InEnableNullFPSuppression) {}
 
   void Profile(llvm::FoldingSetNodeID &ID) const;
 
@@ -159,16 +160,33 @@ private:
 /// \brief Prints path notes when a message is sent to a nil receiver.
 class NilReceiverBRVisitor
   : public BugReporterVisitorImpl<NilReceiverBRVisitor> {
+
+  /// \brief The reciever to track. If null, all receivers should be tracked.
+  const Expr *TrackedReceiver;
+
+  /// If the visitor is tracking the value directly responsible for the
+  /// bug, we are going to employ false positive suppression.
+  bool EnableNullFPSuppression;
+    
 public:
+    NilReceiverBRVisitor(const Expr *InTrackedReceiver = 0,
+                         bool InEnableNullFPSuppression = false):
+      TrackedReceiver(InTrackedReceiver),
+      EnableNullFPSuppression(InEnableNullFPSuppression) {}
+  
   void Profile(llvm::FoldingSetNodeID &ID) const {
     static int x = 0;
     ID.AddPointer(&x);
+    ID.AddPointer(TrackedReceiver);
+    ID.AddBoolean(EnableNullFPSuppression);
   }
 
   PathDiagnosticPiece *VisitNode(const ExplodedNode *N,
                                  const ExplodedNode *PrevN,
                                  BugReporterContext &BRC,
                                  BugReport &BR);
+
+  static const Expr *getReceiver(const Stmt *S);
 };
 
 /// Visitor that tries to report interesting diagnostics from conditions.
@@ -332,12 +350,15 @@ namespace bugreporter {
 /// \param IsArg Whether the statement is an argument to an inlined function.
 ///              If this is the case, \p N \em must be the CallEnter node for
 ///              the function.
+/// \param EnableNullFPSuppression Whether we should employ false positive
+///         suppression (inlined defensive checks, returned null).
 ///
 /// \return Whether or not the function was able to add visitors for this
 ///         statement. Note that returning \c true does not actually imply
 ///         that any visitors were added.
 bool trackNullOrUndefValue(const ExplodedNode *N, const Stmt *S, BugReport &R,
-                           bool IsArg = false);
+                           bool IsArg = false,
+                           bool EnableNullFPSuppression = true);
 
 const Expr *getDerefExpr(const Stmt *S);
 const Stmt *GetDenomExpr(const ExplodedNode *N);

diff --git a/clang/lib/StaticAnalyzer/Checkers/IdempotentOperationChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/IdempotentOperationChecker.cpp
index 579ba9c..271ba47 100644 (file)
--- a/clang/lib/StaticAnalyzer/Checkers/IdempotentOperationChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/IdempotentOperationChecker.cpp
@@ -423,12 +423,12 @@ void IdempotentOperationChecker::checkEndAnalysis(ExplodedGraph &G,
       if (LHSRelevant) {
         const Expr *LHS = i->first->getLHS();
         report->addRange(LHS->getSourceRange());
-        FindLastStoreBRVisitor::registerStatementVarDecls(*report, LHS);
+        FindLastStoreBRVisitor::registerStatementVarDecls(*report, LHS, false);
       }
       if (RHSRelevant) {
         const Expr *RHS = i->first->getRHS();
         report->addRange(i->first->getRHS()->getSourceRange());
-        FindLastStoreBRVisitor::registerStatementVarDecls(*report, RHS);
+        FindLastStoreBRVisitor::registerStatementVarDecls(*report, RHS, false);
       }
 
       BR.emitReport(report);

diff --git a/clang/lib/StaticAnalyzer/Checkers/UndefCapturedBlockVarChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/UndefCapturedBlockVarChecker.cpp
index f0ca8a8..93812f7 100644 (file)
--- a/clang/lib/StaticAnalyzer/Checkers/UndefCapturedBlockVarChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/UndefCapturedBlockVarChecker.cpp
@@ -91,7 +91,8 @@ UndefCapturedBlockVarChecker::checkPostStmt(const BlockExpr *BE,
         BugReport *R = new BugReport(*BT, os.str(), N);
         if (const Expr *Ex = FindBlockDeclRefExpr(BE->getBody(), VD))
           R->addRange(Ex->getSourceRange());
-        R->addVisitor(new FindLastStoreBRVisitor(*V, VR));
+        R->addVisitor(new FindLastStoreBRVisitor(*V, VR,
+                                             /*EnableNullFPSuppression*/false));
         R->disablePathPruning();
         // need location of block
         C.emitReport(R);

diff --git a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
index 3146e6c..31ba572 100644 (file)
--- a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -137,11 +137,12 @@ class ReturnVisitor : public BugReporterVisitorImpl<ReturnVisitor> {
     MaybeUnsuppress,
     Satisfied
   } Mode;
-  bool InitiallySuppressed;
+
+  bool EnableNullFPSuppression;
 
 public:
   ReturnVisitor(const StackFrameContext *Frame, bool Suppressed)
-    : StackFrame(Frame), Mode(Initial), InitiallySuppressed(Suppressed) {}
+    : StackFrame(Frame), Mode(Initial), EnableNullFPSuppression(Suppressed) {}
 
   static void *getTag() {
     static int Tag = 0;
@@ -151,7 +152,7 @@ public:
   virtual void Profile(llvm::FoldingSetNodeID &ID) const {
     ID.AddPointer(ReturnVisitor::getTag());
     ID.AddPointer(StackFrame);
-    ID.AddBoolean(InitiallySuppressed);
+    ID.AddBoolean(EnableNullFPSuppression);
   }
 
   /// Adds a ReturnVisitor if the given statement represents a call that was
@@ -162,7 +163,8 @@ public:
   /// the statement is a call that was inlined, we add the visitor to the
   /// bug report, so it can print a note later.
   static void addVisitorIfNecessary(const ExplodedNode *Node, const Stmt *S,
-                                    BugReport &BR) {
+                                    BugReport &BR,
+                                    bool InEnableNullFPSuppression) {
     if (!CallEvent::isCallStmt(S))
       return;
     
@@ -207,13 +209,13 @@ public:
     assert(Eng && "Cannot file a bug report without an owning engine");
     AnalyzerOptions &Options = Eng->getAnalysisManager().options;
 
-    bool InitiallySuppressed = false;
-    if (Options.shouldSuppressNullReturnPaths())
+    bool EnableNullFPSuppression = false;
+    if (InEnableNullFPSuppression && Options.shouldSuppressNullReturnPaths())
       if (Optional<Loc> RetLoc = RetVal.getAs<Loc>())
-        InitiallySuppressed = State->isNull(*RetLoc).isConstrainedTrue();
+        EnableNullFPSuppression = State->isNull(*RetLoc).isConstrainedTrue();
 
     BR.markInteresting(CalleeContext);
-    BR.addVisitor(new ReturnVisitor(CalleeContext, InitiallySuppressed));
+    BR.addVisitor(new ReturnVisitor(CalleeContext, EnableNullFPSuppression));
   }
 
   /// Returns true if any counter-suppression heuristics are enabled for
@@ -272,12 +274,14 @@ public:
     // make sure to track it into any further inner functions.
     if (!State->isNull(V).isConstrainedTrue()) {
       BR.markInteresting(V);
-      ReturnVisitor::addVisitorIfNecessary(N, RetE, BR);
+      ReturnVisitor::addVisitorIfNecessary(N, RetE, BR,
+                                           EnableNullFPSuppression);
       return 0;
     }
       
     // If we're returning 0, we should track where that 0 came from.
-    bugreporter::trackNullOrUndefValue(N, RetE, BR);
+    bugreporter::trackNullOrUndefValue(N, RetE, BR, /*IsArg*/ false,
+                                       EnableNullFPSuppression);
 
     // Build an appropriate message based on the return value.
     SmallString<64> Msg;
@@ -289,7 +293,7 @@ public:
       // the report is resurrected as valid later on.
       ExprEngine &Eng = BRC.getBugReporter().getEngine();
       AnalyzerOptions &Options = Eng.getAnalysisManager().options;
-      if (InitiallySuppressed && hasCounterSuppression(Options))
+      if (EnableNullFPSuppression && hasCounterSuppression(Options))
         Mode = MaybeUnsuppress;
 
       if (RetE->getType()->isObjCObjectPointerType())
@@ -360,7 +364,8 @@ public:
       if (!State->isNull(*ArgV).isConstrainedTrue())
         continue;
 
-      if (bugreporter::trackNullOrUndefValue(N, ArgE, BR, /*IsArg=*/true))
+      if (bugreporter::trackNullOrUndefValue(N, ArgE, BR, /*IsArg=*/true,
+                                             EnableNullFPSuppression))
         BR.removeInvalidation(ReturnVisitor::getTag(), StackFrame);
 
       // If we /can't/ track the null pointer, we should err on the side of
@@ -390,7 +395,7 @@ public:
   PathDiagnosticPiece *getEndPath(BugReporterContext &BRC,
                                   const ExplodedNode *N,
                                   BugReport &BR) {
-    if (InitiallySuppressed)
+    if (EnableNullFPSuppression)
       BR.markInvalid(ReturnVisitor::getTag(), StackFrame);
     return 0;
   }
@@ -403,6 +408,7 @@ void FindLastStoreBRVisitor ::Profile(llvm::FoldingSetNodeID &ID) const {
   ID.AddPointer(&tag);
   ID.AddPointer(R);
   ID.Add(V);
+  ID.AddBoolean(EnableNullFPSuppression);
 }
 
 PathDiagnosticPiece *FindLastStoreBRVisitor::VisitNode(const ExplodedNode *Succ,
@@ -487,10 +493,11 @@ PathDiagnosticPiece *FindLastStoreBRVisitor::VisitNode(const ExplodedNode *Succ,
     if (V.isUndef() || V.getAs<loc::ConcreteInt>()) {
       if (!IsParam)
         InitE = InitE->IgnoreParenCasts();
-      bugreporter::trackNullOrUndefValue(StoreSite, InitE, BR, IsParam);
+      bugreporter::trackNullOrUndefValue(StoreSite, InitE, BR, IsParam,
+                                         EnableNullFPSuppression);
     } else {
       ReturnVisitor::addVisitorIfNecessary(StoreSite, InitE->IgnoreParenCasts(),
-                                           BR);
+                                           BR, EnableNullFPSuppression);
     }
   }
 
@@ -520,7 +527,8 @@ PathDiagnosticPiece *FindLastStoreBRVisitor::VisitNode(const ExplodedNode *Succ,
           if (const VarRegion *OriginalR = BDR->getOriginalRegion(VR)) {
             if (Optional<KnownSVal> KV =
                 State->getSVal(OriginalR).getAs<KnownSVal>())
-              BR.addVisitor(new FindLastStoreBRVisitor(*KV, OriginalR));
+              BR.addVisitor(new FindLastStoreBRVisitor(*KV, OriginalR,
+                                                      EnableNullFPSuppression));
           }
         }
       }
@@ -807,7 +815,8 @@ static const Expr *peelOffOuterExpr(const Stmt *S,
 
 bool bugreporter::trackNullOrUndefValue(const ExplodedNode *N,
                                         const Stmt *S,
-                                        BugReport &report, bool IsArg) {
+                                        BugReport &report, bool IsArg,
+                                        bool EnableNullFPSuppression) {
   if (!S || !N)
     return false;
 
@@ -815,6 +824,12 @@ bool bugreporter::trackNullOrUndefValue(const ExplodedNode *N,
     S = Ex;
   }
 
+  // The message send could be null if the receiver is null.
+  if (const Expr *Receiver = NilReceiverBRVisitor::getReceiver(S)) {
+    report.addVisitor(new NilReceiverBRVisitor(Receiver,
+                                               EnableNullFPSuppression));
+  }
+
   const Expr *Inner = 0;
   if (const Expr *Ex = dyn_cast<Expr>(S)) {
     Ex = Ex->IgnoreParenCasts();
@@ -881,7 +896,8 @@ bool bugreporter::trackNullOrUndefValue(const ExplodedNode *N,
       // got initialized.
       if (const MemRegion *RR = getLocationRegionIfReference(Inner, N)) {
         if (Optional<KnownSVal> KV = LVal.getAs<KnownSVal>())
-          report.addVisitor(new FindLastStoreBRVisitor(*KV, RR));
+          report.addVisitor(new FindLastStoreBRVisitor(*KV, RR,
+                                                      EnableNullFPSuppression));
       }
     }
 
@@ -921,7 +937,8 @@ bool bugreporter::trackNullOrUndefValue(const ExplodedNode *N,
         report.addVisitor(ConstraintTracker);
 
         // Add visitor, which will suppress inline defensive checks.
-        if (N->getState()->isNull(V).isConstrainedTrue()) {
+        if (N->getState()->isNull(V).isConstrainedTrue() &&
+            EnableNullFPSuppression) {
           BugReporterVisitor *IDCSuppressor =
             new SuppressInlineDefensiveChecksVisitor(V.castAs<DefinedSVal>(),
                                                      N);
@@ -930,7 +947,8 @@ bool bugreporter::trackNullOrUndefValue(const ExplodedNode *N,
       }
 
       if (Optional<KnownSVal> KV = V.getAs<KnownSVal>())
-        report.addVisitor(new FindLastStoreBRVisitor(*KV, R));
+        report.addVisitor(new FindLastStoreBRVisitor(*KV, R,
+                                                     EnableNullFPSuppression));
       return true;
     }
   }
@@ -943,7 +961,8 @@ bool bugreporter::trackNullOrUndefValue(const ExplodedNode *N,
   // sure that function isn't pruned in our output.
   if (const Expr *E = dyn_cast<Expr>(S))
     S = E->IgnoreParenCasts();
-  ReturnVisitor::addVisitorIfNecessary(N, S, report);
+
+  ReturnVisitor::addVisitorIfNecessary(N, S, report, EnableNullFPSuppression);
 
   // Uncomment this to find cases where we aren't properly getting the
   // base value that was dereferenced.
@@ -966,15 +985,11 @@ bool bugreporter::trackNullOrUndefValue(const ExplodedNode *N,
   return true;
 }
 
-BugReporterVisitor *
-FindLastStoreBRVisitor::createVisitorObject(const ExplodedNode *N,
-                                            const MemRegion *R) {
-  assert(R && "The memory region is null.");
-
-  ProgramStateRef state = N->getState();
-  if (Optional<KnownSVal> KV = state->getSVal(R).getAs<KnownSVal>())
-    return new FindLastStoreBRVisitor(*KV, R);
-  return 0;
+const Expr *NilReceiverBRVisitor::getReceiver(const Stmt *S) {
+  const ObjCMessageExpr *ME = dyn_cast<ObjCMessageExpr>(S);
+  if (!ME)
+    return 0;
+  return ME->getInstanceReceiver();
 }
 
 PathDiagnosticPiece *NilReceiverBRVisitor::VisitNode(const ExplodedNode *N,
@@ -984,13 +999,15 @@ PathDiagnosticPiece *NilReceiverBRVisitor::VisitNode(const ExplodedNode *N,
   Optional<PreStmt> P = N->getLocationAs<PreStmt>();
   if (!P)
     return 0;
-  const ObjCMessageExpr *ME = P->getStmtAs<ObjCMessageExpr>();
-  if (!ME)
-    return 0;
-  const Expr *Receiver = ME->getInstanceReceiver();
+
+  const Expr *Receiver = getReceiver(P->getStmt());
   if (!Receiver)
     return 0;
 
+  // Are we tracking a different reciever?
+  if (TrackedReceiver && TrackedReceiver != Receiver)
+    return 0;
+
   ProgramStateRef state = N->getState();
   SVal V = state->getSVal(Receiver, N->getLocationContext());
   if (!state->isNull(V).isConstrainedTrue())
@@ -999,7 +1016,8 @@ PathDiagnosticPiece *NilReceiverBRVisitor::VisitNode(const ExplodedNode *N,
   // The receiver was nil, and hence the method was skipped.
   // Register a BugReporterVisitor to issue a message telling us how
   // the receiver was null.
-  bugreporter::trackNullOrUndefValue(N, Receiver, BR);
+  bugreporter::trackNullOrUndefValue(N, Receiver, BR, /*IsArg*/ false,
+                                    EnableNullFPSuppression);
   // Issue a message saying that the method was skipped.
   PathDiagnosticLocation L(Receiver, BRC.getSourceManager(),
                                      N->getLocationContext());
@@ -1009,7 +1027,8 @@ PathDiagnosticPiece *NilReceiverBRVisitor::VisitNode(const ExplodedNode *N,
 
 // Registers every VarDecl inside a Stmt with a last store visitor.
 void FindLastStoreBRVisitor::registerStatementVarDecls(BugReport &BR,
-                                                       const Stmt *S) {
+                                                const Stmt *S,
+                                                bool EnableNullFPSuppression) {
   const ExplodedNode *N = BR.getErrorNode();
   std::deque<const Stmt *> WorkList;
   WorkList.push_back(S);
@@ -1031,7 +1050,8 @@ void FindLastStoreBRVisitor::registerStatementVarDecls(BugReport &BR,
 
         if (V.getAs<loc::ConcreteInt>() || V.getAs<nonloc::ConcreteInt>()) {
           // Register a new visitor with the BugReport.
-          BR.addVisitor(new FindLastStoreBRVisitor(V.castAs<KnownSVal>(), R));
+          BR.addVisitor(new FindLastStoreBRVisitor(V.castAs<KnownSVal>(), R,
+                                                   EnableNullFPSuppression));
         }
       }
     }

diff --git a/clang/test/Analysis/NSContainers.m b/clang/test/Analysis/NSContainers.m
index 828a9ac..d6fded5 100644 (file)
--- a/clang/test/Analysis/NSContainers.m
+++ b/clang/test/Analysis/NSContainers.m
@@ -153,15 +153,24 @@ void testIDC(NSMutableDictionary *d, NSString *key) {
   d[key] = @"abc"; // no-warning
 }
 
-@interface Foo
+@interface Foo {
+@public
+  int x;
+}
 - (int *)getPtr;
 - (int)getInt;
+- (NSMutableDictionary *)getDictPtr;
+@property (retain, readonly, nonatomic) Foo* data;
+- (NSString*) stringForKeyFE: (id<NSCopying>)key;
 @end
 
 void idc2(id x) {
        if (!x)
                return;
 }
+Foo *retNil() {
+  return 0;
+}
 
 void testIDC2(Foo *obj) {
        idc2(obj);
@@ -173,3 +182,19 @@ int testIDC3(Foo *obj) {
   return 1/[obj getInt];
 }
 
+void testNilReceiverIDC(Foo *obj, NSString *key) {
+       NSMutableDictionary *D = [obj getDictPtr];
+  idc(D);
+  D[key] = @"abc"; // no-warning
+}
+
+void testNilReceiverRetNil2(NSMutableDictionary *D, Foo *FooPtrIn, id value) {
+  NSString* const kKeyIdentifier = @"key";
+       Foo *FooPtr = retNil();
+  NSString *key = [[FooPtr data] stringForKeyFE: kKeyIdentifier];
+  // key is nil because FooPtr is nil. However, FooPtr is set to nil inside an
+  // inlined function, so this error report should be suppressed.
+  [D setObject: value forKey: key]; // no-warning
+}
+
+

diff --git a/clang/test/Analysis/inlining/inline-defensive-checks.m b/clang/test/Analysis/inlining/inline-defensive-checks.m
index bafc812..a757af3 100644 (file)
--- a/clang/test/Analysis/inlining/inline-defensive-checks.m
+++ b/clang/test/Analysis/inlining/inline-defensive-checks.m
@@ -16,7 +16,6 @@ typedef struct objc_object {
 -(id)retain;
 @end
 
-// expected-no-diagnostics
 // Check that inline defensive checks is triggered for null expressions
 // within CompoundLiteralExpr.
 typedef union {
@@ -44,3 +43,71 @@ dispatch_resume(dispatch_object_t object);
   dispatch_resume(p); // no warning
 }
 @end
+
+// Test nil receiver suppression.
+// We only suppress on nil receiver if the nil value is directly causing the bug.
+@interface Foo {
+@public
+  int x;
+}
+- (Foo *)getFooPtr;
+@end
+
+Foo *retNil() {
+  return 0;
+}
+
+Foo *retInputOrNil(Foo *p) {
+  if (p)
+    return p;
+  return 0;
+}
+
+void idc(Foo *p) {
+  if (p)
+    ;
+}
+
+int testNilReceiver(Foo* fPtr) {
+  if (fPtr)
+    ;
+  // On a path where fPtr is nil, mem should be nil.
+  Foo *mem = [fPtr getFooPtr];
+  return mem->x; // expected-warning {{Access to instance variable 'x' results in a dereference of a null pointer}}
+}
+
+int dontSuppressNilReceiverRetNullCond(Foo* fPtr) {
+  unsigned zero = 0;
+  fPtr = retInputOrNil(fPtr);
+  // On a path where fPtr is nil, mem should be nil.
+  // The warning is not suppressed because the receiver being nil is not
+  // directly related to the value that triggers the warning.
+  Foo *mem = [fPtr getFooPtr];
+  if (!mem)
+    return 5/zero; // expected-warning {{Division by zero}}
+  return 0;
+}
+
+int dontSuppressNilReceiverRetNull(Foo* fPtr) {
+  unsigned zero = 0;
+  fPtr = retNil();
+  // On a path where fPtr is nil, mem should be nil.
+  // The warning is not suppressed because the receiver being nil is not
+  // directly related to the value that triggers the warning.
+  Foo *mem = [fPtr getFooPtr];
+  if (!mem)
+    return 5/zero; // expected-warning {{Division by zero}}
+  return 0;
+}
+
+int dontSuppressNilReceiverIDC(Foo* fPtr) {
+  unsigned zero = 0;
+  idc(fPtr);
+  // On a path where fPtr is nil, mem should be nil.
+  // The warning is not suppressed because the receiver being nil is not
+  // directly related to the value that triggers the warning.
+  Foo *mem = [fPtr getFooPtr];
+  if (!mem)
+    return 5/zero; // expected-warning {{Division by zero}}
+  return 0;
+}