NoQ updated this revision to Diff 37094.
NoQ marked an inline comment as done.
NoQ added a comment.
> What are the implications in each case?
Uhm, sorry, right, I guess i have to admit i don't quite understand what
exactly is going on, so it'd take more time for me to conduct a deeper audit of
the reaping code to answer this question. In fact, right now i'm not even sure
that
// 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());
in EnvironmentManager::removeDeadBindings() does anything at all (at least,
it's not covered by tests).
For now i just leave the patch here that fixes the tests with minimal intrusion
(instead of patching markLive(), this diff patches only specific places), other
issues you pointed out are fixed as well. Hopefully, will have time to dig into
this in the nearest future :)
> > I don't notice any significant performance hit with this
>
> > patch (+/- 2% on the whole Android codebase runs).
>
>
> What did you test it on?
On the source code of the whole Android Open Source Project (version 5); i
record analyzer commands after the first scan-build, so that not to rebuild
Android for every analysis (and only substitute clang executable and set of
checkers), and the observed time of analysis is 1 hour 6 minutes (expensive
hardware...) both before and after the patch is applied, so the difference in
time seems to be less than one minute, i.e. less than 2%. And i guess it should
be large and varied enough(?)
http://reviews.llvm.org/D12726
Files:
docs/analyzer/DebugChecks.rst
include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h
lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp
lib/StaticAnalyzer/Core/Environment.cpp
lib/StaticAnalyzer/Core/RegionStore.cpp
lib/StaticAnalyzer/Core/SymbolManager.cpp
test/Analysis/return-ptr-range.cpp
test/Analysis/symbol-reaper.c
Index: test/Analysis/symbol-reaper.c
===================================================================
--- test/Analysis/symbol-reaper.c
+++ test/Analysis/symbol-reaper.c
@@ -0,0 +1,63 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=debug.ExprInspection -verify %s
+
+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
+}
Index: test/Analysis/return-ptr-range.cpp
===================================================================
--- test/Analysis/return-ptr-range.cpp
+++ test/Analysis/return-ptr-range.cpp
@@ -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)}}
+}
Index: lib/StaticAnalyzer/Core/SymbolManager.cpp
===================================================================
--- lib/StaticAnalyzer/Core/SymbolManager.cpp
+++ lib/StaticAnalyzer/Core/SymbolManager.cpp
@@ -393,6 +393,17 @@
RegionRoots.insert(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) {
if (isa<SymbolMetadata>(sym))
MetadataInUse.insert(sym);
Index: lib/StaticAnalyzer/Core/RegionStore.cpp
===================================================================
--- lib/StaticAnalyzer/Core/RegionStore.cpp
+++ lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -2344,8 +2344,12 @@
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) {
@@ -2366,6 +2370,8 @@
// If V is a region, then add it to the worklist.
if (const MemRegion *R = V.getAsRegion()) {
AddToWorkList(R);
+ // Element index of a binding value is live.
+ SymReaper.markElementIndicesLive(R);
// All regions captured by a block are also live.
if (const BlockDataRegion *BR = dyn_cast<BlockDataRegion>(R)) {
Index: lib/StaticAnalyzer/Core/Environment.cpp
===================================================================
--- lib/StaticAnalyzer/Core/Environment.cpp
+++ lib/StaticAnalyzer/Core/Environment.cpp
@@ -131,6 +131,9 @@
}
bool VisitMemRegion(const MemRegion *R) override {
SymReaper.markLive(R);
+
+ // Element index of an environment element region value is live.
+ SymReaper.markElementIndicesLive(R);
return true;
}
};
Index: lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp
===================================================================
--- lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp
+++ lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp
@@ -17,22 +17,26 @@
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 @@
.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,40 @@
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));
+ }
+}
+
void ExprInspectionChecker::analyzerCrash(const CallExpr *CE,
CheckerContext &C) const {
LLVM_BUILTIN_TRAP;
Index: include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h
===================================================================
--- include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h
@@ -639,6 +639,7 @@
}
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.
Index: docs/analyzer/DebugChecks.rst
===================================================================
--- docs/analyzer/DebugChecks.rst
+++ docs/analyzer/DebugChecks.rst
@@ -138,6 +138,29 @@
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
==========
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
