[analyzer] Support determining origins in a conditional operator in WebKit checkers. (#91143)

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.

Author: rniwa

clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp

@@ -16,8 +16,9 @@
 
 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();
@@ -31,12 +32,18 @@ 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
@@ -51,7 +58,7 @@ tryToFindPtrOrigin(const Expr *E, bool StopAtFirstRefCountedObj) {
           if (IsGetterOfRefCt && *IsGetterOfRefCt) {
             E = memberCall->getImplicitObjectArgument();
             if (StopAtFirstRefCountedObj) {
-              return {E, true};
+              return callback(E, true);
             }
             continue;
           }
@@ -68,17 +75,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);
@@ -95,7 +102,7 @@ tryToFindPtrOrigin(const Expr *E, bool StopAtFirstRefCountedObj) {
     break;
   }
   // Some other expression.
-  return {E, false};
+  return callback(E, false);
 }
 
 bool isASafeCallArg(const Expr *E) {

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,12 @@ 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

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 {

clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp

@@ -188,39 +188,50 @@ 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)
+      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;
+                  }
+                }
+
+                return false;
+              }))
         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;
-            }
-          }
-
-          // Parameters are guaranteed to be safe for the duration of the call
-          // by another checker.
-          if (isa<ParmVarDecl>(MaybeGuardian))
-            return;
-        }
-      }
-
       reportBug(V);
     }
   }

clang/test/Analysis/Checkers/WebKit/call-args.cpp

@@ -344,3 +344,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}}
+  }
+}

clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp

@@ -198,3 +198,21 @@ void system_header() {
 }
 
 } // ignore_system_headers
+
+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