[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

llvm-svn: 255236

Author: haoNoQ

clang/docs/analyzer/DebugChecks.rst

@@ -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
 ==========

clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h

@@ -639,6 +639,7 @@ class SymbolReaper {
   }
 
   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.

clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp

@@ -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(const CallExpr *CE,
     .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::analyzerCheckInlined(const CallExpr *CE,
       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;

clang/lib/StaticAnalyzer/Core/Environment.cpp

@@ -171,10 +171,6 @@ EnvironmentManager::removeDeadBindings(Environment Env,
       // 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;

clang/lib/StaticAnalyzer/Core/RegionStore.cpp

@@ -2348,8 +2348,12 @@ void removeDeadBindingsWorker::VisitCluster(const MemRegion *baseR,
   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::VisitBinding(SVal V) {
   // 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)) {

clang/lib/StaticAnalyzer/Core/SymbolManager.cpp

@@ -391,6 +391,18 @@ void SymbolReaper::markLive(SymbolRef sym) {
 
 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) {

clang/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)}}
+}

clang/test/Analysis/symbol-reaper.c

@@ -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