[clang] [alpha.webkit.UncountedCallArgsChecker] Check arguments of function pointers (PR #188162)

[via cfe-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-clang

Author: Ryosuke Niwa (rniwa)

<details>
<summary>Changes</summary>

This PR fixes a hole in WebKit's static analysis that we weren't checking the soundness of argumnets to a function call via a (member) function pointer.

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


2 Files Affected:

- (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp (+58-48) 
- (modified) clang/test/Analysis/Checkers/WebKit/call-args.cpp (+23) 


``````````diff
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp
index b2d87bc267700..ccbec68f9be92 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp
@@ -113,62 +113,35 @@ class RawPtrRefCallArgsChecker
       unsigned ArgIdx =
           isa<CXXOperatorCallExpr>(CE) && isa_and_nonnull<CXXMethodDecl>(F);
 
-      if (auto *MemberCallExpr = dyn_cast<CXXMemberCallExpr>(CE)) {
-        if (auto *MD = MemberCallExpr->getMethodDecl()) {
-          auto name = safeGetName(MD);
-          if (name == "ref" || name == "deref")
-            return;
-          if (name == "incrementCheckedPtrCount" ||
-              name == "decrementCheckedPtrCount")
-            return;
-        }
-        auto *E = MemberCallExpr->getImplicitObjectArgument();
-        QualType ArgType = MemberCallExpr->getObjectType().getCanonicalType();
-        std::optional<bool> IsUnsafe = isUnsafeType(ArgType);
-        if (IsUnsafe && *IsUnsafe && !isPtrOriginSafe(E))
-          reportBugOnThis(E, D);
-      }
+      if (auto *MemberCallExpr = dyn_cast<CXXMemberCallExpr>(CE))
+        checkThisArg(MemberCallExpr, D);
 
       for (auto P = F->param_begin();
-           // FIXME: Also check variadic function parameters.
-           // FIXME: Also check default function arguments. Probably a different
-           // checker. In case there are default arguments the call can have
-           // fewer arguments than the callee has parameters.
            P < F->param_end() && ArgIdx < CE->getNumArgs(); ++P, ++ArgIdx) {
         // TODO: attributes.
         // if ((*P)->hasAttr<SafeRefCntblRawPtrAttr>())
         //  continue;
-
-        QualType ArgType = (*P)->getType();
-        // FIXME: more complex types (arrays, references to raw pointers, etc)
-        std::optional<bool> IsUncounted = isUnsafePtr(ArgType);
-        if (!IsUncounted || !(*IsUncounted))
-          continue;
-
-        const auto *Arg = CE->getArg(ArgIdx);
-
-        if (auto *defaultArg = dyn_cast<CXXDefaultArgExpr>(Arg))
-          Arg = defaultArg->getExpr();
-
-        if (isPtrOriginSafe(Arg))
-          continue;
-
-        reportBug(Arg, *P, D);
+        checkArg(CE->getArg(ArgIdx), (*P)->getType(), *P, D);
       }
       for (; ArgIdx < CE->getNumArgs(); ++ArgIdx) {
-        const auto *Arg = CE->getArg(ArgIdx);
-        auto ArgType = Arg->getType();
-        std::optional<bool> IsUncounted = isUnsafePtr(ArgType);
-        if (!IsUncounted || !(*IsUncounted))
-          continue;
-
-        if (auto *defaultArg = dyn_cast<CXXDefaultArgExpr>(Arg))
-          Arg = defaultArg->getExpr();
-
-        if (isPtrOriginSafe(Arg))
-          continue;
-
-        reportBug(Arg, nullptr, D);
+        auto *Arg = CE->getArg(ArgIdx);
+        checkArg(Arg, Arg->getType(), nullptr, D);
+      }
+    } else if (auto *Decl = CE->getCalleeDecl()) {
+      if (auto *FnType = Decl->getFunctionType()) {
+        if (auto *ProtoType = dyn_cast<FunctionProtoType>(FnType)) {
+          if (auto *MemberCallExpr = dyn_cast<CXXMemberCallExpr>(CE))
+            checkThisArg(MemberCallExpr, D);
+          unsigned ArgIdx = 0;
+          for (auto PT = ProtoType->param_type_begin();
+               PT < ProtoType->param_type_end() && ArgIdx < CE->getNumArgs();
+               ++PT, ++ArgIdx)
+            checkArg(CE->getArg(ArgIdx), *PT, nullptr, D);
+          for (; ArgIdx < CE->getNumArgs(); ++ArgIdx) {
+            auto *Arg = CE->getArg(ArgIdx);
+            checkArg(Arg, Arg->getType(), nullptr, D);
+          }
+        }
       }
     }
   }
@@ -205,6 +178,43 @@ class RawPtrRefCallArgsChecker
     }
   }
 
+  void checkThisArg(const CXXMemberCallExpr* MemberCallExpr,
+                    const Decl* DeclWithIssue) const {
+    if (auto *MD = MemberCallExpr->getMethodDecl()) {
+      auto name = safeGetName(MD);
+      if (name == "ref" || name == "deref")
+        return;
+      if (name == "incrementCheckedPtrCount" ||
+          name == "decrementCheckedPtrCount")
+        return;
+    }
+    auto *ThisExpr = MemberCallExpr->getImplicitObjectArgument();
+    QualType ArgType = MemberCallExpr->getObjectType().getCanonicalType();
+    std::optional<bool> IsUnsafe = isUnsafeType(ArgType);
+    if (!IsUnsafe || !*IsUnsafe)
+      return;
+
+    if (isPtrOriginSafe(ThisExpr))
+      return;
+
+    reportBugOnThis(MemberCallExpr, DeclWithIssue);
+  }
+
+  void checkArg(const Expr* Arg, QualType ParamType, const ParmVarDecl *Param,
+                const Decl* DeclWithIssue) const {
+    std::optional<bool> IsUncounted = isUnsafePtr(ParamType);
+    if (!IsUncounted || !(*IsUncounted))
+      return;
+
+    if (auto *DefaultArg = dyn_cast<CXXDefaultArgExpr>(Arg))
+      Arg = DefaultArg->getExpr();
+
+    if (isPtrOriginSafe(Arg))
+      return;
+
+    reportBug(Arg, Param, DeclWithIssue);
+  }
+
   bool isPtrOriginSafe(const Expr *Arg) const {
     return tryToFindPtrOrigin(
         Arg, /*StopAtFirstRefCountedObj=*/true,
diff --git a/clang/test/Analysis/Checkers/WebKit/call-args.cpp b/clang/test/Analysis/Checkers/WebKit/call-args.cpp
index 1a8bde29080ac..7f6554283b5fb 100644
--- a/clang/test/Analysis/Checkers/WebKit/call-args.cpp
+++ b/clang/test/Analysis/Checkers/WebKit/call-args.cpp
@@ -345,6 +345,29 @@ namespace cxx_member_operator_call {
   }
 }
 
+namespace call_function_ptr {
+  class RefCountableWithWeakPtr : public RefCountable, public CanMakeWeakPtr<RefCountableWithWeakPtr> {
+  public:
+    void method();
+  };
+
+  RefCountableWithWeakPtr* provide();
+
+  void foo(void (*consume)(void*, RefCountableWithWeakPtr*), void (*consumeVar)(RefCountableWithWeakPtr*, ...), void (RefCountableWithWeakPtr::*method)()) {
+    consume(nullptr, provide());
+    // expected-warning at -1{{Call argument is uncounted and unsafe}}
+    consumeVar(nullptr, provide());
+    // expected-warning at -1{{Call argument is uncounted and unsafe}}
+    (provide()->*method)();
+    // expected-warning at -1{{Call argument for 'this' parameter is uncounted and unsafe}}
+  }
+
+  template <typename T, typename U, typename... Arg>
+  void bar(T* obj, int (U::* function)(void*, Arg... args), Arg... args) {
+    (obj->*function)(args...);
+  }
+}
+
 namespace call_with_ptr_on_ref {
   Ref<RefCountable> provideProtected();
   void bar(RefCountable* bad);

``````````

</details>


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