From bbc6d68297c8b0641eb8226dea7746a0d97ae33b Mon Sep 17 00:00:00 2001 From: Artem Dergachev Date: Fri, 30 Nov 2018 03:27:50 +0000 Subject: [PATCH] [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 --- .../Core/PathSensitive/SMTConstraintManager.h | 2 +- .../Core/PathSensitive/SymbolManager.h | 22 +------- .../lib/StaticAnalyzer/Checkers/CStringChecker.cpp | 3 - .../Checkers/DynamicTypePropagation.cpp | 5 -- .../Checkers/MPI-Checker/MPIChecker.cpp | 3 - .../Checkers/MacOSKeychainAPIChecker.cpp | 44 +++++++++++++++ .../lib/StaticAnalyzer/Checkers/MallocChecker.cpp | 3 - .../StaticAnalyzer/Checkers/NullabilityChecker.cpp | 3 - .../RetainCountChecker/RetainCountChecker.cpp | 14 ++--- .../lib/StaticAnalyzer/Checkers/StreamChecker.cpp | 34 +++++------ clang/lib/StaticAnalyzer/Core/Environment.cpp | 5 -- clang/lib/StaticAnalyzer/Core/ExprEngine.cpp | 65 ++++++++++------------ .../StaticAnalyzer/Core/RangeConstraintManager.cpp | 2 +- clang/lib/StaticAnalyzer/Core/RegionStore.cpp | 21 +------ clang/lib/StaticAnalyzer/Core/SymbolManager.cpp | 9 --- clang/test/Analysis/MisusedMovedObject.cpp | 22 ++++++++ clang/test/Analysis/keychainAPI.m | 14 ++++- clang/test/Analysis/loop-block-counts.c | 26 +++++++++ clang/test/Analysis/pr22954.c | 2 +- clang/test/Analysis/retain-release-cpp-classes.cpp | 33 +++++++++++ clang/test/Analysis/self-assign.cpp | 7 ++- clang/test/Analysis/simple-stream-checks.c | 5 ++ clang/test/Analysis/unions.cpp | 3 +- 23 files changed, 208 insertions(+), 139 deletions(-) create mode 100644 clang/test/Analysis/loop-block-counts.c create mode 100644 clang/test/Analysis/retain-release-cpp-classes.cpp diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConstraintManager.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConstraintManager.h index 54cf3c3..8eaa936 100644 --- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConstraintManager.h +++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConstraintManager.h @@ -198,7 +198,7 @@ public: auto &CZFactory = State->get_context(); 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); } diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h index b014c63..d02a8ab 100644 --- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h +++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h @@ -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); diff --git a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp index 98590bc..0a1a108 100644 --- a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp @@ -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(); if (Entries.isEmpty()) diff --git a/clang/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp b/clang/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp index b5a3c71..f83a0ec 100644 --- a/clang/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp @@ -123,11 +123,6 @@ void DynamicTypePropagation::checkDeadSymbols(SymbolReaper &SR, } } - if (!SR.hasDeadSymbols()) { - C.addTransition(State); - return; - } - MostSpecializedTypeArgsMapTy TyArgMap = State->get(); for (MostSpecializedTypeArgsMapTy::iterator I = TyArgMap.begin(), diff --git a/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp index 696cf39..3f89c33 100644 --- a/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp @@ -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(); if (Requests.isEmpty()) diff --git a/clang/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp index cc29895..f5c7d52 100644 --- a/clang/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp @@ -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::PostStmt, check::DeadSymbols, + check::PointerEscape, eval::Assume> { mutable std::unique_ptr 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()) { + SymbolRef Sym = I.first; + if (Escaped.count(Sym)) + State = State->remove(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(Sym)) { + SymbolRef ParentSym = SD->getParentSymbol(); + if (Escaped.count(ParentSym)) + State = State->remove(Sym); + } + } + return State; +} + std::shared_ptr MacOSKeychainAPIChecker::SecKeychainBugVisitor::VisitNode( const ExplodedNode *N, BugReporterContext &BRC, BugReport &BR) { diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp index 07483cd..ded355e 100644 --- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp @@ -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(); RegionStateTy::Factory &F = state->get_context(); diff --git a/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp index 6345c6a..eae0a97 100644 --- a/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp @@ -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(); for (NullabilityMapTy::iterator I = Nullabilities.begin(), diff --git a/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp index f2f0156..a8e75cd 100644 --- a/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp @@ -1442,20 +1442,18 @@ void RetainCountChecker::checkDeadSymbols(SymbolReaper &SymReaper, SmallVector 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()) { + 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); } } diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp index d779755..b383411 100644 --- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp @@ -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(Sym); - if (!SS) + const StreamMapTy &Map = state->get(); + 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( - *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( + *BT_ResourceLeak, BT_ResourceLeak->getDescription(), N)); } } diff --git a/clang/lib/StaticAnalyzer/Core/Environment.cpp b/clang/lib/StaticAnalyzer/Core/Environment.cpp index 43bbcd7..b45f93b 100644 --- a/clang/lib/StaticAnalyzer/Core/Environment.cpp +++ b/clang/lib/StaticAnalyzer/Core/Environment.cpp @@ -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); } } diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp index d001f4d..49ddf1a 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp @@ -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); - - } 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(); - - // 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."); - - // Create a state based on CleanedState with CheckerState GDM and - // generate a transition to that state. - ProgramStateRef CleanedCheckerSt = + // 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(); + + // 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."); + + // 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); } } diff --git a/clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp b/clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp index e8c7bdb..d9b58d0 100644 --- a/clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp +++ b/clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp @@ -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); } diff --git a/clang/lib/StaticAnalyzer/Core/RegionStore.cpp b/clang/lib/StaticAnalyzer/Core/RegionStore.cpp index 0120588..29b6058 100644 --- a/clang/lib/StaticAnalyzer/Core/RegionStore.cpp +++ b/clang/lib/StaticAnalyzer/Core/RegionStore.cpp @@ -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(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); diff --git a/clang/lib/StaticAnalyzer/Core/SymbolManager.cpp b/clang/lib/StaticAnalyzer/Core/SymbolManager.cpp index 4129191..66273f0 100644 --- a/clang/lib/StaticAnalyzer/Core/SymbolManager.cpp +++ b/clang/lib/StaticAnalyzer/Core/SymbolManager.cpp @@ -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; diff --git a/clang/test/Analysis/MisusedMovedObject.cpp b/clang/test/Analysis/MisusedMovedObject.cpp index 42d3608..f7ad266 100644 --- a/clang/test/Analysis/MisusedMovedObject.cpp +++ b/clang/test/Analysis/MisusedMovedObject.cpp @@ -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 + } +} diff --git a/clang/test/Analysis/keychainAPI.m b/clang/test/Analysis/keychainAPI.m index 1725ce1..15a3b66 100644 --- a/clang/test/Analysis/keychainAPI.m +++ b/clang/test/Analysis/keychainAPI.m @@ -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; +} diff --git a/clang/test/Analysis/loop-block-counts.c b/clang/test/Analysis/loop-block-counts.c new file mode 100644 index 0000000..04a3f74 --- /dev/null +++ b/clang/test/Analysis/loop-block-counts.c @@ -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}} +} diff --git a/clang/test/Analysis/pr22954.c b/clang/test/Analysis/pr22954.c index c58a8aa..6d5b044 100644 --- a/clang/test/Analysis/pr22954.c +++ b/clang/test/Analysis/pr22954.c @@ -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}} diff --git a/clang/test/Analysis/retain-release-cpp-classes.cpp b/clang/test/Analysis/retain-release-cpp-classes.cpp new file mode 100644 index 0000000..9ed1c0b --- /dev/null +++ b/clang/test/Analysis/retain-release-cpp-classes.cpp @@ -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); +} diff --git a/clang/test/Analysis/self-assign.cpp b/clang/test/Analysis/self-assign.cpp index 8597e9d..ca28c53 100644 --- a/clang/test/Analysis/self-assign.cpp +++ b/clang/test/Analysis/self-assign.cpp @@ -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; } diff --git a/clang/test/Analysis/simple-stream-checks.c b/clang/test/Analysis/simple-stream-checks.c index ca1c781..f37a703 100644 --- a/clang/test/Analysis/simple-stream-checks.c +++ b/clang/test/Analysis/simple-stream-checks.c @@ -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}} diff --git a/clang/test/Analysis/unions.cpp b/clang/test/Analysis/unions.cpp index 0713bc0..618d4c3 100644 --- a/clang/test/Analysis/unions.cpp +++ b/clang/test/Analysis/unions.cpp @@ -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; -- 2.7.4