[analyzer] Support determining origins in a conditional operator in WebKit checkers.

[rniwa]

This PR adds the support for determining the origin of a pointer in a conditional operator.

Because such an expression can have two distinct origins each of which needs to be visited, this PR refactors tryToFindPtrOrigin to take a callback instead of returning a pair.

The callback is called for the second operand and the third operand of the conditioanl operator (i.e. E2 and E3 in E1 ? E2 : E3).

Also treat nullptr and integer literal as safe pointer origins in the local variable checker.

[llvmbot]

@llvm/pr-subscribers-clang-static-analyzer-1

@llvm/pr-subscribers-clang

Author: Ryosuke Niwa (rniwa)

Changes

This PR adds the support for determining the origin of a pointer in a conditional operator.

Because such an expression can have two distinct origins each of which needs to be visited, this PR refactors tryToFindPtrOrigin to take a callback instead of returning a pair.

The callback is called for the second operand and the third operand of the conditioanl operator (i.e. E2 and E3 in E1 ? E2 : E3).

Also treat nullptr and integer literal as safe pointer origins in the local variable checker.

---

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

6 Files Affected:

• (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp (+13-8)
• (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h (+6-4)
• (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp (+17-19)
• (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp (+38-29)
• (modified) clang/test/Analysis/Checkers/WebKit/call-args.cpp (+14)
• (modified) clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp (+18)

diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
index b36fa95bc73f3e..f2e531837ac848 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
@@ -16,8 +16,8 @@
 
 namespace clang {
 
-std::pair<const Expr *, bool>
-tryToFindPtrOrigin(const Expr *E, bool StopAtFirstRefCountedObj) {
+bool tryToFindPtrOrigin(const Expr *E, bool StopAtFirstRefCountedObj,
+  std::function<bool(const clang::Expr *, bool)> callback) {
   while (E) {
     if (auto *tempExpr = dyn_cast<MaterializeTemporaryExpr>(E)) {
       E = tempExpr->getSubExpr();
@@ -27,12 +27,17 @@ tryToFindPtrOrigin(const Expr *E, bool StopAtFirstRefCountedObj) {
       E = tempExpr->getSubExpr();
       continue;
     }
+    if (auto* Expr = dyn_cast<ConditionalOperator>(E)) {
+      return tryToFindPtrOrigin(Expr->getTrueExpr(), StopAtFirstRefCountedObj,
+        callback) && tryToFindPtrOrigin(Expr->getFalseExpr(),
+        StopAtFirstRefCountedObj, callback);
+    }
     if (auto *cast = dyn_cast<CastExpr>(E)) {
       if (StopAtFirstRefCountedObj) {
         if (auto *ConversionFunc =
                 dyn_cast_or_null<FunctionDecl>(cast->getConversionFunction())) {
           if (isCtorOfRefCounted(ConversionFunc))
-            return {E, true};
+            return callback(E, true);
         }
       }
       // FIXME: This can give false "origin" that would lead to false negatives
@@ -47,7 +52,7 @@ tryToFindPtrOrigin(const Expr *E, bool StopAtFirstRefCountedObj) {
           if (IsGetterOfRefCt && *IsGetterOfRefCt) {
             E = memberCall->getImplicitObjectArgument();
             if (StopAtFirstRefCountedObj) {
-              return {E, true};
+              return callback(E, true);
             }
             continue;
           }
@@ -64,17 +69,17 @@ tryToFindPtrOrigin(const Expr *E, bool StopAtFirstRefCountedObj) {
       if (auto *callee = call->getDirectCallee()) {
         if (isCtorOfRefCounted(callee)) {
           if (StopAtFirstRefCountedObj)
-            return {E, true};
+            return callback(E, true);
 
           E = call->getArg(0);
           continue;
         }
 
         if (isReturnValueRefCounted(callee))
-          return {E, true};
+          return callback(E, true);
 
         if (isSingleton(callee))
-          return {E, true};
+          return callback(E, true);
 
         if (isPtrConversion(callee)) {
           E = call->getArg(0);
@@ -91,7 +96,7 @@ tryToFindPtrOrigin(const Expr *E, bool StopAtFirstRefCountedObj) {
     break;
   }
   // Some other expression.
-  return {E, false};
+  return callback(E, false);
 }
 
 bool isASafeCallArg(const Expr *E) {
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h
index e35ea4ef05dd17..cfcc046dc9d36c 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h
@@ -13,6 +13,7 @@
 #include "llvm/ADT/APInt.h"
 #include "llvm/Support/Casting.h"
 
+#include <functional>
 #include <string>
 #include <utility>
 
@@ -48,10 +49,11 @@ class Expr;
 /// represents ref-counted object during the traversal we return relevant
 /// sub-expression and true.
 ///
-/// \returns subexpression that we traversed to and if \p
-/// StopAtFirstRefCountedObj is true we also return whether we stopped early.
-std::pair<const clang::Expr *, bool>
-tryToFindPtrOrigin(const clang::Expr *E, bool StopAtFirstRefCountedObj);
+/// Calls \p callback with the subexpression that we traversed to and if \p
+/// StopAtFirstRefCountedObj is true we also specify whether we stopped early.
+/// Returns false if any of calls to callbacks returned false. Otherwise true.
+bool tryToFindPtrOrigin(const clang::Expr *E, bool StopAtFirstRefCountedObj,
+  std::function<bool(const clang::Expr *, bool)> callback);
 
 /// For \p E referring to a ref-countable/-counted pointer/reference we return
 /// whether it's a safe call argument. Examples: function parameter or
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp
index 0f40ecc7ba3000..cf98e2d2133ab8 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp
@@ -126,25 +126,23 @@ class UncountedCallArgsChecker
   }
 
   bool isPtrOriginSafe(const Expr *Arg) const {
-    std::pair<const clang::Expr *, bool> ArgOrigin =
-        tryToFindPtrOrigin(Arg, true);
-
-    // Temporary ref-counted object created as part of the call argument
-    // would outlive the call.
-    if (ArgOrigin.second)
-      return true;
-
-    if (isa<CXXNullPtrLiteralExpr>(ArgOrigin.first)) {
-      // foo(nullptr)
-      return true;
-    }
-    if (isa<IntegerLiteral>(ArgOrigin.first)) {
-      // FIXME: Check the value.
-      // foo(NULL)
-      return true;
-    }
-
-    return isASafeCallArg(ArgOrigin.first);
+    return tryToFindPtrOrigin(Arg, /*StopAtFirstRefCountedObj=*/true,
+        [](const clang::Expr* ArgOrigin, bool IsSafe) {
+      if (IsSafe)
+        return true;
+      if (isa<CXXNullPtrLiteralExpr>(ArgOrigin)) {
+        // foo(nullptr)
+        return true;
+      }
+      if (isa<IntegerLiteral>(ArgOrigin)) {
+        // FIXME: Check the value.
+        // foo(NULL)
+        return true;
+      }
+      if (isASafeCallArg(ArgOrigin))
+        return true;
+      return false;
+    });
   }
 
   bool shouldSkipCall(const CallExpr *CE) const {
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp
index 6036ad58cf253c..1d1dfa12e96df6 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp
@@ -188,38 +188,47 @@ class UncountedLocalVarsChecker
       if (!InitExpr)
         return; // FIXME: later on we might warn on uninitialized vars too
 
-      const clang::Expr *const InitArgOrigin =
-          tryToFindPtrOrigin(InitExpr, /*StopAtFirstRefCountedObj=*/false)
-              .first;
-      if (!InitArgOrigin)
-        return;
-
-      if (isa<CXXThisExpr>(InitArgOrigin))
-        return;
-
-      if (auto *Ref = llvm::dyn_cast<DeclRefExpr>(InitArgOrigin)) {
-        if (auto *MaybeGuardian =
-                dyn_cast_or_null<VarDecl>(Ref->getFoundDecl())) {
-          const auto *MaybeGuardianArgType =
-              MaybeGuardian->getType().getTypePtr();
-          if (MaybeGuardianArgType) {
-            const CXXRecordDecl *const MaybeGuardianArgCXXRecord =
-                MaybeGuardianArgType->getAsCXXRecordDecl();
-            if (MaybeGuardianArgCXXRecord) {
-              if (MaybeGuardian->isLocalVarDecl() &&
-                  (isRefCounted(MaybeGuardianArgCXXRecord) ||
-                   isRefcountedStringsHack(MaybeGuardian)) &&
-                  isGuardedScopeEmbeddedInGuardianScope(V, MaybeGuardian))
-                return;
+      if (tryToFindPtrOrigin(InitExpr, /*StopAtFirstRefCountedObj=*/false,
+        [&](const clang::Expr* InitArgOrigin, bool IsSafe) {
+          if (!InitArgOrigin)
+            return true;
+
+          if (isa<CXXThisExpr>(InitArgOrigin))
+            return true;
+
+          if (isa<CXXNullPtrLiteralExpr>(InitArgOrigin))
+            return true;
+
+          if (isa<IntegerLiteral>(InitArgOrigin))
+            return true;
+
+          if (auto *Ref = llvm::dyn_cast<DeclRefExpr>(InitArgOrigin)) {
+            if (auto *MaybeGuardian =
+                    dyn_cast_or_null<VarDecl>(Ref->getFoundDecl())) {
+              const auto *MaybeGuardianArgType =
+                  MaybeGuardian->getType().getTypePtr();
+              if (MaybeGuardianArgType) {
+                const CXXRecordDecl *const MaybeGuardianArgCXXRecord =
+                    MaybeGuardianArgType->getAsCXXRecordDecl();
+                if (MaybeGuardianArgCXXRecord) {
+                  if (MaybeGuardian->isLocalVarDecl() &&
+                      (isRefCounted(MaybeGuardianArgCXXRecord) ||
+                       isRefcountedStringsHack(MaybeGuardian)) &&
+                      isGuardedScopeEmbeddedInGuardianScope(V, MaybeGuardian))
+                    return true;
+                }
+              }
+
+              // Parameters are guaranteed to be safe for the duration of the
+              // call by another checker.
+              if (isa<ParmVarDecl>(MaybeGuardian))
+                return true;
             }
           }
 
-          // Parameters are guaranteed to be safe for the duration of the call
-          // by another checker.
-          if (isa<ParmVarDecl>(MaybeGuardian))
-            return;
-        }
-      }
+          return false;
+      }))
+        return;
 
       reportBug(V);
     }
diff --git a/clang/test/Analysis/Checkers/WebKit/call-args.cpp b/clang/test/Analysis/Checkers/WebKit/call-args.cpp
index 2a4b6bb1f1063a..5d864a90f6da6b 100644
--- a/clang/test/Analysis/Checkers/WebKit/call-args.cpp
+++ b/clang/test/Analysis/Checkers/WebKit/call-args.cpp
@@ -333,3 +333,17 @@ namespace cxx_member_operator_call {
     // expected-warning@-1{{Call argument for parameter 'bad' is uncounted and unsafe}}
   }
 }
+
+namespace call_with_ptr_on_ref {
+  Ref<RefCountable> provideProtected();
+  void bar(RefCountable* bad);
+  bool baz();
+  void foo(bool v) {
+    bar(v ? nullptr : provideProtected().ptr());
+    bar(baz() ? provideProtected().ptr() : nullptr);
+    bar(v ? provide() : provideProtected().ptr());
+    // expected-warning@-1{{Call argument for parameter 'bad' is uncounted and unsafe}}
+    bar(v ? provideProtected().ptr() : provide());
+    // expected-warning@-1{{Call argument for parameter 'bad' is uncounted and unsafe}}
+  }
+}
diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp b/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp
index 00673e91f471ea..00cea6a3b58945 100644
--- a/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp
+++ b/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp
@@ -187,3 +187,21 @@ void bar() {
 }
 
 } // namespace ignore_for_if
+
+namespace conditional_op {
+RefCountable *provide_ref_ctnbl();
+bool bar();
+
+void foo() {
+  RefCountable *a = bar() ? nullptr : provide_ref_ctnbl();
+  // expected-warning@-1{{Local variable 'a' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
+  RefPtr<RefCountable> b = provide_ref_ctnbl();
+  {
+    RefCountable* c = bar() ? nullptr : b.get();
+    c->method();
+    RefCountable* d = bar() ? b.get() : nullptr;
+    d->method();
+  }
+}
+
+} // namespace conditional_op
\ No newline at end of file

[github-actions]

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

[haoNoQ]

Hmm does this code play well with StopAtFirstRefCountedObj? Am I reading this right that both branches need to originate from a smart pointer, so we should never really stop in the middle between the branches?

[rniwa]

So the model is that we'd recurse for each expression. We'd stop at the first appearance of Ref/RefPtr if StopAtFirstRefCountedObj is set. Whether that semantics is correct or not depends on the context in which this function is called / used.

[haoNoQ]

Aha makes sense then!

[haoNoQ]

Aha ok this aligns with my understanding. LGTM!

[haoNoQ]

Aha makes sense then!