Fix array bound checker false negative

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

@@ -917,6 +917,14 @@ let ParentPackage = Security in {
 
   def ArrayBoundChecker : Checker<"ArrayBound">,
                           HelpText<"Warn about out of bounds access to memory">,
+                            CheckerOptions<[
+                              CmdLineOption<Boolean,
+                                            "AggressiveReport",
+                                            "Treat all unknown values as tainted and report a waning if"
+                                            "such values are used as offsets to access array elements",
+                                            "false",
+                                            InAlpha>
+                          ]>,
                           Documentation<HasDocumentation>;
 
   def FloatLoopCounter

clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp

@@ -177,6 +177,10 @@ class ArrayBoundChecker : public Checker<check::PostStmt<ArraySubscriptExpr>,
   static bool isInAddressOf(const Stmt *S, ASTContext &AC);
 
 public:
+  // When this parameter is set to true, the checker treats all
+  // unknown values as tainted and reports wanings if such values
+  // are used as offsets to access array elements
+  bool IsAggressive = false;
   void checkPostStmt(const ArraySubscriptExpr *E, CheckerContext &C) const {
     performCheck(E, C);
   }
@@ -696,27 +700,36 @@ void ArrayBoundChecker::performCheck(const Expr *E, CheckerContext &C) const {
         reportOOB(C, ExceedsUpperBound, Msgs, ByteOffset, KnownSize);
         return;
       }
-      // ...and it can be valid as well...
-      if (isTainted(State, ByteOffset)) {
-        // ...but it's tainted, so report an error.
-
-        // Diagnostic detail: saying "tainted offset" is always correct, but
-        // the common case is that 'idx' is tainted in 'arr[idx]' and then it's
-        // nicer to say "tainted index".
-        const char *OffsetName = "offset";
-        if (const auto *ASE = dyn_cast<ArraySubscriptExpr>(E))
-          if (isTainted(State, ASE->getIdx(), C.getLocationContext()))
-            OffsetName = "index";
+      if (!IsAggressive) {
+        // ...and it can be valid as well...
+        if (isTainted(State, ByteOffset)) {
+          // ...but it's tainted, so report an error.
+
+          // Diagnostic detail: saying "tainted offset" is always correct, but
+          // the common case is that 'idx' is tainted in 'arr[idx]' and then
+          // it's nicer to say "tainted index".
+          const char *OffsetName = "offset";
+          if (const auto *ASE = dyn_cast<ArraySubscriptExpr>(E))
+            if (isTainted(State, ASE->getIdx(), C.getLocationContext()))
+              OffsetName = "index";
+
+          Messages Msgs =
+              getTaintMsgs(Space, Reg, OffsetName, AlsoMentionUnderflow);
+          reportOOB(C, ExceedsUpperBound, Msgs, ByteOffset, KnownSize,
+                    /*IsTaintBug=*/true);
+          return;
+        }
 
+        // ...and it isn't tainted, so the checker will (optimistically) assume
+        // that the offset is in bounds and mention this in the note tag.
+        SUR.recordUpperBoundAssumption(*KnownSize);
+      } else {
         Messages Msgs =
-            getTaintMsgs(Space, Reg, OffsetName, AlsoMentionUnderflow);
+            getTaintMsgs(Space, Reg, "offset", AlsoMentionUnderflow);
         reportOOB(C, ExceedsUpperBound, Msgs, ByteOffset, KnownSize,
                   /*IsTaintBug=*/true);
         return;
       }
-      // ...and it isn't tainted, so the checker will (optimistically) assume
-      // that the offset is in bounds and mention this in the note tag.
-      SUR.recordUpperBoundAssumption(*KnownSize);
     }
 
     // Actually update the state. The "if" only fails in the extremely unlikely
@@ -837,7 +850,9 @@ bool ArrayBoundChecker::isIdiomaticPastTheEndPtr(const Expr *E,
 }
 
 void ento::registerArrayBoundChecker(CheckerManager &mgr) {
-  mgr.registerChecker<ArrayBoundChecker>();
+  ArrayBoundChecker *checker = mgr.registerChecker<ArrayBoundChecker>();
+  checker->IsAggressive = mgr.getAnalyzerOptions().getCheckerBooleanOption(
+      checker, "AggressiveReport");
 }
 
 bool ento::shouldRegisterArrayBoundChecker(const CheckerManager &mgr) {

clang/test/Analysis/ArrayBound/cplusplus-tainted-index.cpp

@@ -0,0 +1,51 @@
+// RUN: %clang_analyze_cc1 -std=c++11 -Wno-array-bounds -analyzer-config unroll-loops=true -analyzer-config security.ArrayBound:AggressiveReport=true -analyzer-checker=unix,core,security.ArrayBound  -verify %s
+
+// Test the interactions of `security.ArrayBound` with C++ features.
+
+void test_tainted_index_local() {
+  int arr[10];
+  unsigned index = 10;
+  arr[index] = 7;
+  // expected-warning@-1{{Out of bound access to memory after the end of 'arr'}}
+}
+
+void test_tainted_index_local_range() {
+  int arr[10];
+  for (unsigned index = 0; index < 11; index++)
+    arr[index] = index;
+    // expected-warning@-1{{Out of bound access to memory after the end of 'arr'}}
+}
+
+void test_tainted_index1(unsigned index) {
+  int arr[10];
+  if (index < 12)
+    arr[index] = index;
+  // expected-warning@-1{{Potential out of bound access to 'arr' with tainted offset}}
+  if (index == 12)
+    arr[index] = index;
+  // expected-warning@-1{{Out of bound access to memory after the end of 'arr'}}
+}
+
+void test_tainted_index2(unsigned index) {
+  int arr[10];
+  if (index < 12)
+    arr[index] = index;
+  // expected-warning@-1{{Potential out of bound access to 'arr' with tainted offset}}
+}
+
+unsigned strlen(const char *s);
+void strcpy(char *dst, char *src);
+void test_ex(int argc, char *argv[]) {
+  char proc_name[16];
+  unsigned offset = strlen(argv[0]);
+  if (offset == 16) {
+    strcpy(proc_name, argv[0]);
+    proc_name[offset] = 'x';
+    // expected-warning@-1{{Out of bound access to memory after the end of 'proc_name'}}
+  }
+  if (offset <= 16) {
+    strcpy(proc_name, argv[0]);
+    proc_name[offset] = argv[0][16];
+    // expected-warning@-1{{Potential out of bound access to the region with tainted offset}}
+  }
+}

clang/test/Analysis/analyzer-config.c

@@ -120,6 +120,7 @@
 // CHECK-NEXT: region-store-small-array-limit = 5
 // CHECK-NEXT: region-store-small-struct-limit = 2
 // CHECK-NEXT: report-in-main-source-file = false
+// CHECK-NEXT: security.ArrayBound:AggressiveReport = false
 // CHECK-NEXT: security.cert.env.InvalidPtr:InvalidatingGetEnv = false
 // CHECK-NEXT: serialize-stats = false
 // CHECK-NEXT: silence-checkers = ""