[clang] [alpha.webkit.UncountedCallArgsChecker] Use canonical type (PR #109393)

[Ryosuke Niwa via cfe-commits]

https://github.com/rniwa created https://github.com/llvm/llvm-project/pull/109393

This PR fixes a bug in UncountedCallArgsChecker that calling a function with a member variable which is Ref/RefPtr is erroneously treated as safe by canoniclizing the type before checking whether it's ref counted or not.

>From 7b74a32cd93f7812ec48b60ffcf379a91c7bdc6c Mon Sep 17 00:00:00 2001
From: Ryosuke Niwa <rniwa at webkit.org>
Date: Fri, 20 Sep 2024 02:02:41 -0700
Subject: [PATCH] [alpha.webkit.UncountedCallArgsChecker] Use canonical type

This PR fixes a bug in UncountedCallArgsChecker that calling a function
with a member variable which is Ref/RefPtr is erroneously treated as safe
by canoniclizing the type before checking whether it's ref counted or not.
---
 .../Checkers/WebKit/PtrTypesSemantics.cpp     |  2 +-
 .../WebKit/UncountedCallArgsChecker.cpp       |  9 ++++---
 .../WebKit/uncounted-obj-const-v-muable.cpp   | 27 +++++++++++++++++++
 3 files changed, 33 insertions(+), 5 deletions(-)
 create mode 100644 clang/test/Analysis/Checkers/WebKit/uncounted-obj-const-v-muable.cpp

diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
index 9da3e54e454317..54c99c3c1b37f9 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
@@ -155,7 +155,7 @@ std::optional<bool> isUncounted(const QualType T) {
 std::optional<bool> isUncounted(const CXXRecordDecl* Class)
 {
   // Keep isRefCounted first as it's cheaper.
-  if (isRefCounted(Class))
+  if (!Class || isRefCounted(Class))
     return false;
 
   std::optional<bool> IsRefCountable = isRefCountable(Class);
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp
index 81c2434ce64775..31e9b3c4b9d412 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp
@@ -86,7 +86,7 @@ class UncountedCallArgsChecker
             return;
         }
         auto *E = MemberCallExpr->getImplicitObjectArgument();
-        QualType ArgType = MemberCallExpr->getObjectType();
+        QualType ArgType = MemberCallExpr->getObjectType().getCanonicalType();
         std::optional<bool> IsUncounted = isUncounted(ArgType);
         if (IsUncounted && *IsUncounted && !isPtrOriginSafe(E))
           reportBugOnThis(E);
@@ -102,12 +102,13 @@ class UncountedCallArgsChecker
         // if ((*P)->hasAttr<SafeRefCntblRawPtrAttr>())
         //  continue;
 
-        const auto *ArgType = (*P)->getType().getTypePtrOrNull();
-        if (!ArgType)
+        QualType ArgType = (*P)->getType().getCanonicalType();
+        const auto *TypePtr = ArgType.getTypePtrOrNull();
+        if (!TypePtr)
           continue; // FIXME? Should we bail?
 
         // FIXME: more complex types (arrays, references to raw pointers, etc)
-        std::optional<bool> IsUncounted = isUncountedPtr(ArgType);
+        std::optional<bool> IsUncounted = isUncountedPtr(TypePtr);
         if (!IsUncounted || !(*IsUncounted))
           continue;
 
diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-obj-const-v-muable.cpp b/clang/test/Analysis/Checkers/WebKit/uncounted-obj-const-v-muable.cpp
new file mode 100644
index 00000000000000..2721cd8474e1b4
--- /dev/null
+++ b/clang/test/Analysis/Checkers/WebKit/uncounted-obj-const-v-muable.cpp
@@ -0,0 +1,27 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.webkit.UncountedCallArgsChecker -verify %s
+
+#include "mock-types.h"
+
+class Object {
+public:
+    void ref() const;
+    void deref() const;
+
+    bool constFunc() const;
+    void mutableFunc();
+};
+
+class Caller {
+  void someFunction();
+  void otherFunction();
+private:
+    RefPtr<Object> m_obj;
+};
+
+void Caller::someFunction()
+{
+    m_obj->constFunc();
+    // expected-warning at -1{{Call argument for 'this' parameter is uncounted and unsafe}}
+    m_obj->mutableFunc();
+    // expected-warning at -1{{Call argument for 'this' parameter is uncounted and unsafe}}
+}