[clang] [-Wunsafe-buffer-usage] Ignore constant safe indices in array subscripts (PR #80504)
via cfe-commits
cfe-commits at lists.llvm.org
Wed Feb 14 17:58:32 PST 2024
https://github.com/jkorous-apple updated https://github.com/llvm/llvm-project/pull/80504
>From e075dc3ac10c0cd2e12b223988ec4821b40b55d2 Mon Sep 17 00:00:00 2001
From: Jan Korous <jkorous at apple.com>
Date: Fri, 2 Feb 2024 14:46:59 -0800
Subject: [PATCH 1/6] [-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.).
---
clang/lib/Analysis/UnsafeBufferUsage.cpp | 39 +++++++++++++++----
.../warn-unsafe-buffer-usage-array.cpp | 18 ++++++++-
2 files changed, 49 insertions(+), 8 deletions(-)
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index ca346444e047e5..d150a679597a92 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -406,6 +406,31 @@ AST_MATCHER(CXXConstructExpr, isSafeSpanTwoParamConstruct) {
}
return false;
}
+
+AST_MATCHER(ArraySubscriptExpr, isSafeArraySubscript) {
+ const DeclRefExpr * BaseDRE = dyn_cast_or_null<DeclRefExpr>(Node.getBase()->IgnoreParenImpCasts());
+ if (!BaseDRE)
+ return false;
+ if (!BaseDRE->getDecl())
+ return false;
+ auto BaseVarDeclTy = BaseDRE->getDecl()->getType();
+ if (!BaseVarDeclTy->isConstantArrayType())
+ return false;
+ const auto * CATy = dyn_cast_or_null<ConstantArrayType>(BaseVarDeclTy);
+ 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 +623,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
}
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp
index 90c11b1be95c25..4804223e8be058 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 at -1{{change type of 'buffer' to 'std::array' to harden it}}
+ buffer[10] = 0; // expected-note{{used in buffer access here}}
+}
>From 9f3940e5e1a8555a784521999d296ed46b1ae0c2 Mon Sep 17 00:00:00 2001
From: Jan Korous <jkorous at apple.com>
Date: Fri, 2 Feb 2024 14:48:41 -0800
Subject: [PATCH 2/6] [-Wunsafe-buffer-usage][NFC] Update existing tests after
constant safe index is ignored
---
...afe-buffer-usage-fixits-pointer-access.cpp | 8 +--
...ge-fixits-pointer-arg-to-func-ptr-call.cpp | 3 +-
.../test/SemaCXX/warn-unsafe-buffer-usage.cpp | 53 ++++++++++---------
3 files changed, 33 insertions(+), 31 deletions(-)
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 f94072015ff87d..b3c64f1b0d085e 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 0459d6549fd86f..216813ce45bfd5 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 67cdf252d6a8b6..9738b357834eed 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 at -1{{'p' is an unsafe pointer used for buffer access}}
// expected-warning at -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 at -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 at -1{{used in buffer access here}}
- 4[b][3], // expected-warning{{unsafe buffer access}}
- // expected-note at -1{{used in buffer access here}}
- 4[3[b]]); // expected-warning{{unsafe buffer access}}
- // expected-note at -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 at -1{{used in buffer access here}}
+ (idx + 1)[b][idx],// expected-warning{{unsafe buffer access}}
+ // expected-note at -1{{used in buffer access here}}
+ (idx + 1)[idx[b]]);
+ // expected-warning at -1{{unsafe buffer access}}
+ // expected-note at -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 at -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 at -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 at -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 at -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,31 +345,31 @@ 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 at -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 at -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 at -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 at -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 at -1{{unsafe buffer access}}
- auto cPtr = cArr[1][2]; // expected-note{{used in buffer access here}}
- // expected-warning at -1{{unsafe buffer access}}
+ foo(cArr[idx][idx + 1]); // expected-note{{used in buffer access here}}
+ // expected-warning at -1{{unsafe buffer access}}
+ auto cPtr = cArr[idx][idx * 2]; // expected-note{{used in buffer access here}}
+ // expected-warning at -1{{unsafe buffer access}}
foo(cPtr);
// Typdefs
>From d6dda2c821a94a5ad7922f3ab6b95475f88c888d Mon Sep 17 00:00:00 2001
From: Jan Korous <jkorous at apple.com>
Date: Tue, 6 Feb 2024 11:36:08 -0800
Subject: [PATCH 3/6] [-Wunsafe-buffer-usage][NFC] Note future work for
isSafeArraySubscript
-Warray-bounds implemented in Sema::CheckArrayAccess() already solves a similar
(opposite) problem 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 | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index d150a679597a92..aa3240a86e562b 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -408,6 +408,12 @@ AST_MATCHER(CXXConstructExpr, isSafeSpanTwoParamConstruct) {
}
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 DeclRefExpr * BaseDRE = dyn_cast_or_null<DeclRefExpr>(Node.getBase()->IgnoreParenImpCasts());
if (!BaseDRE)
return false;
>From e4cf18427c94a9ebca00194f4bf08ad292d9bece Mon Sep 17 00:00:00 2001
From: Jan Korous <jkorous at apple.com>
Date: Wed, 14 Feb 2024 14:26:49 -0800
Subject: [PATCH 4/6] [-Wunsafe-buffer-usage][NFC] clang-format the PR
---
clang/lib/Analysis/UnsafeBufferUsage.cpp | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index aa3240a86e562b..62a82ff1e83c21 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -411,10 +411,12 @@ 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
+ // - e. g. "Try harder to find a NamedDecl to point at in the note."
+ // already duplicated
// - call both from Sema and from here
- const DeclRefExpr * BaseDRE = dyn_cast_or_null<DeclRefExpr>(Node.getBase()->IgnoreParenImpCasts());
+ const DeclRefExpr *BaseDRE =
+ dyn_cast_or_null<DeclRefExpr>(Node.getBase()->IgnoreParenImpCasts());
if (!BaseDRE)
return false;
if (!BaseDRE->getDecl())
@@ -422,15 +424,17 @@ AST_MATCHER(ArraySubscriptExpr, isSafeArraySubscript) {
auto BaseVarDeclTy = BaseDRE->getDecl()->getType();
if (!BaseVarDeclTy->isConstantArrayType())
return false;
- const auto * CATy = dyn_cast_or_null<ConstantArrayType>(BaseVarDeclTy);
+ const auto *CATy = dyn_cast_or_null<ConstantArrayType>(BaseVarDeclTy);
if (!CATy)
return false;
const APInt ArrSize = CATy->getSize();
- if (const auto * IdxLit = dyn_cast<IntegerLiteral>(Node.getIdx())) {
+ 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())
+ // FIXME: ArrIdx.isNegative() we could immediately emit an error as that's a
+ // bug
+ if (ArrIdx.isNonNegative() &&
+ ArrIdx.getLimitedValue() < ArrSize.getLimitedValue())
return true;
}
>From cbfde25913db1af43ea05a3f86f38cedce936b86 Mon Sep 17 00:00:00 2001
From: Jan Korous <jkorous at apple.com>
Date: Wed, 14 Feb 2024 17:55:05 -0800
Subject: [PATCH 5/6] [-Wunsafe-buffer-usage][NFC] Rebase the PR
---
clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp
index 4804223e8be058..8b2f103ec66708 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp
@@ -35,6 +35,6 @@ void constant_idx_safe0(unsigned idx) {
void constant_idx_unsafe(unsigned idx) {
int buffer[10]; // expected-warning{{'buffer' is an unsafe buffer that does not perform bounds checks}}
- // expected-note at -1{{change type of 'buffer' to 'std::array' to harden it}}
+ // expected-note at -1{{change type of 'buffer' to 'std::array' to label it for hardening}}
buffer[10] = 0; // expected-note{{used in buffer access here}}
}
>From 752794e794f4fa82e6a0f11c68d83f05cf2d538b Mon Sep 17 00:00:00 2001
From: Jan Korous <jkorous at apple.com>
Date: Wed, 14 Feb 2024 17:57:06 -0800
Subject: [PATCH 6/6] [-Wunsafe-buffer-usage] Use
ASTContext::getAsConstantArrayType()
---
clang/lib/Analysis/UnsafeBufferUsage.cpp | 6 ++----
clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp | 3 +--
2 files changed, 3 insertions(+), 6 deletions(-)
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index 62a82ff1e83c21..552418df8e2183 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -421,10 +421,8 @@ AST_MATCHER(ArraySubscriptExpr, isSafeArraySubscript) {
return false;
if (!BaseDRE->getDecl())
return false;
- auto BaseVarDeclTy = BaseDRE->getDecl()->getType();
- if (!BaseVarDeclTy->isConstantArrayType())
- return false;
- const auto *CATy = dyn_cast_or_null<ConstantArrayType>(BaseVarDeclTy);
+ const auto *CATy = Finder->getASTContext().getAsConstantArrayType(
+ BaseDRE->getDecl()->getType());
if (!CATy)
return false;
const APInt ArrSize = CATy->getSize();
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
index 9738b357834eed..642db0e9d3c632 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
@@ -375,8 +375,7 @@ int testArrayAccesses(int n, int idx) {
// Typdefs
typedef int A[3];
const A tArr = {4, 5, 6};
- // expected-warning at -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}}
}
More information about the cfe-commits
mailing list