[clang] Fix array bound checker false negative (PR #161723)

[via cfe-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-static-analyzer-1

Author: Rashmi Mudduluru (t-rasmud)

<details>
<summary>Changes</summary>

This PR removes the check for whether an array offset is tainted before reporting an OOB in the `security.ArrayBound` checker. The `isTainted` check was not working as intended and leading to false negatives in the case of conditional checks and loops.

---
Full diff: https://github.com/llvm/llvm-project/pull/161723.diff


2 Files Affected:

- (modified) clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp (+10-42) 
- (added) clang/test/Analysis/ArrayBound/cplusplus-tainted-index.cpp (+52) 


``````````diff
diff --git a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp
index 6ad5acd4e76f2..05b55da4c0312 100644
--- a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp
@@ -478,17 +478,6 @@ static Messages getNonTaintMsgs(const ASTContext &ACtx,
           std::string(Buf)};
 }
 
-static Messages getTaintMsgs(const MemSpaceRegion *Space,
-                             const SubRegion *Region, const char *OffsetName,
-                             bool AlsoMentionUnderflow) {
-  std::string RegName = getRegionName(Space, Region);
-  return {formatv("Potential out of bound access to {0} with tainted {1}",
-                  RegName, OffsetName),
-          formatv("Access of {0} with a tainted {1} that may be {2}too large",
-                  RegName, OffsetName,
-                  AlsoMentionUnderflow ? "negative or " : "")};
-}
-
 const NoteTag *StateUpdateReporter::createNoteTag(CheckerContext &C) const {
   // Don't create a note tag if we didn't assume anything:
   if (!AssumedNonNegative && !AssumedUpperBound)
@@ -686,37 +675,16 @@ void ArrayBoundChecker::performCheck(const Expr *E, CheckerContext &C) const {
           C.addTransition(ExceedsUpperBound, SUR.createNoteTag(C));
           return;
         }
-
-        BadOffsetKind Problem = AlsoMentionUnderflow
-                                    ? BadOffsetKind::Indeterminate
-                                    : BadOffsetKind::Overflowing;
-        Messages Msgs =
-            getNonTaintMsgs(C.getASTContext(), Space, Reg, ByteOffset,
-                            *KnownSize, Location, Problem);
-        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";
-
-        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);
+
+      BadOffsetKind Problem = AlsoMentionUnderflow
+                                  ? BadOffsetKind::Indeterminate
+                                  : BadOffsetKind::Overflowing;
+      Messages Msgs =
+          getNonTaintMsgs(C.getASTContext(), Space, Reg, ByteOffset,
+                          *KnownSize, Location, Problem);
+      reportOOB(C, ExceedsUpperBound, Msgs, ByteOffset, KnownSize);
+      return;
     }
 
     // Actually update the state. The "if" only fails in the extremely unlikely
@@ -758,7 +726,7 @@ void ArrayBoundChecker::reportOOB(CheckerContext &C, ProgramStateRef ErrorState,
                                   std::optional<NonLoc> Extent,
                                   bool IsTaintBug /*=false*/) const {
 
-  ExplodedNode *ErrorNode = C.generateErrorNode(ErrorState);
+  ExplodedNode *ErrorNode = C.generateNonFatalErrorNode(ErrorState);
   if (!ErrorNode)
     return;
 
diff --git a/clang/test/Analysis/ArrayBound/cplusplus-tainted-index.cpp b/clang/test/Analysis/ArrayBound/cplusplus-tainted-index.cpp
new file mode 100644
index 0000000000000..0912f5057f7ed
--- /dev/null
+++ b/clang/test/Analysis/ArrayBound/cplusplus-tainted-index.cpp
@@ -0,0 +1,52 @@
+// RUN: %clang_analyze_cc1 -std=c++11 -Wno-array-bounds -analyzer-config unroll-loops=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 at -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 at -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 at -1{{Out of bound access to memory after the end of 'arr'}}
+  if (index == 12)
+    arr[index] = index;
+  // expected-warning at -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 at -1{{Out of bound access to memory after the end of 'arr'}}
+}
+
+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 at -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 at -1{{Out of bound access to memory after the end of the region}}
+    // expected-warning at -2{{Out of bound access to memory after the end of 'proc_name'}}
+  }
+}

``````````

</details>


https://github.com/llvm/llvm-project/pull/161723