[Wunsafe-buffer-usage] Fix false positives in handling string literals.

[malavikasamak]

Do not warn when a string literal is indexed and the idex value is within the bounds of the length of the string.

(rdar://139106996)

[llvmbot]

@llvm/pr-subscribers-clang-analysis

@llvm/pr-subscribers-clang

Author: Malavika Samak (malavikasamak)

Changes

Do not warn when a string literal is indexed and the idex value is within the bounds of the length of the string.

(rdar://139106996)

---

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

2 Files Affected:

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

diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index 2c68409b846bc8..650d51bebd66f7 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -436,21 +436,31 @@ AST_MATCHER(ArraySubscriptExpr, isSafeArraySubscript) {
 
   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)
+  const auto *SLiteral = dyn_cast<StringLiteral>(Node.getBase()->IgnoreParenImpCasts());
+  uint64_t size;
+
+  if (!BaseDRE && !SLiteral)
     return false;
 
+  if(BaseDRE) {
+    if (!BaseDRE->getDecl())
+      return false;
+    const auto *CATy = Finder->getASTContext().getAsConstantArrayType(
+        BaseDRE->getDecl()->getType());
+    if (!CATy) {
+      return false;
+    }
+    size = CATy->getLimitedSize();
+  } else if(SLiteral) {
+    size = SLiteral->getLength();
+  }
+
   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() < CATy->getLimitedSize())
+        ArrIdx.getLimitedValue() < size)
       return true;
   }
 
@@ -1142,7 +1152,7 @@ class ArraySubscriptGadget : public WarningGadget {
     // clang-format off
       return stmt(arraySubscriptExpr(
             hasBase(ignoringParenImpCasts(
-              anyOf(hasPointerType(), hasArrayType()))),
+              anyOf(hasPointerType(), hasArrayType(), stringLiteral()))),
             unless(anyOf(
               isSafeArraySubscript(),
               hasIndex(
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp
index 8b2f103ec66708..0a443543d3f604 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp
@@ -38,3 +38,10 @@ void constant_idx_unsafe(unsigned idx) {
                         // 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}}
 }
+
+void constant_id_string() {
+  char safe_char = "abc"[1]; // no-warning
+  char abcd[5] = "abc";
+  abcd[2]; // no-warning
+  safe_char = "abc"[3]; //expected-warning{{unsafe buffer access}}
+}

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[jkorous-apple]

This shouldn't be a warning either.

[malavikasamak]

In this case, I think we should warn, as the length here including the null terminator is 1.

[jkorous-apple]

Sorry, my bad. You're right

[ziqingluo-90]

LGTM! Thank you @malavikasamak

[jkorous-apple]

LGTM
Thank you!