[clang] [alpha.webkit.UncountedCallArgsChecker] Check the safety of the object argument in a member function call. (PR #81400)

[Ryosuke Niwa via cfe-commits]

https://github.com/rniwa updated https://github.com/llvm/llvm-project/pull/81400

>From c8ac9ad6ecd95a3dbd023458a572b08a4664de03 Mon Sep 17 00:00:00 2001
From: Ryosuke Niwa <rniwa at webkit.org>
Date: Sun, 11 Feb 2024 00:07:30 -0800
Subject: [PATCH] [alpha.webkit.UncountedCallArgsChecker] Check the safety of
 the object argument in a member function call.

This PR makes alpha.webkit.UncountedCallArgsChecker eplicitly check the safety of the object argument in
a member function call. It also removes the exemption of local variables from this checker so that each
local variable's safety is checked if it's used in a function call instead of relying on the local variable
checker to find those since local variable checker currently has exemption for "for" and "if" statements.
---
 .../Checkers/WebKit/ASTUtils.cpp              |  2 +-
 .../WebKit/UncountedCallArgsChecker.cpp       | 67 +++++++++++++------
 .../WebKit/call-args-inside-if-statement.cpp  | 20 ++++++
 3 files changed, 69 insertions(+), 20 deletions(-)
 create mode 100644 clang/test/Analysis/Checkers/WebKit/call-args-inside-if-statement.cpp

diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
index 4526fac64735bf..da0d52e361c946 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
@@ -85,7 +85,7 @@ bool isASafeCallArg(const Expr *E) {
   assert(E);
   if (auto *Ref = dyn_cast<DeclRefExpr>(E)) {
     if (auto *D = dyn_cast_or_null<VarDecl>(Ref->getFoundDecl())) {
-      if (isa<ParmVarDecl>(D) || D->isLocalVarDecl())
+      if (isa<ParmVarDecl>(D))
         return true;
     }
   }
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp
index 31ccae8b097b89..cda96b70ea8735 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp
@@ -70,6 +70,15 @@ class UncountedCallArgsChecker
       // or std::function call operator).
       unsigned ArgIdx = isa<CXXOperatorCallExpr>(CE) && isa_and_nonnull<CXXMethodDecl>(F);
 
+      if (auto *MemberCallExpr = dyn_cast<CXXMemberCallExpr>(CE)) {
+        auto *E = MemberCallExpr->getImplicitObjectArgument();
+        auto *ArgType = MemberCallExpr->getObjectType().getTypePtrOrNull();
+        std::optional<bool> IsUncounted =
+            isUncounted(ArgType->getAsCXXRecordDecl());
+        if (IsUncounted && *IsUncounted && !isPtrOriginSafe(E))
+          reportBugOnThis(E);
+      }
+
       for (auto P = F->param_begin();
            // FIXME: Also check variadic function parameters.
            // FIXME: Also check default function arguments. Probably a different
@@ -91,25 +100,7 @@ class UncountedCallArgsChecker
 
         const auto *Arg = CE->getArg(ArgIdx);
 
-        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)
-          continue;
-
-        if (isa<CXXNullPtrLiteralExpr>(ArgOrigin.first)) {
-          // foo(nullptr)
-          continue;
-        }
-        if (isa<IntegerLiteral>(ArgOrigin.first)) {
-          // FIXME: Check the value.
-          // foo(NULL)
-          continue;
-        }
-
-        if (isASafeCallArg(ArgOrigin.first))
+        if (isPtrOriginSafe(Arg))
           continue;
 
         reportBug(Arg, *P);
@@ -117,6 +108,28 @@ 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);
+  }
+
   bool shouldSkipCall(const CallExpr *CE) const {
     if (CE->getNumArgs() == 0)
       return false;
@@ -183,6 +196,22 @@ class UncountedCallArgsChecker
     Report->addRange(CallArg->getSourceRange());
     BR->emitReport(std::move(Report));
   }
+
+  void reportBugOnThis(const Expr *CallArg) const {
+    assert(CallArg);
+
+    SmallString<100> Buf;
+    llvm::raw_svector_ostream Os(Buf);
+
+    Os << "Call argument for 'this' parameter is uncounted and unsafe.";
+
+    const SourceLocation SrcLocToReport = CallArg->getSourceRange().getBegin();
+
+    PathDiagnosticLocation BSLoc(SrcLocToReport, BR->getSourceManager());
+    auto Report = std::make_unique<BasicBugReport>(Bug, Os.str(), BSLoc);
+    Report->addRange(CallArg->getSourceRange());
+    BR->emitReport(std::move(Report));
+  }
 };
 } // namespace
 
diff --git a/clang/test/Analysis/Checkers/WebKit/call-args-inside-if-statement.cpp b/clang/test/Analysis/Checkers/WebKit/call-args-inside-if-statement.cpp
new file mode 100644
index 00000000000000..6f7c959b2fccca
--- /dev/null
+++ b/clang/test/Analysis/Checkers/WebKit/call-args-inside-if-statement.cpp
@@ -0,0 +1,20 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.webkit.UncountedCallArgsChecker -verify %s
+
+#include "mock-types.h"
+
+class RefCounted {
+public:
+  void ref() const;
+  void deref() const;
+  void someFunction();
+};
+
+RefCounted* refCountedObj();
+
+void test()
+{
+  if (auto* obj = refCountedObj()) {
+    obj->someFunction();
+     // expected-warning at -1{{Call argument for 'this' parameter is uncounted and unsafe}}
+  }
+}