r337213 - [analyzer] Bugfix for an overly eager suppression for null pointer return from macros.
George Karpenkov via cfe-commits
cfe-commits at lists.llvm.org
Mon Jul 16 13:33:25 PDT 2018
Author: george.karpenkov
Date: Mon Jul 16 13:33:25 2018
New Revision: 337213
URL: http://llvm.org/viewvc/llvm-project?rev=337213&view=rev
Log:
[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
Modified:
cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
cfe/trunk/test/Analysis/diagnostics/macro-null-return-suppression.cpp
Modified: cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp?rev=337213&r1=337212&r2=337213&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp Mon Jul 16 13:33:25 2018
@@ -228,6 +228,38 @@ static bool isFunctionMacroExpansion(Sou
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 @@ 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<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 @@ 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) {}
-
- static void *getTag() {
- static int Tag = 0;
- return static_cast<void *>(&Tag);
- }
+ // Do not invalidate the reports where the value was modified
+ // after it got assigned to from the macro.
+ bool WasModified = false;
- 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 @@ 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<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:
Modified: cfe/trunk/test/Analysis/diagnostics/macro-null-return-suppression.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/diagnostics/macro-null-return-suppression.cpp?rev=337213&r1=337212&r2=337213&view=diff
==============================================================================
--- cfe/trunk/test/Analysis/diagnostics/macro-null-return-suppression.cpp (original)
+++ cfe/trunk/test/Analysis/diagnostics/macro-null-return-suppression.cpp Mon Jul 16 13:33:25 2018
@@ -43,3 +43,26 @@ int testDivision(int a) {
DEREF_IN_MACRO(0) // expected-warning{{Dereference of null pointer}}
// expected-note at -1{{'p' initialized to a null}}
// expected-note at -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 at -1{{Assuming 'x' is null}}
+ return *x; // expected-warning{{Dereference of null pointer}}
+ // expected-note at -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 at -1{{Dereference}}
+}
More information about the cfe-commits
mailing list