[-Wunsafe-buffer-usage] Fix false positives for string literals

[jkorous-apple]

fixes rdar://139106996

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-analysis

Author: None (jkorous-apple)

Changes

fixes rdar://139106996

---

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

2 Files Affected:

• (modified) clang/lib/Analysis/UnsafeBufferUsage.cpp (+16-10)
• (added) clang/test/SemaCXX/warn-unsafe-buffer-usage-string-literal.cpp (+18)

diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index 2c68409b846bc8..116d098075b6bf 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -434,16 +434,22 @@ AST_MATCHER(ArraySubscriptExpr, isSafeArraySubscript) {
   //    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;
+  APInt ArrSize{};
+  if (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;
+    ArrSize = CATy->getSize();
+  } else if (const auto *BaseStrLit = dyn_cast<StringLiteral>(Node.getBase()->IgnoreParenImpCasts())) {
+    // Add 1 for the terminating null character.
+    ArrSize = APInt{64, BaseStrLit->getLength() + 1, false};
+  }
 
   if (const auto *IdxLit = dyn_cast<IntegerLiteral>(Node.getIdx())) {
     const APInt ArrIdx = IdxLit->getValue();
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-string-literal.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-string-literal.cpp
new file mode 100644
index 00000000000000..e983a8f135d8a4
--- /dev/null
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-string-literal.cpp
@@ -0,0 +1,18 @@
+// RUN: %clang_cc1 -std=c++20 -Wno-everything -Wunsafe-buffer-usage \
+// RUN:            -fsafe-buffer-usage-suggestions \
+// RUN:            -verify %s
+
+// CHECK-NOT: [-Wunsafe-buffer-usage]
+
+
+void foo(unsigned idx) {
+  char c = '0';
+  c = "abc"[0];
+  c = "abc"[1];
+  c = "abc"[2];
+  c = "abc"[3];
+  c = "abc"[4]; // expected-warning{{unsafe buffer access}}
+  c = "abc"[idx]; // expected-warning{{unsafe buffer access}}
+  c = ""[0];
+  c = ""[1]; // expected-warning{{unsafe buffer access}}
+}

[github-actions]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff 8a7a7b5ffc690bd012cf090d31d47ec938248ba3 8db304f11ba708abd096b4b8df998c55548e5b4d --extensions cpp -- clang/test/SemaCXX/warn-unsafe-buffer-usage-string-literal.cpp clang/lib/Analysis/UnsafeBufferUsage.cpp

View the diff from clang-format here.

diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index 116d098075..65a4fecd54 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -436,7 +436,7 @@ AST_MATCHER(ArraySubscriptExpr, isSafeArraySubscript) {
 
   APInt ArrSize{};
   if (const auto *BaseDRE =
-    dyn_cast<DeclRefExpr>(Node.getBase()->IgnoreParenImpCasts())) {
+          dyn_cast<DeclRefExpr>(Node.getBase()->IgnoreParenImpCasts())) {
     if (!BaseDRE)
       return false;
     if (!BaseDRE->getDecl())
@@ -446,7 +446,8 @@ AST_MATCHER(ArraySubscriptExpr, isSafeArraySubscript) {
     if (!CATy)
       return false;
     ArrSize = CATy->getSize();
-  } else if (const auto *BaseStrLit = dyn_cast<StringLiteral>(Node.getBase()->IgnoreParenImpCasts())) {
+  } else if (const auto *BaseStrLit = dyn_cast<StringLiteral>(
+                 Node.getBase()->IgnoreParenImpCasts())) {
     // Add 1 for the terminating null character.
     ArrSize = APInt{64, BaseStrLit->getLength() + 1, false};
   }

[malavikasamak]

I think we can close this PR, since we already merged this as part of #115552