[analyzer] Bugfix for an overly eager suppression for null pointer re…

…turn from macros. Only suppress those cases where the null which came from the macro is relevant to the bug, and was not overwritten in between. rdar://41497323 Differential Revision: https://reviews.llvm.org/D48856 llvm-svn: 337213
llvm · Jul 16, 2018 · bccd6ec · bccd6ec
1 parent 09885d0
commit bccd6ec
Show file tree

Hide file tree

Showing 2 changed files with 83 additions and 39 deletions.
diff --git a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -228,6 +228,38 @@ static bool isFunctionMacroExpansion(SourceLocation Loc,
   return EInfo.isFunctionMacroExpansion();
 }
 
+/// \return Whether \c RegionOfInterest was modified at \p N,
+/// where \p ReturnState is a state associated with the return
+/// from the current frame.
+static bool wasRegionOfInterestModifiedAt(
+        const SubRegion *RegionOfInterest,
+        const ExplodedNode *N,
+        SVal ValueAfter) {
+  ProgramStateRef State = N->getState();
+  ProgramStateManager &Mgr = N->getState()->getStateManager();
+
+  if (!N->getLocationAs<PostStore>()
+      && !N->getLocationAs<PostInitializer>()
+      && !N->getLocationAs<PostStmt>())
+    return false;
+
+  // Writing into region of interest.
+  if (auto PS = N->getLocationAs<PostStmt>())
+    if (auto *BO = PS->getStmtAs<BinaryOperator>())
+      if (BO->isAssignmentOp() && RegionOfInterest->isSubRegionOf(
+            N->getSVal(BO->getLHS()).getAsRegion()))
+        return true;
+
+  // SVal after the state is possibly different.
+  SVal ValueAtN = N->getState()->getSVal(RegionOfInterest);
+  if (!Mgr.getSValBuilder().areEqual(State, ValueAtN, ValueAfter).isConstrainedTrue() &&
+      (!ValueAtN.isUndef() || !ValueAfter.isUndef()))
+    return true;
+
+  return false;
+}
+
+
 namespace {
 
 /// Put a diagnostic on return statement of all inlined functions
@@ -346,7 +378,7 @@ class NoStoreFuncVisitor final : public BugReporterVisitor {
       FramesModifyingCalculated.insert(
         N->getLocationContext()->getStackFrame());
 
-      if (wasRegionOfInterestModifiedAt(N, LastReturnState, ValueAtReturn)) {
+      if (wasRegionOfInterestModifiedAt(RegionOfInterest, N, ValueAtReturn)) {
         const StackFrameContext *SCtx = N->getStackFrame();
         while (!SCtx->inTopFrame()) {
           auto p = FramesModifyingRegion.insert(SCtx);
@@ -365,33 +397,6 @@ class NoStoreFuncVisitor final : public BugReporterVisitor {
     } while (N);
   }
 
-  /// \return Whether \c RegionOfInterest was modified at \p N,
-  /// where \p ReturnState is a state associated with the return
-  /// from the current frame.
-  bool wasRegionOfInterestModifiedAt(const ExplodedNode *N,
-                                     ProgramStateRef ReturnState,
-                                     SVal ValueAtReturn) {
-    if (!N->getLocationAs<PostStore>()
-        && !N->getLocationAs<PostInitializer>()
-        && !N->getLocationAs<PostStmt>())
-      return false;
-
-    // Writing into region of interest.
-    if (auto PS = N->getLocationAs<PostStmt>())
-      if (auto *BO = PS->getStmtAs<BinaryOperator>())
-        if (BO->isAssignmentOp() && RegionOfInterest->isSubRegionOf(
-                                        N->getSVal(BO->getLHS()).getAsRegion()))
-          return true;
-
-    // SVal after the state is possibly different.
-    SVal ValueAtN = N->getState()->getSVal(RegionOfInterest);
-    if (!ReturnState->areEqual(ValueAtN, ValueAtReturn).isConstrainedTrue() &&
-        (!ValueAtN.isUndef() || !ValueAtReturn.isUndef()))
-      return true;
-
-    return false;
-  }
-
   /// Get parameters associated with runtime definition in order
   /// to get the correct parameter name.
   ArrayRef<ParmVarDecl *> getCallParameters(CallEventRef<> Call) {
@@ -524,25 +529,28 @@ class NoStoreFuncVisitor final : public BugReporterVisitor {
   }
 };
 
+/// Suppress null-pointer-dereference bugs where dereferenced null was returned
+/// the macro.
 class MacroNullReturnSuppressionVisitor final : public BugReporterVisitor {
   const SubRegion *RegionOfInterest;
+  const SVal ValueAtDereference;
 
-public:
-  MacroNullReturnSuppressionVisitor(const SubRegion *R) : RegionOfInterest(R) {}
+  // Do not invalidate the reports where the value was modified
+  // after it got assigned to from the macro.
+  bool WasModified = false;
 
-  static void *getTag() {
-    static int Tag = 0;
-    return static_cast<void *>(&Tag);
-  }
-
-  void Profile(llvm::FoldingSetNodeID &ID) const override {
-    ID.AddPointer(getTag());
-  }
+public:
+  MacroNullReturnSuppressionVisitor(const SubRegion *R,
+                                    const SVal V) : RegionOfInterest(R),
+                                                    ValueAtDereference(V) {}
 
   std::shared_ptr<PathDiagnosticPiece> VisitNode(const ExplodedNode *N,
                                                  const ExplodedNode *PrevN,
                                                  BugReporterContext &BRC,
                                                  BugReport &BR) override {
+    if (WasModified)
+      return nullptr;
+
     auto BugPoint = BR.getErrorNode()->getLocation().getAs<StmtPoint>();
     if (!BugPoint)
       return nullptr;
@@ -556,6 +564,10 @@ class MacroNullReturnSuppressionVisitor final : public BugReporterVisitor {
           BR.markInvalid(getTag(), MacroName.c_str());
       }
     }
+
+    if (wasRegionOfInterestModifiedAt(RegionOfInterest, N, ValueAtDereference))
+      WasModified = true;
+
     return nullptr;
   }
 
@@ -568,7 +580,16 @@ class MacroNullReturnSuppressionVisitor final : public BugReporterVisitor {
     if (EnableNullFPSuppression && Options.shouldSuppressNullReturnPaths()
           && V.getAs<Loc>())
       BR.addVisitor(llvm::make_unique<MacroNullReturnSuppressionVisitor>(
-              R->getAs<SubRegion>()));
+              R->getAs<SubRegion>(), V));
+  }
+
+  void* getTag() const {
+    static int Tag = 0;
+    return static_cast<void *>(&Tag);
+  }
+
+  void Profile(llvm::FoldingSetNodeID &ID) const override {
+    ID.AddPointer(getTag());
   }
 
 private:

diff --git a/clang/test/Analysis/diagnostics/macro-null-return-suppression.cpp b/clang/test/Analysis/diagnostics/macro-null-return-suppression.cpp
@@ -43,3 +43,26 @@ int testDivision(int a) {
 DEREF_IN_MACRO(0) // expected-warning{{Dereference of null pointer}}
                   // expected-note@-1{{'p' initialized to a null}}
                   // expected-note@-2{{Dereference of null pointer}}
+
+// Warning should not be suppressed if the null returned by the macro
+// is not related to the warning.
+#define RETURN_NULL() (0)
+extern int* returnFreshPointer();
+int noSuppressMacroUnrelated() {
+  int *x = RETURN_NULL();
+  x = returnFreshPointer();  // expected-note{{Value assigned to 'x'}}
+  if (x) {} // expected-note{{Taking false branch}}
+            // expected-note@-1{{Assuming 'x' is null}}
+  return *x; // expected-warning{{Dereference of null pointer}}
+             // expected-note@-1{{Dereference}}
+}
+
+// Value haven't changed by the assignment, but the null pointer
+// did not come from the macro.
+int noSuppressMacroUnrelatedOtherReason() {
+  int *x = RETURN_NULL();
+  x = returnFreshPointer();  
+  x = 0; // expected-note{{Null pointer value stored to 'x'}}
+  return *x; // expected-warning{{Dereference of null pointer}}
+             // expected-note@-1{{Dereference}}
+}