[analyzer] Fix the "Zombie Symbols" bug.

It's an old bug that consists in stale references to symbols remaining in the
GDM if they disappear from other program state sections as a result of any
operation that isn't the actual dead symbol collection. The most common example
here is:

   FILE *fp = fopen("myfile.txt", "w");
   fp = 0; // leak of file descriptor

In this example the leak were not detected previously because the symbol
disappears from the public part of the program state due to evaluating
the assignment. For that reason the checker never receives a notification
that the symbol is dead, and never reports a leak.

This patch not only causes leak false negatives, but also a number of other
problems, including false positives on some checkers.

What's worse, even though the program state contains a finite number of symbols,
the set of symbols that dies is potentially infinite. This means that is
impossible to compute the set of all dead symbols to pass off to the checkers
for cleaning up their part of the GDM.

No longer compute the dead set at all. Disallow iterating over dead symbols.
Disallow querying if any symbols are dead. Remove the API for marking symbols
as dead, as it is no longer necessary. Update checkers accordingly.

Differential Revision: https://reviews.llvm.org/D18860

llvm-svn: 347953
This commit is contained in:
Artem Dergachev 2018-11-30 03:27:50 +00:00
parent 41c4fb40fc
commit bbc6d68297
23 changed files with 204 additions and 135 deletions

View File

@ -198,7 +198,7 @@ public:
auto &CZFactory = State->get_context<ConstraintSMT>();
for (auto I = CZ.begin(), E = CZ.end(); I != E; ++I) {
if (SymReaper.maybeDead(I->first))
if (SymReaper.isDead(I->first))
CZ = CZFactory.remove(CZ, *I);
}

View File

@ -558,7 +558,6 @@ class SymbolReaper {
SymbolMapTy TheLiving;
SymbolSetTy MetadataInUse;
SymbolSetTy TheDead;
RegionSetTy RegionRoots;
@ -603,21 +602,6 @@ public:
/// symbol marking has occurred, i.e. in the MarkLiveSymbols callback.
void markInUse(SymbolRef sym);
/// If a symbol is known to be live, marks the symbol as live.
///
/// Otherwise, if the symbol cannot be proven live, it is marked as dead.
/// Returns true if the symbol is dead, false if live.
bool maybeDead(SymbolRef sym);
using dead_iterator = SymbolSetTy::const_iterator;
dead_iterator dead_begin() const { return TheDead.begin(); }
dead_iterator dead_end() const { return TheDead.end(); }
bool hasDeadSymbols() const {
return !TheDead.empty();
}
using region_iterator = RegionSetTy::const_iterator;
region_iterator region_begin() const { return RegionRoots.begin(); }
@ -626,9 +610,9 @@ public:
/// Returns whether or not a symbol has been confirmed dead.
///
/// This should only be called once all marking of dead symbols has completed.
/// (For checkers, this means only in the evalDeadSymbols callback.)
bool isDead(SymbolRef sym) const {
return TheDead.count(sym);
/// (For checkers, this means only in the checkDeadSymbols callback.)
bool isDead(SymbolRef sym) {
return !isLive(sym);
}
void markLive(const MemRegion *region);

View File

@ -2385,9 +2385,6 @@ void CStringChecker::checkLiveSymbols(ProgramStateRef state,
void CStringChecker::checkDeadSymbols(SymbolReaper &SR,
CheckerContext &C) const {
if (!SR.hasDeadSymbols())
return;
ProgramStateRef state = C.getState();
CStringLengthTy Entries = state->get<CStringLength>();
if (Entries.isEmpty())

View File

@ -123,11 +123,6 @@ void DynamicTypePropagation::checkDeadSymbols(SymbolReaper &SR,
}
}
if (!SR.hasDeadSymbols()) {
C.addTransition(State);
return;
}
MostSpecializedTypeArgsMapTy TyArgMap =
State->get<MostSpecializedTypeArgsMap>();
for (MostSpecializedTypeArgsMapTy::iterator I = TyArgMap.begin(),

View File

@ -100,9 +100,6 @@ void MPIChecker::checkUnmatchedWaits(const CallEvent &PreCallEvent,
void MPIChecker::checkMissingWaits(SymbolReaper &SymReaper,
CheckerContext &Ctx) const {
if (!SymReaper.hasDeadSymbols())
return;
ProgramStateRef State = Ctx.getState();
const auto &Requests = State->get<RequestMap>();
if (Requests.isEmpty())

View File

@ -16,6 +16,7 @@
#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
#include "clang/StaticAnalyzer/Core/Checker.h"
#include "clang/StaticAnalyzer/Core/CheckerManager.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
@ -29,6 +30,7 @@ namespace {
class MacOSKeychainAPIChecker : public Checker<check::PreStmt<CallExpr>,
check::PostStmt<CallExpr>,
check::DeadSymbols,
check::PointerEscape,
eval::Assume> {
mutable std::unique_ptr<BugType> BT;
@ -58,6 +60,10 @@ public:
void checkPreStmt(const CallExpr *S, CheckerContext &C) const;
void checkPostStmt(const CallExpr *S, CheckerContext &C) const;
void checkDeadSymbols(SymbolReaper &SR, CheckerContext &C) const;
ProgramStateRef checkPointerEscape(ProgramStateRef State,
const InvalidatedSymbols &Escaped,
const CallEvent *Call,
PointerEscapeKind Kind) const;
ProgramStateRef evalAssume(ProgramStateRef state, SVal Cond,
bool Assumption) const;
void printState(raw_ostream &Out, ProgramStateRef State,
@ -570,6 +576,44 @@ void MacOSKeychainAPIChecker::checkDeadSymbols(SymbolReaper &SR,
C.addTransition(State, N);
}
ProgramStateRef MacOSKeychainAPIChecker::checkPointerEscape(
ProgramStateRef State, const InvalidatedSymbols &Escaped,
const CallEvent *Call, PointerEscapeKind Kind) const {
// FIXME: This branch doesn't make any sense at all, but it is an overfitted
// replacement for a previous overfitted code that was making even less sense.
if (!Call || Call->getDecl())
return State;
for (auto I : State->get<AllocatedData>()) {
SymbolRef Sym = I.first;
if (Escaped.count(Sym))
State = State->remove<AllocatedData>(Sym);
// This checker is special. Most checkers in fact only track symbols of
// SymbolConjured type, eg. symbols returned from functions such as
// malloc(). This checker tracks symbols returned as out-parameters.
//
// When a function is evaluated conservatively, the out-parameter's pointee
// base region gets invalidated with a SymbolConjured. If the base region is
// larger than the region we're interested in, the value we're interested in
// would be SymbolDerived based on that SymbolConjured. However, such
// SymbolDerived will never be listed in the Escaped set when the base
// region is invalidated because ExprEngine doesn't know which symbols
// were derived from a given symbol, while there can be infinitely many
// valid symbols derived from any given symbol.
//
// Hence the extra boilerplate: remove the derived symbol when its parent
// symbol escapes.
//
if (const auto *SD = dyn_cast<SymbolDerived>(Sym)) {
SymbolRef ParentSym = SD->getParentSymbol();
if (Escaped.count(ParentSym))
State = State->remove<AllocatedData>(Sym);
}
}
return State;
}
std::shared_ptr<PathDiagnosticPiece>
MacOSKeychainAPIChecker::SecKeychainBugVisitor::VisitNode(
const ExplodedNode *N, BugReporterContext &BRC, BugReport &BR) {

View File

@ -2345,9 +2345,6 @@ void MallocChecker::reportLeak(SymbolRef Sym, ExplodedNode *N,
void MallocChecker::checkDeadSymbols(SymbolReaper &SymReaper,
CheckerContext &C) const
{
if (!SymReaper.hasDeadSymbols())
return;
ProgramStateRef state = C.getState();
RegionStateTy RS = state->get<RegionState>();
RegionStateTy::Factory &F = state->get_context<RegionState>();

View File

@ -446,9 +446,6 @@ void NullabilityChecker::reportBugIfInvariantHolds(StringRef Msg,
/// Cleaning up the program state.
void NullabilityChecker::checkDeadSymbols(SymbolReaper &SR,
CheckerContext &C) const {
if (!SR.hasDeadSymbols())
return;
ProgramStateRef State = C.getState();
NullabilityMapTy Nullabilities = State->get<NullabilityMap>();
for (NullabilityMapTy::iterator I = Nullabilities.begin(),

View File

@ -1442,20 +1442,18 @@ void RetainCountChecker::checkDeadSymbols(SymbolReaper &SymReaper,
SmallVector<SymbolRef, 10> Leaked;
// Update counts from autorelease pools
for (SymbolReaper::dead_iterator I = SymReaper.dead_begin(),
E = SymReaper.dead_end(); I != E; ++I) {
SymbolRef Sym = *I;
if (const RefVal *T = B.lookup(Sym)){
// Use the symbol as the tag.
// FIXME: This might not be as unique as we would like.
for (const auto &I: state->get<RefBindings>()) {
SymbolRef Sym = I.first;
if (SymReaper.isDead(Sym)) {
static CheckerProgramPointTag Tag(this, "DeadSymbolAutorelease");
state = handleAutoreleaseCounts(state, Pred, &Tag, C, Sym, *T);
const RefVal &V = I.second;
state = handleAutoreleaseCounts(state, Pred, &Tag, C, Sym, V);
if (!state)
return;
// Fetch the new reference count from the state, and use it to handle
// this symbol.
state = handleSymbolDeath(state, *I, *getRefBinding(state, Sym), Leaked);
state = handleSymbolDeath(state, Sym, *getRefBinding(state, Sym), Leaked);
}
}

View File

@ -383,26 +383,26 @@ ProgramStateRef StreamChecker::CheckDoubleClose(const CallExpr *CE,
void StreamChecker::checkDeadSymbols(SymbolReaper &SymReaper,
CheckerContext &C) const {
ProgramStateRef state = C.getState();
// TODO: Clean up the state.
for (SymbolReaper::dead_iterator I = SymReaper.dead_begin(),
E = SymReaper.dead_end(); I != E; ++I) {
SymbolRef Sym = *I;
ProgramStateRef state = C.getState();
const StreamState *SS = state->get<StreamMap>(Sym);
if (!SS)
const StreamMapTy &Map = state->get<StreamMap>();
for (const auto &I: Map) {
SymbolRef Sym = I.first;
const StreamState &SS = I.second;
if (!SymReaper.isDead(Sym) || !SS.isOpened())
continue;
if (SS->isOpened()) {
ExplodedNode *N = C.generateErrorNode();
if (N) {
if (!BT_ResourceLeak)
BT_ResourceLeak.reset(new BuiltinBug(
this, "Resource Leak",
"Opened File never closed. Potential Resource leak."));
C.emitReport(llvm::make_unique<BugReport>(
*BT_ResourceLeak, BT_ResourceLeak->getDescription(), N));
}
}
ExplodedNode *N = C.generateErrorNode();
if (!N)
return;
if (!BT_ResourceLeak)
BT_ResourceLeak.reset(
new BuiltinBug(this, "Resource Leak",
"Opened File never closed. Potential Resource leak."));
C.emitReport(llvm::make_unique<BugReport>(
*BT_ResourceLeak, BT_ResourceLeak->getDescription(), N));
}
}

View File

@ -193,11 +193,6 @@ EnvironmentManager::removeDeadBindings(Environment Env,
// Mark all symbols in the block expr's value live.
RSScaner.scan(X);
continue;
} else {
SymExpr::symbol_iterator SI = X.symbol_begin(), SE = X.symbol_end();
for (; SI != SE; ++SI)
SymReaper.maybeDead(*SI);
}
}

View File

@ -675,44 +675,35 @@ void ExprEngine::removeDead(ExplodedNode *Pred, ExplodedNodeSet &Out,
// Process any special transfer function for dead symbols.
// A tag to track convenience transitions, which can be removed at cleanup.
static SimpleProgramPointTag cleanupTag(TagProviderName, "Clean Node");
if (!SymReaper.hasDeadSymbols()) {
// Generate a CleanedNode that has the environment and store cleaned
// up. Since no symbols are dead, we can optimize and not clean out
// the constraint manager.
StmtNodeBuilder Bldr(Pred, Out, *currBldrCtx);
Bldr.generateNode(DiagnosticStmt, Pred, CleanedState, &cleanupTag, K);
// Call checkers with the non-cleaned state so that they could query the
// values of the soon to be dead symbols.
ExplodedNodeSet CheckedSet;
getCheckerManager().runCheckersForDeadSymbols(CheckedSet, Pred, SymReaper,
DiagnosticStmt, *this, K);
} else {
// Call checkers with the non-cleaned state so that they could query the
// values of the soon to be dead symbols.
ExplodedNodeSet CheckedSet;
getCheckerManager().runCheckersForDeadSymbols(CheckedSet, Pred, SymReaper,
DiagnosticStmt, *this, K);
// For each node in CheckedSet, generate CleanedNodes that have the
// environment, the store, and the constraints cleaned up but have the
// user-supplied states as the predecessors.
StmtNodeBuilder Bldr(CheckedSet, Out, *currBldrCtx);
for (const auto I : CheckedSet) {
ProgramStateRef CheckerState = I->getState();
// For each node in CheckedSet, generate CleanedNodes that have the
// environment, the store, and the constraints cleaned up but have the
// user-supplied states as the predecessors.
StmtNodeBuilder Bldr(CheckedSet, Out, *currBldrCtx);
for (const auto I : CheckedSet) {
ProgramStateRef CheckerState = I->getState();
// The constraint manager has not been cleaned up yet, so clean up now.
CheckerState =
getConstraintManager().removeDeadBindings(CheckerState, SymReaper);
// The constraint manager has not been cleaned up yet, so clean up now.
CheckerState = getConstraintManager().removeDeadBindings(CheckerState,
SymReaper);
assert(StateMgr.haveEqualEnvironments(CheckerState, Pred->getState()) &&
"Checkers are not allowed to modify the Environment as a part of "
"checkDeadSymbols processing.");
assert(StateMgr.haveEqualStores(CheckerState, Pred->getState()) &&
"Checkers are not allowed to modify the Store as a part of "
"checkDeadSymbols processing.");
assert(StateMgr.haveEqualEnvironments(CheckerState, Pred->getState()) &&
"Checkers are not allowed to modify the Environment as a part of "
"checkDeadSymbols processing.");
assert(StateMgr.haveEqualStores(CheckerState, Pred->getState()) &&
"Checkers are not allowed to modify the Store as a part of "
"checkDeadSymbols processing.");
// Create a state based on CleanedState with CheckerState GDM and
// generate a transition to that state.
ProgramStateRef CleanedCheckerSt =
// Create a state based on CleanedState with CheckerState GDM and
// generate a transition to that state.
ProgramStateRef CleanedCheckerSt =
StateMgr.getPersistentStateWithGDM(CleanedState, CheckerState);
Bldr.generateNode(DiagnosticStmt, I, CleanedCheckerSt, &cleanupTag, K);
}
Bldr.generateNode(DiagnosticStmt, I, CleanedCheckerSt, &cleanupTag, K);
}
}

View File

@ -399,7 +399,7 @@ RangeConstraintManager::removeDeadBindings(ProgramStateRef State,
for (ConstraintRangeTy::iterator I = CR.begin(), E = CR.end(); I != E; ++I) {
SymbolRef Sym = I.getKey();
if (SymReaper.maybeDead(Sym)) {
if (SymReaper.isDead(Sym)) {
Changed = true;
CR = CRFactory.remove(CR, Sym);
}

View File

@ -2571,24 +2571,9 @@ StoreRef RegionStoreManager::removeDeadBindings(Store store,
const MemRegion *Base = I.getKey();
// If the cluster has been visited, we know the region has been marked.
if (W.isVisited(Base))
continue;
// Remove the dead entry.
B = B.remove(Base);
if (const SymbolicRegion *SymR = dyn_cast<SymbolicRegion>(Base))
SymReaper.maybeDead(SymR->getSymbol());
// Mark all non-live symbols that this binding references as dead.
const ClusterBindings &Cluster = I.getData();
for (ClusterBindings::iterator CI = Cluster.begin(), CE = Cluster.end();
CI != CE; ++CI) {
SVal X = CI.getData();
SymExpr::symbol_iterator SI = X.symbol_begin(), SE = X.symbol_end();
for (; SI != SE; ++SI)
SymReaper.maybeDead(*SI);
}
// Otherwise, remove the dead entry.
if (!W.isVisited(Base))
B = B.remove(Base);
}
return StoreRef(B.asStore(), *this);

View File

@ -401,7 +401,6 @@ void SymbolReaper::markDependentsLive(SymbolRef sym) {
void SymbolReaper::markLive(SymbolRef sym) {
TheLiving[sym] = NotProcessed;
TheDead.erase(sym);
markDependentsLive(sym);
}
@ -426,14 +425,6 @@ void SymbolReaper::markInUse(SymbolRef sym) {
MetadataInUse.insert(sym);
}
bool SymbolReaper::maybeDead(SymbolRef sym) {
if (isLive(sym))
return false;
TheDead.insert(sym);
return true;
}
bool SymbolReaper::isLiveRegion(const MemRegion *MR) {
if (RegionRoots.count(MR))
return true;

View File

@ -688,3 +688,25 @@ void reportSuperClass() {
c.foo(); // expected-warning {{Method call on a 'moved-from' object 'c'}} expected-note {{Method call on a 'moved-from' object 'c'}}
C c2 = c; // no-warning
}
struct Empty {};
Empty inlinedCall() {
// Used to warn because region 'e' failed to be cleaned up because no symbols
// have ever died during the analysis and the checkDeadSymbols callback
// was skipped entirely.
Empty e{};
return e; // no-warning
}
void checkInlinedCallZombies() {
while (true)
inlinedCall();
}
void checkLoopZombies() {
while (true) {
Empty e{};
Empty f = std::move(e); // no-warning
}
}

View File

@ -212,7 +212,7 @@ int foo(CFTypeRef keychainOrArray, SecProtocolType protocol,
if (st == noErr)
SecKeychainItemFreeContent(ptr, outData[3]);
}
if (length) { // TODO: We do not report a warning here since the symbol is no longer live, but it's not marked as dead.
if (length) { // expected-warning{{Allocated data is not released: missing a call to 'SecKeychainItemFreeContent'}}
length++;
}
return 0;
@ -454,3 +454,15 @@ int radar_19196494() {
}
return 0;
}
int radar_19196494_v2() {
@autoreleasepool {
AuthorizationValue login_password = {};
OSStatus err = SecKeychainFindGenericPassword(0, 0, "", 0, "", (UInt32 *)&login_password.length, (void**)&login_password.data, 0);
if (!login_password.data) return 0;
cb.SetContextVal(&login_password);
if (err == noErr) {
SecKeychainItemFreeContent(0, login_password.data);
}
}
return 0;
}

View File

@ -0,0 +1,26 @@
// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -verify %s
void clang_analyzer_eval(int);
void callee(void **p) {
int x;
*p = &x;
}
void loop() {
void *arr[2];
for (int i = 0; i < 2; ++i)
callee(&arr[i]);
// FIXME: Should be UNKNOWN.
clang_analyzer_eval(arr[0] == arr[1]); // expected-warning{{TRUE}}
}
void loopWithCall() {
void *arr[2];
for (int i = 0; i < 2; ++i) {
int x;
arr[i] = &x;
}
// FIXME: Should be UNKNOWN.
clang_analyzer_eval(arr[0] == arr[1]); // expected-warning{{TRUE}}
}

View File

@ -585,7 +585,7 @@ int f28(int i, int j, int k, int l) {
m28[j].s3[k] = 1;
struct ll * l28 = (struct ll*)(&m28[1]);
l28->s1[l] = 2;
char input[] = {'a', 'b', 'c', 'd'};
char input[] = {'a', 'b', 'c', 'd'}; // expected-warning{{Potential leak of memory pointed to by field 's4'}}
memcpy(l28->s1, input, 4);
clang_analyzer_eval(m28[0].s3[0] == 1); // expected-warning{{UNKNOWN}}
clang_analyzer_eval(m28[0].s3[1] == 1); // expected-warning{{UNKNOWN}}

View File

@ -0,0 +1,33 @@
// RUN: %clang_analyze_cc1 -analyzer-checker=core,osx -analyzer-output=text -verify %s
// expected-no-diagnostics
typedef void *CFTypeRef;
typedef struct _CFURLCacheRef *CFURLCacheRef;
CFTypeRef CustomCFRetain(CFTypeRef);
void invalidate(void *);
struct S1 {
CFTypeRef s;
CFTypeRef returnFieldAtPlus0() {
return s;
}
};
struct S2 {
S1 *s1;
};
void foo(S1 *s1) {
invalidate(s1);
S2 s2;
s2.s1 = s1;
CustomCFRetain(s1->returnFieldAtPlus0());
// Definitely no leak end-of-path note here. The retained pointer
// is still accessible through s1 and s2.
((void) 0); // no-warning
// FIXME: Ideally we need to warn after this invalidate(). The per-function
// retain-release contract is violated: the programmer should release
// the symbol after it was retained, within the same function.
invalidate(&s2);
}

View File

@ -32,13 +32,14 @@ StringUsed& StringUsed::operator=(const StringUsed &rhs) { // expected-note{{Ass
clang_analyzer_eval(*this == rhs); // expected-warning{{TRUE}} expected-warning{{UNKNOWN}} expected-note{{TRUE}} expected-note{{UNKNOWN}}
free(str); // expected-note{{Memory is released}}
str = strdup(rhs.str); // expected-warning{{Use of memory after it is freed}} expected-note{{Use of memory after it is freed}}
// expected-note@-1{{Memory is allocated}}
return *this;
}
StringUsed& StringUsed::operator=(StringUsed &&rhs) { // expected-note{{Assuming rhs == *this}} expected-note{{Assuming rhs != *this}}
clang_analyzer_eval(*this == rhs); // expected-warning{{TRUE}} expected-warning{{UNKNOWN}} expected-note{{TRUE}} expected-note{{UNKNOWN}}
str = rhs.str;
rhs.str = nullptr; // FIXME: An improved leak checker should warn here
rhs.str = nullptr; // expected-warning{{Potential memory leak}} expected-note{{Potential memory leak}}
return *this;
}
@ -83,7 +84,7 @@ StringUnused::operator const char*() const {
int main() {
StringUsed s1 ("test"), s2;
s2 = s1;
s2 = std::move(s1);
s2 = s1; // expected-note{{Calling copy assignment operator for 'StringUsed'}} // expected-note{{Returned allocated memory}}
s2 = std::move(s1); // expected-note{{Calling move assignment operator for 'StringUsed'}}
return 0;
}

View File

@ -89,3 +89,8 @@ void testPassToSystemHeaderFunctionIndirectly() {
fs.p = fopen("myfile.txt", "w");
fakeSystemHeaderCall(&fs); // invalidates fs, making fs.p unreachable
} // no-warning
void testOverwrite() {
FILE *fp = fopen("myfile.txt", "w");
fp = 0;
} // expected-warning {{Opened file is never closed; potential resource leak}}

View File

@ -90,9 +90,8 @@ namespace PR17596 {
char str[] = "abc";
vv.s = str;
// FIXME: This is a leak of uu.s.
uu = vv;
}
} // expected-warning{{leak}}
void testIndirectInvalidation() {
IntOrString uu;