Introduce analyzer config option for aggressive reporting of tainted offsets

t-rasmud · t-rasmud · commit cc282001d148 · 2025-10-07T13:20:07.000-07:00
diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/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
diff --git a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp b/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);
   }
@@ -478,6 +482,17 @@ 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)
@@ -675,15 +690,46 @@ 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;
       }
+      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;
+        }
 
-      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 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, "offset", AlsoMentionUnderflow);
+        reportOOB(C, ExceedsUpperBound, Msgs, ByteOffset, KnownSize,
+                  /*IsTaintBug=*/true);
+        return;
+      }
     }
 
     // Actually update the state. The "if" only fails in the extremely unlikely
@@ -725,7 +771,7 @@ void ArrayBoundChecker::reportOOB(CheckerContext &C, ProgramStateRef ErrorState,
                                   std::optional<NonLoc> Extent,
                                   bool IsTaintBug /*=false*/) const {
 
-  ExplodedNode *ErrorNode = C.generateNonFatalErrorNode(ErrorState);
+  ExplodedNode *ErrorNode = C.generateErrorNode(ErrorState);
   if (!ErrorNode)
     return;
 
@@ -804,7 +850,10 @@ 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) {
diff --git a/clang/test/Analysis/ArrayBound/cplusplus-tainted-index.cpp b/clang/test/Analysis/ArrayBound/cplusplus-tainted-index.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_analyze_cc1 -std=c++11 -Wno-array-bounds -analyzer-config unroll-loops=true -analyzer-checker=unix,core,security.ArrayBound -verify %s
+// 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.
 
@@ -20,7 +20,7 @@ void test_tainted_index1(unsigned index) {
   int arr[10];
   if (index < 12)
     arr[index] = index;
-  // expected-warning@-1{{Out of bound access to memory after the end of 'arr'}}
+  // 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'}}
@@ -30,7 +30,7 @@ void test_tainted_index2(unsigned index) {
   int arr[10];
   if (index < 12)
     arr[index] = index;
-  // expected-warning@-1{{Out of bound access to memory after the end of 'arr'}}
+  // expected-warning@-1{{Potential out of bound access to 'arr' with tainted offset}}
 }
 
 unsigned strlen(const char *s);
@@ -46,7 +46,6 @@ void test_ex(int argc, char *argv[]) {
   if (offset <= 16) {
     strcpy(proc_name, argv[0]);
     proc_name[offset] = argv[0][16];
-    // expected-warning@-1{{Out of bound access to memory after the end of the region}}
-    // expected-warning@-2{{Out of bound access to memory after the end of 'proc_name'}}
+    // expected-warning@-1{{Potential out of bound access to the region with tainted offset}}
   }
 }
diff --git a/clang/test/Analysis/analyzer-config.c b/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 = ""
