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 ]>, Dependencies<[CStringModeling]>, Documentation, 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 #include @@ -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 FramesModifying; + llvm::SmallPtrSet 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/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; + + // 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()) + N = N->getFirstSucc(); + return N; + } + + virtual bool + wasModifiedBeforeCallExit(const ExplodedNode *CurrN, + const ExplodedNode *CallExitN) override { + if (CurrN->getLocationAs()) + 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(Sym) != + CallExitN->getState()->get(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( + 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 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(&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(Sym, true); + if (ShouldRegisterNoOwnershipChangeVisitor) + R->addVisitor(Sym); C.emitReport(std::move(R)); } @@ -3395,6 +3537,9 @@ auto *checker = mgr.registerChecker(); checker->ShouldIncludeOwnershipAnnotatedFunctions = mgr.getAnalyzerOptions().getCheckerBooleanOption(checker, "Optimistic"); + checker->ShouldRegisterNoOwnershipChangeVisitor = + mgr.getAnalyzerOptions().getCheckerBooleanOption( + checker, "AddNoOwnershipChangeNotes"); } bool ento::shouldRegisterDynamicMemoryModeling(const CheckerManager &mgr) { 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()); + const ExplodedNode *LastReturnN = CallExitBeginN; + const StackFrameContext *const OriginalSCtx = + CallExitBeginN->getLocationContext()->getStackFrame(); + + const ExplodedNode *CurrN = CallExitBeginN; + + do { + ProgramStateRef State = CurrN->getState(); + auto CallExitLoc = CurrN->getLocationAs(); + 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()) + 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(); + + // 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(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(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 FramesModifyingRegion; - llvm::SmallPtrSet FramesModifyingCalculated; - using RegionVector = SmallVector; 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(&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 getCallParameters(CallEventRef<> Call) { - // Use runtime definition, if available. - RuntimeDefinition RD = Call->getRuntimeDefinition(); - if (const auto *FD = dyn_cast_or_null(RD.getDecl())) - return FD->parameters(); - if (const auto *MD = dyn_cast_or_null(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(); - - // 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(Call)) { - if (const auto *IvarR = dyn_cast(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(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(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 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 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 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()); - 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(); - 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()) - 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(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/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/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