[Wunsafe-buffer-usage] Fix false positive when const sized array is indexed by const evaluatable expressions

[malavikasamak]

Do not warn when constant sized array is indexed by expressions that evaluate to a const value. For instance, sizeof(T) expression value can be evaluated at compile time and if an array is indexed by such an expression, it's bounds can be validated.

(rdar://140320289)

[llvmbot]

@llvm/pr-subscribers-clang-analysis

@llvm/pr-subscribers-clang

Author: Malavika Samak (malavikasamak)

Changes

Do not warn when constant sized array is indexed by expressions that evaluate to a const value. For instance, sizeof(T) expression value can be evaluated at compile time and if an array is indexed by such an expression, it's bounds can be validated.

(rdar://140320289)

---

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

2 Files Affected:

• (modified) clang/lib/Analysis/UnsafeBufferUsage.cpp (+3-2)
• (modified) clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp (+32)

diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index 40f529e52b44af..ce973a3f23af38 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -461,8 +461,9 @@ AST_MATCHER(ArraySubscriptExpr, isSafeArraySubscript) {
     size = SLiteral->getLength() + 1;
   }
 
-  if (const auto *IdxLit = dyn_cast<IntegerLiteral>(Node.getIdx())) {
-    const APInt ArrIdx = IdxLit->getValue();
+  Expr::EvalResult EVResult;
+  if (Node.getIdx()->EvaluateAsInt(EVResult, Finder->getASTContext())) {
+    llvm::APSInt ArrIdx = EVResult.Val.getInt();
     // FIXME: ArrIdx.isNegative() we could immediately emit an error as that's a
     // bug
     if (ArrIdx.isNonNegative() && ArrIdx.getLimitedValue() < size)
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp
index c6c93a27e4b969..ec57b6ace844f0 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp
@@ -52,3 +52,35 @@ void constant_id_string(unsigned idx) {
   unsafe_char = ""[1]; //expected-warning{{unsafe buffer access}} 
   unsafe_char = ""[idx]; //expected-warning{{unsafe buffer access}}
 }
+
+struct T {
+  int array[10];
+};
+
+const int index = 1;
+
+constexpr int get_const(int x) {
+  if(x < 3)
+    return ++x;
+  else
+    return x + 5;
+};
+
+void array_indexed_const_expr(unsigned idx) {
+  // expected-note@+2 {{change type of 'arr' to 'std::array' to label it for hardening}}
+  // expected-warning@+1{{'arr' is an unsafe buffer that does not perform bounds checks}}
+  int arr[10];
+  arr[sizeof(int)] = 5;
+
+  int array[sizeof(T)];
+  array[sizeof(int)] = 5;
+  array[sizeof(T) -1 ] = 3;
+
+  int k = arr[6 & 5];
+  k = arr[2 << index];
+  k = arr[8 << index]; // expected-note {{used in buffer access here}}
+  k = arr[16 >> 1];
+  k = arr[get_const(index)];
+  k = arr[get_const(5)]; // expected-note {{used in buffer access here}}
+  k = arr[get_const(4)];
+}

[ziqingluo-90]

LGTM, thank you @malavikasamak

[nico]

This makes clang assert on many inputs. Here's a repro:

repro.zip

% ./input_file_parsers-161259.sh
...
Assertion failed: (!isValueDependent() && "Expression evaluator can't be called on a dependent expression."), function EvaluateAsInt, file ExprConstant.cpp, line 16671.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace, preprocessed source, and associated run script.
Stack dump:
0.	Program arguments: /Users/thakis/src/llvm-project/out/gn/bin/clang -cc1 -triple x86_64-unknown-linux-gnu -emit-obj --crel -disable-free -clear-ast-before-backend -main-file-name input_file_parsers.cc -mrelocation-model pic -pic-level 2 -fhalf-no-semantic-interposition -fmerge-all-constants -fno-delete-null-pointer-checks -mframe-pointer=all -relaxed-aliasing -ffp-contract=off -fno-rounding-math -mconstructor-aliases -funwind-tables=2 -target-cpu x86-64 -target-feature +sse3 -tune-cpu generic -debug-info-kind=constructor -dwarf-version=4 -debugger-tuning=gdb -ggnu-pubnames -gsimple-template-names=simple -mllvm -generate-arange-section -debug-forward-template-params -fdebug-compilation-dir=. -split-dwarf-file tmp.dwo -split-dwarf-output tmp.dwo -mllvm -crash-diagnostics-dir=../../tools/clang/crashreports -ffunction-sections -fdata-sections -fno-unique-section-names -fcoverage-compilation-dir=. -nostdinc++ -O2 -std=c++20 -fdeprecated-macro -ferror-limit 19 -fvisibility=hidden -fvisibility-inlines-hidden -fwrapv -pthread -stack-protector 1 -ftrivial-auto-var-init=pattern -fno-rtti -fgnuc-version=4.2.1 -fno-implicit-modules -fskip-odr-check-in-gmf -fno-sized-deallocation -Qn -fcolor-diagnostics -vectorize-loops -vectorize-slp -fuse-ctor-homing -mllvm -instcombine-lower-dbg-declare=0 -mllvm -split-threshold-for-reg-with-hint=0 -fcomplete-member-pointers -faddrsig -x c++ input_file_parsers-161259.cpp -Wno-gnu-line-marker -Wunsafe-buffer-usage
1.	<eof> parser at end of file

[nico]

Reverted in 7dd34ba for now.

[malavikasamak]

@nico Thanks for the reproducer. I have relanded this change with the fix for the failing assert. 2a8c12b. Please let me know if there are any more issues.

[nico]

If you haven't, it'd be good to also add a minimal regression test for the assert :)