[analyzer] Fix overly eager suppression of NPE when the value used is returned from a macro

[george.karpenkov]

Summary

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

[NoQ]

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

591–593 ↗

(On Diff #153815)

Are you sure we don't need to .Profile() the region of interest and maybe the value? I already forgot how this works and if we still need this at all :/

lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

static bool isFunctionMacroExpansion(SourceLocation Loc,
   while (SM.isMacroArgExpansion(Loc))
     Loc = SM.getImmediateExpansionRange(Loc).getBegin();
   std::pair<FileID, unsigned> TLInfo = SM.getDecomposedLoc(Loc);
   SrcMgr::SLocEntry SE = SM.getSLocEntry(TLInfo.first);
   const SrcMgr::ExpansionInfo &EInfo = SE.getExpansion();
   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
 /// for which  the region of interest \p RegionOfInterest was passed into,
 /// but not written inside, and it has caused an undefined read or a null
 /// pointer dereference outside.
 class NoStoreFuncVisitor final : public BugReporterVisitor {
   const SubRegion *RegionOfInterest;
do {
       if (CallExitLoc) {
         LastReturnState = State;
         ValueAtReturn = LastReturnState->getSVal(RegionOfInterest);
       }
 
       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);
           if (!p.second)
             break; // Frame and all its parents already inserted.
           SCtx = SCtx->getParent()->getStackFrame();
         }
       }
 
       // Stop calculation at the call to the current function.
       if (auto CE = N->getLocationAs<CallEnter>())
         if (CE->getCalleeContext() == OriginalSCtx)
           break;
 
       N = N->getFirstPred();
     } 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) {
     // Use runtime definition, if available.
     RuntimeDefinition RD = Call->getRuntimeDefinition();
     if (const auto *FD = dyn_cast_or_null<FunctionDecl>(RD.getDecl()))
       return FD->parameters();
 
for (auto I = Subregions.rbegin(), E = Subregions.rend(); I != E; ++I) {
       } else {
         llvm_unreachable("Previous check has missed an unexpected region");
       }
     }
     return true;
   }
 };
 
+/// 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;
 
     const SourceManager &SMgr = BRC.getSourceManager();
     if (auto Loc = matchAssignment(N, BRC)) {
       if (isFunctionMacroExpansion(*Loc, SMgr)) {
         std::string MacroName = getMacroName(*Loc, BRC);
         SourceLocation BugLoc = BugPoint->getStmt()->getLocStart();
         if (!BugLoc.isMacroID() || getMacroName(BugLoc, BRC) != MacroName)
           BR.markInvalid(getTag(), MacroName.c_str());
       }
     }
+
+    if (wasRegionOfInterestModifiedAt(RegionOfInterest, N, ValueAtDereference))
+      WasModified = true;
+
     return nullptr;
   }
 
   static void addMacroVisitorIfNecessary(
         const ExplodedNode *N, const MemRegion *R,
         bool EnableNullFPSuppression, BugReport &BR,
         const SVal V) {
     AnalyzerOptions &Options = N->getState()->getStateManager()
         .getOwningEngine()->getAnalysisManager().options;
     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:
   /// \return Source location of right hand side of an assignment
   /// into \c RegionOfInterest, empty optional if none found.
   Optional<SourceLocation> matchAssignment(const ExplodedNode *N,
                                            BugReporterContext &BRC) {
     const Stmt *S = PathDiagnosticLocation::getStmt(N);

test/Analysis/diagnostics/macro-null-return-suppression.cpp

 }
 
 // Warning should not be suppressed if it happens in the same macro.
 #define DEREF_IN_MACRO(X) int fn() {int *p = 0; return *p; }
 
 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}}
+}