https://github.com/mxms0 updated https://github.com/llvm/llvm-project/pull/117370
>From 8fed333cf4221dbf1826351da80164db5d209c21 Mon Sep 17 00:00:00 2001 From: mxms <m...@google.com> Date: Fri, 22 Nov 2024 15:09:07 -0500 Subject: [PATCH 1/4] [Wunsafe-buffer-usage] Fix false positives in handling enums Do not warn if the index is an enum and we an determine statically that it's within bounds. --- clang/lib/Analysis/UnsafeBufferUsage.cpp | 7 +++++++ .../SemaCXX/warn-unsafe-buffer-usage-array.cpp | 17 +++++++++++++++++ 2 files changed, 24 insertions(+) diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp index 5f36ffa926b269..addb724e2e2c9a 100644 --- a/clang/lib/Analysis/UnsafeBufferUsage.cpp +++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp @@ -463,6 +463,13 @@ AST_MATCHER(ArraySubscriptExpr, isSafeArraySubscript) { return true; } + // Array index wasn't an integer literal, let's see if it was an enum or + // something similar + const auto IntConst = Node.getIdx()->getIntegerConstantExpr(Finder->getASTContext()); + if (IntConst && *IntConst > 0 && *IntConst < size) { + return true; + } + return false; } diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp index c6c93a27e4b969..a65ecdf39edfcc 100644 --- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp +++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp @@ -39,6 +39,23 @@ void constant_idx_unsafe(unsigned idx) { buffer[10] = 0; // expected-note{{used in buffer access here}} } +enum FooEnum { + A = 0, + B = 1, + C = 2, + D +}; + +void constant_enum_safe() { + int buffer[FooEnum::D] = { 0, 1, 2 }; + buffer[C] = 0; // no-warning +} + +void constant_enum_unsafe(FooEnum e) { + int buffer[FooEnum::D] = { 0, 1, 2 }; + buffer[e] = 0; // expected-warning{{unsafe buffer access}} +} + void constant_id_string(unsigned idx) { char safe_char = "abc"[1]; // no-warning safe_char = ""[0]; >From 35207ea84425902a70b46f153e9619cc9d544f46 Mon Sep 17 00:00:00 2001 From: mxms <m...@google.com> Date: Fri, 22 Nov 2024 23:45:39 -0500 Subject: [PATCH 2/4] Detect when the buffer is a member access, fix tests Add a case for when it's a member variable access and we can statically determine the size. Also add new test to ensure the change works reliably and update old tests to not expect this warning. --- clang/lib/Analysis/UnsafeBufferUsage.cpp | 2 +- .../warn-unsafe-buffer-usage-array.cpp | 5 ++++- .../warn-unsafe-buffer-usage-field-attr.cpp | 1 - .../test/SemaCXX/warn-unsafe-buffer-usage.cpp | 21 +++++++++---------- 4 files changed, 15 insertions(+), 14 deletions(-) diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp index addb724e2e2c9a..c005b34c2f63ad 100644 --- a/clang/lib/Analysis/UnsafeBufferUsage.cpp +++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp @@ -466,7 +466,7 @@ AST_MATCHER(ArraySubscriptExpr, isSafeArraySubscript) { // Array index wasn't an integer literal, let's see if it was an enum or // something similar const auto IntConst = Node.getIdx()->getIntegerConstantExpr(Finder->getASTContext()); - if (IntConst && *IntConst > 0 && *IntConst < size) { + if (IntConst && *IntConst >= 0 && *IntConst < size) { return true; } diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp index a65ecdf39edfcc..f5672db18de0ca 100644 --- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp +++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp @@ -47,8 +47,11 @@ enum FooEnum { }; void constant_enum_safe() { - int buffer[FooEnum::D] = { 0, 1, 2 }; + int buffer[FooEnum::D] = { 0, 1, 2 }; // 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[A] = 0; // no-warning buffer[C] = 0; // no-warning + buffer[D] = 0; // expected-note{{used in buffer access here}} } void constant_enum_unsafe(FooEnum e) { diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-field-attr.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-field-attr.cpp index 0ba605475925b9..1636c948da075a 100644 --- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-field-attr.cpp +++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-field-attr.cpp @@ -96,7 +96,6 @@ void test_attribute_multiple_fields (D d) { int v = d.buf[0]; //expected-warning{{field 'buf' prone to unsafe buffer manipulation}} - //expected-warning@+1{{unsafe buffer access}} v = d.buf[5]; //expected-warning{{field 'buf' prone to unsafe buffer manipulation}} } diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp index 642db0e9d3c632..41194a8e3f5222 100644 --- a/clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp +++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp @@ -128,25 +128,25 @@ T_t funRetT(); T_t * funRetTStar(); void testStructMembers(struct T * sp, struct T s, T_t * sp2, T_t s2) { - foo(sp->a[1], // expected-warning{{unsafe buffer access}} + foo(sp->a[1], sp->b[1], // expected-warning{{unsafe buffer access}} - sp->c.a[1], // expected-warning{{unsafe buffer access}} + sp->c.a[1], sp->c.b[1], // expected-warning{{unsafe buffer access}} - s.a[1], // expected-warning{{unsafe buffer access}} + s.a[1], s.b[1], // expected-warning{{unsafe buffer access}} - s.c.a[1], // expected-warning{{unsafe buffer access}} + s.c.a[1], s.c.b[1], // expected-warning{{unsafe buffer access}} - sp2->a[1], // expected-warning{{unsafe buffer access}} + sp2->a[1], sp2->b[1], // expected-warning{{unsafe buffer access}} - sp2->c.a[1], // expected-warning{{unsafe buffer access}} + sp2->c.a[1], sp2->c.b[1], // expected-warning{{unsafe buffer access}} - s2.a[1], // expected-warning{{unsafe buffer access}} + s2.a[1], s2.b[1], // expected-warning{{unsafe buffer access}} - s2.c.a[1], // expected-warning{{unsafe buffer access}} + s2.c.a[1], s2.c.b[1], // expected-warning{{unsafe buffer access}} - funRetT().a[1], // expected-warning{{unsafe buffer access}} + funRetT().a[1], funRetT().b[1], // expected-warning{{unsafe buffer access}} - funRetTStar()->a[1], // expected-warning{{unsafe buffer access}} + funRetTStar()->a[1], funRetTStar()->b[1] // expected-warning{{unsafe buffer access}} ); } @@ -213,7 +213,6 @@ void testTypedefs(T_ptr_t p) { // expected-warning@-1{{'p' is an unsafe pointer used for buffer access}} foo(p[1], // expected-note{{used in buffer access here}} p[1].a[1], // expected-note{{used in buffer access here}} - // expected-warning@-1{{unsafe buffer access}} p[1].b[1] // expected-note{{used in buffer access here}} // expected-warning@-1{{unsafe buffer access}} ); >From f534ebe89a31449388dd157979a4ef8f18060e3f Mon Sep 17 00:00:00 2001 From: mxms <m...@google.com> Date: Fri, 22 Nov 2024 23:55:21 -0500 Subject: [PATCH 3/4] Fix test for unsafe enum --- clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp index f5672db18de0ca..0ae5b1b06b5c21 100644 --- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp +++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp @@ -55,8 +55,10 @@ void constant_enum_safe() { } void constant_enum_unsafe(FooEnum e) { - int buffer[FooEnum::D] = { 0, 1, 2 }; - buffer[e] = 0; // expected-warning{{unsafe buffer access}} + int buffer[FooEnum::D] = { 0, 1, 2 }; // 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[e] = 0; // expected-note{{used in buffer access here}} } void constant_id_string(unsigned idx) { >From 3cdadc637ddbe2ce7a3267a572a372452c74ce48 Mon Sep 17 00:00:00 2001 From: mxms <m...@google.com> Date: Sun, 24 Nov 2024 20:12:03 -0500 Subject: [PATCH 4/4] Minor nit, reorder expression --- clang/lib/Analysis/UnsafeBufferUsage.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp index c005b34c2f63ad..85754ea6deefaf 100644 --- a/clang/lib/Analysis/UnsafeBufferUsage.cpp +++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp @@ -466,7 +466,7 @@ AST_MATCHER(ArraySubscriptExpr, isSafeArraySubscript) { // Array index wasn't an integer literal, let's see if it was an enum or // something similar const auto IntConst = Node.getIdx()->getIntegerConstantExpr(Finder->getASTContext()); - if (IntConst && *IntConst >= 0 && *IntConst < size) { + if (IntConst && 0 <= *IntConst && *IntConst < size) { return true; } _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits