NoQ updated this revision to Diff 35067.
NoQ added a comment.
Thanks for the quick reply, sorry for the delay! Was afk for a couple of days.
Yeah, right, in fact i didn't even fix the issue for store keys at all; only
for store values and environment values.
It also seems much harder to test store keys, because it's quite a problem to
guess the symbolic key once the symbol is not present anywhere else, though i
can imagine an artificial checker that would rely on that. A test like...
int a[1];
{
int x = conjure_index();
a[x] = 0;
if (x != 0)
return;
clang_analyzer_eval(a[0] == 0); // expected-warning{{TRUE}}
}
clang_analyzer_eval(a[0] == 0); // expected-warning{{TRUE}}
...should have exposed such problem, but this kind of lookup doesn't seem to be
supported by the store yet (that is, the first `expected-warning{{TRUE}}` fails
as well).
Hmm, what if i expand the `debug.ExprInspection` checker to allow testing
`SymbolReaper` directly? Updated the diff with a proof of concept, which fixes
the issue for the store keys and adds a test. I can split the `ExprInspection`
change into a separate commit/review if necessary. It might be useful for
testing other `SymbolReaper`-related patches as well.
http://reviews.llvm.org/D12726
Files:
lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp
lib/StaticAnalyzer/Core/Environment.cpp
lib/StaticAnalyzer/Core/RegionStore.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,23 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=debug.ExprInspection -verify %s
+
+void clang_analyzer_warnOnDeadSymbol(int);
+
+int arr[3];
+
+int conjure_index();
+
+void test_that_expr_inspection_works() {
+ do {
+ int x = conjure_index();
+ clang_analyzer_warnOnDeadSymbol(x);
+ } while(0); // expected-warning{{SYMBOL DEAD}}
+}
+
+void test_element_index_lifetime() {
+ do {
+ int x = conjure_index();
+ clang_analyzer_warnOnDeadSymbol(x);
+ arr[x] = 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/RegionStore.cpp
===================================================================
--- lib/StaticAnalyzer/Core/RegionStore.cpp
+++ lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -2237,8 +2237,20 @@
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) {
+
+ // Mark the index symbol of any ElementRegion key as live.
+ const BindingKey &K = I.getKey();
+ auto subR = dyn_cast<SubRegion>(K.getRegion());
+ while (subR) {
+ if (auto elemR = dyn_cast<ElementRegion>(subR))
+ if (SymbolRef Sym = elemR->getIndex().getAsSymbol())
+ SymReaper.markLive(Sym);
+ subR = dyn_cast<SubRegion>(subR->getSuperRegion());
+ }
+
VisitBinding(I.getData());
+ }
}
void removeDeadBindingsWorker::VisitBinding(SVal V) {
@@ -2260,6 +2272,16 @@
if (const MemRegion *R = V.getAsRegion()) {
AddToWorkList(R);
+ // Element index of an element region is live.
+ if (const ElementRegion *ER = dyn_cast<ElementRegion>(R)) {
+ SVal Idx = ER->getIndex();
+ for (SymExpr::symbol_iterator SI = Idx.symbol_begin(),
+ SE = Idx.symbol_end();
+ SI != SE; ++SI) {
+ SymReaper.markLive(*SI);
+ }
+ }
+
// All regions captured by a block are also live.
if (const BlockDataRegion *BR = dyn_cast<BlockDataRegion>(R)) {
BlockDataRegion::referenced_vars_iterator I = BR->referenced_vars_begin(),
Index: lib/StaticAnalyzer/Core/Environment.cpp
===================================================================
--- lib/StaticAnalyzer/Core/Environment.cpp
+++ lib/StaticAnalyzer/Core/Environment.cpp
@@ -171,8 +171,15 @@
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());
+ if (Optional<loc::MemRegionVal> R = X.getAs<loc::MemRegionVal>()) {
+ const MemRegion *MR = R->getRegion();
+ SymReaper.markLive(MR);
+
+ // Mark the element index as live.
+ if (const ElementRegion *ER = dyn_cast<ElementRegion>(MR))
+ if (SymbolRef Sym = ER->getIndex().getAsSymbol())
+ SymReaper.markLive(Sym);
+ }
// Mark all symbols in the block expr's value live.
RSScaner.scan(X);
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;
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
