From 7d4694547a6b6593fb537cd90893121f484e9b4d Mon Sep 17 00:00:00 2001 From: Artem Dergachev Date: Sat, 13 Apr 2019 02:01:45 +0000 Subject: [PATCH] [analyzer] Escape pointers stored into top-level parameters with destructors. Writing stuff into an argument variable is usually equivalent to writing stuff to a local variable: it will have no effect outside of the function. There's an important exception from this rule: if the argument variable has a non-trivial destructor, the destructor would be invoked on the parent stack frame, exposing contents of the otherwise dead argument variable to the caller. If such argument is the last place where a pointer is stored before the function exits and the function is the one we've started our analysis from (i.e., we have no caller context for it), we currently diagnose a leak. This is incorrect because the destructor of the argument still has access to the pointer. The destructor may deallocate the pointer or even pass it further. Treat writes into such argument regions as "escapes" instead, suppressing spurious memory leak reports but not messing with dead symbol removal. Differential Revision: https://reviews.llvm.org/D60112 llvm-svn: 358321 --- clang/lib/StaticAnalyzer/Core/ExprEngine.cpp | 58 +++++++++----------- clang/test/Analysis/malloc.cpp | 23 ++++++++ 2 files changed, 50 insertions(+), 31 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp index fb86ceab135d..000edef1ca8e 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp @@ -2621,43 +2621,39 @@ void ExprEngine::VisitAtomicExpr(const AtomicExpr *AE, ExplodedNode *Pred, getCheckerManager().runCheckersForPostStmt(Dst, AfterInvalidateSet, AE, *this); } -// A value escapes in three possible cases: +// A value escapes in four possible cases: // (1) We are binding to something that is not a memory region. -// (2) We are binding to a MemrRegion that does not have stack storage. -// (3) We are binding to a MemRegion with stack storage that the store +// (2) We are binding to a MemRegion that does not have stack storage. +// (3) We are binding to a top-level parameter region with a non-trivial +// destructor. We won't see the destructor during analysis, but it's there. +// (4) We are binding to a MemRegion with stack storage that the store // does not understand. -ProgramStateRef ExprEngine::processPointerEscapedOnBind(ProgramStateRef State, - SVal Loc, - SVal Val, - const LocationContext *LCtx) { - // Are we storing to something that causes the value to "escape"? - bool escapes = true; +ProgramStateRef +ExprEngine::processPointerEscapedOnBind(ProgramStateRef State, SVal Loc, + SVal Val, const LocationContext *LCtx) { - // TODO: Move to StoreManager. - if (Optional regionLoc = Loc.getAs()) { - escapes = !regionLoc->getRegion()->hasStackStorage(); + // Cases (1) and (2). + const MemRegion *MR = Loc.getAsRegion(); + if (!MR || !MR->hasStackStorage()) + return escapeValue(State, Val, PSK_EscapeOnBind); - 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). - // Do this only if we know that the store is not supposed to generate the - // same state. - SVal StoredVal = State->getSVal(regionLoc->getRegion()); - if (StoredVal != Val) - escapes = (State == (State->bindLoc(*regionLoc, Val, LCtx))); - } - } + // Case (3). + if (const auto *VR = dyn_cast(MR->getBaseRegion())) + if (VR->hasStackParametersStorage() && VR->getStackFrame()->inTopFrame()) + if (const auto *RD = VR->getValueType()->getAsCXXRecordDecl()) + if (!RD->hasTrivialDestructor()) + return escapeValue(State, Val, PSK_EscapeOnBind); - // 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 State; + // Case (4): in order to test that, generate a new state with the binding + // added. If it is the same state, then it escapes (since the store cannot + // represent the binding). + // Do this only if we know that the store is not supposed to generate the + // same state. + SVal StoredVal = State->getSVal(MR); + if (StoredVal != Val) + if (State == (State->bindLoc(loc::MemRegionVal(MR), Val, LCtx))) + return escapeValue(State, Val, PSK_EscapeOnBind); - // Otherwise, find all symbols referenced by 'val' that we are tracking - // and stop tracking them. - State = escapeValue(State, Val, PSK_EscapeOnBind); return State; } diff --git a/clang/test/Analysis/malloc.cpp b/clang/test/Analysis/malloc.cpp index b93c73e591cd..6e5a0e4d5971 100644 --- a/clang/test/Analysis/malloc.cpp +++ b/clang/test/Analysis/malloc.cpp @@ -141,3 +141,26 @@ char* test_cxa_demangle(const char* sym) { } return funcname; // no-warning } + +namespace argument_leak { +class A { + char *name; + +public: + char *getName() { + if (!name) { + name = static_cast(malloc(10)); + } + return name; + } + ~A() { + if (name) { + delete[] name; + } + } +}; + +void test(A a) { + (void)a.getName(); +} +} // namespace argument_leak