[clang] [-Wunsafe-buffer-usage] Check isValueDependent before EvaluateAsBooleanCondition (PR #172091)

[via cfe-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-clang-analysis

Author: Ziqing Luo (ziqingluo-90)

<details>
<summary>Changes</summary>

The function `EvaluateAsBooleanCondition` assumes (asserts) that the input `Expr` is not in a dependent context. So it is caller's responsibility to check the condition before the call.  This commit fixes the issue in `UnsafeBufferUsage.cpp`.

The issue was first found downstream because of some code not upstreamed.  This commit also includes those un-upstreamed code.

rdar://166217941

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


3 Files Affected:

- (modified) clang/lib/Analysis/UnsafeBufferUsage.cpp (+40-34) 
- (modified) clang/test/SemaCXX/warn-unsafe-buffer-usage-fold-conditional.cpp (+13) 
- (modified) clang/test/SemaCXX/warn-unsafe-buffer-usage-libc-functions.cpp (+2) 


``````````diff
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index da155d31d4a88..0b9e908517412 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -736,6 +736,42 @@ static bool isSafeArraySubscript(const ArraySubscriptExpr &Node,
   return false;
 }
 
+// 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;
+    const auto *Cond = CE->getCond();
+
+    if (!Cond->isValueDependent() &&
+        Cond->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, ASTContext &Ctx) {
+  Ptr = tryConstantFoldConditionalExpr(Ptr, Ctx);
+  if (isa<clang::StringLiteral>(Ptr->IgnoreParenImpCasts()))
+    return true;
+  if (isa<PredefinedExpr>(Ptr->IgnoreParenImpCasts()))
+    return true;
+  if (auto *MCE = dyn_cast<CXXMemberCallExpr>(Ptr->IgnoreParenImpCasts())) {
+    const CXXMethodDecl *MD = MCE->getMethodDecl();
+    const CXXRecordDecl *RD = MCE->getRecordDecl()->getCanonicalDecl();
+
+    if (MD && RD && RD->isInStdNamespace() && MD->getIdentifier())
+      if (MD->getName() == "c_str" && RD->getName() == "basic_string")
+        return true;
+  }
+  return false;
+}
+
 namespace libc_func_matchers {
 // Under `libc_func_matchers`, define a set of matchers that match unsafe
 // functions in libc and unsafe calls to them.
@@ -781,40 +817,6 @@ 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, ASTContext &Ctx) {
-  Ptr = tryConstantFoldConditionalExpr(Ptr, Ctx);
-  if (isa<clang::StringLiteral>(Ptr->IgnoreParenImpCasts()))
-    return true;
-  if (isa<PredefinedExpr>(Ptr->IgnoreParenImpCasts()))
-    return true;
-  if (auto *MCE = dyn_cast<CXXMemberCallExpr>(Ptr->IgnoreParenImpCasts())) {
-    const CXXMethodDecl *MD = MCE->getMethodDecl();
-    const CXXRecordDecl *RD = MCE->getRecordDecl()->getCanonicalDecl();
-
-    if (MD && RD && RD->isInStdNamespace() && MD->getIdentifier())
-      if (MD->getName() == "c_str" && RD->getName() == "basic_string")
-        return true;
-  }
-  return false;
-}
-
 // Return true iff at least one of following cases holds:
 //  1. Format string is a literal and there is an unsafe pointer argument
 //     corresponding to an `s` specifier;
@@ -2106,6 +2108,10 @@ class UnsafeLibcFunctionCallGadget : public WarningGadget {
     // function that is not in any namespace:
     if (!FD->isInStdNamespace() && !IsGlobalAndNotInAnyNamespace)
       return false;
+    // If the call has a sole null-terminated argument, e.g., strlen,
+    //  printf, atoi, we consider it safe:
+    if (CE->getNumArgs() == 1 && isNullTermPointer(CE->getArg(0), Ctx))
+      return false;
     auto isSingleStringLiteralArg = false;
     if (CE->getNumArgs() == 1) {
       isSingleStringLiteralArg =
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fold-conditional.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fold-conditional.cpp
index b4f30b533bc4b..ad914b4fea89c 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-fold-conditional.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-fold-conditional.cpp
@@ -29,3 +29,16 @@ void f(int x, int y) {
   return;
 }
 
+// Test that the analysis will not crash when a conditional expression
+// appears in dependent context:
+#ifdef __cplusplus
+struct Foo {
+  static void static_method(int);
+};
+void conditional_inside_dependent_context(void) {
+  auto lambda = [](auto result) { // opens a dependent context
+    Foo::static_method(result ? 1 : 2);
+  };
+  (void)lambda;
+}
+#endif // __cplusplus
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 765dcbcc07df5..b23b4acd0d1c7 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-libc-functions.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-libc-functions.cpp
@@ -156,6 +156,8 @@ void safe_examples(std::string s1, int *p) {
   snprintf(a, 10, "%s%d%s%p%s", __PRETTY_FUNCTION__, *p, "hello", s1.c_str());                // no warn
   snprintf(&c, 1, "%s%d%s%p%s", __PRETTY_FUNCTION__, *p, "hello", s1.c_str());                // no warn
   snprintf(nullptr, 0, "%s%d%s%p%s", __PRETTY_FUNCTION__, *p, "hello", s1.c_str());           // no warn
+
+  strlen(s1.c_str());
 }
 
 void test_sarg_precision(std::string Str, std::string_view Sv, std::wstring_view WSv,

``````````

</details>


https://github.com/llvm/llvm-project/pull/172091