From: Anna Zaks Date: Thu, 28 Mar 2013 23:15:29 +0000 (+0000) Subject: [analyzer] Add support for escape of const pointers and use it to allow “newed” point... X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=41988f331a74a72cf243a2a68ffb56418e9a174e;p=clang [analyzer] Add support for escape of const pointers and use it to allow “newed” pointers to escape Add a new callback that notifies checkers when a const pointer escapes. Currently, this only works for const pointers passed as a top level parameter into a function. We need to differentiate the const pointers escape from regular escape since the content pointed by const pointer will not change; if it’s a file handle, a file cannot be closed; but delete is allowed on const pointers. This should suppress several false positives reported by the NewDelete checker on llvm codebase. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@178310 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/include/clang/StaticAnalyzer/Core/Checker.h b/include/clang/StaticAnalyzer/Core/Checker.h index 305ae2579d..0dbaab033d 100644 --- a/include/clang/StaticAnalyzer/Core/Checker.h +++ b/include/clang/StaticAnalyzer/Core/Checker.h @@ -324,11 +324,14 @@ class PointerEscape { ProgramStateRef State, const InvalidatedSymbols &Escaped, const CallEvent *Call, - PointerEscapeKind Kind) { - return ((const CHECKER *)checker)->checkPointerEscape(State, - Escaped, - Call, - Kind); + PointerEscapeKind Kind, + bool IsConst) { + if (!IsConst) + return ((const CHECKER *)checker)->checkPointerEscape(State, + Escaped, + Call, + Kind); + return State; } public: @@ -340,6 +343,33 @@ public: } }; +class ConstPointerEscape { + template + static ProgramStateRef + _checkConstPointerEscape(void *checker, + ProgramStateRef State, + const InvalidatedSymbols &Escaped, + const CallEvent *Call, + PointerEscapeKind Kind, + bool IsConst) { + if (IsConst) + return ((const CHECKER *)checker)->checkConstPointerEscape(State, + Escaped, + Call, + Kind); + return State; + } + +public: + template + static void _register(CHECKER *checker, CheckerManager &mgr) { + mgr._registerForPointerEscape( + CheckerManager::CheckPointerEscapeFunc(checker, + _checkConstPointerEscape)); + } +}; + + template class Event { template diff --git a/include/clang/StaticAnalyzer/Core/CheckerManager.h b/include/clang/StaticAnalyzer/Core/CheckerManager.h index 4353ebf015..6f99fc1457 100644 --- a/include/clang/StaticAnalyzer/Core/CheckerManager.h +++ b/include/clang/StaticAnalyzer/Core/CheckerManager.h @@ -346,12 +346,14 @@ public: /// \param Escaped The list of escaped symbols. /// \param Call The corresponding CallEvent, if the symbols escape as /// parameters to the given call. + /// \param IsConst Specifies if the pointer is const. /// \returns Checkers can modify the state by returning a new one. ProgramStateRef runCheckersForPointerEscape(ProgramStateRef State, const InvalidatedSymbols &Escaped, const CallEvent *Call, - PointerEscapeKind Kind); + PointerEscapeKind Kind, + bool IsConst = false); /// \brief Run checkers for handling assumptions on symbolic values. ProgramStateRef runCheckersForEvalAssume(ProgramStateRef state, @@ -442,7 +444,8 @@ public: typedef CheckerFn + PointerEscapeKind Kind, + bool IsConst)> CheckPointerEscapeFunc; typedef CheckerFn ExplicitRegions, ArrayRef Regions, - const CallEvent *Call); + const CallEvent *Call, + bool IsConst); public: // FIXME: 'tag' should be removed, and a LocationContext should be used diff --git a/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h b/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h index 8318fe0c26..9ae24c446e 100644 --- a/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h +++ b/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h @@ -187,6 +187,8 @@ public: /// accessible. Pass \c NULL if this information will not be used. /// \param[in] Call The call expression which will be used to determine which /// globals should get invalidated. + /// \param[in,out] ConstIS A set to fill with any symbols corresponding to + /// the ConstRegions. /// \param[in,out] Invalidated A vector to fill with any regions being /// invalidated. This should include any regions explicitly invalidated /// even if they do not currently have bindings. Pass \c NULL if this @@ -198,6 +200,7 @@ public: InvalidatedSymbols &IS, const CallEvent *Call, ArrayRef ConstRegions, + InvalidatedSymbols &ConstIS, InvalidatedRegions *Invalidated) = 0; /// enterStackFrame - Let the StoreManager to do something when execution diff --git a/include/clang/StaticAnalyzer/Core/PathSensitive/SubEngine.h b/include/clang/StaticAnalyzer/Core/PathSensitive/SubEngine.h index 0e9f25375d..4578f6f323 100644 --- a/include/clang/StaticAnalyzer/Core/PathSensitive/SubEngine.h +++ b/include/clang/StaticAnalyzer/Core/PathSensitive/SubEngine.h @@ -120,11 +120,12 @@ public: processPointerEscapedOnBind(ProgramStateRef State, SVal Loc, SVal Val) = 0; virtual ProgramStateRef - processPointerEscapedOnInvalidateRegions(ProgramStateRef State, + notifyCheckersOfPointerEscape(ProgramStateRef State, const InvalidatedSymbols *Invalidated, ArrayRef ExplicitRegions, ArrayRef Regions, - const CallEvent *Call) = 0; + const CallEvent *Call, + bool IsConst = false) = 0; /// printState - Called by ProgramStateManager to print checker-specific data. virtual void printState(raw_ostream &Out, ProgramStateRef State, diff --git a/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/lib/StaticAnalyzer/Checkers/MallocChecker.cpp index b4e45a29d6..57666499c4 100644 --- a/lib/StaticAnalyzer/Checkers/MallocChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/MallocChecker.cpp @@ -134,6 +134,7 @@ typedef std::pair LeakInfo; class MallocChecker : public Checker, check::PreStmt, check::PostStmt, @@ -185,6 +186,10 @@ public: const InvalidatedSymbols &Escaped, const CallEvent *Call, PointerEscapeKind Kind) const; + ProgramStateRef checkConstPointerEscape(ProgramStateRef State, + const InvalidatedSymbols &Escaped, + const CallEvent *Call, + PointerEscapeKind Kind) const; void printState(raw_ostream &Out, ProgramStateRef State, const char *NL, const char *Sep) const; @@ -270,6 +275,13 @@ private: bool doesNotFreeMemOrInteresting(const CallEvent *Call, ProgramStateRef State) const; + // Implementation of the checkPointerEscape callabcks. + ProgramStateRef checkPointerEscapeAux(ProgramStateRef State, + const InvalidatedSymbols &Escaped, + const CallEvent *Call, + PointerEscapeKind Kind, + bool(*CheckRefState)(const RefState*)) const; + static bool SummarizeValue(raw_ostream &os, SVal V); static bool SummarizeRegion(raw_ostream &os, const MemRegion *MR); void ReportBadFree(CheckerContext &C, SVal ArgVal, SourceRange Range, @@ -1865,10 +1877,35 @@ bool MallocChecker::doesNotFreeMemOrInteresting(const CallEvent *Call, return true; } +static bool retTrue(const RefState *RS) { + return true; +} + +static bool checkIfNewOrNewArrayFamily(const RefState *RS) { + return (RS->getAllocationFamily() == AF_CXXNewArray || + RS->getAllocationFamily() == AF_CXXNew); +} + ProgramStateRef MallocChecker::checkPointerEscape(ProgramStateRef State, const InvalidatedSymbols &Escaped, const CallEvent *Call, PointerEscapeKind Kind) const { + return checkPointerEscapeAux(State, Escaped, Call, Kind, &retTrue); +} + +ProgramStateRef MallocChecker::checkConstPointerEscape(ProgramStateRef State, + const InvalidatedSymbols &Escaped, + const CallEvent *Call, + PointerEscapeKind Kind) const { + return checkPointerEscapeAux(State, Escaped, Call, Kind, + &checkIfNewOrNewArrayFamily); +} + +ProgramStateRef MallocChecker::checkPointerEscapeAux(ProgramStateRef State, + const InvalidatedSymbols &Escaped, + const CallEvent *Call, + PointerEscapeKind Kind, + bool(*CheckRefState)(const RefState*)) const { // If we know that the call does not free memory, or we want to process the // call later, keep tracking the top level arguments. if ((Kind == PSK_DirectEscapeOnCall || @@ -1878,12 +1915,12 @@ ProgramStateRef MallocChecker::checkPointerEscape(ProgramStateRef State, } for (InvalidatedSymbols::const_iterator I = Escaped.begin(), - E = Escaped.end(); - I != E; ++I) { + E = Escaped.end(); + I != E; ++I) { SymbolRef sym = *I; if (const RefState *RS = State->get(sym)) { - if (RS->isAllocated()) + if (RS->isAllocated() && CheckRefState(RS)) State = State->remove(sym); } } diff --git a/lib/StaticAnalyzer/Core/CheckerManager.cpp b/lib/StaticAnalyzer/Core/CheckerManager.cpp index a383799788..8adf3262b3 100644 --- a/lib/StaticAnalyzer/Core/CheckerManager.cpp +++ b/lib/StaticAnalyzer/Core/CheckerManager.cpp @@ -489,19 +489,19 @@ ProgramStateRef CheckerManager::runCheckersForPointerEscape(ProgramStateRef State, const InvalidatedSymbols &Escaped, const CallEvent *Call, - PointerEscapeKind Kind) { + PointerEscapeKind Kind, + bool IsConst) { assert((Call != NULL || (Kind != PSK_DirectEscapeOnCall && Kind != PSK_IndirectEscapeOnCall)) && "Call must not be NULL when escaping on call"); - - for (unsigned i = 0, e = PointerEscapeCheckers.size(); i != e; ++i) { - // If any checker declares the state infeasible (or if it starts that way), - // bail out. - if (!State) - return NULL; - State = PointerEscapeCheckers[i](State, Escaped, Call, Kind); - } + for (unsigned i = 0, e = PointerEscapeCheckers.size(); i != e; ++i) { + // If any checker declares the state infeasible (or if it starts that + // way), bail out. + if (!State) + return NULL; + State = PointerEscapeCheckers[i](State, Escaped, Call, Kind, IsConst); + } return State; } @@ -666,6 +666,11 @@ void CheckerManager::_registerForPointerEscape(CheckPointerEscapeFunc checkfn){ PointerEscapeCheckers.push_back(checkfn); } +void CheckerManager::_registerForConstPointerEscape( + CheckPointerEscapeFunc checkfn) { + PointerEscapeCheckers.push_back(checkfn); +} + void CheckerManager::_registerForEvalAssume(EvalAssumeFunc checkfn) { EvalAssumeCheckers.push_back(checkfn); } diff --git a/lib/StaticAnalyzer/Core/ExprEngine.cpp b/lib/StaticAnalyzer/Core/ExprEngine.cpp index f245bbf49e..8ad5a3dbde 100644 --- a/lib/StaticAnalyzer/Core/ExprEngine.cpp +++ b/lib/StaticAnalyzer/Core/ExprEngine.cpp @@ -1714,11 +1714,12 @@ ProgramStateRef ExprEngine::processPointerEscapedOnBind(ProgramStateRef State, } ProgramStateRef -ExprEngine::processPointerEscapedOnInvalidateRegions(ProgramStateRef State, +ExprEngine::notifyCheckersOfPointerEscape(ProgramStateRef State, const InvalidatedSymbols *Invalidated, ArrayRef ExplicitRegions, ArrayRef Regions, - const CallEvent *Call) { + const CallEvent *Call, + bool IsConst) { if (!Invalidated || Invalidated->empty()) return State; @@ -1728,7 +1729,17 @@ ExprEngine::processPointerEscapedOnInvalidateRegions(ProgramStateRef State, *Invalidated, 0, PSK_EscapeOther); - + + // Note: Due to current limitations of RegionStore, we only process the top + // level const pointers correctly. The lower level const pointers are + // currently treated as non-const. + if (IsConst) + return getCheckerManager().runCheckersForPointerEscape(State, + *Invalidated, + Call, + PSK_DirectEscapeOnCall, + true); + // If the symbols were invalidated by a call, we want to find out which ones // were invalidated directly due to being arguments to the call. InvalidatedSymbols SymbolsDirectlyInvalidated; diff --git a/lib/StaticAnalyzer/Core/ProgramState.cpp b/lib/StaticAnalyzer/Core/ProgramState.cpp index 3e47dcef2b..9aac8df0a2 100644 --- a/lib/StaticAnalyzer/Core/ProgramState.cpp +++ b/lib/StaticAnalyzer/Core/ProgramState.cpp @@ -170,25 +170,34 @@ ProgramState::invalidateRegionsImpl(RegionList Regions, RegionList ConstRegions) const { ProgramStateManager &Mgr = getStateManager(); SubEngine* Eng = Mgr.getOwningEngine(); - + InvalidatedSymbols ConstIS; + if (Eng) { StoreManager::InvalidatedRegions Invalidated; const StoreRef &newStore = Mgr.StoreMgr->invalidateRegions(getStore(), Regions, E, Count, LCtx, IS, - Call, ConstRegions, &Invalidated); + Call, ConstRegions, ConstIS, + &Invalidated); ProgramStateRef newState = makeWithStore(newStore); - if (CausedByPointerEscape) - newState = Eng->processPointerEscapedOnInvalidateRegions(newState, + if (CausedByPointerEscape) { + newState = Eng->notifyCheckersOfPointerEscape(newState, &IS, Regions, Invalidated, Call); + if (!ConstRegions.empty()) { + StoreManager::InvalidatedRegions Empty; + newState = Eng->notifyCheckersOfPointerEscape(newState, &ConstIS, + ConstRegions, Empty, Call, + true); + } + } return Eng->processRegionChanges(newState, &IS, Regions, Invalidated, Call); } const StoreRef &newStore = Mgr.StoreMgr->invalidateRegions(getStore(), Regions, E, Count, LCtx, IS, - Call, ConstRegions, NULL); + Call, ConstRegions, ConstIS, NULL); return makeWithStore(newStore); } diff --git a/lib/StaticAnalyzer/Core/RegionStore.cpp b/lib/StaticAnalyzer/Core/RegionStore.cpp index b866a58d04..08110dd3b9 100644 --- a/lib/StaticAnalyzer/Core/RegionStore.cpp +++ b/lib/StaticAnalyzer/Core/RegionStore.cpp @@ -371,6 +371,7 @@ public: InvalidatedSymbols &IS, const CallEvent *Call, ArrayRef ConstRegions, + InvalidatedSymbols &ConstIS, InvalidatedRegions *Invalidated); bool scanReachableSymbols(Store S, const MemRegion *R, @@ -882,6 +883,7 @@ class invalidateRegionsWorker : public ClusterAnalysis unsigned Count; const LocationContext *LCtx; InvalidatedSymbols &IS; + InvalidatedSymbols &ConstIS; StoreManager::InvalidatedRegions *Regions; public: invalidateRegionsWorker(RegionStoreManager &rm, @@ -890,13 +892,16 @@ public: const Expr *ex, unsigned count, const LocationContext *lctx, InvalidatedSymbols &is, + InvalidatedSymbols &inConstIS, StoreManager::InvalidatedRegions *r, bool includeGlobals) : ClusterAnalysis(rm, stateMgr, b, includeGlobals), - Ex(ex), Count(count), LCtx(lctx), IS(is), Regions(r) {} + Ex(ex), Count(count), LCtx(lctx), IS(is), ConstIS(inConstIS), Regions(r){} + /// \param IsConst Specifies if the region we are invalidating is constant. + /// If it is, we invalidate all subregions, but not the base region itself. void VisitCluster(const MemRegion *baseR, const ClusterBindings *C, - bool Flag); + bool IsConst); void VisitBinding(SVal V); }; } @@ -964,12 +969,19 @@ void invalidateRegionsWorker::VisitCluster(const MemRegion *baseR, return; } - if (IsConst) - return; - - // Symbolic region? Mark that symbol touched by the invalidation. + // Symbolic region? + SymbolRef RegionSym = 0; if (const SymbolicRegion *SR = dyn_cast(baseR)) - IS.insert(SR->getSymbol()); + RegionSym = SR->getSymbol(); + + if (IsConst) { + // Mark that symbol touched by the invalidation. + ConstIS.insert(RegionSym); + return; + } + + // Mark that symbol touched by the invalidation. + IS.insert(RegionSym); // Otherwise, we have a normal data region. Record that we touched the region. if (Regions) @@ -1058,9 +1070,10 @@ RegionStoreManager::invalidateRegions(Store store, InvalidatedSymbols &IS, const CallEvent *Call, ArrayRef ConstRegions, + InvalidatedSymbols &ConstIS, InvalidatedRegions *Invalidated) { RegionBindingsRef B = RegionStoreManager::getRegionBindings(store); - invalidateRegionsWorker W(*this, StateMgr, B, Ex, Count, LCtx, IS, + invalidateRegionsWorker W(*this, StateMgr, B, Ex, Count, LCtx, IS, ConstIS, Invalidated, false); // Scan the bindings and generate the clusters. diff --git a/test/Analysis/NewDelete-checker-test.mm b/test/Analysis/NewDelete-checker-test.mm index f14f924816..5311ff6fe7 100644 --- a/test/Analysis/NewDelete-checker-test.mm +++ b/test/Analysis/NewDelete-checker-test.mm @@ -158,3 +158,43 @@ void testStandardPlacementNewAfterDelete() { delete p; p = new(p) int; // expected-warning{{Use of memory after it is freed}} } + +//-------------------------------- +// Test escape of newed const pointer. Note, a const pointer can be deleted. +//-------------------------------- +struct StWithConstPtr { + const int *memp; +}; +void escape(const int &x); +void escapeStruct(const StWithConstPtr &x); +void escapePtr(const StWithConstPtr *x); +void escapeVoidPtr(const void *x); + +void testConstEscape() { + int *p = new int(1); + escape(*p); +} // no-warning + +void testConstEscapeStruct() { + StWithConstPtr *St = new StWithConstPtr(); + escapeStruct(*St); +} // no-warning + +void testConstEscapeStructPtr() { + StWithConstPtr *St = new StWithConstPtr(); + escapePtr(St); +} // no-warning + +void testConstEscapeMember() { + StWithConstPtr St; + St.memp = new int(2); + escapeVoidPtr(St.memp); +} // no-warning + +void testConstEscapePlacementNew() { + int *x = (int *)malloc(sizeof(int)); + void *y = new (x) int; + escapeVoidPtr(y); +} // no-warning + +