[-Wunsafe-buffer-usage] Reduce false positives with constant arrays in libc warnings

[ziqingluo-90]

For snprintf(a, sizeof a, ...), the first two arguments form a safe pattern if a is a constant array. In such a case, this commit will suppress the warning.

(rdar://117182250)

This is a follow-up PR to #101583
This pattern will effectively remove a lot of false positives in our downstream projects.

[llvmbot]

@llvm/pr-subscribers-clang-analysis

@llvm/pr-subscribers-clang

Author: Ziqing Luo (ziqingluo-90)

Changes

For snprintf(a, sizeof a, ...), the first two arguments form a safe pattern if a is a constant array. In such a case, this commit will suppress the warning.

(rdar://117182250)

This pattern will effectively remove a lot of false positives in our downstream projects.

---

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

2 Files Affected:

• (modified) clang/lib/Analysis/UnsafeBufferUsage.cpp (+26-1)
• (modified) clang/test/SemaCXX/warn-unsafe-buffer-usage-libc-functions.cpp (+13-1)

diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index 21d4368151eb47..3451ba7cc34901 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -668,7 +668,7 @@ AST_MATCHER(FunctionDecl, isPredefinedUnsafeLibcFunc) {
 
   auto *II = Node.getIdentifier();
 
-  if (!II)
+ if (!II)
     return false;
 
   StringRef Name = LibcFunNamePrefixSuffixParser().matchName(
@@ -833,9 +833,16 @@ AST_MATCHER_P(CallExpr, hasUnsafePrintfStringArg,
 //
 // For the first two arguments: `ptr` and `size`, they are safe if in the
 // following patterns:
+//
+// Pattern 1:
 //    ptr := DRE.data();
 //    size:= DRE.size()/DRE.size_bytes()
 // And DRE is a hardened container or view.
+//
+// Pattern 2:
+//    ptr := Constant-Array-DRE;
+//    size:= any expression that has compile-time constant value equivalent to
+//           sizeof (Constant-Array-DRE)
 AST_MATCHER(CallExpr, hasUnsafeSnprintfBuffer) {
   const FunctionDecl *FD = Node.getDirectCallee();
 
@@ -856,6 +863,7 @@ AST_MATCHER(CallExpr, hasUnsafeSnprintfBuffer) {
       !Size->getType()->isIntegerType())
     return false; // not an snprintf call
 
+  // Pattern 1:
   static StringRef SizedObjs[] = {"span", "array", "vector",
                                   "basic_string_view", "basic_string"};
   Buf = Buf->IgnoreParenImpCasts();
@@ -886,6 +894,23 @@ AST_MATCHER(CallExpr, hasUnsafeSnprintfBuffer) {
                   SizedObj)
             return false; // It is in fact safe
     }
+
+  // Pattern 2:
+  if (auto *DRE = dyn_cast<DeclRefExpr>(Buf->IgnoreParenImpCasts())) {
+    ASTContext &Ctx = Finder->getASTContext();
+
+    if (auto *CAT = Ctx.getAsConstantArrayType(DRE->getType())) {
+      Expr::EvalResult ER;
+      // The array element type must be compatible with `char` otherwise an
+      // explicit cast will be needed, which will make this check unreachable.
+      // Therefore, the array extent is same as its' bytewise size.
+      if (Size->EvaluateAsConstantExpr(ER, Ctx)) {
+        APSInt EVal = ER.Val.getInt(); // Size must have integer type
+
+        return APSInt::compareValues(EVal, APSInt(CAT->getSize(), true)) != 0;
+      }
+    }
+  }
   return true; // ptr and size are not in safe pattern
 }
 } // namespace libc_func_matchers
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-libc-functions.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-libc-functions.cpp
index a3716073609f6f..8e777a7a51c994 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-libc-functions.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-libc-functions.cpp
@@ -83,6 +83,12 @@ void f(char * p, char * q, std::span<char> s, std::span<char> s2) {
   sscanf(p, "%s%d", "hello", *p);    // expected-warning{{function 'sscanf' is unsafe}}
   sscanf_s(p, "%s%d", "hello", *p);  // expected-warning{{function 'sscanf_s' is unsafe}}
   fprintf((FILE*)p, "%P%d%p%i hello world %32s", *p, *p, p, *p, p); // expected-warning{{function 'fprintf' is unsafe}} expected-note{{string argument is not guaranteed to be null-terminated}}
+
+  char a[10], b[11];
+  int c[10];
+
+  snprintf(a, sizeof(b), "%s", __PRETTY_FUNCTION__);         // expected-warning{{function 'snprintf' is unsafe}} expected-note{{buffer pointer and size may not match}}
+  snprintf((char*)c, sizeof(c), "%s", __PRETTY_FUNCTION__);  // expected-warning{{function 'snprintf' is unsafe}} expected-note{{buffer pointer and size may not match}}
   fprintf((FILE*)p, "%P%d%p%i hello world %32s", *p, *p, p, *p, "hello"); // no warn
   printf("%s%d", "hello", *p); // no warn
   snprintf(s.data(), s.size_bytes(), "%s%d", "hello", *p); // no warn
@@ -93,13 +99,19 @@ void f(char * p, char * q, std::span<char> s, std::span<char> s2) {
   __asan_printf();// a printf but no argument, so no warn
 }
 
-void v(std::string s1, int *p) {
+void safe_examples(std::string s1, int *p) {
   snprintf(s1.data(), s1.size_bytes(), "%s%d%s%p%s", __PRETTY_FUNCTION__, *p, "hello", p, s1.c_str()); // no warn
   snprintf(s1.data(), s1.size_bytes(), s1.c_str(), __PRETTY_FUNCTION__, *p, "hello", s1.c_str());      // no warn
   printf("%s%d%s%p%s", __PRETTY_FUNCTION__, *p, "hello", p, s1.c_str());              // no warn
   printf(s1.c_str(), __PRETTY_FUNCTION__, *p, "hello", s1.c_str());                   // no warn
   fprintf((FILE*)0, "%s%d%s%p%s", __PRETTY_FUNCTION__, *p, "hello", p, s1.c_str());   // no warn
   fprintf((FILE*)0, s1.c_str(), __PRETTY_FUNCTION__, *p, "hello", s1.c_str());        // no warn
+
+  char a[10];
+
+  snprintf(a, sizeof a, "%s%d%s%p%s", __PRETTY_FUNCTION__, *p, "hello", s1.c_str());         // no warn
+  snprintf(a, sizeof(decltype(a)), "%s%d%s%p%s", __PRETTY_FUNCTION__, *p, "hello", s1.c_str());          // no warn
+  snprintf(a, 10, "%s%d%s%p%s", __PRETTY_FUNCTION__, *p, "hello", s1.c_str());                // no warn
 }
 
 

[github-actions]

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

[jkorous-apple]

LGTM