[-Wunsafe-buffer-usage] Ignore constant safe indices in array subscripts (#80504)

[-Wunsafe-buffer-usage] Ignore safe array subscripts
Don't emit warnings for array subscripts on constant size arrays where the index is constant and within bounds.

Example:
int arr[10];
arr[5] = 0; //safe, no warning

This patch recognizes only array indices that are integer literals - it doesn't understand more complex expressions (arithmetic on constants, etc.).

-Warray-bounds implemented in Sema::CheckArrayAccess() already solves a similar
(opposite) problem, handles complex expressions and is battle-tested.

Adding -Wunsafe-buffer-usage diagnostics to Sema is a non-starter as we need to emit
both the warnings and fixits and the performance impact of the fixit machine is
unacceptable for Sema.

CheckArrayAccess() as is doesn't distinguish between "safe" and "unknown" array
accesses. It also mixes the analysis that decides if an index is out of bounds
with crafting the diagnostics.

A refactor of CheckArrayAccess() might serve both the original purpose
and help us avoid false-positive with -Wunsafe-buffer-usage on constant
size arrrays.

5 files changed, 91 insertions, 41 deletions

diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index ca346444e047..a74c113e29f1 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -406,6 +406,39 @@ AST_MATCHER(CXXConstructExpr, isSafeSpanTwoParamConstruct) {
   }
   return false;
 }
+
+AST_MATCHER(ArraySubscriptExpr, isSafeArraySubscript) {
+  // FIXME: Proper solution:
+  //  - refactor Sema::CheckArrayAccess
+  //    - split safe/OOB/unknown decision logic from diagnostics emitting code
+  //    -  e. g. "Try harder to find a NamedDecl to point at in the note."
+  //    already duplicated
+  //  - call both from Sema and from here
+
+  const auto *BaseDRE =
+      dyn_cast<DeclRefExpr>(Node.getBase()->IgnoreParenImpCasts());
+  if (!BaseDRE)
+    return false;
+  if (!BaseDRE->getDecl())
+    return false;
+  const auto *CATy = Finder->getASTContext().getAsConstantArrayType(
+      BaseDRE->getDecl()->getType());
+  if (!CATy)
+    return false;
+  const APInt ArrSize = CATy->getSize();
+
+  if (const auto *IdxLit = dyn_cast<IntegerLiteral>(Node.getIdx())) {
+    const APInt ArrIdx = IdxLit->getValue();
+    // FIXME: ArrIdx.isNegative() we could immediately emit an error as that's a
+    // bug
+    if (ArrIdx.isNonNegative() &&
+        ArrIdx.getLimitedValue() < ArrSize.getLimitedValue())
+      return true;
+  }
+
+  return false;
+}
+
 } // namespace clang::ast_matchers
 
 namespace {
@@ -598,16 +631,16 @@ public:
   }
 
   static Matcher matcher() {
-    // FIXME: What if the index is integer literal 0? Should this be
-    // a safe gadget in this case?
-      // clang-format off
+    // clang-format off
       return stmt(arraySubscriptExpr(
             hasBase(ignoringParenImpCasts(
               anyOf(hasPointerType(), hasArrayType()))),
-            unless(hasIndex(
-                anyOf(integerLiteral(equals(0)), arrayInitIndexExpr())
-             )))
-            .bind(ArraySubscrTag));
+            unless(anyOf(
+              isSafeArraySubscript(),
+              hasIndex(
+                  anyOf(integerLiteral(equals(0)), arrayInitIndexExpr())
+              )
+            ))).bind(ArraySubscrTag));
     // clang-format on
   }
 
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp
index 90c11b1be95c..8b2f103ec667 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -std=c++20 -Wno-all -Wunsafe-buffer-usage \
+// RUN: %clang_cc1 -std=c++20 -Wno-everything -Wunsafe-buffer-usage \
 // RUN:            -fsafe-buffer-usage-suggestions \
 // RUN:            -verify %s
 
@@ -22,3 +22,19 @@ struct Foo {
 void foo2(Foo& f, unsigned idx) {
   f.member_buffer[idx] = 0; // expected-warning{{unsafe buffer access}}
 }
+
+void constant_idx_safe(unsigned idx) {
+  int buffer[10];
+  buffer[9] = 0;
+}
+
+void constant_idx_safe0(unsigned idx) {
+  int buffer[10];
+  buffer[0] = 0;
+}
+
+void constant_idx_unsafe(unsigned idx) {
+  int buffer[10];       // expected-warning{{'buffer' is an unsafe buffer that does not perform bounds checks}}
+                        // expected-note@-1{{change type of 'buffer' to 'std::array' to label it for hardening}}
+  buffer[10] = 0;       // expected-note{{used in buffer access here}}
+}
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-access.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-access.cpp
index f94072015ff8..b3c64f1b0d08 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-access.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-access.cpp
@@ -83,11 +83,11 @@ void unsafe_method_invocation_single_param() {
 
 }
 
-void unsafe_method_invocation_single_param_array() {
+void unsafe_method_invocation_single_param_array(int idx) {
   int p[32];
   // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::array<int, 32> p"
 
-  int tmp = p[5];
+  int tmp = p[idx];
   foo(p);
   // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:8-[[@LINE-1]]:8}:".data()"
 }
@@ -126,14 +126,14 @@ void unsafe_method_invocation_double_param() {
   m1(q, q, 8);
 }
 
-void unsafe_method_invocation_double_param_array() {
+void unsafe_method_invocation_double_param_array(int idx) {
   int p[14];
   // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::array<int, 14> p"
 
   int q[40];
   // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::array<int, 40> q"
 
-  q[5] = p[5];
+  q[idx] = p[idx];
 
   m1(p, p, 10);
   // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:7-[[@LINE-1]]:7}:".data()"
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-arg-to-func-ptr-call.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-arg-to-func-ptr-call.cpp
index 0459d6549fd8..216813ce45bf 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-arg-to-func-ptr-call.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-arg-to-func-ptr-call.cpp
@@ -6,7 +6,8 @@ void unsafe_array_func_ptr_call(void (*fn_ptr)(int *param)) {
   int p[32];
   // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:12}:"std::array<int, 32> p"
 
-  p[5] = 10;
+  int idx;
+  p[idx] = 10;
   fn_ptr(p);
   // CHECK-DAG: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:11}:".data()"
 }
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
index 67cdf252d6a8..642db0e9d3c6 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
@@ -36,7 +36,7 @@ void testIncrement(char *p) { // expected-warning{{'p' is an unsafe pointer used
 void * voidPtrCall(void);
 char * charPtrCall(void);
 
-void testArraySubscripts(int *p, int **pp) {
+void testArraySubscripts(int idx, int *p, int **pp) {
 // expected-warning@-1{{'p' is an unsafe pointer used for buffer access}}
 // expected-warning@-2{{'pp' is an unsafe pointer used for buffer access}}
   foo(p[1],             // expected-note{{used in buffer access here}}
@@ -64,13 +64,14 @@ void testArraySubscripts(int *p, int **pp) {
                         // expected-note@-1{{change type of 'a' to 'std::array' to label it for hardening}}
     int b[10][10];      // expected-warning{{'b' is an unsafe buffer that does not perform bounds checks}}
 
-  foo(a[1], 1[a],   // expected-note2{{used in buffer access here}}
-      b[3][4],      // expected-warning{{unsafe buffer access}}
-                    // expected-note@-1{{used in buffer access here}}
-      4[b][3],      // expected-warning{{unsafe buffer access}}
-                    // expected-note@-1{{used in buffer access here}}
-      4[3[b]]);     // expected-warning{{unsafe buffer access}}
-                    // expected-note@-1{{used in buffer access here}}
+  foo(a[idx], idx[a],   // expected-note2{{used in buffer access here}}
+      b[idx][idx + 1],  // expected-warning{{unsafe buffer access}}
+                        // expected-note@-1{{used in buffer access here}}
+      (idx + 1)[b][idx],// expected-warning{{unsafe buffer access}}
+                        // expected-note@-1{{used in buffer access here}}
+      (idx + 1)[idx[b]]);
+                        // expected-warning@-1{{unsafe buffer access}}
+                        // expected-note@-2{{used in buffer access here}}
 
   // Not to warn when index is zero
   foo(p[0], pp[0][0], 0[0[pp]], 0[pp][0],
@@ -158,9 +159,9 @@ void testLambdaCaptureAndGlobal(int * p) {
   // expected-warning@-1{{'p' is an unsafe pointer used for buffer access}}
   int a[10];              // expected-warning{{'a' is an unsafe buffer that does not perform bounds checks}}
 
-  auto Lam = [p, a]() {
+  auto Lam = [p, a](int idx) {
     return p[1]           // expected-note{{used in buffer access here}}
-      + a[1] + garray[1]  // expected-note2{{used in buffer access here}}
+      + a[idx] + garray[idx]// expected-note2{{used in buffer access here}}
       + gp[1];            // expected-note{{used in buffer access here}}
 
   };
@@ -178,31 +179,31 @@ void testLambdaCapture() {
                           // expected-note@-1{{change type of 'b' to 'std::array' to label it for hardening}}
   int c[10];
 
-  auto Lam1 = [a]() {
-    return a[1];           // expected-note{{used in buffer access here}}
+  auto Lam1 = [a](unsigned idx) {
+    return a[idx];           // expected-note{{used in buffer access here}}
   };
 
-  auto Lam2 = [x = b[3]]() { // expected-note{{used in buffer access here}}
+  auto Lam2 = [x = b[c[5]]]() { // expected-note{{used in buffer access here}}
     return x;
   };
 
-  auto Lam = [x = c]() { // expected-warning{{'x' is an unsafe pointer used for buffer access}}
-    return x[3]; // expected-note{{used in buffer access here}}
+  auto Lam = [x = c](unsigned idx) { // expected-warning{{'x' is an unsafe pointer used for buffer access}}
+    return x[idx]; // expected-note{{used in buffer access here}}
   };
 }
 
-void testLambdaImplicitCapture() {
+void testLambdaImplicitCapture(long idx) {
   int a[10];              // expected-warning{{'a' is an unsafe buffer that does not perform bounds checks}}
                           // expected-note@-1{{change type of 'a' to 'std::array' to label it for hardening}}
   int b[10];              // expected-warning{{'b' is an unsafe buffer that does not perform bounds checks}}
                           // expected-note@-1{{change type of 'b' to 'std::array' to label it for hardening}}
   
   auto Lam1 = [=]() {
-    return a[1];           // expected-note{{used in buffer access here}}
+    return a[idx];           // expected-note{{used in buffer access here}}
   };
   
   auto Lam2 = [&]() {
-    return b[1];           // expected-note{{used in buffer access here}}
+    return b[idx];           // expected-note{{used in buffer access here}}
   };
 }
 
@@ -344,38 +345,37 @@ int testVariableDecls(int * p) {
   return p[1];        // expected-note{{used in buffer access here}}
 }
 
-template<typename T> void fArr(T t[]) {
+template<typename T> void fArr(T t[], long long idx) {
   // expected-warning@-1{{'t' is an unsafe pointer used for buffer access}}
   foo(t[1]);    // expected-note{{used in buffer access here}}
   T ar[8];      // expected-warning{{'ar' is an unsafe buffer that does not perform bounds checks}}
                 // expected-note@-1{{change type of 'ar' to 'std::array' to label it for hardening}}
-  foo(ar[5]);   // expected-note{{used in buffer access here}}
+  foo(ar[idx]);   // expected-note{{used in buffer access here}}
 }
 
-template void fArr<int>(int t[]); // FIXME: expected note {{in instantiation of}}
+template void fArr<int>(int t[], long long); // FIXME: expected note {{in instantiation of}}
 
 int testReturn(int t[]) {// expected-note{{change type of 't' to 'std::span' to preserve bounds information}}
   // expected-warning@-1{{'t' is an unsafe pointer used for buffer access}}
   return t[1]; // expected-note{{used in buffer access here}}
 }
 
-int testArrayAccesses(int n) {
+int testArrayAccesses(int n, int idx) {
     // auto deduced array type
     int cArr[2][3] = {{1, 2, 3}, {4, 5, 6}};
     // expected-warning@-1{{'cArr' is an unsafe buffer that does not perform bounds checks}}
     int d = cArr[0][0];
     foo(cArr[0][0]);
-    foo(cArr[1][2]);        // expected-note{{used in buffer access here}}
-                            // expected-warning@-1{{unsafe buffer access}}
-    auto cPtr = cArr[1][2]; // expected-note{{used in buffer access here}}
-                            // expected-warning@-1{{unsafe buffer access}}
+    foo(cArr[idx][idx + 1]);        // expected-note{{used in buffer access here}}
+                                    // expected-warning@-1{{unsafe buffer access}}
+    auto cPtr = cArr[idx][idx * 2]; // expected-note{{used in buffer access here}}
+                                    // expected-warning@-1{{unsafe buffer access}}
     foo(cPtr);
 
     // Typdefs
     typedef int A[3];
     const A tArr = {4, 5, 6};
-    // expected-warning@-1{{'tArr' is an unsafe buffer that does not perform bounds checks}}
-    foo(tArr[0], tArr[1]);  // expected-note{{used in buffer access here}}
+    foo(tArr[0], tArr[1]);
     return cArr[0][1];      // expected-warning{{unsafe buffer access}}
 }