Author: jkorous-apple Date: 2024-02-14T18:16:44-08:00 New Revision: 9a1e6373ab3edc38486af504154db2d804e72d3d
URL: https://github.com/llvm/llvm-project/commit/9a1e6373ab3edc38486af504154db2d804e72d3d DIFF: https://github.com/llvm/llvm-project/commit/9a1e6373ab3edc38486af504154db2d804e72d3d.diff LOG: [-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. Added: Modified: clang/lib/Analysis/UnsafeBufferUsage.cpp clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-access.cpp clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits-pointer-arg-to-func-ptr-call.cpp clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp Removed: ################################################################################ diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp index ca346444e047e5..a74c113e29f1cf 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 @@ 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..8b2f103ec66708 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 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..642db0e9d3c632 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}} } _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
