From: Anna Zaks Date: Sat, 11 Feb 2012 21:02:35 +0000 (+0000) Subject: [analyzer] MallocChecker: refactor/improve the symbol escape logic. X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=4fb548710837dc4e709e1a84f241c4bea121e895;p=clang [analyzer] MallocChecker: refactor/improve the symbol escape logic. We use the same logic here as the RetainRelease checker. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@150311 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/lib/StaticAnalyzer/Checkers/MallocChecker.cpp index 8b6964bff7..430a77a570 100644 --- a/lib/StaticAnalyzer/Checkers/MallocChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/MallocChecker.cpp @@ -72,7 +72,8 @@ class MallocChecker : public Checker, check::Location, check::Bind, - eval::Assume> + eval::Assume, + check::RegionChanges> { mutable OwningPtr BT_DoubleFree; mutable OwningPtr BT_Leak; @@ -105,6 +106,14 @@ public: CheckerContext &C) const; void checkBind(SVal location, SVal val, const Stmt*S, CheckerContext &C) const; + ProgramStateRef + checkRegionChanges(ProgramStateRef state, + const StoreManager::InvalidatedSymbols *invalidated, + ArrayRef ExplicitRegions, + ArrayRef Regions) const; + bool wantsRegionChangeUpdate(ProgramStateRef state) const { + return true; + } private: static void MallocMem(CheckerContext &C, const CallExpr *CE); @@ -187,6 +196,20 @@ namespace ento { } } +namespace { +class StopTrackingCallback : public SymbolVisitor { + ProgramStateRef state; +public: + StopTrackingCallback(ProgramStateRef st) : state(st) {} + ProgramStateRef getState() const { return state; } + + bool VisitSymbol(SymbolRef sym) { + state = state->remove(sym); + return true; + } +}; +} // end anonymous namespace + void MallocChecker::initIdentifierInfo(CheckerContext &C) const { ASTContext &Ctx = C.getASTContext(); if (!II_malloc) @@ -734,23 +757,6 @@ void MallocChecker::checkPreStmt(const ReturnStmt *S, CheckerContext &C) const { checkEscape(Sym, S, C); } -ProgramStateRef MallocChecker::evalAssume(ProgramStateRef state, - SVal Cond, - bool Assumption) const { - // If a symbolic region is assumed to NULL, set its state to AllocateFailed. - // FIXME: should also check symbols assumed to non-null. - - RegionStateTy RS = state->get(); - - for (RegionStateTy::iterator I = RS.begin(), E = RS.end(); I != E; ++I) { - // If the symbol is assumed to NULL, this will return an APSInt*. - if (state->getSymVal(I.getKey())) - state = state->set(I.getKey(),RefState::getAllocateFailed()); - } - - return state; -} - bool MallocChecker::checkUseAfterFree(SymbolRef Sym, CheckerContext &C, const Stmt *S) const { assert(Sym); @@ -780,65 +786,91 @@ void MallocChecker::checkLocation(SVal l, bool isLoad, const Stmt *S, checkUseAfterFree(Sym, C); } -void MallocChecker::checkBind(SVal location, SVal val, - const Stmt *BindS, CheckerContext &C) const { - // The PreVisitBind implements the same algorithm as already used by the - // Objective C ownership checker: if the pointer escaped from this scope by - // assignment, let it go. However, assigning to fields of a stack-storage - // structure does not transfer ownership. +//===----------------------------------------------------------------------===// +// Check various ways a symbol can be invalidated. +// TODO: This logic (the next 3 functions) is copied/similar to the +// RetainRelease checker. We might want to factor this out. +//===----------------------------------------------------------------------===// +// Stop tracking symbols when a value escapes as a result of checkBind. +// A value escapes in three possible cases: +// (1) we are binding to something that is not a memory region. +// (2) we are binding to a memregion that does not have stack storage +// (3) we are binding to a memregion with stack storage that the store +// does not understand. +void MallocChecker::checkBind(SVal loc, SVal val, const Stmt *S, + CheckerContext &C) const { + // Are we storing to something that causes the value to "escape"? + bool escapes = true; ProgramStateRef state = C.getState(); - if (!isa(location)) - return; - DefinedOrUnknownSVal l = cast(location); - // Check for null dereferences. - if (!isa(l)) - return; + if (loc::MemRegionVal *regionLoc = dyn_cast(&loc)) { + escapes = !regionLoc->getRegion()->hasStackStorage(); - // Before checking if the state is null, check if 'val' has a RefState. - // Only then should we check for null and bifurcate the state. - SymbolRef Sym = val.getLocSymbolInBase(); - if (Sym) { - if (const RefState *RS = state->get(Sym)) { - // If ptr is NULL, no operation is performed. - ProgramStateRef notNullState, nullState; - llvm::tie(notNullState, nullState) = state->assume(l); - - // Generate a transition for 'nullState' to record the assumption - // that the state was null. - if (nullState) - C.addTransition(nullState); - - if (!notNullState) - return; - - if (RS->isAllocated()) { - // Something we presently own is being assigned somewhere. - const MemRegion *AR = location.getAsRegion(); - if (!AR) - return; - AR = AR->StripCasts()->getBaseRegion(); - do { - // If it is on the stack, we still own it. - if (AR->hasStackNonParametersStorage()) - break; - - // If the state can't represent this binding, we still own it. - if (notNullState == (notNullState->bindLoc(cast(location), - UnknownVal()))) - break; - - // We no longer own this pointer. - notNullState = - notNullState->set(Sym, - RefState::getRelinquished(BindS)); - } - while (false); - } - C.addTransition(notNullState); + if (!escapes) { + // To test (3), generate a new state with the binding added. If it is + // the same state, then it escapes (since the store cannot represent + // the binding). + escapes = (state == (state->bindLoc(*regionLoc, val))); } } + + // If our store can represent the binding and we aren't storing to something + // that doesn't have local storage then just return and have the simulation + // state continue as is. + if (!escapes) + return; + + // Otherwise, find all symbols referenced by 'val' that we are tracking + // and stop tracking them. + state = state->scanReachableSymbols(val).getState(); + C.addTransition(state); +} + +// If a symbolic region is assumed to NULL (or another constant), stop tracking +// it - assuming that allocation failed on this path. +ProgramStateRef MallocChecker::evalAssume(ProgramStateRef state, + SVal Cond, + bool Assumption) const { + RegionStateTy RS = state->get(); + + for (RegionStateTy::iterator I = RS.begin(), E = RS.end(); I != E; ++I) { + // If the symbol is assumed to NULL or another constant, this will + // return an APSInt*. + if (state->getSymVal(I.getKey())) + state = state->remove(I.getKey()); + } + + return state; +} + +// If the symbol we are tracking is invalidated, but not explicitly (ex: the &p +// escapes, when we are tracking p), do not track the symbol as we cannot reason +// about it anymore. +ProgramStateRef +MallocChecker::checkRegionChanges(ProgramStateRef state, + const StoreManager::InvalidatedSymbols *invalidated, + ArrayRef ExplicitRegions, + ArrayRef Regions) const { + if (!invalidated) + return state; + + llvm::SmallPtrSet WhitelistedSymbols; + for (ArrayRef::iterator I = ExplicitRegions.begin(), + E = ExplicitRegions.end(); I != E; ++I) { + if (const SymbolicRegion *SR = (*I)->StripCasts()->getAs()) + WhitelistedSymbols.insert(SR->getSymbol()); + } + + for (StoreManager::InvalidatedSymbols::const_iterator I=invalidated->begin(), + E = invalidated->end(); I!=E; ++I) { + SymbolRef sym = *I; + if (WhitelistedSymbols.count(sym)) + continue; + // Don't track the symbol. + state = state->remove(sym); + } + return state; } PathDiagnosticPiece * diff --git a/test/Analysis/malloc.c b/test/Analysis/malloc.c index ec767050be..a219e92787 100644 --- a/test/Analysis/malloc.c +++ b/test/Analysis/malloc.c @@ -259,6 +259,13 @@ void mallocEscapeFooNonSymbolArg() { return; // no warning } +void mallocFailedOrNotLeak() { + int *p = malloc(12); + if (p == 0) + return; // no warning + else + return; // expected-warning {{Allocated memory never released. Potential memory leak.}} +} int *Gl; struct GlStTy { @@ -286,10 +293,55 @@ void GlobalStructMallocFree() { free(GlS.x); } +// Region escape testing. + +unsigned takePtrToPtr(int **p); +void PassTheAddrOfAllocatedData(int f) { + int *p = malloc(12); + // We don't know what happens after the call. Should stop tracking here. + if (takePtrToPtr(&p)) + f++; + free(p); // no warning +} + +struct X { + int *p; +}; +unsigned takePtrToStruct(struct X *s); +int ** foo2(int *g, int f) { + int *p = malloc(12); + struct X *px= malloc(sizeof(struct X)); + px->p = p; + // We don't know what happens after this call. Should not track px nor p. + if (takePtrToStruct(px)) + f++; + free(p); + return 0; +} + +struct X* RegInvalidationDetect1(struct X *s2) { + struct X *px= malloc(sizeof(struct X)); + px->p = 0; + px = s2; + return px; // expected-warning {{Allocated memory never released. Potential memory leak.}} +} + +struct X* RegInvalidationGiveUp1() { + int *p = malloc(12); + struct X *px= malloc(sizeof(struct X)); + px->p = p; + return px; +} + +int **RegInvalidationDetect2(int **pp) { + int *p = malloc(12); + pp = &p; + pp++; + return 0;// expected-warning {{Allocated memory never released. Potential memory leak.}} +} // Below are the known false positives. -// TODO: There should be no warning here. extern void exit(int) __attribute__ ((__noreturn__)); void mallocExit(int *g) { struct xx *p = malloc(12); @@ -316,16 +368,6 @@ void mallocAssert(int *g) { return; } -// TODO: There should be no warning here. -unsigned takePtrToPtr(int **p); -void PassTheAddrOfAllocatedData(int *g, int f) { - int *p = malloc(12); - // This call is causing the problem. - if (takePtrToPtr(&p)) - f++; // expected-warning{{Allocated memory never released. Potential memory leak}} - free(p); // expected-warning{{Allocated memory never released. Potential memory leak}} -} - // TODO: There should be no warning here. void reallocFails(int *g, int f) { char *p = malloc(12);