[alpha.webkit.UncountedCallArgsChecker] Use canonical type

[rniwa]

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.

[llvmbot]

@llvm/pr-subscribers-clang

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

Author: Ryosuke Niwa (rniwa)

Changes

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.

---

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

3 Files Affected:

• (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp (+1-1)
• (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp (+5-4)
• (added) clang/test/Analysis/Checkers/WebKit/uncounted-obj-const-v-muable.cpp (+27)

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@-1{{Call argument for 'this' parameter is uncounted and unsafe}}
+    m_obj->mutableFunc();
+    // expected-warning@-1{{Call argument for 'this' parameter is uncounted and unsafe}}
+}

[haoNoQ]

Ah classic! LGTM!

[haoNoQ]

getTypePtrOrNull() is unnecessary most of the time because you can do all the same operations on QualType directly, thanks to the overloaded QualType::operator->().

I think isUncountedPtr() should simply accept a QualType directly.

[rniwa]

Oh, I see. Let me make that change in a separate PR.

[rniwa]

#110213