[clang] [Wunsafe-buffer-usage] Fix false positives in handling array indices that are decidably correct (PR #117370)
via cfe-commits
cfe-commits at lists.llvm.org
Sun Nov 24 19:48:24 PST 2024
https://github.com/mxms0 updated https://github.com/llvm/llvm-project/pull/117370
>From 8fed333cf4221dbf1826351da80164db5d209c21 Mon Sep 17 00:00:00 2001
From: mxms <mxms at google.com>
Date: Fri, 22 Nov 2024 15:09:07 -0500
Subject: [PATCH 1/5] [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 <mxms at google.com>
Date: Fri, 22 Nov 2024 23:45:39 -0500
Subject: [PATCH 2/5] 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 at -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 at +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 at -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 at -1{{unsafe buffer access}}
p[1].b[1] // expected-note{{used in buffer access here}}
// expected-warning at -1{{unsafe buffer access}}
);
>From f534ebe89a31449388dd157979a4ef8f18060e3f Mon Sep 17 00:00:00 2001
From: mxms <mxms at google.com>
Date: Fri, 22 Nov 2024 23:55:21 -0500
Subject: [PATCH 3/5] 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 at -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 <mxms at google.com>
Date: Sun, 24 Nov 2024 20:12:03 -0500
Subject: [PATCH 4/5] 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;
}
>From 2ffd0df82bdb3a086d4b75744a7f0478a0b20032 Mon Sep 17 00:00:00 2001
From: mxms <mxms at google.com>
Date: Sun, 24 Nov 2024 22:46:21 -0500
Subject: [PATCH 5/5] Try harder to find an element we can analyze
Add logic to handle MemberExprs, so we can attempt to find the size
information for the struct member variables. This is probably still
incomplete in terms of the full space of expressions, but eh, just needs
to be better, not perfect.
---
clang/lib/Analysis/UnsafeBufferUsage.cpp | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index 85754ea6deefaf..0cf8257665a08c 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -439,8 +439,16 @@ AST_MATCHER(ArraySubscriptExpr, isSafeArraySubscript) {
dyn_cast<StringLiteral>(Node.getBase()->IgnoreParenImpCasts());
uint64_t size;
- if (!BaseDRE && !SLiteral)
- return false;
+ if (!BaseDRE && !SLiteral) {
+ // Try harder to find something that looks like a DeclRefExpr
+ const auto *Member = dyn_cast<MemberExpr>(Node.getBase()->IgnoreParenImpCasts());
+ if (!Member) return false;
+
+ const auto *Value = Finder->getASTContext().getAsConstantArrayType(Member->getMemberDecl()->getType());
+ if (!Value) return false;
+
+ size = Value->getLimitedSize();
+ }
if (BaseDRE) {
if (!BaseDRE->getDecl())
More information about the cfe-commits
mailing list