Szelethus created this revision.
Szelethus added reviewers: NoQ, baloghadamsoftware, xazax.hun, rnkovacs,
dcoughlin, martong, balazske.
Szelethus added a project: clang.
Herald added subscribers: cfe-commits, danielkiss, steakhal, Charusso,
gamesh411, dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, szepet,
kristof.beyls, whisperity, mgorny.
You may be familiar with reports that are demonstrated in the test file --
While invalidation is a fundamental part of static analysis, it is
unfortunately not an under-approximation (resulting in fewer but more precise
paths of execution). Invalidation are often incorrect, so this patch attempts
to suppress reports where a tracked expression's last value change was a result
of an invalidation.
This is solved by a adding a new checker that notes which regions at which
`ProgramPoint` were invalidated, and adding a new `BugReporterVisitor` to the
army of `trackExpressionValue` related visitors that looks for the last value
change, and suppresses the report if it was a previously noted invalidation.
The big scare in such changes is always whether we would suppress so many
things, so this patch says little without results. I'll share some as soon as I
have them :)
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D75698
Files:
clang/include/clang/Analysis/ProgramPoint.h
clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
clang/lib/Analysis/ProgramPoint.cpp
clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
clang/lib/StaticAnalyzer/Checkers/SuppressInvalidationRelatedReports.cpp
clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
clang/lib/StaticAnalyzer/Core/ExplodedGraph.cpp
clang/test/Analysis/analyzer-enabled-checkers.c
clang/test/Analysis/suppress-invalidation-related-reports.cpp
Index: clang/test/Analysis/suppress-invalidation-related-reports.cpp
===================================================================
--- /dev/null
+++ clang/test/Analysis/suppress-invalidation-related-reports.cpp
@@ -0,0 +1,102 @@
+// RUN: %clang_analyze_cc1 -verify=expected %s \
+// RUN: -analyzer-checker=core,apiModeling.SuppressInvalidationRelatedReports
+
+// RUN: %clang_analyze_cc1 -verify=non-suppressed,expected %s \
+// RUN: -analyzer-checker=core
+
+namespace invalidated_global {
+int flag;
+void foo();
+
+void f() {
+ flag = 0; // Initialize flag to false.
+ int *x = 0; // x is initialized to a nullptr.
+
+ foo(); // Invalidate flag's value.
+
+ if (flag) // Assume flag is true.
+ *x = 5; // non-suppressed-warning{{Dereference of null pointer}}
+}
+} // namespace invalidated_global
+
+namespace invalidated_field {
+struct S {
+ int flag;
+ void invalidate();
+};
+
+void f() {
+ S s;
+ s.flag = 0; // Initialize s.flag to false.
+ int *x = 0; // x is initialized to a nullptr.
+
+ s.invalidate(); // Invalidate s.flag's value.
+
+ if (s.flag) // Assume s.flag is true.
+ *x = 5; // non-suppressed-warning{{Dereference of null pointer}}
+}
+} // namespace invalidated_field
+
+namespace invalidated_field_before_bind {
+struct S {
+ int flag;
+ void invalidate();
+ void setFlagToTrue() {
+ invalidate();
+ flag = 1;
+ }
+};
+
+void f() {
+ S s;
+ s.flag = 0; // Initialize s.flag to false.
+ int *x = 0; // x is initialized to a nullptr.
+
+ s.setFlagToTrue(); // Invalidate s.flag's, but also set it to true.
+
+ if (s.flag) // Assume s.flag is true.
+ *x = 5; // expected-warning{{Dereference of null pointer}}
+}
+} // namespace invalidated_field_before_bind
+
+namespace invalidated_field_twice {
+struct S {
+ int flag;
+ void invalidate();
+ void setFlagToTrue() {
+ invalidate();
+ flag = 1;
+ invalidate();
+ }
+};
+
+void f() {
+ S s;
+ s.flag = 0; // Initialize s.flag to false.
+ int *x = 0; // x is initialized to a nullptr.
+
+ s.setFlagToTrue(); // Invalidate s.flag's, but also set it to true.
+
+ if (s.flag) // Assume s.flag is true.
+ *x = 5; // non-suppressed-warning{{Dereference of null pointer}}
+}
+} // namespace invalidated_field_twice
+
+namespace invalidated_field_through_bind {
+struct S {
+ int flag;
+};
+
+void f() {
+ S s;
+ s.flag = 0; // Initialize s.flag to false.
+ S s2;
+ s2.flag = 1; // Initialize s2.flag to true
+ int *x = 0; // x is initialized to a nullptr.
+
+ s = s2; // "Invalidate" s.flag's value through bind.
+
+ if (s.flag) // Assume s.flag is true.
+ *x = 5; // expected-warning{{Dereference of null pointer}}
+}
+} // namespace invalidated_field_through_bind
Index: clang/test/Analysis/analyzer-enabled-checkers.c
===================================================================
--- clang/test/Analysis/analyzer-enabled-checkers.c
+++ clang/test/Analysis/analyzer-enabled-checkers.c
@@ -7,6 +7,7 @@
// CHECK: OVERVIEW: Clang Static Analyzer Enabled Checkers List
// CHECK-EMPTY:
// CHECK-NEXT: apiModeling.StdCLibraryFunctions
+// CHECK-NEXT: apiModeling.SuppressInvalidationRelatedReports
// CHECK-NEXT: apiModeling.TrustNonnull
// CHECK-NEXT: apiModeling.llvm.CastValue
// CHECK-NEXT: apiModeling.llvm.ReturnValue
Index: clang/lib/StaticAnalyzer/Core/ExplodedGraph.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Core/ExplodedGraph.cpp
+++ clang/lib/StaticAnalyzer/Core/ExplodedGraph.cpp
@@ -330,24 +330,7 @@
->getCallSite();
}
// Otherwise, see if the node's program point directly points to a statement.
- // FIXME: Refactor into a ProgramPoint method?
- ProgramPoint P = getLocation();
- if (auto SP = P.getAs<StmtPoint>())
- return SP->getStmt();
- if (auto BE = P.getAs<BlockEdge>())
- return BE->getSrc()->getTerminatorStmt();
- if (auto CE = P.getAs<CallEnter>())
- return CE->getCallExpr();
- if (auto CEE = P.getAs<CallExitEnd>())
- return CEE->getCalleeContext()->getCallSite();
- if (auto PIPP = P.getAs<PostInitializer>())
- return PIPP->getInitializer()->getInit();
- if (auto CEB = P.getAs<CallExitBegin>())
- return CEB->getReturnStmt();
- if (auto FEP = P.getAs<FunctionExitPoint>())
- return FEP->getStmt();
-
- return nullptr;
+ return getLocation().getStmtForDiagnostics();
}
const Stmt *ExplodedNode::getNextStmtForDiagnostics() const {
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -32,6 +32,7 @@
#include "clang/Basic/LLVM.h"
#include "clang/Basic/SourceLocation.h"
#include "clang/Basic/SourceManager.h"
+#include "clang/Basic/TargetBuiltins.h"
#include "clang/Lex/Lexer.h"
#include "clang/StaticAnalyzer/Core/AnalyzerOptions.h"
#include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
@@ -1595,6 +1596,60 @@
return nullptr;
}
+//===----------------------------------------------------------------------===//
+// Implementation of SuppressInvalidationVisitor.
+//===----------------------------------------------------------------------===//
+
+SuppressInvalidationVisitor::SuppressInvalidationVisitor(const MemRegion *R,
+ const ExplodedNode *N)
+ : R(R), SuperRegion(R) {
+ for (HadInvalidation::value_type &HI : N->getState()->get<HadInvalidation>())
+ if (R->isSubRegionOf(HI.first))
+ SuperRegion = HI.first;
+ if (!N->getState()->contains<HadInvalidation>(SuperRegion))
+ IsSatisfied = true;
+}
+
+void SuppressInvalidationVisitor::Profile(llvm::FoldingSetNodeID &ID) const {
+ static int id = 0;
+ ID.AddPointer(&id);
+ ID.Add(R);
+}
+
+const char *SuppressInvalidationVisitor::getTag() {
+ return "InvalidSuppVisitor";
+}
+
+PathDiagnosticPieceRef
+SuppressInvalidationVisitor::VisitNode(const ExplodedNode *Succ,
+ BugReporterContext &BRC,
+ PathSensitiveBugReport &BR) {
+ if (IsSatisfied)
+ return nullptr;
+
+ ProgramStateRef SuccState = Succ->getState();
+ ProgramStateRef PredState = Succ->getFirstPred()->getState();
+
+ // Look for the last write of R.
+ if (PredState->getSVal(R) != SuccState->getSVal(R)) {
+ Succ->getStmtForDiagnostics();
+
+ // We found the last write, invalidations previous to this point are not
+ // interesting.
+ IsSatisfied = true;
+
+ // R may have been invalidated multiple times, is the last invalidation
+ // also the last write?
+ if (Succ->getLocation() != *SuccState->get<HadInvalidation>(SuperRegion))
+ return nullptr;
+ const LocationContext *CurLC = Succ->getLocationContext();
+
+ BR.markInvalid("Suppress IV", CurLC);
+ return nullptr;
+ }
+ return nullptr;
+}
+
//===----------------------------------------------------------------------===//
// Implementation of SuppressInlineDefensiveChecksVisitor.
//===----------------------------------------------------------------------===//
@@ -1811,9 +1866,9 @@
// isn't sufficient, because a new visitor is created for each tracked
// expression, hence the BugReport level set.
if (BR.addTrackedCondition(N)) {
- bugreporter::trackExpressionValue(
- N, Condition, BR, bugreporter::TrackingKind::Condition,
- /*EnableNullFPSuppression=*/true);
+ bugreporter::trackExpressionValue(N, Condition, BR,
+ bugreporter::TrackingKind::Condition,
+ /*EnableNullFPSuppression=*/true);
return constructDebugPieceForTrackedCondition(Condition, N, BRC);
}
}
@@ -1988,6 +2043,9 @@
if (R) {
+ report.addVisitor(
+ std::make_unique<SuppressInvalidationVisitor>(R, InputNode));
+
// Mark both the variable region and its contents as interesting.
SVal V = LVState->getRawSVal(loc::MemRegionVal(R));
report.addVisitor(
@@ -2006,7 +2064,7 @@
V.castAs<DefinedSVal>(), false));
// Add visitor, which will suppress inline defensive checks.
- if (auto DV = V.getAs<DefinedSVal>())
+ if (auto DV = V.getAs<DefinedSVal>()) {
if (!DV->isZeroConstant() && EnableNullFPSuppression) {
// Note that LVNode may be too late (i.e., too far from the InputNode)
// because the lvalue may have been computed before the inlined call
@@ -2017,6 +2075,7 @@
std::make_unique<SuppressInlineDefensiveChecksVisitor>(
*DV, InputNode));
}
+ }
if (auto KV = V.getAs<KnownSVal>())
report.addVisitor(std::make_unique<FindLastStoreBRVisitor>(
Index: clang/lib/StaticAnalyzer/Checkers/SuppressInvalidationRelatedReports.cpp
===================================================================
--- /dev/null
+++ clang/lib/StaticAnalyzer/Checkers/SuppressInvalidationRelatedReports.cpp
@@ -0,0 +1,69 @@
+//=== FIXME.cpp -------------------------------------------*- C++ -*--//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// TODO
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/AST/ExprCXX.h"
+#include "clang/Basic/IdentifierTable.h"
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/SymExpr.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/Support/ErrorHandling.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+
+class SuppressInvalidationRelatedReportsChecker
+ : public Checker<check::RegionChanges> {
+public:
+ ProgramStateRef
+ checkRegionChanges(ProgramStateRef State,
+ const InvalidatedSymbols *Invalidated,
+ ArrayRef<const MemRegion *> ExplicitRegions,
+ ArrayRef<const MemRegion *> Regions,
+ const LocationContext *LCtx, const CallEvent *Call) const {
+ if (!Call)
+ return State;
+
+ for (const MemRegion *MR : Regions)
+ State = State->set<HadInvalidation>(MR, Call->getProgramPoint());
+
+ return State;
+ }
+
+ void printState(raw_ostream &Out, ProgramStateRef State, const char *NL,
+ const char *Sep) const override {
+ for (const HadInvalidation::value_type &I : State->get<HadInvalidation>()) {
+ I.first->dumpToStream(Out);
+ Out << " was invalidated on '";
+ if (const Stmt *InvalidatingStmt = I.second.getStmtForDiagnostics())
+ InvalidatingStmt->getBeginLoc().print(
+ Out, State->getAnalysisManager().getSourceManager());
+ Out << "'" << NL;
+ }
+ }
+};
+
+} // end anonymous namespace
+
+void ento::registerSuppressInvalidationRelatedReportsChecker(
+ CheckerManager &mgr) {
+ mgr.registerChecker<SuppressInvalidationRelatedReportsChecker>();
+}
+
+bool ento::shouldRegisterSuppressInvalidationRelatedReportsChecker(
+ const LangOptions &LO) {
+ return true;
+}
Index: clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
+++ clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
@@ -102,6 +102,7 @@
StdLibraryFunctionsChecker.cpp
STLAlgorithmModeling.cpp
StreamChecker.cpp
+ SuppressInvalidationRelatedReports.cpp
Taint.cpp
TaintTesterChecker.cpp
TestAfterDivZeroChecker.cpp
Index: clang/lib/Analysis/ProgramPoint.cpp
===================================================================
--- clang/lib/Analysis/ProgramPoint.cpp
+++ clang/lib/Analysis/ProgramPoint.cpp
@@ -43,6 +43,24 @@
}
}
+const Stmt *ProgramPoint::getStmtForDiagnostics() const {
+ if (auto SP = getAs<StmtPoint>())
+ return SP->getStmt();
+ if (auto BE = getAs<BlockEdge>())
+ return BE->getSrc()->getTerminatorStmt();
+ if (auto CE = getAs<CallEnter>())
+ return CE->getCallExpr();
+ if (auto CEE = getAs<CallExitEnd>())
+ return CEE->getCalleeContext()->getCallSite();
+ if (auto PIPP = getAs<PostInitializer>())
+ return PIPP->getInitializer()->getInit();
+ if (auto CEB = getAs<CallExitBegin>())
+ return CEB->getReturnStmt();
+ if (auto FEP = getAs<FunctionExitPoint>())
+ return FEP->getStmt();
+ return nullptr;
+}
+
LLVM_DUMP_METHOD void ProgramPoint::dump() const {
return printJson(llvm::errs());
}
Index: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
===================================================================
--- clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
+++ clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
@@ -16,8 +16,11 @@
#include "clang/Analysis/ProgramPoint.h"
#include "clang/Basic/LLVM.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState_Fwd.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/Store.h"
#include "llvm/ADT/FoldingSet.h"
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/StringRef.h"
@@ -367,6 +370,56 @@
PathSensitiveBugReport &BR) override;
};
+/// Looks for and suppresses reports where the tracked region's last value
+/// change was a result of an invalidation, which is prone to false positives.
+/// In this example, foo *could* change flags value, but it most likely doesn't.
+///
+/// int flag;
+/// void foo(); // Function body is unknown.
+///
+/// void f() {
+/// flag = 0; // flag is initialized to false.
+/// int *x = 0; // x is initialized to a nullptr
+///
+/// foo(); // Invalidate flag's value.
+///
+/// if (flag) // Assume flag is true.
+/// *x = 5; // x is dereferenced as a nullptr
+/// }
+class SuppressInvalidationVisitor final : public BugReporterVisitor {
+ /// The region whose changes we're analyzing.
+ const MemRegion *const R;
+ /// R may not be directly listed in the invalidation set, but its super region
+ /// might be. In case R *is* in the invalidation set, this field is equal to
+ /// it.
+ const MemRegion *SuperRegion;
+
+ /// Track until the last value change only, whether or not its an
+ /// invalidation.
+ bool IsSatisfied = false;
+
+public:
+ SuppressInvalidationVisitor(const MemRegion *R, const ExplodedNode *N);
+
+ void Profile(llvm::FoldingSetNodeID &ID) const override;
+
+ static const char *getTag();
+
+ PathDiagnosticPieceRef VisitNode(const ExplodedNode *Succ,
+ BugReporterContext &BRC,
+ PathSensitiveBugReport &BR) override;
+};
+
+} // namespace ento
+} // namespace clang
+
+REGISTER_MAP_FACTORY_WITH_PROGRAMSTATE(HadInvalidation,
+ const clang::ento::MemRegion *,
+ clang::ProgramPoint)
+
+namespace clang {
+namespace ento {
+
/// The bug visitor will walk all the nodes in a path and collect all the
/// constraints. When it reaches the root node, will create a refutation
/// manager and check if the constraints are satisfiable
@@ -388,7 +441,6 @@
PathSensitiveBugReport &BR) override;
};
-
/// The visitor detects NoteTags and displays the event notes they contain.
class TagVisitor : public BugReporterVisitor {
public:
Index: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
===================================================================
--- clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -300,6 +300,13 @@
"are not null">,
Documentation<NotDocumented>;
+def SuppressInvalidationRelatedReportsChecker
+ : Checker<"SuppressInvalidationRelatedReports">,
+ HelpText<"Suppress reports where the expression that was a cause of a bug "
+ "or other expressions related to this expression have been "
+ "invalidated">,
+ Documentation<NotDocumented>;
+
} // end "apiModeling"
//===----------------------------------------------------------------------===//
Index: clang/include/clang/Analysis/ProgramPoint.h
===================================================================
--- clang/include/clang/Analysis/ProgramPoint.h
+++ clang/include/clang/Analysis/ProgramPoint.h
@@ -191,6 +191,10 @@
return ID.ComputeHash();
}
+ /// If the program point corresponds to a statement, retrieve that statement.
+ /// Useful for figuring out where to put a warning or a note.
+ const Stmt *getStmtForDiagnostics() const;
+
bool operator==(const ProgramPoint & RHS) const {
return Data1 == RHS.Data1 &&
Data2 == RHS.Data2 &&
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
