[PATCH] D12726: [analyzer] A fix for symbolic element region index lifetime.

[Artem Dergachev via cfe-commits]

NoQ created this revision.
NoQ added reviewers: zaks.anna, krememek.
NoQ added subscribers: cfe-commits, dergachev.a.

In Clang Static Analyzer, when the symbol is referenced by an index value of an element region, it does not prevent garbage collection (reaping) of that symbol. Hence, if the element index value is the only place where the symbol is stored, the symbol gets reaped, and range information is removed from the constraint manager.

It seems that both the store and the environment need to mark the element indices of their regions as live.

http://reviews.llvm.org/D12726

Files:
  lib/StaticAnalyzer/Core/Environment.cpp
  lib/StaticAnalyzer/Core/RegionStore.cpp
  test/Analysis/return-ptr-range.cpp

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
@@ -2260,6 +2260,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);


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D12726.34322.patch
Type: text/x-patch
Size: 2763 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150909/73760b07/attachment.bin>