[-Wunsafe-buffer-usage] Fold the expression "cond ? E1 : E2" when checking safe patterns, if "cond" is a constant

[ziqingluo-90]

In -Wunsafe-buffer-usage, many safe pattern checks can benefit from constant folding. This commit improves null-terminated pointer checks by folding conditional expressions.

rdar://159374822

[llvmbot]

@llvm/pr-subscribers-clang-analysis

@llvm/pr-subscribers-clang

Author: Ziqing Luo (ziqingluo-90)

Changes

In -Wunsafe-buffer-usage, many safe pattern checks can benefit from constant folding. This commit improves null-terminated pointer checks by folding conditional expressions.

rdar://159374822

---

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

2 Files Affected:

• (modified) clang/lib/Analysis/UnsafeBufferUsage.cpp (+21-5)
• (added) clang/test/SemaCXX/warn-unsafe-buffer-usage-fold-conditional.cpp (+33)

diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index f5a368636c43d..1203a27e7dae5 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -781,9 +781,25 @@ struct LibcFunNamePrefixSuffixParser {
   }
 };
 
+// Constant fold a conditional expression 'cond ? A : B' to
+// - 'A', if 'cond' has constant true value;
+// - 'B', if 'cond' has constant false value.
+static const Expr *tryConstantFoldConditionalExpr(const Expr *E,
+                                               const ASTContext &Ctx) {
+  // FIXME: more places can use this function
+  if (const auto *CE = dyn_cast<ConditionalOperator>(E)) {
+    bool CondEval;
+
+    if (CE->getCond()->EvaluateAsBooleanCondition(CondEval, Ctx))
+      return CondEval ? CE->getLHS() : CE->getRHS();
+  }
+  return E;
+}
+
 // A pointer type expression is known to be null-terminated, if it has the
 // form: E.c_str(), for any expression E of `std::string` type.
-static bool isNullTermPointer(const Expr *Ptr) {
+static bool isNullTermPointer(const Expr *Ptr, ASTContext &Ctx) {
+  Ptr = tryConstantFoldConditionalExpr(Ptr, Ctx);
   if (isa<clang::StringLiteral>(Ptr->IgnoreParenImpCasts()))
     return true;
   if (isa<PredefinedExpr>(Ptr->IgnoreParenImpCasts()))
@@ -874,7 +890,7 @@ static bool hasUnsafeFormatOrSArg(const CallExpr *Call, const Expr *&UnsafeArg,
 
       const Expr *Arg = Call->getArg(ArgIdx);
 
-      if (isNullTermPointer(Arg))
+      if (isNullTermPointer(Arg, Ctx))
         // If Arg is a null-terminated pointer, it is safe anyway.
         return true; // continue parsing
 
@@ -922,8 +938,8 @@ static bool hasUnsafeFormatOrSArg(const CallExpr *Call, const Expr *&UnsafeArg,
   // (including the format argument) is unsafe pointer.
   return llvm::any_of(
       llvm::make_range(Call->arg_begin() + FmtArgIdx, Call->arg_end()),
-      [&UnsafeArg](const Expr *Arg) -> bool {
-        if (Arg->getType()->isPointerType() && !isNullTermPointer(Arg)) {
+      [&UnsafeArg, &Ctx](const Expr *Arg) -> bool {
+        if (Arg->getType()->isPointerType() && !isNullTermPointer(Arg, Ctx)) {
           UnsafeArg = Arg;
           return true;
         }
@@ -1175,7 +1191,7 @@ static bool hasUnsafePrintfStringArg(const CallExpr &Node, ASTContext &Ctx,
   // We don't really recognize this "normal" printf, the only thing we
   // can do is to require all pointers to be null-terminated:
   for (const auto *Arg : Node.arguments())
-    if (Arg->getType()->isPointerType() && !isNullTermPointer(Arg)) {
+    if (Arg->getType()->isPointerType() && !isNullTermPointer(Arg, Ctx)) {
       Result.addNode(Tag, DynTypedNode::create(*Arg));
       return true;
     }
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fold-conditional.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fold-conditional.cpp
new file mode 100644
index 0000000000000..edb71c299708a
--- /dev/null
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fold-conditional.cpp
@@ -0,0 +1,33 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++20 -Wno-all -Wunsafe-buffer-usage -verify %s
+// RUN: %clang_cc1 -fsyntax-only -x c -Wno-all -Wunsafe-buffer-usage -verify %s
+// expected-no-diagnostics
+
+#include <ptrcheck.h>
+
+typedef struct {} FILE;
+int fprintf( FILE* stream, const char* format, ... );
+FILE * stderr;
+
+#define DEBUG_ASSERT_MESSAGE(name, assertion, label, message, file, line, value) \
+  fprintf(stderr, "AssertMacros: %s, %s file: %s, line: %d, value: %lld\n",      \
+          assertion, (message!=0) ? message : "", file, line, (long long) (value));
+
+
+#define Require(assertion, exceptionLabel)                              \
+  do                                                                    \
+    {                                                                   \
+      if ( __builtin_expect(!(assertion), 0) ) {                        \
+        DEBUG_ASSERT_MESSAGE(                                           \
+	  "DEBUG_ASSERT_COMPONENT_NAME_STRING",                         \
+	  #assertion, #exceptionLabel, 0, __FILE__, __LINE__,  0);      \
+	goto exceptionLabel;                                            \
+      }									\
+    } while ( 0 )
+
+
+void f(int x, int y) {
+  Require(x == y, L1);
+ L1:
+  return;
+}
+

[github-actions]

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

[steakhal]

To me the change looks good FWIW.

[ziqingluo-90]

@steakhal Thanks for the rapid review!

[github-actions]

🐧 Linux x64 Test Results

• 111277 tests passed
• 4414 tests skipped