[-Wunsafe-buffer-usage] Ignore constant safe indices in array subscri…

…pts (#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.

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 @@ class ArraySubscriptGadget : public WarningGadget {
   }
 
   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
   }
 

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}}
+}

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()"

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()"
 }

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}}
 }