[alpha.webkit.UncountedCallArgsChecker] Add support for Objective-C++ property access

[rniwa]

Treat a function call or property access via a Objective-C++ selector which returns a Ref/RefPtr as safe.

[llvmbot]

@llvm/pr-subscribers-clang

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

Author: Ryosuke Niwa (rniwa)

Changes

Treat a function call or property access via a Objective-C++ selector which returns a Ref/RefPtr as safe.

---

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

4 Files Affected:

• (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp (+14-1)
• (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp (+2-3)
• (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h (+2-2)
• (modified) clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.mm (+22)

diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
index be07cf51eefb3d..394cb26f03cf99 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
@@ -12,6 +12,7 @@
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/ExprCXX.h"
+#include "clang/AST/ExprObjC.h"
 #include <optional>
 
 namespace clang {
@@ -35,6 +36,12 @@ bool tryToFindPtrOrigin(
         break;
       }
     }
+    if (auto *POE = dyn_cast<PseudoObjectExpr>(E)) {
+      if (auto *RF = POE->getResultExpr()) {
+        E = RF;
+        continue;
+      }
+    }
     if (auto *tempExpr = dyn_cast<ParenExpr>(E)) {
       E = tempExpr->getSubExpr();
       continue;
@@ -88,7 +95,7 @@ bool tryToFindPtrOrigin(
           continue;
         }
 
-        if (isReturnValueRefCounted(callee))
+        if (isRefType(callee->getReturnType()))
           return callback(E, true);
 
         if (isSingleton(callee))
@@ -100,6 +107,12 @@ bool tryToFindPtrOrigin(
         }
       }
     }
+    if (auto *ObjCMsgExpr = dyn_cast<ObjCMessageExpr>(E)) {
+      if (auto *Method = ObjCMsgExpr->getMethodDecl()) {
+        if (isRefType(Method->getReturnType()))
+          return callback(E, true);
+      }
+    }
     if (auto *unaryOp = dyn_cast<UnaryOperator>(E)) {
       // FIXME: Currently accepts ANY unary operator. Is it OK?
       E = unaryOp->getSubExpr();
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
index f48b2fd9dca71b..9da3e54e454317 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
@@ -123,9 +123,8 @@ bool isCtorOfRefCounted(const clang::FunctionDecl *F) {
          || FunctionName == "Identifier";
 }
 
-bool isReturnValueRefCounted(const clang::FunctionDecl *F) {
-  assert(F);
-  QualType type = F->getReturnType();
+bool isRefType(const clang::QualType T) {
+  QualType type = T;
   while (!type.isNull()) {
     if (auto *elaboratedT = type->getAs<ElaboratedType>()) {
       type = elaboratedT->desugar();
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
index 2932e62ad06e4b..e2d0342bebd52c 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
@@ -62,8 +62,8 @@ bool isRefType(const std::string &Name);
 /// false if not.
 bool isCtorOfRefCounted(const clang::FunctionDecl *F);
 
-/// \returns true if \p F returns a ref-counted object, false if not.
-bool isReturnValueRefCounted(const clang::FunctionDecl *F);
+/// \returns true if \p T is RefPtr, Ref, or its variant, false if not.
+bool isRefType(const clang::QualType T);
 
 /// \returns true if \p M is getter of a ref-counted class, false if not.
 std::optional<bool> isGetterOfRefCounted(const clang::CXXMethodDecl* Method);
diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.mm b/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.mm
index db0c5b19eec5bb..1d81a8851ece0c 100644
--- a/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.mm
+++ b/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.mm
@@ -11,6 +11,7 @@ @interface Foo : NSObject
 
 - (void)execute;
 - (RefPtr<RefCountable>)_protectedRefCountable;
+- (Ref<RefCountable>)_protectedRefCountable2;
 @end
 
 @implementation Foo
@@ -23,4 +24,25 @@ - (void)execute {
   return _countable;
 }
 
+- (Ref<RefCountable>)_protectedRefCountable2 {
+  return *_countable;
+}
+
 @end
+
+class RefCountedObject {
+public:
+  void ref() const;
+  void deref() const;
+  Ref<RefCountedObject> copy() const;
+};
+
+@interface WrapperObj : NSObject
+
+- (Ref<RefCountedObject>)_protectedWebExtensionControllerConfiguration;
+
+@end
+
+static void foo(WrapperObj *configuration) {
+  configuration._protectedWebExtensionControllerConfiguration->copy();
+}

[haoNoQ]

Yeah sounds about right!

The property syntax is confusing, I don't have enough expertise to confirm that getResultExpr() is the right solution so I think we should keep trying and incrementally figuring out what's going on.

Setters probably look very different but also you probably don't need to support them anyway.

[haoNoQ]

I'm also somewhat terrified of returning C++ objects from ObjC methods by value, ever since I learned that when you call an ObjC method on a nil it returns a zero-initialized object without calling a constructor on it.

[rniwa]

I'm also somewhat terrified of returning C++ objects from ObjC methods by value, ever since I learned that when you call an ObjC method on a nil it returns a zero-initialized object without calling a constructor on it.

Yikes. I didn't know that!