Szelethus created this revision.
Szelethus added reviewers: NoQ, vsavchenko, martong, steakhal, ASDenysPetrov.
Szelethus added a project: clang.
Herald added subscribers: manas, gamesh411, dkrupp, donat.nagy,
mikhail.ramalho, a.sidorin, sunfish, rnkovacs, szepet, baloghadamsoftware,
xazax.hun, whisperity.
Szelethus requested review of this revision.
Herald added subscribers: cfe-commits, aheejin.
Preceding discussion on cfe-dev:
https://lists.llvm.org/pipermail/cfe-dev/2021-June/068450.html
`NoStoreFuncVisitor` is a rather unique visitor. As `VisitNode` is invoked on
most other visitors, they are looking for the point where something changed --
change on a value, some checker-specific GDM trait, a new constraint.
`NoStoreFuncVisitor`, however, looks specifically for functions that *didn't*
write to a `MemRegion` of interesting. Quoting from its comments:
/// 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.
It so happens that there are a number of other similar properties that are
worth checking. For instance, if some memory leaks, it might be interesting why
a function didn't take ownership of said memory:
void sink(int *P) {} // no notes
void f() {
sink(new int(5)); // note: Memory is allocated
// Well hold on, sink() was supposed to deal with
// that, this must be a false positive...
} // warning: Potential memory leak [cplusplus.NewDeleteLeaks]
In here, the entity of interest isn't a `MemRegion`, but a symbol. The property
that changed here isn't a change of value, but rather liveness and GDM traits
managed by `MalloChecker`.
This patch moves some of the logic of `NoStoreFuncVisitor` to a new abstract
class, `NoStateChangeFuncVisitor`. This is mostly calculating and caching the
stack frames in which the entity of interest wasn't changed.
Descendants of this interface have to define 3 things:
- What constitutes as a change to an entity (this is done by overriding
`wasModifiedBeforeCallExit`)
- What the diagnostic message should be (this is done by overriding
`maybeEmitNote`)
- What constitutes as the entity of interest being passed into the function
(this is also done by overriding `maybeEmitNote`, to be factored out a bit more
in a later patch)
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D105553
Files:
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -343,38 +343,118 @@
return P;
}
+//===----------------------------------------------------------------------===//
+// Implementation of NoStateChangeFuncVisitor.
+//===----------------------------------------------------------------------===//
+
+bool NoStateChangeFuncVisitor::isModifiedInFrame(const ExplodedNode *N) {
+ const LocationContext *Ctx = N->getLocationContext();
+ const StackFrameContext *SCtx = Ctx->getStackFrame();
+ if (!FramesModifyingCalculated.count(SCtx))
+ findModifyingFrames(N);
+ return FramesModifying.count(SCtx);
+}
+
+void NoStateChangeFuncVisitor::findModifyingFrames(
+ const ExplodedNode *const CallExitN) {
+
+ assert(CallExitN->getLocationAs<CallExitBegin>());
+ const ExplodedNode *LastReturnN = CallExitN;
+ const StackFrameContext *const OriginalSCtx =
+ CallExitN->getLocationContext()->getStackFrame();
+
+ const ExplodedNode *CurrN = CallExitN;
+
+ do {
+ ProgramStateRef State = CurrN->getState();
+ auto CallExitLoc = CurrN->getLocationAs<CallExitBegin>();
+ if (CallExitLoc) {
+ LastReturnN = CurrN;
+ }
+
+ FramesModifyingCalculated.insert(
+ CurrN->getLocationContext()->getStackFrame());
+
+ if (wasModifiedBeforeCallExit(CurrN, LastReturnN)) {
+ const StackFrameContext *SCtx = CurrN->getStackFrame();
+ while (!SCtx->inTopFrame()) {
+ auto p = FramesModifying.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 = CurrN->getLocationAs<CallEnter>())
+ if (CE->getCalleeContext() == OriginalSCtx)
+ break;
+
+ CurrN = CurrN->getFirstPred();
+ } while (CurrN);
+}
+
+PathDiagnosticPieceRef NoStateChangeFuncVisitor::VisitNode(
+ const ExplodedNode *N, BugReporterContext &BR, PathSensitiveBugReport &R) {
+
+ const LocationContext *Ctx = N->getLocationContext();
+ const StackFrameContext *SCtx = Ctx->getStackFrame();
+ ProgramStateRef State = N->getState();
+ auto CallExitLoc = N->getLocationAs<CallExitBegin>();
+
+ // No diagnostic if region was modified inside the frame.
+ if (!CallExitLoc || isModifiedInFrame(N))
+ return nullptr;
+
+ CallEventRef<> Call =
+ BR.getStateManager().getCallEventManager().getCaller(SCtx, State);
+
+ // Optimistically suppress uninitialized value bugs that result
+ // from system headers having a chance to initialize the value
+ // but failing to do so. It's too unlikely a system header's fault.
+ // It's much more likely a situation in which the function has a failure
+ // mode that the user decided not to check. If we want to hunt such
+ // omitted checks, we should provide an explicit function-specific note
+ // describing the precondition under which the function isn't supposed to
+ // initialize its out-parameter, and additionally check that such
+ // precondition can actually be fulfilled on the current path.
+ if (Call->isInSystemHeader()) {
+ // We make an exception for system header functions that have no branches.
+ // Such functions unconditionally fail to initialize the variable.
+ // If they call other functions that have more paths within them,
+ // this suppression would still apply when we visit these inner functions.
+ // One common example of a standard function that doesn't ever initialize
+ // its out parameter is operator placement new; it's up to the follow-up
+ // constructor (if any) to initialize the memory.
+ if (!N->getStackFrame()->getCFG()->isLinear()) {
+ static int i = 0;
+ R.markInvalid(&i, nullptr);
+ }
+ return nullptr;
+ }
+
+ return maybeEmitNote(R, *Call, N);
+}
+
//===----------------------------------------------------------------------===//
// Implementation of NoStoreFuncVisitor.
//===----------------------------------------------------------------------===//
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 {
+class NoStoreFuncVisitor final : public NoStateChangeFuncVisitor {
const SubRegion *RegionOfInterest;
MemRegionManager &MmrMgr;
const SourceManager &SM;
- bugreporter::TrackingKind TKind;
/// Recursion limit for dereferencing fields when looking for the
/// region of interest.
/// The limit of two indicates that we will dereference fields only once.
static const unsigned DEREFERENCE_LIMIT = 2;
- /// Frames writing into \c RegionOfInterest.
- /// This visitor generates a note only if a function does not write into
- /// a region of interest. This information is not immediately available
- /// by looking at the node associated with the exit from the function
- /// (usually the return statement). To avoid recomputing the same information
- /// many times (going up the path for each node and checking whether the
- /// region was written into) we instead lazily compute the
- /// stack frames along the path which write into the region of interest.
- llvm::SmallPtrSet<const StackFrameContext *, 32> FramesModifyingRegion;
- llvm::SmallPtrSet<const StackFrameContext *, 32> FramesModifyingCalculated;
-
using RegionVector = SmallVector<const MemRegion *, 5>;
class RegionPrinter {
@@ -413,8 +493,9 @@
public:
NoStoreFuncVisitor(const SubRegion *R, bugreporter::TrackingKind TKind)
- : RegionOfInterest(R), MmrMgr(R->getMemRegionManager()),
- SM(MmrMgr.getContext().getSourceManager()), TKind(TKind) {}
+ : NoStateChangeFuncVisitor(TKind), RegionOfInterest(R),
+ MmrMgr(R->getMemRegionManager()),
+ SM(MmrMgr.getContext().getSourceManager()) {}
void Profile(llvm::FoldingSetNodeID &ID) const override {
static int Tag = 0;
@@ -427,11 +508,13 @@
return static_cast<void *>(&Tag);
}
- PathDiagnosticPieceRef VisitNode(const ExplodedNode *N,
- BugReporterContext &BR,
- PathSensitiveBugReport &R) override;
-
private:
+ /// \return Whether \c RegionOfInterest was modified at \p CurrN compared to
+ /// the value it holds in \p CallExitN.
+ virtual bool
+ wasModifiedBeforeCallExit(const ExplodedNode *CurrN,
+ const ExplodedNode *CallExitN) override;
+
/// Attempts to find the region of interest in a given record decl,
/// by either following the base classes or fields.
/// Dereferences fields up to a given recursion limit.
@@ -443,20 +526,9 @@
const MemRegion *R, const RegionVector &Vec = {},
int depth = 0);
- /// Check and lazily calculate whether the region of interest is
- /// modified in the stack frame to which \p N belongs.
- /// The calculation is cached in FramesModifyingRegion.
- bool isRegionOfInterestModifiedInFrame(const ExplodedNode *N) {
- const LocationContext *Ctx = N->getLocationContext();
- const StackFrameContext *SCtx = Ctx->getStackFrame();
- if (!FramesModifyingCalculated.count(SCtx))
- findModifyingFrames(N);
- return FramesModifyingRegion.count(SCtx);
- }
-
- /// Write to \c FramesModifyingRegion all stack frames along
- /// the path in the current stack frame which modify \c RegionOfInterest.
- void findModifyingFrames(const ExplodedNode *N);
+ virtual PathDiagnosticPieceRef maybeEmitNote(PathSensitiveBugReport &R,
+ const CallEvent &Call,
+ const ExplodedNode *N) override;
/// Consume the information on the no-store stack frame in order to
/// either emit a note or suppress the report enirely.
@@ -466,8 +538,7 @@
const CallEvent &Call,
const ExplodedNode *N, RegionPrinter RP);
};
-
-} // end of anonymous namespace
+} // namespace
/// \return Whether the method declaration \p Parent
/// syntactically has a binary operation writing into the ivar \p Ivar.
@@ -504,15 +575,15 @@
/// Get parameters associated with runtime definition in order
/// to get the correct parameter name.
-static ArrayRef<ParmVarDecl *> getCallParameters(CallEventRef<> Call) {
+static ArrayRef<ParmVarDecl *> getCallParameters(const CallEvent &Call) {
// Use runtime definition, if available.
- RuntimeDefinition RD = Call->getRuntimeDefinition();
+ RuntimeDefinition RD = Call.getRuntimeDefinition();
if (const auto *FD = dyn_cast_or_null<FunctionDecl>(RD.getDecl()))
return FD->parameters();
if (const auto *MD = dyn_cast_or_null<ObjCMethodDecl>(RD.getDecl()))
return MD->parameters();
- return Call->parameters();
+ return Call.parameters();
}
/// \return whether \p Ty points to a const type, or is a const reference.
@@ -579,31 +650,18 @@
return None;
}
-PathDiagnosticPieceRef
-NoStoreFuncVisitor::VisitNode(const ExplodedNode *N, BugReporterContext &BR,
- PathSensitiveBugReport &R) {
-
- const LocationContext *Ctx = N->getLocationContext();
- const StackFrameContext *SCtx = Ctx->getStackFrame();
- ProgramStateRef State = N->getState();
- auto CallExitLoc = N->getLocationAs<CallExitBegin>();
-
- // No diagnostic if region was modified inside the frame.
- if (!CallExitLoc || isRegionOfInterestModifiedInFrame(N))
- return nullptr;
-
- CallEventRef<> Call =
- BR.getStateManager().getCallEventManager().getCaller(SCtx, State);
+PathDiagnosticPieceRef NoStoreFuncVisitor::maybeEmitNote(
+ PathSensitiveBugReport &R, const CallEvent &Call, const ExplodedNode *N) {
// Region of interest corresponds to an IVar, exiting a method
// which could have written into that IVar, but did not.
- if (const auto *MC = dyn_cast<ObjCMethodCall>(Call)) {
+ if (const auto *MC = dyn_cast<ObjCMethodCall>(&Call)) {
if (const auto *IvarR = dyn_cast<ObjCIvarRegion>(RegionOfInterest)) {
const MemRegion *SelfRegion = MC->getReceiverSVal().getAsRegion();
if (RegionOfInterest->isSubRegionOf(SelfRegion) &&
- potentiallyWritesIntoIvar(Call->getRuntimeDefinition().getDecl(),
+ potentiallyWritesIntoIvar(Call.getRuntimeDefinition().getDecl(),
IvarR->getDecl()))
- return maybeEmitNote(R, *Call, N,
+ return maybeEmitNote(R, Call, N,
RegionPrinter{{},
RegionOfInterest,
SelfRegion,
@@ -613,11 +671,11 @@
}
}
- if (const auto *CCall = dyn_cast<CXXConstructorCall>(Call)) {
+ if (const auto *CCall = dyn_cast<CXXConstructorCall>(&Call)) {
const MemRegion *ThisR = CCall->getCXXThisVal().getAsRegion();
if (RegionOfInterest->isSubRegionOf(ThisR) &&
!CCall->getDecl()->isImplicit())
- return maybeEmitNote(R, *Call, N,
+ return maybeEmitNote(R, Call, N,
RegionPrinter{{},
RegionOfInterest,
ThisR,
@@ -631,9 +689,9 @@
}
ArrayRef<ParmVarDecl *> parameters = getCallParameters(Call);
- for (unsigned I = 0; I < Call->getNumArgs() && I < parameters.size(); ++I) {
+ for (unsigned I = 0; I < Call.getNumArgs() && I < parameters.size(); ++I) {
const ParmVarDecl *PVD = parameters[I];
- SVal V = Call->getArgSVal(I);
+ SVal V = Call.getArgSVal(I);
bool ParamIsReferenceType = PVD->getType()->isReferenceType();
std::string ParamName = PVD->getNameAsString();
@@ -641,7 +699,7 @@
QualType T = PVD->getType();
while (const MemRegion *MR = V.getAsRegion()) {
if (RegionOfInterest->isSubRegionOf(MR) && !isPointerToConst(T))
- return maybeEmitNote(R, *Call, N,
+ return maybeEmitNote(R, Call, N,
RegionPrinter{{},
RegionOfInterest,
MR,
@@ -653,11 +711,13 @@
if (PT.isNull() || PT->isVoidType())
break;
+ ProgramStateRef State = N->getState();
+
if (const RecordDecl *RD = PT->getAsRecordDecl())
if (Optional<RegionVector> P =
findRegionOfInterestInRecord(RD, State, MR))
return maybeEmitNote(
- R, *Call, N,
+ R, Call, N,
RegionPrinter{*P, RegionOfInterest, RegionOfInterest, ParamName,
ParamIsReferenceType, IndirectionLevel});
@@ -670,40 +730,11 @@
return nullptr;
}
-void NoStoreFuncVisitor::findModifyingFrames(const ExplodedNode *N) {
- assert(N->getLocationAs<CallExitBegin>());
- ProgramStateRef LastReturnState = N->getState();
- SVal ValueAtReturn = LastReturnState->getSVal(RegionOfInterest);
- const LocationContext *Ctx = N->getLocationContext();
- const StackFrameContext *OriginalSCtx = Ctx->getStackFrame();
-
- do {
- ProgramStateRef State = N->getState();
- auto CallExitLoc = N->getLocationAs<CallExitBegin>();
- if (CallExitLoc) {
- LastReturnState = State;
- ValueAtReturn = LastReturnState->getSVal(RegionOfInterest);
- }
-
- FramesModifyingCalculated.insert(N->getLocationContext()->getStackFrame());
-
- 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);
+bool NoStoreFuncVisitor::wasModifiedBeforeCallExit(
+ const ExplodedNode *CurrN, const ExplodedNode *CallExitN) {
+ return ::wasRegionOfInterestModifiedAt(
+ RegionOfInterest, CurrN,
+ CallExitN->getState()->getSVal(RegionOfInterest));
}
static llvm::StringLiteral WillBeUsedForACondition =
@@ -713,27 +744,6 @@
NoStoreFuncVisitor::maybeEmitNote(PathSensitiveBugReport &R,
const CallEvent &Call, const ExplodedNode *N,
RegionPrinter RP) {
- // Optimistically suppress uninitialized value bugs that result
- // from system headers having a chance to initialize the value
- // but failing to do so. It's too unlikely a system header's fault.
- // It's much more likely a situation in which the function has a failure
- // mode that the user decided not to check. If we want to hunt such
- // omitted checks, we should provide an explicit function-specific note
- // describing the precondition under which the function isn't supposed to
- // initialize its out-parameter, and additionally check that such
- // precondition can actually be fulfilled on the current path.
- if (Call.isInSystemHeader()) {
- // We make an exception for system header functions that have no branches.
- // Such functions unconditionally fail to initialize the variable.
- // If they call other functions that have more paths within them,
- // this suppression would still apply when we visit these inner functions.
- // One common example of a standard function that doesn't ever initialize
- // its out parameter is operator placement new; it's up to the follow-up
- // constructor (if any) to initialize the memory.
- if (!N->getStackFrame()->getCFG()->isLinear())
- R.markInvalid(getTag(), nullptr);
- return nullptr;
- }
PathDiagnosticLocation L =
PathDiagnosticLocation::create(N->getLocation(), SM);
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
@@ -21,6 +21,7 @@
#include "llvm/ADT/FoldingSet.h"
#include "llvm/ADT/IntrusiveRefCntPtr.h"
#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallPtrSet.h"
#include "llvm/ADT/StringRef.h"
#include <list>
#include <memory>
@@ -622,6 +623,60 @@
PathSensitiveBugReport &R) override;
};
+/// Put a diagnostic on return statement of all inlined functions for which some
+/// property remained unchanged.
+/// Resulting diagnostics may read such as "Returning without writing to X".
+///
+/// Descendants can define what a "state change is", like a change of value
+/// to a memory region, liveness, etc. For function calls where the state did
+/// not change as defined, a custom note may be constructed.
+class NoStateChangeFuncVisitor : public BugReporterVisitor {
+private:
+ /// Frames modifying the state as defined in \c wasModifiedBeforeCallExit.
+ /// This visitor generates a note only if a function does *not* change the
+ /// state that way. This information is not immediately available
+ /// by looking at the node associated with the exit from the function
+ /// (usually the return statement). To avoid recomputing the same information
+ /// many times (going up the path for each node and checking whether the
+ /// region was written into) we instead lazily compute the stack frames
+ /// along the path.
+ llvm::SmallPtrSet<const StackFrameContext *, 32> FramesModifying;
+ llvm::SmallPtrSet<const StackFrameContext *, 32> FramesModifyingCalculated;
+
+ /// Check and lazily calculate whether the state is modified in the stack
+ /// frame to which \p CallExitN belongs.
+ /// The calculation is cached in FramesModifying.
+ bool isModifiedInFrame(const ExplodedNode *CallExitN);
+
+ /// Write to \c FramesModifying all stack frames along the path in the current
+ /// stack frame which modifies the state.
+ void findModifyingFrames(const ExplodedNode *const CallExitN);
+
+protected:
+ bugreporter::TrackingKind TKind;
+
+ /// \return Whether the state was modified from the current node, \CurrN, to
+ /// the end of the stack fram, at \p CallExitN.
+ virtual bool wasModifiedBeforeCallExit(const ExplodedNode *CurrN,
+ const ExplodedNode *CallExitN) = 0;
+
+ /// Consume the information on the non-modifying stack frame in order to
+ /// either emit a note or not. May suppress the report entirely.
+ /// \return Diagnostics piece for the unmodified state in the current
+ /// function, if it decides to emit one. A good description might start with
+ /// "Returning without...".
+ virtual PathDiagnosticPieceRef maybeEmitNote(PathSensitiveBugReport &R,
+ const CallEvent &Call,
+ const ExplodedNode *N) = 0;
+
+public:
+ NoStateChangeFuncVisitor(bugreporter::TrackingKind TKind) : TKind(TKind) {}
+
+ PathDiagnosticPieceRef VisitNode(const ExplodedNode *N,
+ BugReporterContext &BR,
+ PathSensitiveBugReport &R) override final;
+};
+
} // namespace ento
} // namespace clang
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
