diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h index 2121b5bc04ff..85eb4b865abc 100644 --- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h +++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h @@ -773,9 +773,6 @@ class SymbolicRegion : public SubRegion { assert(s->getType()->isAnyPointerType() || s->getType()->isReferenceType() || s->getType()->isBlockPointerType()); - - // populateWorklistFromSymbol() relies on this assertion, and needs to be - // updated if more cases are introduced. assert(isa(sreg) || isa(sreg)); } diff --git a/clang/lib/StaticAnalyzer/Core/RegionStore.cpp b/clang/lib/StaticAnalyzer/Core/RegionStore.cpp index 679d95b7c589..3323bb97a712 100644 --- a/clang/lib/StaticAnalyzer/Core/RegionStore.cpp +++ b/clang/lib/StaticAnalyzer/Core/RegionStore.cpp @@ -2386,10 +2386,7 @@ RegionStoreManager::bindAggregate(RegionBindingsConstRef B, namespace { class RemoveDeadBindingsWorker : public ClusterAnalysis { - using ChildrenListTy = SmallVector; - using MapParentsToDerivedTy = llvm::DenseMap; - - MapParentsToDerivedTy ParentsToDerived; + SmallVector Postponed; SymbolReaper &SymReaper; const StackFrameContext *CurrentLCtx; @@ -2410,10 +2407,8 @@ public: bool AddToWorkList(const MemRegion *R); + bool UpdatePostponed(); void VisitBinding(SVal V); - -private: - void populateWorklistFromSymbol(SymbolRef s); }; } @@ -2433,11 +2428,10 @@ void RemoveDeadBindingsWorker::VisitAddedToCluster(const MemRegion *baseR, } if (const SymbolicRegion *SR = dyn_cast(baseR)) { - if (SymReaper.isLive(SR->getSymbol())) { + if (SymReaper.isLive(SR->getSymbol())) AddToWorkList(SR, &C); - } else if (const auto *SD = dyn_cast(SR->getSymbol())) { - ParentsToDerived[SD->getParentSymbol()].push_back(SD); - } + else + Postponed.push_back(SR); return; } @@ -2450,7 +2444,7 @@ void RemoveDeadBindingsWorker::VisitAddedToCluster(const MemRegion *baseR, // CXXThisRegion in the current or parent location context is live. if (const CXXThisRegion *TR = dyn_cast(baseR)) { const auto *StackReg = - cast(TR->getSuperRegion()); + cast(TR->getSuperRegion()); const StackFrameContext *RegCtx = StackReg->getStackFrame(); if (CurrentLCtx && (RegCtx == CurrentLCtx || RegCtx->isParentOf(CurrentLCtx))) @@ -2494,15 +2488,6 @@ void RemoveDeadBindingsWorker::VisitBinding(SVal V) { // If V is a region, then add it to the worklist. if (const MemRegion *R = V.getAsRegion()) { AddToWorkList(R); - - if (const auto *TVR = dyn_cast(R)) { - DefinedOrUnknownSVal RVS = - RM.getSValBuilder().getRegionValueSymbolVal(TVR); - if (const MemRegion *SR = RVS.getAsRegion()) { - AddToWorkList(SR); - } - } - SymReaper.markLive(R); // All regions captured by a block are also live. @@ -2516,30 +2501,25 @@ void RemoveDeadBindingsWorker::VisitBinding(SVal V) { // Update the set of live symbols. - for (auto SI = V.symbol_begin(), SE = V.symbol_end(); SI != SE; ++SI) { - populateWorklistFromSymbol(*SI); - - for (const auto *SD : ParentsToDerived[*SI]) - populateWorklistFromSymbol(SD); - + for (auto SI = V.symbol_begin(), SE = V.symbol_end(); SI!=SE; ++SI) SymReaper.markLive(*SI); - } } -void RemoveDeadBindingsWorker::populateWorklistFromSymbol(SymbolRef S) { - if (const auto *SD = dyn_cast(S)) { - if (Loc::isLocType(SD->getType()) && !SymReaper.isLive(SD)) { - const SymbolicRegion *SR = RM.getRegionManager().getSymbolicRegion(SD); +bool RemoveDeadBindingsWorker::UpdatePostponed() { + // See if any postponed SymbolicRegions are actually live now, after + // having done a scan. + bool Changed = false; - if (B.contains(SR)) - AddToWorkList(SR); - - const SymbolicRegion *SHR = - RM.getRegionManager().getSymbolicHeapRegion(SD); - if (B.contains(SHR)) - AddToWorkList(SHR); + for (auto I = Postponed.begin(), E = Postponed.end(); I != E; ++I) { + if (const SymbolicRegion *SR = *I) { + if (SymReaper.isLive(SR->getSymbol())) { + Changed |= AddToWorkList(SR); + *I = nullptr; + } } } + + return Changed; } StoreRef RegionStoreManager::removeDeadBindings(Store store, @@ -2555,7 +2535,7 @@ StoreRef RegionStoreManager::removeDeadBindings(Store store, W.AddToWorkList(*I); } - W.RunWorkList(); + do W.RunWorkList(); while (W.UpdatePostponed()); // We have now scanned the store, marking reachable regions and symbols // as live. We now remove all the regions that are dead from the store diff --git a/clang/test/Analysis/malloc.c b/clang/test/Analysis/malloc.c index d4a44c576241..cbc21b492e59 100644 --- a/clang/test/Analysis/malloc.c +++ b/clang/test/Analysis/malloc.c @@ -1794,6 +1794,24 @@ void testNoCrashOnOffendingParameter() { allocateSomeMemory(offendingParameter, &ptr); } // expected-warning {{Potential leak of memory pointed to by 'ptr'}} + +// Test a false positive caused by a bug in liveness analysis. +struct A { + int *buf; +}; +struct B { + struct A *a; +}; +void livenessBugRealloc(struct A *a) { + a->buf = realloc(a->buf, sizeof(int)); // no-warning +} +void testLivenessBug(struct B *in_b) { + struct B *b = in_b; + livenessBugRealloc(b->a); + ((void) 0); // An attempt to trick liveness analysis. + livenessBugRealloc(b->a); +} + // ---------------------------------------------------------------------------- // False negatives. diff --git a/clang/test/Analysis/symbol-reaper.c b/clang/test/Analysis/symbol-reaper.c index ef8ff18a2d83..62bb5d6c6b34 100644 --- a/clang/test/Analysis/symbol-reaper.c +++ b/clang/test/Analysis/symbol-reaper.c @@ -133,3 +133,28 @@ void test_zombie_referenced_only_through_field_in_store_value() { clang_analyzer_warnOnDeadSymbol((int) s); int *x = &s->field; } // expected-warning{{SYMBOL DEAD}} + +void double_dereference_of_implicit_value_aux1(int *p) { + *p = 0; +} + +void double_dereference_of_implicit_value_aux2(int *p) { + if (*p != 0) + clang_analyzer_warnIfReached(); // no-warning +} + +void test_double_dereference_of_implicit_value(int **x) { + clang_analyzer_warnOnDeadSymbol(**x); + int **y = x; + { + double_dereference_of_implicit_value_aux1(*y); + // Give time for symbol reaping to happen. + ((void)0); + // The symbol for **y was cleaned up from the Store at this point, + // even though it was not perceived as dead when asked explicitly. + // For that reason the SYMBOL DEAD warning never appeared at this point. + double_dereference_of_implicit_value_aux2(*y); + } + // The symbol is generally reaped here regardless. + ((void)0); // expected-warning{{SYMBOL DEAD}} +}