Szelethus updated this revision to Diff 366269.
Szelethus set the repository for this revision to rG LLVM Github Monorepo.
Szelethus added a comment.
- Add the new feature behind a new, off-by-default alpha checker option.
- Restore `test/Analysis/self-assign.cpp` (to be formatted in a standalone
commit).
If you don't mind, I'll commit this as is. We seem to agree on the general
direction, and doesn't bother any users from now on.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D105819/new/
https://reviews.llvm.org/D105819
Files:
clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
clang/test/Analysis/NewDeleteLeaks.cpp
clang/test/Analysis/analyzer-config.c
Index: clang/test/Analysis/analyzer-config.c
===================================================================
--- clang/test/Analysis/analyzer-config.c
+++ clang/test/Analysis/analyzer-config.c
@@ -116,6 +116,7 @@
// CHECK-NEXT: suppress-null-return-paths = true
// CHECK-NEXT: track-conditions = true
// CHECK-NEXT: track-conditions-debug = false
+// CHECK-NEXT: unix.DynamicMemoryModeling:AddNoOwnershipChangeNotes = false
// CHECK-NEXT: unix.DynamicMemoryModeling:Optimistic = false
// CHECK-NEXT: unroll-loops = false
// CHECK-NEXT: widen-loops = false
Index: clang/test/Analysis/NewDeleteLeaks.cpp
===================================================================
--- /dev/null
+++ clang/test/Analysis/NewDeleteLeaks.cpp
@@ -0,0 +1,142 @@
+// RUN: %clang_analyze_cc1 -verify -analyzer-output=text %s \
+// RUN: -analyzer-checker=core \
+// RUN: -analyzer-checker=cplusplus \
+// RUN: -analyzer-checker=unix \
+// RUN: -analyzer-config \
+// RUN: unix.DynamicMemoryModeling:AddNoOwnershipChangeNotes=false
+
+// RUN: %clang_analyze_cc1 -verify=expected,ownership -analyzer-output=text %s \
+// RUN: -analyzer-checker=core \
+// RUN: -analyzer-checker=cplusplus \
+// RUN: -analyzer-checker=unix \
+// RUN: -analyzer-config \
+// RUN: unix.DynamicMemoryModeling:AddNoOwnershipChangeNotes=true
+
+#include "Inputs/system-header-simulator-for-malloc.h"
+
+//===----------------------------------------------------------------------===//
+// Report for which we expect NoOwnershipChangeVisitor to add a new note.
+//===----------------------------------------------------------------------===//
+
+bool coin();
+
+namespace memory_allocated_in_fn_call {
+
+void sink(int *P) {
+} // ownership-note {{Returning without deallocating memory or storing the pointer for later deallocation}}
+
+void foo() {
+ sink(new int(5)); // expected-note {{Memory is allocated}}
+ // ownership-note@-1 {{Calling 'sink'}}
+ // ownership-note@-2 {{Returning from 'sink'}}
+} // expected-warning {{Potential memory leak [cplusplus.NewDeleteLeaks]}}
+// expected-note@-1 {{Potential memory leak}}
+
+} // namespace memory_allocated_in_fn_call
+
+namespace memory_passed_to_fn_call {
+
+void sink(int *P) {
+ if (coin()) // ownership-note {{Assuming the condition is false}}
+ // ownership-note@-1 {{Taking false branch}}
+ delete P;
+} // ownership-note {{Returning without deallocating memory or storing the pointer for later deallocation}}
+
+void foo() {
+ int *ptr = new int(5); // expected-note {{Memory is allocated}}
+ sink(ptr); // ownership-note {{Calling 'sink'}}
+ // ownership-note@-1 {{Returning from 'sink'}}
+} // expected-warning {{Potential leak of memory pointed to by 'ptr' [cplusplus.NewDeleteLeaks]}}
+// expected-note@-1 {{Potential leak}}
+
+} // namespace memory_passed_to_fn_call
+
+namespace memory_shared_with_ptr_of_shorter_lifetime {
+
+void sink(int *P) {
+ int *Q = P;
+ if (coin()) // ownership-note {{Assuming the condition is false}}
+ // ownership-note@-1 {{Taking false branch}}
+ delete P;
+ (void)Q;
+} // ownership-note {{Returning without deallocating memory or storing the pointer for later deallocation}}
+
+void foo() {
+ int *ptr = new int(5); // expected-note {{Memory is allocated}}
+ sink(ptr); // ownership-note {{Calling 'sink'}}
+ // ownership-note@-1 {{Returning from 'sink'}}
+} // expected-warning {{Potential leak of memory pointed to by 'ptr' [cplusplus.NewDeleteLeaks]}}
+// expected-note@-1 {{Potential leak}}
+
+} // namespace memory_shared_with_ptr_of_shorter_lifetime
+
+//===----------------------------------------------------------------------===//
+// Report for which we *do not* expect NoOwnershipChangeVisitor add a new note,
+// nor do we want it to.
+//===----------------------------------------------------------------------===//
+
+namespace memory_not_passed_to_fn_call {
+
+void sink(int *P) {
+ if (coin())
+ delete P;
+}
+
+void foo() {
+ int *ptr = new int(5); // expected-note {{Memory is allocated}}
+ int *q = nullptr;
+ sink(q);
+ (void)ptr;
+} // expected-warning {{Potential leak of memory pointed to by 'ptr' [cplusplus.NewDeleteLeaks]}}
+// expected-note@-1 {{Potential leak}}
+
+} // namespace memory_not_passed_to_fn_call
+
+namespace memory_shared_with_ptr_of_same_lifetime {
+
+void sink(int *P, int **Q) {
+ // NOTE: Not a job of NoOwnershipChangeVisitor, but maybe this could be
+ // highlighted still?
+ *Q = P;
+}
+
+void foo() {
+ int *ptr = new int(5); // expected-note {{Memory is allocated}}
+ int *q = nullptr;
+ sink(ptr, &q);
+} // expected-warning {{Potential leak of memory pointed to by 'q' [cplusplus.NewDeleteLeaks]}}
+// expected-note@-1 {{Potential leak}}
+
+} // namespace memory_shared_with_ptr_of_same_lifetime
+
+// TODO: We don't want a note here. sink() doesn't seem like a function that
+// even attempts to take care of any memory ownership problems.
+namespace memory_passed_into_fn_that_doesnt_intend_to_free {
+
+void sink(int *P) {
+} // ownership-note {{Returning without deallocating memory or storing the pointer for later deallocation}}
+
+void foo() {
+ int *ptr = new int(5); // expected-note {{Memory is allocated}}
+ sink(ptr); // ownership-note {{Calling 'sink'}}
+ // ownership-note@-1 {{Returning from 'sink'}}
+} // expected-warning {{Potential leak of memory pointed to by 'ptr' [cplusplus.NewDeleteLeaks]}}
+// expected-note@-1 {{Potential leak}}
+
+} // namespace memory_passed_into_fn_that_doesnt_intend_to_free
+
+namespace refkind_from_unoallocated_to_allocated {
+
+// RefKind of the symbol changed from nothing to Allocated. We don't want to
+// emit notes when the RefKind changes in the stack frame.
+static char *malloc_wrapper_ret() {
+ return (char *)malloc(12); // expected-note {{Memory is allocated}}
+}
+void use_ret() {
+ char *v;
+ v = malloc_wrapper_ret(); // expected-note {{Calling 'malloc_wrapper_ret'}}
+ // expected-note@-1 {{Returned allocated memory}}
+} // expected-warning {{Potential leak of memory pointed to by 'v' [unix.Malloc]}}
+// expected-note@-1 {{Potential leak of memory pointed to by 'v'}}
+
+} // namespace refkind_from_unoallocated_to_allocated
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -343,46 +343,140 @@
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 CallExitBeginN) {
+
+ assert(CallExitBeginN->getLocationAs<CallExitBegin>());
+ const ExplodedNode *LastReturnN = CallExitBeginN;
+ const StackFrameContext *const OriginalSCtx =
+ CallExitBeginN->getLocationContext()->getStackFrame();
+
+ const ExplodedNode *CurrN = CallExitBeginN;
+
+ 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;
+ }
+
+ if (const auto *MC = dyn_cast<ObjCMethodCall>(Call)) {
+ // If we failed to construct a piece for self, we still want to check
+ // whether the entity of interest is in a parameter.
+ if (PathDiagnosticPieceRef Piece = maybeEmitNoteForObjCSelf(R, *MC, N))
+ return Piece;
+ }
+
+ if (const auto *CCall = dyn_cast<CXXConstructorCall>(Call)) {
+ // Do not generate diagnostics for not modified parameters in
+ // constructors.
+ return maybeEmitNoteForCXXThis(R, *CCall, N);
+ }
+
+ return maybeEmitNoteForParameters(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;
const PrintingPolicy &PP;
- 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>;
public:
NoStoreFuncVisitor(const SubRegion *R, bugreporter::TrackingKind TKind)
- : RegionOfInterest(R), MmrMgr(R->getMemRegionManager()),
+ : NoStateChangeFuncVisitor(TKind), RegionOfInterest(R),
+ MmrMgr(R->getMemRegionManager()),
SM(MmrMgr.getContext().getSourceManager()),
- PP(MmrMgr.getContext().getPrintingPolicy()), TKind(TKind) {}
+ PP(MmrMgr.getContext().getPrintingPolicy()) {}
void Profile(llvm::FoldingSetNodeID &ID) const override {
static int Tag = 0;
@@ -395,11 +489,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 CallExitBeginN.
+ virtual bool
+ wasModifiedBeforeCallExit(const ExplodedNode *CurrN,
+ const ExplodedNode *CallExitBeginN) 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.
@@ -411,20 +507,21 @@
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);
- }
+ // Region of interest corresponds to an IVar, exiting a method
+ // which could have written into that IVar, but did not.
+ virtual PathDiagnosticPieceRef
+ maybeEmitNoteForObjCSelf(PathSensitiveBugReport &R,
+ const ObjCMethodCall &Call,
+ const ExplodedNode *N) override final;
+
+ virtual PathDiagnosticPieceRef
+ maybeEmitNoteForCXXThis(PathSensitiveBugReport &R,
+ const CXXConstructorCall &Call,
+ const ExplodedNode *N) override final;
- /// 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
+ maybeEmitNoteForParameters(PathSensitiveBugReport &R, const CallEvent &Call,
+ const ExplodedNode *N) override final;
/// Consume the information on the no-store stack frame in order to
/// either emit a note or suppress the report enirely.
@@ -436,22 +533,18 @@
const MemRegion *MatchedRegion, StringRef FirstElement,
bool FirstIsReferenceType, unsigned IndirectionLevel);
- /// Pretty-print region \p MatchedRegion to \p os.
- /// \return Whether printing succeeded.
- bool prettyPrintRegionName(StringRef FirstElement, bool FirstIsReferenceType,
+ bool prettyPrintRegionName(const RegionVector &FieldChain,
const MemRegion *MatchedRegion,
- const RegionVector &FieldChain,
- int IndirectionLevel,
+ StringRef FirstElement, bool FirstIsReferenceType,
+ unsigned IndirectionLevel,
llvm::raw_svector_ostream &os);
- /// Print first item in the chain, return new separator.
- static StringRef prettyPrintFirstElement(StringRef FirstElement,
- bool MoreItemsExpected,
- int IndirectionLevel,
- llvm::raw_svector_ostream &os);
+ StringRef prettyPrintFirstElement(StringRef FirstElement,
+ bool MoreItemsExpected,
+ int IndirectionLevel,
+ llvm::raw_svector_ostream &os);
};
-
-} // end of anonymous namespace
+} // namespace
/// \return Whether the method declaration \p Parent
/// syntactically has a binary operation writing into the ivar \p Ivar.
@@ -486,25 +579,6 @@
return false;
}
-/// Get parameters associated with runtime definition in order
-/// to get the correct parameter name.
-static 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();
- if (const auto *MD = dyn_cast_or_null<ObjCMethodDecl>(RD.getDecl()))
- return MD->parameters();
-
- return Call->parameters();
-}
-
-/// \return whether \p Ty points to a const type, or is a const reference.
-static bool isPointerToConst(QualType Ty) {
- return !Ty->getPointeeType().isNull() &&
- Ty->getPointeeType().getCanonicalType().isConstQualified();
-}
-
/// Attempts to find the region of interest in a given CXX decl,
/// by either following the base classes or fields.
/// Dereferences fields up to a given recursion limit.
@@ -564,68 +638,66 @@
}
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);
-
- // 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 *IvarR = dyn_cast<ObjCIvarRegion>(RegionOfInterest)) {
- const MemRegion *SelfRegion = MC->getReceiverSVal().getAsRegion();
- if (RegionOfInterest->isSubRegionOf(SelfRegion) &&
- potentiallyWritesIntoIvar(Call->getRuntimeDefinition().getDecl(),
- IvarR->getDecl()))
- return maybeEmitNote(R, *Call, N, {}, SelfRegion, "self",
- /*FirstIsReferenceType=*/false, 1);
- }
+NoStoreFuncVisitor::maybeEmitNoteForObjCSelf(PathSensitiveBugReport &R,
+ const ObjCMethodCall &Call,
+ const ExplodedNode *N) {
+ if (const auto *IvarR = dyn_cast<ObjCIvarRegion>(RegionOfInterest)) {
+ const MemRegion *SelfRegion = Call.getReceiverSVal().getAsRegion();
+ if (RegionOfInterest->isSubRegionOf(SelfRegion) &&
+ potentiallyWritesIntoIvar(Call.getRuntimeDefinition().getDecl(),
+ IvarR->getDecl()))
+ return maybeEmitNote(R, Call, N, {}, SelfRegion, "self",
+ /*FirstIsReferenceType=*/false, 1);
}
+ return nullptr;
+}
- 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, {}, ThisR, "this",
- /*FirstIsReferenceType=*/false, 1);
+PathDiagnosticPieceRef
+NoStoreFuncVisitor::maybeEmitNoteForCXXThis(PathSensitiveBugReport &R,
+ const CXXConstructorCall &Call,
+ const ExplodedNode *N) {
+ const MemRegion *ThisR = Call.getCXXThisVal().getAsRegion();
+ if (RegionOfInterest->isSubRegionOf(ThisR) && !Call.getDecl()->isImplicit())
+ return maybeEmitNote(R, Call, N, {}, ThisR, "this",
+ /*FirstIsReferenceType=*/false, 1);
+
+ // Do not generate diagnostics for not modified parameters in
+ // constructors.
+ return nullptr;
+}
- // Do not generate diagnostics for not modified parameters in
- // constructors.
- return nullptr;
- }
+/// \return whether \p Ty points to a const type, or is a const reference.
+static bool isPointerToConst(QualType Ty) {
+ return !Ty->getPointeeType().isNull() &&
+ Ty->getPointeeType().getCanonicalType().isConstQualified();
+}
- ArrayRef<ParmVarDecl *> parameters = getCallParameters(Call);
- for (unsigned I = 0; I < Call->getNumArgs() && I < parameters.size(); ++I) {
- const ParmVarDecl *PVD = parameters[I];
- SVal V = Call->getArgSVal(I);
+PathDiagnosticPieceRef NoStoreFuncVisitor::maybeEmitNoteForParameters(
+ PathSensitiveBugReport &R, const CallEvent &Call, const ExplodedNode *N) {
+ ArrayRef<ParmVarDecl *> Parameters = Call.parameters();
+ for (unsigned I = 0; I < Call.getNumArgs() && I < Parameters.size(); ++I) {
+ const ParmVarDecl *PVD = Parameters[I];
+ SVal V = Call.getArgSVal(I);
bool ParamIsReferenceType = PVD->getType()->isReferenceType();
std::string ParamName = PVD->getNameAsString();
- int IndirectionLevel = 1;
+ unsigned IndirectionLevel = 1;
QualType T = PVD->getType();
while (const MemRegion *MR = V.getAsRegion()) {
if (RegionOfInterest->isSubRegionOf(MR) && !isPointerToConst(T))
- return maybeEmitNote(R, *Call, N, {}, MR, ParamName,
+ return maybeEmitNote(R, Call, N, {}, MR, ParamName,
ParamIsReferenceType, IndirectionLevel);
QualType PT = T->getPointeeType();
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, *P, RegionOfInterest, ParamName,
+ return maybeEmitNote(R, Call, N, *P, RegionOfInterest, ParamName,
ParamIsReferenceType, IndirectionLevel);
V = State->getSVal(MR, PT);
@@ -637,40 +709,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 *CallExitBeginN) {
+ return ::wasRegionOfInterestModifiedAt(
+ RegionOfInterest, CurrN,
+ CallExitBeginN->getState()->getSVal(RegionOfInterest));
}
static llvm::StringLiteral WillBeUsedForACondition =
@@ -681,27 +724,6 @@
const RegionVector &FieldChain, const MemRegion *MatchedRegion,
StringRef FirstElement, bool FirstIsReferenceType,
unsigned IndirectionLevel) {
- // 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);
@@ -717,8 +739,8 @@
os << "Returning without writing to '";
// Do not generate the note if failed to pretty-print.
- if (!prettyPrintRegionName(FirstElement, FirstIsReferenceType, MatchedRegion,
- FieldChain, IndirectionLevel, os))
+ if (!prettyPrintRegionName(FieldChain, MatchedRegion, FirstElement,
+ FirstIsReferenceType, IndirectionLevel, os))
return nullptr;
os << "'";
@@ -727,11 +749,11 @@
return std::make_shared<PathDiagnosticEventPiece>(L, os.str());
}
-bool NoStoreFuncVisitor::prettyPrintRegionName(StringRef FirstElement,
- bool FirstIsReferenceType,
+bool NoStoreFuncVisitor::prettyPrintRegionName(const RegionVector &FieldChain,
const MemRegion *MatchedRegion,
- const RegionVector &FieldChain,
- int IndirectionLevel,
+ StringRef FirstElement,
+ bool FirstIsReferenceType,
+ unsigned IndirectionLevel,
llvm::raw_svector_ostream &os) {
if (FirstIsReferenceType)
Index: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -48,6 +48,7 @@
#include "InterCheckerAPI.h"
#include "clang/AST/Attr.h"
#include "clang/AST/DeclCXX.h"
+#include "clang/AST/DeclTemplate.h"
#include "clang/AST/Expr.h"
#include "clang/AST/ExprCXX.h"
#include "clang/AST/ParentMap.h"
@@ -64,12 +65,15 @@
#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/DynamicExtent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState_Fwd.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/StoreRef.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h"
#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SetOperations.h"
#include "llvm/ADT/SmallString.h"
#include "llvm/ADT/StringExtras.h"
#include "llvm/Support/Compiler.h"
@@ -298,6 +302,8 @@
/// which might free a pointer are annotated.
DefaultBool ShouldIncludeOwnershipAnnotatedFunctions;
+ DefaultBool ShouldRegisterNoOwnershipChangeVisitor;
+
/// Many checkers are essentially built into this one, so enabling them will
/// make MallocChecker perform additional modeling and reporting.
enum CheckKind {
@@ -722,11 +728,146 @@
bool isArgZERO_SIZE_PTR(ProgramStateRef State, CheckerContext &C,
SVal ArgVal) const;
};
+} // end anonymous namespace
+
+//===----------------------------------------------------------------------===//
+// Definition of NoOwnershipChangeVisitor.
+//===----------------------------------------------------------------------===//
+
+namespace {
+class NoOwnershipChangeVisitor final : public NoStateChangeFuncVisitor {
+ SymbolRef Sym;
+ using OwnerSet = llvm::SmallPtrSet<const MemRegion *, 8>;
+
+ // Collect which entities point to the allocated memory, and could be
+ // responsible for deallocating it.
+ class OwnershipBindingsHandler : public StoreManager::BindingsHandler {
+ SymbolRef Sym;
+ OwnerSet &Owners;
+
+ public:
+ OwnershipBindingsHandler(SymbolRef Sym, OwnerSet &Owners)
+ : Sym(Sym), Owners(Owners) {}
+
+ bool HandleBinding(StoreManager &SMgr, Store Store, const MemRegion *Region,
+ SVal Val) override {
+ if (Val.getAsSymbol() == Sym)
+ Owners.insert(Region);
+ return true;
+ }
+ };
+
+protected:
+ OwnerSet getOwnersAtNode(const ExplodedNode *N) {
+ OwnerSet Ret;
+
+ ProgramStateRef State = N->getState();
+ OwnershipBindingsHandler Handler{Sym, Ret};
+ State->getStateManager().getStoreManager().iterBindings(State->getStore(),
+ Handler);
+ return Ret;
+ }
+
+ static const ExplodedNode *getCallExitEnd(const ExplodedNode *N) {
+ while (N && !N->getLocationAs<CallExitEnd>())
+ N = N->getFirstSucc();
+ return N;
+ }
+
+ virtual bool
+ wasModifiedBeforeCallExit(const ExplodedNode *CurrN,
+ const ExplodedNode *CallExitN) override {
+ if (CurrN->getLocationAs<CallEnter>())
+ return true;
+
+ // Its the state right *after* the call that is interesting. Any pointers
+ // inside the call that pointed to the allocated memory are of little
+ // consequence if their lifetime ends within the function.
+ CallExitN = getCallExitEnd(CallExitN);
+ if (!CallExitN)
+ return true;
+
+ if (CurrN->getState()->get<RegionState>(Sym) !=
+ CallExitN->getState()->get<RegionState>(Sym))
+ return true;
+
+ OwnerSet CurrOwners = getOwnersAtNode(CurrN);
+ OwnerSet ExitOwners = getOwnersAtNode(CallExitN);
+
+ // Owners in the current set may be purged from the analyzer later on.
+ // If a variable is dead (is not referenced directly or indirectly after
+ // some point), it will be removed from the Store before the end of its
+ // actual lifetime.
+ // This means that that if the ownership status didn't change, CurrOwners
+ // must be a superset of, but not necessarily equal to ExitOwners.
+ return !llvm::set_is_subset(ExitOwners, CurrOwners);
+ }
+
+ static PathDiagnosticPieceRef emitNote(const ExplodedNode *N) {
+ PathDiagnosticLocation L = PathDiagnosticLocation::create(
+ N->getLocation(),
+ N->getState()->getStateManager().getContext().getSourceManager());
+ return std::make_shared<PathDiagnosticEventPiece>(
+ L, "Returning without deallocating memory or storing the pointer for "
+ "later deallocation");
+ }
+
+ virtual PathDiagnosticPieceRef
+ maybeEmitNoteForObjCSelf(PathSensitiveBugReport &R,
+ const ObjCMethodCall &Call,
+ const ExplodedNode *N) override {
+ // TODO: Implement.
+ return nullptr;
+ }
+
+ virtual PathDiagnosticPieceRef
+ maybeEmitNoteForCXXThis(PathSensitiveBugReport &R,
+ const CXXConstructorCall &Call,
+ const ExplodedNode *N) override {
+ // TODO: Implement.
+ return nullptr;
+ }
+
+ virtual PathDiagnosticPieceRef
+ maybeEmitNoteForParameters(PathSensitiveBugReport &R, const CallEvent &Call,
+ const ExplodedNode *N) override {
+ // TODO: Factor the logic of "what constitutes as an entity being passed
+ // into a function call" out by reusing the code in
+ // NoStoreFuncVisitor::maybeEmitNoteForParameters, maybe by incorporating
+ // the printing technology in UninitializedObject's FieldChainInfo.
+ ArrayRef<ParmVarDecl *> Parameters = Call.parameters();
+ for (unsigned I = 0; I < Call.getNumArgs() && I < Parameters.size(); ++I) {
+ SVal V = Call.getArgSVal(I);
+ if (V.getAsSymbol() == Sym)
+ return emitNote(N);
+ }
+ return nullptr;
+ }
+
+public:
+ NoOwnershipChangeVisitor(SymbolRef Sym)
+ : NoStateChangeFuncVisitor(bugreporter::TrackingKind::Thorough),
+ Sym(Sym) {}
+
+ void Profile(llvm::FoldingSetNodeID &ID) const override {
+ static int Tag = 0;
+ ID.AddPointer(&Tag);
+ ID.AddPointer(Sym);
+ }
+
+ void *getTag() const {
+ static int Tag = 0;
+ return static_cast<void *>(&Tag);
+ }
+};
+
+} // end anonymous namespace
//===----------------------------------------------------------------------===//
// Definition of MallocBugVisitor.
//===----------------------------------------------------------------------===//
+namespace {
/// The bug visitor which allows us to print extra diagnostics along the
/// BugReport path. For example, showing the allocation site of the leaked
/// region.
@@ -851,7 +992,6 @@
}
};
};
-
} // end anonymous namespace
// A map from the freed symbol to the symbol representing the return value of
@@ -2579,6 +2719,8 @@
AllocNode->getLocationContext()->getDecl());
R->markInteresting(Sym);
R->addVisitor<MallocBugVisitor>(Sym, true);
+ if (ShouldRegisterNoOwnershipChangeVisitor)
+ R->addVisitor<NoOwnershipChangeVisitor>(Sym);
C.emitReport(std::move(R));
}
@@ -3395,6 +3537,9 @@
auto *checker = mgr.registerChecker<MallocChecker>();
checker->ShouldIncludeOwnershipAnnotatedFunctions =
mgr.getAnalyzerOptions().getCheckerBooleanOption(checker, "Optimistic");
+ checker->ShouldRegisterNoOwnershipChangeVisitor =
+ mgr.getAnalyzerOptions().getCheckerBooleanOption(
+ checker, "AddNoOwnershipChangeNotes");
}
bool ento::shouldRegisterDynamicMemoryModeling(const CheckerManager &mgr) {
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,84 @@
PathSensitiveBugReport &R) override;
};
+class ObjCMethodCall;
+class CXXConstructorCall;
+
+/// Put a diagnostic on return statement (or on } in its absence) 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 CallExitBeginN belongs.
+ /// The calculation is cached in FramesModifying.
+ bool isModifiedInFrame(const ExplodedNode *CallExitBeginN);
+
+ /// Write to \c FramesModifying all stack frames along the path in the current
+ /// stack frame which modifies the state.
+ void findModifyingFrames(const ExplodedNode *const CallExitBeginN);
+
+protected:
+ bugreporter::TrackingKind TKind;
+
+ /// \return Whether the state was modified from the current node, \CurrN, to
+ /// the end of the stack fram, at \p CallExitBeginN.
+ virtual bool
+ wasModifiedBeforeCallExit(const ExplodedNode *CurrN,
+ const ExplodedNode *CallExitBeginN) = 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
+ maybeEmitNoteForObjCSelf(PathSensitiveBugReport &R,
+ const ObjCMethodCall &Call,
+ const ExplodedNode *N) = 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
+ maybeEmitNoteForCXXThis(PathSensitiveBugReport &R,
+ const CXXConstructorCall &Call,
+ const ExplodedNode *N) = 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
+ maybeEmitNoteForParameters(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
Index: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
===================================================================
--- clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -485,7 +485,17 @@
"allocating and deallocating functions are annotated with "
"ownership_holds, ownership_takes and ownership_returns.",
"false",
- InAlpha>
+ InAlpha>,
+ CmdLineOption<Boolean,
+ "AddNoOwnershipChangeNotes",
+ "Add an additional note to the bug report for leak-like "
+ "bugs. Dynamically allocated objects passed to functions "
+ "that neither deallocated it, or have taken responsibility "
+ "of the ownership are noted, similarly to "
+ "NoStoreFuncVisitor.",
+ "false",
+ InAlpha,
+ Hide>
]>,
Dependencies<[CStringModeling]>,
Documentation<NotDocumented>,
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
