From: George Karpenkov Date: Mon, 16 Jul 2018 20:33:25 +0000 (+0000) Subject: [analyzer] Bugfix for an overly eager suppression for null pointer return from macros. X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=627e5ecaad1f7438f6a7f586de7c700f2a876643;p=clang [analyzer] Bugfix for an overly eager suppression for null pointer return 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 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@337213 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp b/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp index 025c2898ee..f17f41a7ac 100644 --- a/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp +++ b/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() + && !N->getLocationAs() + && !N->getLocationAs()) + return false; + + // Writing into region of interest. + if (auto PS = N->getLocationAs()) + if (auto *BO = PS->getStmtAs()) + 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 @@ private: 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 @@ private: } 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() - && !N->getLocationAs() - && !N->getLocationAs()) - return false; - - // Writing into region of interest. - if (auto PS = N->getLocationAs()) - if (auto *BO = PS->getStmtAs()) - 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 getCallParameters(CallEventRef<> Call) { @@ -524,25 +529,28 @@ private: } }; +/// 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(&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 VisitNode(const ExplodedNode *N, const ExplodedNode *PrevN, BugReporterContext &BRC, BugReport &BR) override { + if (WasModified) + return nullptr; + auto BugPoint = BR.getErrorNode()->getLocation().getAs(); if (!BugPoint) return nullptr; @@ -556,6 +564,10 @@ public: BR.markInvalid(getTag(), MacroName.c_str()); } } + + if (wasRegionOfInterestModifiedAt(RegionOfInterest, N, ValueAtDereference)) + WasModified = true; + return nullptr; } @@ -568,7 +580,16 @@ public: if (EnableNullFPSuppression && Options.shouldSuppressNullReturnPaths() && V.getAs()) BR.addVisitor(llvm::make_unique( - R->getAs())); + R->getAs(), V)); + } + + void* getTag() const { + static int Tag = 0; + return static_cast(&Tag); + } + + void Profile(llvm::FoldingSetNodeID &ID) const override { + ID.AddPointer(getTag()); } private: diff --git a/test/Analysis/diagnostics/macro-null-return-suppression.cpp b/test/Analysis/diagnostics/macro-null-return-suppression.cpp index 9a14f03a32..a2928f15c1 100644 --- a/test/Analysis/diagnostics/macro-null-return-suppression.cpp +++ b/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}} +}