[Wunsafe-buffer-usage] Address some false positives in handling array indices that are decidably correct

[mxms0]

This change aims to prevent Wunsafe-buffer-usage warnings when the code is trivially correct and we don't need to warn about it. This is done for two cases:

• When enums are used as indices
• When the primitive array is part of a MemberExpr

I've also adjusted the tests to match the expected behavior, which is to not warn when the code is correct.

There's more work to be done in making this comprehensive, which I'm happy to do in a follow-up.

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-analysis

Author: Max (mxms0)

Changes

Do not warn if the index is an enum and we an determine statically that it's within bounds.

---

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

2 Files Affected:

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

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];

[rnk]

I think there's a policy question here of how much trust we want to put into the type system. Clearly, we've already put some trust into it to reduce false positives, but we could decide to trust any old constant array type bounds, and that would be a good code simplification.

[rnk]

I think getting the integer constant expr is more general, we can remove the IntegerLiteral test here, and if all tests pass, I would consider it functionally equivalent. This code seems like it was tested to a reasonably high standard.

[mxms0]

I changed the code and tests still pass, it looks like you're correct. :)

[rnk]

There's a FIXME above about refactoring Sema::CheckArrayAccess to avoid duplication, and I'm trying to decide if that makes sense or not, or if we should just copy the logic.

It seems like the main behavior difference is that Sema::CheckArrayAccess doesn't go looking for a Decl with a ConstantArray type, it just looks at the type of any old expression, which means it doesn't warn on cases involving casts or other expressions with constant array type, like these:

int arr[1];
int oob() {return (*(int(*)[2])&arr)[1]; } // currently warns, should we keep warning or trust the cast?
int *arrayAddr()[1] { return &arr; }
int safe() { return (*arrayAddr())[0] } // currently warns, but should we warn?

[mxms0]

I think ultimately going to the Sema code is probably the better place to be, but I'm not sure who's working on that or if they have any urgency to land it. Ergo I'm in favor of landing this and working with the original owner to find the desired end state, as long as you all think that's an acceptable medium state. :)

[mxms0]

I think there's a policy question here of how much trust we want to put into the type system. Clearly, we've already put some trust into it to reduce false positives, but we could decide to trust any old constant array type bounds, and that would be a good code simplification.

My view is that we should trust it (the type system) until we have evidence that it's more of a problem. Developers doing really hacky bad things I expect to be a lot less common than developers doing bad things by accident (i.e. a classic index out of bounds)

[mxms0]

Friendly ping @rnk, @jkorous-apple :)