]> granicus.if.org Git - clang/commitdiff
[analyzer] Apply the suppression rules to the nil receiver only if the value particip...
authorAnna Zaks <ganna@apple.com>
Thu, 28 Mar 2013 23:15:22 +0000 (23:15 +0000)
committerAnna Zaks <ganna@apple.com>
Thu, 28 Mar 2013 23:15:22 +0000 (23:15 +0000)
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.

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@178309 91177308-0d34-0410-b5e6-96231b3b80d8

include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h
lib/StaticAnalyzer/Checkers/IdempotentOperationChecker.cpp
lib/StaticAnalyzer/Checkers/UndefCapturedBlockVarChecker.cpp
lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
test/Analysis/NSContainers.m
test/Analysis/inlining/inline-defensive-checks.m

index fab70e935b36c14c5699f70a54b9e399f9b4cf42..6eb5f259b3ad6082e560d60f69eba67c779e1217 100644 (file)
@@ -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);
index 579ba9cf80755227652d47f6599c2f1a34c9e5a8..271ba4702c579960f936d97c5c4c4782f393daa9 100644 (file)
@@ -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);
index f0ca8a8312b229cd34202674d1ff301ee24f1088..93812f71485674003c1e71ff6e2e35d085955eff 100644 (file)
@@ -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);
index 3146e6c60e2d6e72195f9fe32d655079b1f55a7e..31ba5729eace4158ee859cd459450adc295ec97a 100644 (file)
@@ -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));
         }
       }
     }
index 828a9acfdd18b9ff3bee876c9db38216d246d89e..d6fded5fd056a2c89bbdaf4534e7b8adf706076c 100644 (file)
@@ -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
+}
+
+
index bafc812486c7305f3c549e23491b4ff91fcc53cb..a757af3d2fa8d4068aba3138aad6fd81b97d44a7 100644 (file)
@@ -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;
+}