r255236 - [analyzer] Fix symbolic element index lifetime.
Artem Dergachev via cfe-commits
cfe-commits at lists.llvm.org
Thu Dec 10 01:28:07 PST 2015
Author: dergachev
Date: Thu Dec 10 03:28:06 2015
New Revision: 255236
URL: http://llvm.org/viewvc/llvm-project?rev=255236&view=rev
Log:
[analyzer] Fix symbolic element index lifetime.
SymbolReaper was destroying the symbol too early when it was referenced only
from an index SVal of a live ElementRegion.
In order to test certain aspects of this patch, extend the debug.ExprInspection
checker to allow testing SymbolReaper in a direct manner.
Differential Revision: http://reviews.llvm.org/D12726
Added:
cfe/trunk/test/Analysis/return-ptr-range.cpp
cfe/trunk/test/Analysis/symbol-reaper.c
Modified:
cfe/trunk/docs/analyzer/DebugChecks.rst
cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h
cfe/trunk/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Core/Environment.cpp
cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp
cfe/trunk/lib/StaticAnalyzer/Core/SymbolManager.cpp
Modified: cfe/trunk/docs/analyzer/DebugChecks.rst
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/analyzer/DebugChecks.rst?rev=255236&r1=255235&r2=255236&view=diff
==============================================================================
--- cfe/trunk/docs/analyzer/DebugChecks.rst (original)
+++ cfe/trunk/docs/analyzer/DebugChecks.rst Thu Dec 10 03:28:06 2015
@@ -138,6 +138,29 @@ ExprInspection checks
clang_analyzer_warnIfReached(); // no-warning
}
+- void clang_analyzer_warnOnDeadSymbol(int);
+
+ Subscribe for a delayed warning when the symbol that represents the value of
+ the argument is garbage-collected by the analyzer.
+
+ When calling 'clang_analyzer_warnOnDeadSymbol(x)', if value of 'x' is a
+ symbol, then this symbol is marked by the ExprInspection checker. Then,
+ during each garbage collection run, the checker sees if the marked symbol is
+ being collected and issues the 'SYMBOL DEAD' warning if it does.
+ This way you know where exactly, up to the line of code, the symbol dies.
+
+ It is unlikely that you call this function after the symbol is already dead,
+ because the very reference to it as the function argument prevents it from
+ dying. However, if the argument is not a symbol but a concrete value,
+ no warning would be issued.
+
+ Example usage::
+
+ do {
+ int x = generate_some_integer();
+ clang_analyzer_warnOnDeadSymbol(x);
+ } while(0); // expected-warning{{SYMBOL DEAD}}
+
Statistics
==========
Modified: cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h?rev=255236&r1=255235&r2=255236&view=diff
==============================================================================
--- cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h (original)
+++ cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h Thu Dec 10 03:28:06 2015
@@ -639,6 +639,7 @@ public:
}
void markLive(const MemRegion *region);
+ void markElementIndicesLive(const MemRegion *region);
/// \brief Set to the value of the symbolic store after
/// StoreManager::removeDeadBindings has been called.
Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp?rev=255236&r1=255235&r2=255236&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp Thu Dec 10 03:28:06 2015
@@ -17,22 +17,26 @@ using namespace clang;
using namespace ento;
namespace {
-class ExprInspectionChecker : public Checker< eval::Call > {
+class ExprInspectionChecker : public Checker<eval::Call, check::DeadSymbols> {
mutable std::unique_ptr<BugType> BT;
void analyzerEval(const CallExpr *CE, CheckerContext &C) const;
void analyzerCheckInlined(const CallExpr *CE, CheckerContext &C) const;
void analyzerWarnIfReached(const CallExpr *CE, CheckerContext &C) const;
void analyzerCrash(const CallExpr *CE, CheckerContext &C) const;
+ void analyzerWarnOnDeadSymbol(const CallExpr *CE, CheckerContext &C) const;
typedef void (ExprInspectionChecker::*FnCheck)(const CallExpr *,
CheckerContext &C) const;
public:
bool evalCall(const CallExpr *CE, CheckerContext &C) const;
+ void checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const;
};
}
+REGISTER_SET_WITH_PROGRAMSTATE(MarkedSymbols, const void *)
+
bool ExprInspectionChecker::evalCall(const CallExpr *CE,
CheckerContext &C) const {
// These checks should have no effect on the surrounding environment
@@ -42,7 +46,10 @@ bool ExprInspectionChecker::evalCall(con
.Case("clang_analyzer_checkInlined",
&ExprInspectionChecker::analyzerCheckInlined)
.Case("clang_analyzer_crash", &ExprInspectionChecker::analyzerCrash)
- .Case("clang_analyzer_warnIfReached", &ExprInspectionChecker::analyzerWarnIfReached)
+ .Case("clang_analyzer_warnIfReached",
+ &ExprInspectionChecker::analyzerWarnIfReached)
+ .Case("clang_analyzer_warnOnDeadSymbol",
+ &ExprInspectionChecker::analyzerWarnOnDeadSymbol)
.Default(nullptr);
if (!Handler)
@@ -137,6 +144,41 @@ void ExprInspectionChecker::analyzerChec
llvm::make_unique<BugReport>(*BT, getArgumentValueString(CE, C), N));
}
+void ExprInspectionChecker::analyzerWarnOnDeadSymbol(const CallExpr *CE,
+ CheckerContext &C) const {
+ if (CE->getNumArgs() == 0)
+ return;
+ SVal Val = C.getSVal(CE->getArg(0));
+ SymbolRef Sym = Val.getAsSymbol();
+ if (!Sym)
+ return;
+
+ ProgramStateRef State = C.getState();
+ State = State->add<MarkedSymbols>(Sym);
+ C.addTransition(State);
+}
+
+void ExprInspectionChecker::checkDeadSymbols(SymbolReaper &SymReaper,
+ CheckerContext &C) const {
+ ProgramStateRef State = C.getState();
+ const MarkedSymbolsTy &Syms = State->get<MarkedSymbols>();
+ for (auto I = Syms.begin(), E = Syms.end(); I != E; ++I) {
+ SymbolRef Sym = static_cast<SymbolRef>(*I);
+ if (!SymReaper.isDead(Sym))
+ continue;
+
+ if (!BT)
+ BT.reset(new BugType(this, "Checking analyzer assumptions", "debug"));
+
+ ExplodedNode *N = C.generateNonFatalErrorNode();
+ if (!N)
+ return;
+
+ C.emitReport(llvm::make_unique<BugReport>(*BT, "SYMBOL DEAD", N));
+ C.addTransition(State->remove<MarkedSymbols>(Sym), N);
+ }
+}
+
void ExprInspectionChecker::analyzerCrash(const CallExpr *CE,
CheckerContext &C) const {
LLVM_BUILTIN_TRAP;
Modified: cfe/trunk/lib/StaticAnalyzer/Core/Environment.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/Environment.cpp?rev=255236&r1=255235&r2=255236&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Core/Environment.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Core/Environment.cpp Thu Dec 10 03:28:06 2015
@@ -171,10 +171,6 @@ EnvironmentManager::removeDeadBindings(E
// Copy the binding to the new map.
EBMapRef = EBMapRef.add(BlkExpr, X);
- // If the block expr's value is a memory region, then mark that region.
- if (Optional<loc::MemRegionVal> R = X.getAs<loc::MemRegionVal>())
- SymReaper.markLive(R->getRegion());
-
// Mark all symbols in the block expr's value live.
RSScaner.scan(X);
continue;
Modified: cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp?rev=255236&r1=255235&r2=255236&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp Thu Dec 10 03:28:06 2015
@@ -2348,8 +2348,12 @@ void removeDeadBindingsWorker::VisitClus
if (const SymbolicRegion *SymR = dyn_cast<SymbolicRegion>(baseR))
SymReaper.markLive(SymR->getSymbol());
- for (ClusterBindings::iterator I = C->begin(), E = C->end(); I != E; ++I)
+ for (ClusterBindings::iterator I = C->begin(), E = C->end(); I != E; ++I) {
+ // Element index of a binding key is live.
+ SymReaper.markElementIndicesLive(I.getKey().getRegion());
+
VisitBinding(I.getData());
+ }
}
void removeDeadBindingsWorker::VisitBinding(SVal V) {
@@ -2370,6 +2374,7 @@ void removeDeadBindingsWorker::VisitBind
// If V is a region, then add it to the worklist.
if (const MemRegion *R = V.getAsRegion()) {
AddToWorkList(R);
+ SymReaper.markLive(R);
// All regions captured by a block are also live.
if (const BlockDataRegion *BR = dyn_cast<BlockDataRegion>(R)) {
Modified: cfe/trunk/lib/StaticAnalyzer/Core/SymbolManager.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/SymbolManager.cpp?rev=255236&r1=255235&r2=255236&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Core/SymbolManager.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Core/SymbolManager.cpp Thu Dec 10 03:28:06 2015
@@ -391,6 +391,18 @@ void SymbolReaper::markLive(SymbolRef sy
void SymbolReaper::markLive(const MemRegion *region) {
RegionRoots.insert(region);
+ markElementIndicesLive(region);
+}
+
+void SymbolReaper::markElementIndicesLive(const MemRegion *region) {
+ for (auto SR = dyn_cast<SubRegion>(region); SR;
+ SR = dyn_cast<SubRegion>(SR->getSuperRegion())) {
+ if (auto ER = dyn_cast<ElementRegion>(SR)) {
+ SVal Idx = ER->getIndex();
+ for (auto SI = Idx.symbol_begin(), SE = Idx.symbol_end(); SI != SE; ++SI)
+ markLive(*SI);
+ }
+ }
}
void SymbolReaper::markInUse(SymbolRef sym) {
Added: cfe/trunk/test/Analysis/return-ptr-range.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/return-ptr-range.cpp?rev=255236&view=auto
==============================================================================
--- cfe/trunk/test/Analysis/return-ptr-range.cpp (added)
+++ cfe/trunk/test/Analysis/return-ptr-range.cpp Thu Dec 10 03:28:06 2015
@@ -0,0 +1,27 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=alpha.security.ReturnPtrRange -verify %s
+
+int arr[10];
+int *ptr;
+
+int conjure_index();
+
+int *test_element_index_lifetime() {
+ do {
+ int x = conjure_index();
+ ptr = arr + x;
+ if (x != 20)
+ return arr; // no-warning
+ } while (0);
+ return ptr; // expected-warning{{Returned pointer value points outside the original object (potential buffer overflow)}}
+}
+
+int *test_element_index_lifetime_with_local_ptr() {
+ int *local_ptr;
+ do {
+ int x = conjure_index();
+ local_ptr = arr + x;
+ if (x != 20)
+ return arr; // no-warning
+ } while (0);
+ return local_ptr; // expected-warning{{Returned pointer value points outside the original object (potential buffer overflow)}}
+}
Added: cfe/trunk/test/Analysis/symbol-reaper.c
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/symbol-reaper.c?rev=255236&view=auto
==============================================================================
--- cfe/trunk/test/Analysis/symbol-reaper.c (added)
+++ cfe/trunk/test/Analysis/symbol-reaper.c Thu Dec 10 03:28:06 2015
@@ -0,0 +1,76 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=debug.ExprInspection -verify %s
+
+void clang_analyzer_eval(int);
+void clang_analyzer_warnOnDeadSymbol(int);
+
+int conjure_index();
+
+void test_that_expr_inspection_works() {
+ do {
+ int x = conjure_index();
+ clang_analyzer_warnOnDeadSymbol(x);
+ } while(0); // expected-warning{{SYMBOL DEAD}}
+}
+
+// These tests verify the reaping of symbols that are only referenced as
+// index values in element regions. Most of the time, depending on where
+// the element region, as Loc value, is stored, it is possible to
+// recover the index symbol in checker code, which is also demonstrated
+// in the return_ptr_range.c test file.
+
+int arr[3];
+
+int *test_element_index_lifetime_in_environment_values() {
+ int *ptr;
+ do {
+ int x = conjure_index();
+ clang_analyzer_warnOnDeadSymbol(x);
+ ptr = arr + x;
+ } while (0);
+ return ptr;
+}
+
+void test_element_index_lifetime_in_store_keys() {
+ do {
+ int x = conjure_index();
+ clang_analyzer_warnOnDeadSymbol(x);
+ arr[x] = 1;
+ if (x) {}
+ } while (0); // no-warning
+}
+
+int *ptr;
+void test_element_index_lifetime_in_store_values() {
+ do {
+ int x = conjure_index();
+ clang_analyzer_warnOnDeadSymbol(x);
+ ptr = arr + x;
+ } while (0); // no-warning
+}
+
+struct S1 {
+ int field;
+};
+struct S2 {
+ struct S1 array[5];
+} s2;
+
+void test_element_index_lifetime_with_complicated_hierarchy_of_regions() {
+ do {
+ int x = conjure_index();
+ clang_analyzer_warnOnDeadSymbol(x);
+ s2.array[x].field = 1;
+ if (x) {}
+ } while (0); // no-warning
+}
+
+// Test below checks lifetime of SymbolRegionValue in certain conditions.
+
+int **ptrptr;
+void test_region_lifetime_as_store_value(int *x) {
+ clang_analyzer_warnOnDeadSymbol((int) x);
+ *x = 1;
+ ptrptr = &x;
+ (void)0; // No-op; make sure the environment forgets things and the GC runs.
+ clang_analyzer_eval(**ptrptr); // expected-warning{{TRUE}}
+} // no-warning
More information about the cfe-commits
mailing list