[clang] [alpha.webkit.UncountedCallArgsChecker] Emit a warning for a WeakPtr argument. (PR #184563)

[via cfe-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang

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

Author: Ryosuke Niwa (rniwa)

<details>
<summary>Changes</summary>

This PR fixes a bug in UncountedCallArgsChecker that it would not emit a warning when a function is called with a WeakPtr local variable as an argument.

We normally don't generate a warning for a local variable passed to a function argument in UncountedCallArgsChecker as the variable may have a guardian in an outer scope but only UncountedLocalVarsChecker is capable of locating one. So rather than generating a warning in UncountedCallArgsChecker directly, we rely on UncountedLocalVarsChecker to generate a warning for the local variable.

This all falls apart in the case of a WeakPtr local variable because a WeakPtr is explicitly allowed as a local variable by UncountedLocalVarsChecker.

So, this PR fixes the bug by detecting this exact scenario (a WeakPtr local variable used as a function argument), and generate a warning directly in UncountedCallArgsChecker.

Also added the support for recognizing more weak smart pointer types in WebKit.

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


5 Files Affected:

- (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp (+14-4) 
- (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp (+15-2) 
- (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h (+5-1) 
- (modified) clang/test/Analysis/Checkers/WebKit/call-args.cpp (+18) 
- (modified) clang/test/Analysis/Checkers/WebKit/mock-types.h (+3) 


``````````diff
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
index 4a9a3be2b6614..f2ee9c742f4b8 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
@@ -239,10 +239,19 @@ bool tryToFindPtrOrigin(
 
 bool isASafeCallArg(const Expr *E) {
   assert(E);
+  auto IsCheckedLocalVarOrParam = [](const VarDecl *Decl) {
+      if (auto *Type = Decl->getType().getTypePtrOrNull()) {
+        if (auto *CXXRD = Type->getAsCXXRecordDecl()) {
+          if (isWeakPtr(CXXRD))
+            return false;
+        }
+      }
+      return Decl->isLocalVarDeclOrParm();
+  };
   if (auto *Ref = dyn_cast<DeclRefExpr>(E)) {
     auto *FoundDecl = Ref->getFoundDecl();
     if (auto *D = dyn_cast_or_null<VarDecl>(FoundDecl)) {
-      if (isa<ParmVarDecl>(D) || D->isLocalVarDecl())
+      if (IsCheckedLocalVarOrParam(D))
         return true;
       if (auto *ImplicitP = dyn_cast<ImplicitParamDecl>(D)) {
         auto Kind = ImplicitP->getParameterKind();
@@ -253,9 +262,10 @@ bool isASafeCallArg(const Expr *E) {
           return true;
       }
     } else if (auto *BD = dyn_cast_or_null<BindingDecl>(FoundDecl)) {
-      VarDecl *VD = BD->getHoldingVar();
-      if (VD && (isa<ParmVarDecl>(VD) || VD->isLocalVarDecl()))
-        return true;
+      if (VarDecl *VD = BD->getHoldingVar()) {
+        if (IsCheckedLocalVarOrParam(VD))
+          return true;
+      }
     }
   }
   if (isa<CXXTemporaryObjectExpr>(E))
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
index 7a7e22ead74a8..a95bf6faaab52 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
@@ -143,11 +143,17 @@ bool isOwnerPtr(const std::string &Name) {
          Name == "UniqueRef" || Name == "LazyUniqueRef";
 }
 
+static bool isWeakPtrClass(const std::string &Name) {
+  return Name == "WeakPtr" || Name == "SingleThreadPackedWeakPtr" ||
+         Name == "SingleThreadWeakPtr" || Name == "ThreadSafeWeakPtr" ||
+         Name == "ThreadSafeWeakOrStrongPtr" || Name == "InlineWeakPtr";
+}
+
 bool isSmartPtrClass(const std::string &Name) {
   return isRefType(Name) || isCheckedPtr(Name) || isRetainPtrOrOSPtr(Name) ||
-         Name == "WeakPtr" || Name == "WeakPtrFactory" ||
+         isWeakPtrClass(Name) || Name == "WeakPtrFactory" ||
          Name == "WeakPtrFactoryWithBitField" || Name == "WeakPtrImplBase" ||
-         Name == "WeakPtrImplBaseSingleThread" || Name == "ThreadSafeWeakPtr" ||
+         Name == "WeakPtrImplBaseSingleThread" ||
          Name == "ThreadSafeWeakOrStrongPtr" ||
          Name == "ThreadSafeWeakPtrControlBlock" ||
          Name == "ThreadSafeRefCountedAndCanMakeThreadSafeWeakPtr";
@@ -399,6 +405,13 @@ bool isRetainPtrOrOSPtr(const CXXRecordDecl *R) {
   return false;
 }
 
+bool isWeakPtr(const CXXRecordDecl *R) {
+  assert(R);
+  if (auto *TmplR = R->getTemplateInstantiationPattern())
+    return isWeakPtrClass(safeGetName(TmplR));
+  return false;
+}
+
 bool isSmartPtr(const CXXRecordDecl *R) {
   assert(R);
   if (auto *TmplR = R->getTemplateInstantiationPattern())
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
index bcb4eee16bd2c..d26a18c456c77 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
@@ -60,6 +60,10 @@ bool isCheckedPtr(const clang::CXXRecordDecl *Class);
 /// \returns true if \p Class is a RetainPtr, false if not.
 bool isRetainPtrOrOSPtr(const clang::CXXRecordDecl *Class);
 
+/// \returns true if \p Class is a weak smart pointer (WeakPtr, InlineWeakPtr,
+/// etc...), false if not.
+bool isWeakPtr(const clang::CXXRecordDecl *Class);
+
 /// \returns true if \p Class is a smart pointer (RefPtr, WeakPtr, etc...),
 /// false if not.
 bool isSmartPtr(const clang::CXXRecordDecl *Class);
@@ -139,7 +143,7 @@ bool isCheckedPtr(const std::string &Name);
 /// \returns true if \p Name is RetainPtr or its variant, false if not.
 bool isRetainPtrOrOSPtr(const std::string &Name);
 
-/// \returns true if \p Name is an owning smar pointer such as Ref, CheckedPtr,
+/// \returns true if \p Name is an owning smart pointer such as Ref, CheckedPtr,
 /// and unique_ptr.
 bool isOwnerPtr(const std::string &Name);
 
diff --git a/clang/test/Analysis/Checkers/WebKit/call-args.cpp b/clang/test/Analysis/Checkers/WebKit/call-args.cpp
index 1a8bde29080ac..c74e7ab7b9d7e 100644
--- a/clang/test/Analysis/Checkers/WebKit/call-args.cpp
+++ b/clang/test/Analysis/Checkers/WebKit/call-args.cpp
@@ -440,3 +440,21 @@ namespace call_with_adopt_ref {
     adoptRef(new Obj)->method();
   }
 }
+
+namespace call_with_weak_ptr {
+
+  class RefCountableWithWeakPtr : public RefCountable, public CanMakeWeakPtr<RefCountableWithWeakPtr> {
+  };
+
+  RefCountableWithWeakPtr* provide();
+  void consume(RefCountableWithWeakPtr*);
+
+  void foo() {
+    WeakPtr weakPtr = provide();
+    consume(weakPtr);
+    // expected-warning at -1{{Call argument is uncounted and unsafe}}
+    weakPtr->method();
+    // expected-warning at -1{{Call argument for 'this' parameter is uncounted and unsafe}}
+  }
+
+}
diff --git a/clang/test/Analysis/Checkers/WebKit/mock-types.h b/clang/test/Analysis/Checkers/WebKit/mock-types.h
index c139a5cb13de7..a5d9ed5ae174d 100644
--- a/clang/test/Analysis/Checkers/WebKit/mock-types.h
+++ b/clang/test/Analysis/Checkers/WebKit/mock-types.h
@@ -415,6 +415,9 @@ class WeakPtr {
     return *this;
   }
 
+  T* operator->() { return get(); }
+  operator T*() { return get(); }
+
   T* get() {
     return impl ? impl->get<T>() : nullptr;
   }

``````````

</details>


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