[alpha.webkit.UnretainedLocalVarsChecker] Add a checker for local variables to NS and CF types. #127554

rniwa · 2025-02-18T02:25:22Z

This PR adds alpha.webkit.UnretainedLocalVarsChecker by generalizing RawPtrRefLocalVarsChecker. It checks local variables to NS or CF types are guarded with a RetainPtr or not. The new checker is effective for NS and CF types in Objective-C++ code without ARC, and it's effective for CF types in code with ARC.

…iables to NS and CF types. This PR adds alpha.webkit.UnretainedLocalVarsChecker by generalizing RawPtrRefLocalVarsChecker. It checks local variables to NS or CF types are guarded with a RetainPtr or not. The new checker is effective for NS and CF types in Objective-C++ code without ARC, and it's effective for CF types in code with ARC.

llvmbot · 2025-02-18T02:25:53Z

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

Author: Ryosuke Niwa (rniwa)

Changes

This PR adds alpha.webkit.UnretainedLocalVarsChecker by generalizing RawPtrRefLocalVarsChecker. It checks local variables to NS or CF types are guarded with a RetainPtr or not. The new checker is effective for NS and CF types in Objective-C++ code without ARC, and it's effective for CF types in code with ARC.

Patch is 35.63 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/127554.diff

12 Files Affected:

(modified) clang/docs/analyzer/checkers.rst (+6)
(modified) clang/include/clang/StaticAnalyzer/Checkers/Checkers.td (+4)
(modified) clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp (+5-3)
(modified) clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h (+2)
(modified) clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp (+76-9)
(modified) clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h (+12-2)
(modified) clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp (+24)
(modified) clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp (+57-5)
(modified) clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp (+2-2)
(added) clang/test/Analysis/Checkers/WebKit/objc-mock-types.h (+148)
(added) clang/test/Analysis/Checkers/WebKit/unretained-local-vars-arc.mm (+46)
(added) clang/test/Analysis/Checkers/WebKit/unretained-local-vars.mm (+360)

diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index 707067358fdfe..0a976866ea8de 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/clang/docs/analyzer/checkers.rst
@@ -3668,6 +3668,12 @@ Here are some examples of situations that we warn about as they *might* be poten
       RefCountable* uncounted = counted.get(); // warn
     }
 
+alpha.webkit.UnretainedLocalVarsChecker
+""""""""""""""""""""""""""""""""""""""
+The goal of this rule is to make sure that any NS or CF local variable is backed by a RetainPtr with lifetime that is strictly larger than the scope of the unretained local variable. To be on the safe side we require the scope of an unretained variable to be embedded in the scope of Retainptr object that backs it.
+
+The rules of when to use and not to use RetainPtr are same as alpha.webkit.UncountedCallArgsChecker for ref-counted objects.
+
 Debug Checkers
 ---------------
 
diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
index 9bf491eac1e0e..410f841630660 100644
--- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -1782,4 +1782,8 @@ def UncheckedLocalVarsChecker : Checker<"UncheckedLocalVarsChecker">,
   HelpText<"Check unchecked local variables.">,
   Documentation<HasDocumentation>;
 
+def UnretainedLocalVarsChecker : Checker<"UnretainedLocalVarsChecker">,
+  HelpText<"Check unretained local variables.">,
+  Documentation<HasDocumentation>;
+
 } // end alpha.webkit
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
index a12853d01819f..dc86c4fcc64b1 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
@@ -24,6 +24,8 @@ bool isSafePtr(clang::CXXRecordDecl *Decl) {
 
 bool tryToFindPtrOrigin(
     const Expr *E, bool StopAtFirstRefCountedObj,
+    std::function<bool(const clang::CXXRecordDecl *)> isSafePtr,
+    std::function<bool(const clang::QualType)> isSafePtrType,
     std::function<bool(const clang::Expr *, bool)> callback) {
   while (E) {
     if (auto *tempExpr = dyn_cast<MaterializeTemporaryExpr>(E)) {
@@ -53,9 +55,9 @@ bool tryToFindPtrOrigin(
     }
     if (auto *Expr = dyn_cast<ConditionalOperator>(E)) {
       return tryToFindPtrOrigin(Expr->getTrueExpr(), StopAtFirstRefCountedObj,
-                                callback) &&
+                                isSafePtr, isSafePtrType, callback) &&
              tryToFindPtrOrigin(Expr->getFalseExpr(), StopAtFirstRefCountedObj,
-                                callback);
+                                isSafePtr, isSafePtrType, callback);
     }
     if (auto *cast = dyn_cast<CastExpr>(E)) {
       if (StopAtFirstRefCountedObj) {
@@ -92,7 +94,7 @@ bool tryToFindPtrOrigin(
       }
 
       if (auto *callee = call->getDirectCallee()) {
-        if (isCtorOfRefCounted(callee) || isCtorOfCheckedPtr(callee)) {
+        if (isCtorOfSafePtr(callee)) {
           if (StopAtFirstRefCountedObj)
             return callback(E, true);
 
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h
index b508043d0f37f..e2cc7b976adfc 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h
@@ -54,6 +54,8 @@ class Expr;
 /// Returns false if any of calls to callbacks returned false. Otherwise true.
 bool tryToFindPtrOrigin(
     const clang::Expr *E, bool StopAtFirstRefCountedObj,
+    std::function<bool(const clang::CXXRecordDecl *)> isSafePtr,
+    std::function<bool(const clang::QualType)> isSafePtrType,
     std::function<bool(const clang::Expr *, bool)> callback);
 
 /// For \p E referring to a ref-countable/-counted pointer/reference we return
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
index 8340de9e5a7a9..6acd5215ae553 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
@@ -13,6 +13,7 @@
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/StmtVisitor.h"
+#include "clang/Analysis/DomainSpecific/CocoaConventions.h"
 #include <optional>
 
 using namespace clang;
@@ -117,6 +118,10 @@ bool isRefType(const std::string &Name) {
          Name == "RefPtr" || Name == "RefPtrAllowingPartiallyDestroyed";
 }
 
+bool isRetainPtr(const std::string &Name) {
+  return Name == "RetainPtr";
+}
+
 bool isCheckedPtr(const std::string &Name) {
   return Name == "CheckedPtr" || Name == "CheckedRef";
 }
@@ -140,8 +145,15 @@ bool isCtorOfCheckedPtr(const clang::FunctionDecl *F) {
   return isCheckedPtr(safeGetName(F));
 }
 
+bool isCtorOfRetainPtr(const clang::FunctionDecl *F) {
+  const std::string &FunctionName = safeGetName(F);
+  return FunctionName == "RetainPtr" || FunctionName == "adoptNS" ||
+         FunctionName == "adoptCF";
+}
+
 bool isCtorOfSafePtr(const clang::FunctionDecl *F) {
-  return isCtorOfRefCounted(F) || isCtorOfCheckedPtr(F);
+  return isCtorOfRefCounted(F) || isCtorOfCheckedPtr(F) ||
+         isCtorOfRetainPtr(F);
 }
 
 template <typename Predicate>
@@ -163,11 +175,16 @@ static bool isPtrOfType(const clang::QualType T, Predicate Pred) {
   return false;
 }
 
-bool isSafePtrType(const clang::QualType T) {
+bool isRefOrCheckedPtrType(const clang::QualType T) {
   return isPtrOfType(
       T, [](auto Name) { return isRefType(Name) || isCheckedPtr(Name); });
 }
 
+bool isRetainPtrType(const clang::QualType T) {
+  return isPtrOfType(
+      T, [](auto Name) { return Name == "RetainPtr"; });
+}
+
 bool isOwnerPtrType(const clang::QualType T) {
   return isPtrOfType(T, [](auto Name) {
     return isRefType(Name) || isCheckedPtr(Name) || Name == "unique_ptr" ||
@@ -195,6 +212,37 @@ std::optional<bool> isUnchecked(const QualType T) {
   return isUnchecked(T->getAsCXXRecordDecl());
 }
 
+std::optional<bool> isUnretained(const QualType T, bool IsARCEnabled) {
+  if (auto *Subst = dyn_cast<SubstTemplateTypeParmType>(T)) {
+    if (auto *Decl = Subst->getAssociatedDecl()) {
+      if (isRetainPtr(safeGetName(Decl)))
+        return false;
+    }
+  }
+  if ((ento::cocoa::isCocoaObjectRef(T) && !IsARCEnabled) ||
+      ento::coreFoundation::isCFObjectRef(T))
+    return true;
+
+  // RetainPtr strips typedef for CF*Ref. Manually check for struct __CF* types.
+  auto CanonicalType = T.getCanonicalType();
+  auto *Type = CanonicalType.getTypePtrOrNull();
+  if (!Type)
+    return false;
+  auto Pointee = Type->getPointeeType();
+  auto *PointeeType = Pointee.getTypePtrOrNull();
+  if (!PointeeType)
+    return false;
+  auto *Record = PointeeType->getAsStructureType();
+  if (!Record)
+    return false;
+  auto *Decl = Record->getDecl();
+  if (!Decl)
+    return false;
+  auto TypeName = Decl->getName();
+  return TypeName.starts_with("__CF") || TypeName.starts_with("__CG") ||
+         TypeName.starts_with("__CM");
+}
+
 std::optional<bool> isUncounted(const CXXRecordDecl* Class)
 {
   // Keep isRefCounted first as it's cheaper.
@@ -230,16 +278,20 @@ std::optional<bool> isUncheckedPtr(const QualType T) {
   return false;
 }
 
-std::optional<bool> isUnsafePtr(const QualType T) {
+std::optional<bool> isUnsafePtr(const QualType T, bool IsArcEnabled) {
   if (T->isPointerType() || T->isReferenceType()) {
     if (auto *CXXRD = T->getPointeeCXXRecordDecl()) {
       auto isUncountedPtr = isUncounted(CXXRD);
       auto isUncheckedPtr = isUnchecked(CXXRD);
-      if (isUncountedPtr && isUncheckedPtr)
-        return *isUncountedPtr || *isUncheckedPtr;
+      auto isUnretainedPtr = isUnretained(T, IsArcEnabled);
+      std::optional<bool> result;
       if (isUncountedPtr)
-        return *isUncountedPtr;
-      return isUncheckedPtr;
+        result = *isUncountedPtr;
+      if (isUncheckedPtr)
+        result = result ? *result || *isUncheckedPtr : *isUncheckedPtr;
+      if (isUnretainedPtr)
+        result = result ? *result || *isUnretainedPtr : *isUnretainedPtr;
+      return result;
     }
   }
   return false;
@@ -263,16 +315,24 @@ std::optional<bool> isGetterOfSafePtr(const CXXMethodDecl *M) {
          method == "impl"))
       return true;
 
+    if (className == "RetainPtr" && method == "get")
+      return true;
+
     // Ref<T> -> T conversion
     // FIXME: Currently allowing any Ref<T> -> whatever cast.
     if (isRefType(className)) {
       if (auto *maybeRefToRawOperator = dyn_cast<CXXConversionDecl>(M))
-        return isUnsafePtr(maybeRefToRawOperator->getConversionType());
+        return isUnsafePtr(maybeRefToRawOperator->getConversionType(), false);
     }
 
     if (isCheckedPtr(className)) {
       if (auto *maybeRefToRawOperator = dyn_cast<CXXConversionDecl>(M))
-        return isUnsafePtr(maybeRefToRawOperator->getConversionType());
+        return isUnsafePtr(maybeRefToRawOperator->getConversionType(), false);
+    }
+
+    if (className == "RetainPtr") {
+      if (auto *maybeRefToRawOperator = dyn_cast<CXXConversionDecl>(M))
+        return isUnsafePtr(maybeRefToRawOperator->getConversionType(), false);
     }
   }
   return false;
@@ -297,6 +357,13 @@ bool isCheckedPtr(const CXXRecordDecl *R) {
   return false;
 }
 
+bool isRetainPtr(const CXXRecordDecl *R) {
+  assert(R);
+  if (auto *TmplR = R->getTemplateInstantiationPattern())
+    return safeGetName(TmplR) == "RetainPtr";
+  return false;
+}
+
 bool isPtrConversion(const FunctionDecl *F) {
   assert(F);
   if (isCtorOfRefCounted(F))
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
index 9adfc0f1f5726..d220b42bbad6d 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
@@ -51,6 +51,9 @@ bool isRefCounted(const clang::CXXRecordDecl *Class);
 /// \returns true if \p Class is a CheckedPtr / CheckedRef, false if not.
 bool isCheckedPtr(const clang::CXXRecordDecl *Class);
 
+/// \returns true if \p Class is a RetainPtr, false if not.
+bool isRetainPtr(const clang::CXXRecordDecl *Class);
+
 /// \returns true if \p Class is ref-countable AND not ref-counted, false if
 /// not, std::nullopt if inconclusive.
 std::optional<bool> isUncounted(const clang::QualType T);
@@ -59,6 +62,10 @@ std::optional<bool> isUncounted(const clang::QualType T);
 /// not, std::nullopt if inconclusive.
 std::optional<bool> isUnchecked(const clang::QualType T);
 
+/// \returns true if \p Class is NS or CF objects AND not retained, false if
+/// not, std::nullopt if inconclusive.
+std::optional<bool> isUnretained(const clang::QualType T, bool IsARCEnabled);
+
 /// \returns true if \p Class is ref-countable AND not ref-counted, false if
 /// not, std::nullopt if inconclusive.
 std::optional<bool> isUncounted(const clang::CXXRecordDecl* Class);
@@ -77,11 +84,14 @@ std::optional<bool> isUncheckedPtr(const clang::QualType T);
 
 /// \returns true if \p T is either a raw pointer or reference to an uncounted
 /// or unchecked class, false if not, std::nullopt if inconclusive.
-std::optional<bool> isUnsafePtr(const QualType T);
+std::optional<bool> isUnsafePtr(const QualType T, bool IsArcEnabled);
 
 /// \returns true if \p T is a RefPtr, Ref, CheckedPtr, CheckedRef, or its
 /// variant, false if not.
-bool isSafePtrType(const clang::QualType T);
+bool isRefOrCheckedPtrType(const clang::QualType T);
+
+/// \returns true if \p T is a RetainPtr, false if not.
+bool isRetainPtrType(const clang::QualType T);
 
 /// \returns true if \p T is a RefPtr, Ref, CheckedPtr, CheckedRef, or
 /// unique_ptr, false if not.
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp
index 56fa72c100ec8..af236f404e2d5 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp
@@ -41,6 +41,8 @@ class RawPtrRefCallArgsChecker
 
   virtual std::optional<bool> isUnsafeType(QualType) const = 0;
   virtual std::optional<bool> isUnsafePtr(QualType) const = 0;
+  virtual bool isSafePtr(const CXXRecordDecl *Record) const = 0;
+  virtual bool isSafePtrType(const QualType type) const = 0;
   virtual const char *ptrKind() const = 0;
 
   void checkASTDecl(const TranslationUnitDecl *TUD, AnalysisManager &MGR,
@@ -141,6 +143,12 @@ class RawPtrRefCallArgsChecker
 
   bool isPtrOriginSafe(const Expr *Arg) const {
     return tryToFindPtrOrigin(Arg, /*StopAtFirstRefCountedObj=*/true,
+                              [&](const clang::CXXRecordDecl *Record) {
+                                return isSafePtr(Record);
+                              },
+                              [&](const clang::QualType T) {
+                                return isSafePtrType(T);
+                              },
                               [&](const clang::Expr *ArgOrigin, bool IsSafe) {
                                 if (IsSafe)
                                   return true;
@@ -315,6 +323,14 @@ class UncountedCallArgsChecker final : public RawPtrRefCallArgsChecker {
     return isUncountedPtr(QT);
   }
 
+  bool isSafePtr(const CXXRecordDecl *Record) const final {
+    return isRefCounted(Record) || isCheckedPtr(Record);
+  }
+
+  bool isSafePtrType(const QualType type) const final {
+    return isRefOrCheckedPtrType(type);
+  }
+
   const char *ptrKind() const final { return "uncounted"; }
 };
 
@@ -332,6 +348,14 @@ class UncheckedCallArgsChecker final : public RawPtrRefCallArgsChecker {
     return isUncheckedPtr(QT);
   }
 
+  bool isSafePtr(const CXXRecordDecl *Record) const final {
+    return isRefCounted(Record) || isCheckedPtr(Record);
+  }
+
+  bool isSafePtrType(const QualType type) const final {
+    return isRefOrCheckedPtrType(type);
+  }
+
   const char *ptrKind() const final { return "unchecked"; }
 };
 
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp
index d586668399502..f7eacdd6b54e0 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp
@@ -15,6 +15,7 @@
 #include "clang/AST/DynamicRecursiveASTVisitor.h"
 #include "clang/AST/ParentMapContext.h"
 #include "clang/Basic/SourceLocation.h"
+#include "clang/Analysis/DomainSpecific/CocoaConventions.h"
 #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
@@ -81,7 +82,7 @@ struct GuardianVisitor : DynamicRecursiveASTVisitor {
   bool VisitCXXMemberCallExpr(CXXMemberCallExpr *MCE) override {
     auto MethodName = safeGetName(MCE->getMethodDecl());
     if (MethodName == "swap" || MethodName == "leakRef" ||
-        MethodName == "releaseNonNull") {
+        MethodName == "releaseNonNull" || MethodName == "clear") {
       auto *ThisArg = MCE->getImplicitObjectArgument()->IgnoreParenCasts();
       if (auto *VarRef = dyn_cast<DeclRefExpr>(ThisArg)) {
         if (VarRef->getDecl() == Guardian)
@@ -173,10 +174,12 @@ class RawPtrRefLocalVarsChecker
       : Bug(this, description, "WebKit coding guidelines") {}
 
   virtual std::optional<bool> isUnsafePtr(const QualType T) const = 0;
+  virtual bool isSafePtr(const CXXRecordDecl *) const = 0;
+  virtual bool isSafePtrType(const QualType) const = 0;
   virtual const char *ptrKind() const = 0;
 
-  void checkASTDecl(const TranslationUnitDecl *TUD, AnalysisManager &MGR,
-                    BugReporter &BRArg) const {
+  virtual void checkASTDecl(const TranslationUnitDecl *TUD,
+                            AnalysisManager &MGR, BugReporter &BRArg) const {
     BR = &BRArg;
 
     // The calls to checkAST* from AnalysisConsumer don't
@@ -263,6 +266,12 @@ class RawPtrRefLocalVarsChecker
     if (IsUncountedPtr && *IsUncountedPtr) {
       if (tryToFindPtrOrigin(
               Value, /*StopAtFirstRefCountedObj=*/false,
+              [&](const clang::CXXRecordDecl *Record) {
+                return isSafePtr(Record);
+              },
+              [&](const clang::QualType Type) {
+                return isSafePtrType(Type);
+              },
               [&](const clang::Expr *InitArgOrigin, bool IsSafe) {
                 if (!InitArgOrigin || IsSafe)
                   return true;
@@ -292,8 +301,7 @@ class RawPtrRefLocalVarsChecker
                           MaybeGuardianArgType->getAsCXXRecordDecl();
                       if (MaybeGuardianArgCXXRecord) {
                         if (MaybeGuardian->isLocalVarDecl() &&
-                            (isRefCounted(MaybeGuardianArgCXXRecord) ||
-                             isCheckedPtr(MaybeGuardianArgCXXRecord) ||
+                            (isSafePtr(MaybeGuardianArgCXXRecord) ||
                              isRefcountedStringsHack(MaybeGuardian)) &&
                             isGuardedScopeEmbeddedInGuardianScope(
                                 V, MaybeGuardian))
@@ -365,6 +373,12 @@ class UncountedLocalVarsChecker final : public RawPtrRefLocalVarsChecker {
   std::optional<bool> isUnsafePtr(const QualType T) const final {
     return isUncountedPtr(T);
   }
+  bool isSafePtr(const CXXRecordDecl *Record) const final {
+    return isRefCounted(Record) || isCheckedPtr(Record);
+  }
+  bool isSafePtrType(const QualType type) const final {
+    return isRefOrCheckedPtrType(type);
+  }
   const char *ptrKind() const final { return "uncounted"; }
 };
 
@@ -376,9 +390,39 @@ class UncheckedLocalVarsChecker final : public RawPtrRefLocalVarsChecker {
   std::optional<bool> isUnsafePtr(const QualType T) const final {
     return isUncheckedPtr(T);
   }
+  bool isSafePtr(const CXXRecordDecl *Record) const final {
+    return isRefCounted(Record) || isCheckedPtr(Record);
+  }
+  bool isSafePtrType(const QualType type) const final {
+    return isRefOrCheckedPtrType(type);
+  }
   const char *ptrKind() const final { return "unchecked"; }
 };
 
+class UnretainedLocalVarsChecker final : public RawPtrRefLocalVarsChecker {
+  mutable bool IsARCEnabled{false};
+public:
+  UnretainedLocalVarsChecker()
+      : RawPtrRefLocalVarsChecker("Unretained raw pointer or reference not "
+                                  "provably backed by a RetainPtr") {}
+  std::optional<bool> isUnsafePtr(const QualType T) const final {
+    return isUnretained(T, IsARCEnabled);
+  }
+  bool isSafePtr(const CXXRecordDecl *Record) const final {
+    return isRetainPtr(Record);
+  }
+  bool isSafePtrType(const QualType type) const final {
+    return isRetainPtrType(type);
+  }
+  const char *ptrKind() const final { return "unretained"; }
+
+  void checkASTDecl(const TranslationUnitDecl *TUD,
+                    AnalysisManager &MGR, BugReporter &BRArg) const final {
+    IsARCEnabled = TUD->getLangOpts().ObjCAutoRefCount;
+    RawPtrRefLocalVarsChecker::checkASTDecl(TUD, MGR, BRArg);
+  }
+};
+
 } // namespace
 
 void ento::registerUncountedLocalVarsChecker(CheckerManager &Mgr) {
@@ -396,3 +440,11 @@ void ento::registerUncheckedLocalVarsChecker(CheckerManager &Mgr) {
 bool ento::shouldRegisterUncheckedLocalVarsChecker(const CheckerManager &) {
   return true;
 }
+
+void ento::registerUnretainedLocalVarsChecker(CheckerManager &Mgr) {
+  Mgr.registerChecker<UnretainedLocalVarsChecker>();
+}
+
+bool ento::shouldRegisterUnretainedLocalVarsChecker(const CheckerManager &) {
+  return true;
+}
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp
index 9527993d0edeb..fd710177e5426 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp
@@ -61,7 +61,7 @@ class UncountedLambdaCapturesChecker
       }
 
       bool shouldCheckThis() {
-        auto result = !ClsType.isNull() ? isUnsafePtr(ClsType) : std::nullopt;
+        auto result = !ClsType.isNull() ? isUnsafePtr(ClsType, false) : std::nullopt;
         r...
[truncated]

llvmbot · 2025-02-18T02:25:53Z

@llvm/pr-subscribers-clang

Author: Ryosuke Niwa (rniwa)

Changes

This PR adds alpha.webkit.UnretainedLocalVarsChecker by generalizing RawPtrRefLocalVarsChecker. It checks local variables to NS or CF types are guarded with a RetainPtr or not. The new checker is effective for NS and CF types in Objective-C++ code without ARC, and it's effective for CF types in code with ARC.

Patch is 35.63 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/127554.diff

12 Files Affected:

(modified) clang/docs/analyzer/checkers.rst (+6)
(modified) clang/include/clang/StaticAnalyzer/Checkers/Checkers.td (+4)
(modified) clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp (+5-3)
(modified) clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h (+2)
(modified) clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp (+76-9)
(modified) clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h (+12-2)
(modified) clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp (+24)
(modified) clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp (+57-5)
(modified) clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp (+2-2)
(added) clang/test/Analysis/Checkers/WebKit/objc-mock-types.h (+148)
(added) clang/test/Analysis/Checkers/WebKit/unretained-local-vars-arc.mm (+46)
(added) clang/test/Analysis/Checkers/WebKit/unretained-local-vars.mm (+360)

diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index 707067358fdfe..0a976866ea8de 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/clang/docs/analyzer/checkers.rst
@@ -3668,6 +3668,12 @@ Here are some examples of situations that we warn about as they *might* be poten
       RefCountable* uncounted = counted.get(); // warn
     }
 
+alpha.webkit.UnretainedLocalVarsChecker
+""""""""""""""""""""""""""""""""""""""
+The goal of this rule is to make sure that any NS or CF local variable is backed by a RetainPtr with lifetime that is strictly larger than the scope of the unretained local variable. To be on the safe side we require the scope of an unretained variable to be embedded in the scope of Retainptr object that backs it.
+
+The rules of when to use and not to use RetainPtr are same as alpha.webkit.UncountedCallArgsChecker for ref-counted objects.
+
 Debug Checkers
 ---------------
 
diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
index 9bf491eac1e0e..410f841630660 100644
--- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -1782,4 +1782,8 @@ def UncheckedLocalVarsChecker : Checker<"UncheckedLocalVarsChecker">,
   HelpText<"Check unchecked local variables.">,
   Documentation<HasDocumentation>;
 
+def UnretainedLocalVarsChecker : Checker<"UnretainedLocalVarsChecker">,
+  HelpText<"Check unretained local variables.">,
+  Documentation<HasDocumentation>;
+
 } // end alpha.webkit
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
index a12853d01819f..dc86c4fcc64b1 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
@@ -24,6 +24,8 @@ bool isSafePtr(clang::CXXRecordDecl *Decl) {
 
 bool tryToFindPtrOrigin(
     const Expr *E, bool StopAtFirstRefCountedObj,
+    std::function<bool(const clang::CXXRecordDecl *)> isSafePtr,
+    std::function<bool(const clang::QualType)> isSafePtrType,
     std::function<bool(const clang::Expr *, bool)> callback) {
   while (E) {
     if (auto *tempExpr = dyn_cast<MaterializeTemporaryExpr>(E)) {
@@ -53,9 +55,9 @@ bool tryToFindPtrOrigin(
     }
     if (auto *Expr = dyn_cast<ConditionalOperator>(E)) {
       return tryToFindPtrOrigin(Expr->getTrueExpr(), StopAtFirstRefCountedObj,
-                                callback) &&
+                                isSafePtr, isSafePtrType, callback) &&
              tryToFindPtrOrigin(Expr->getFalseExpr(), StopAtFirstRefCountedObj,
-                                callback);
+                                isSafePtr, isSafePtrType, callback);
     }
     if (auto *cast = dyn_cast<CastExpr>(E)) {
       if (StopAtFirstRefCountedObj) {
@@ -92,7 +94,7 @@ bool tryToFindPtrOrigin(
       }
 
       if (auto *callee = call->getDirectCallee()) {
-        if (isCtorOfRefCounted(callee) || isCtorOfCheckedPtr(callee)) {
+        if (isCtorOfSafePtr(callee)) {
           if (StopAtFirstRefCountedObj)
             return callback(E, true);
 
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h
index b508043d0f37f..e2cc7b976adfc 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h
@@ -54,6 +54,8 @@ class Expr;
 /// Returns false if any of calls to callbacks returned false. Otherwise true.
 bool tryToFindPtrOrigin(
     const clang::Expr *E, bool StopAtFirstRefCountedObj,
+    std::function<bool(const clang::CXXRecordDecl *)> isSafePtr,
+    std::function<bool(const clang::QualType)> isSafePtrType,
     std::function<bool(const clang::Expr *, bool)> callback);
 
 /// For \p E referring to a ref-countable/-counted pointer/reference we return
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
index 8340de9e5a7a9..6acd5215ae553 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
@@ -13,6 +13,7 @@
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/StmtVisitor.h"
+#include "clang/Analysis/DomainSpecific/CocoaConventions.h"
 #include <optional>
 
 using namespace clang;
@@ -117,6 +118,10 @@ bool isRefType(const std::string &Name) {
          Name == "RefPtr" || Name == "RefPtrAllowingPartiallyDestroyed";
 }
 
+bool isRetainPtr(const std::string &Name) {
+  return Name == "RetainPtr";
+}
+
 bool isCheckedPtr(const std::string &Name) {
   return Name == "CheckedPtr" || Name == "CheckedRef";
 }
@@ -140,8 +145,15 @@ bool isCtorOfCheckedPtr(const clang::FunctionDecl *F) {
   return isCheckedPtr(safeGetName(F));
 }
 
+bool isCtorOfRetainPtr(const clang::FunctionDecl *F) {
+  const std::string &FunctionName = safeGetName(F);
+  return FunctionName == "RetainPtr" || FunctionName == "adoptNS" ||
+         FunctionName == "adoptCF";
+}
+
 bool isCtorOfSafePtr(const clang::FunctionDecl *F) {
-  return isCtorOfRefCounted(F) || isCtorOfCheckedPtr(F);
+  return isCtorOfRefCounted(F) || isCtorOfCheckedPtr(F) ||
+         isCtorOfRetainPtr(F);
 }
 
 template <typename Predicate>
@@ -163,11 +175,16 @@ static bool isPtrOfType(const clang::QualType T, Predicate Pred) {
   return false;
 }
 
-bool isSafePtrType(const clang::QualType T) {
+bool isRefOrCheckedPtrType(const clang::QualType T) {
   return isPtrOfType(
       T, [](auto Name) { return isRefType(Name) || isCheckedPtr(Name); });
 }
 
+bool isRetainPtrType(const clang::QualType T) {
+  return isPtrOfType(
+      T, [](auto Name) { return Name == "RetainPtr"; });
+}
+
 bool isOwnerPtrType(const clang::QualType T) {
   return isPtrOfType(T, [](auto Name) {
     return isRefType(Name) || isCheckedPtr(Name) || Name == "unique_ptr" ||
@@ -195,6 +212,37 @@ std::optional<bool> isUnchecked(const QualType T) {
   return isUnchecked(T->getAsCXXRecordDecl());
 }
 
+std::optional<bool> isUnretained(const QualType T, bool IsARCEnabled) {
+  if (auto *Subst = dyn_cast<SubstTemplateTypeParmType>(T)) {
+    if (auto *Decl = Subst->getAssociatedDecl()) {
+      if (isRetainPtr(safeGetName(Decl)))
+        return false;
+    }
+  }
+  if ((ento::cocoa::isCocoaObjectRef(T) && !IsARCEnabled) ||
+      ento::coreFoundation::isCFObjectRef(T))
+    return true;
+
+  // RetainPtr strips typedef for CF*Ref. Manually check for struct __CF* types.
+  auto CanonicalType = T.getCanonicalType();
+  auto *Type = CanonicalType.getTypePtrOrNull();
+  if (!Type)
+    return false;
+  auto Pointee = Type->getPointeeType();
+  auto *PointeeType = Pointee.getTypePtrOrNull();
+  if (!PointeeType)
+    return false;
+  auto *Record = PointeeType->getAsStructureType();
+  if (!Record)
+    return false;
+  auto *Decl = Record->getDecl();
+  if (!Decl)
+    return false;
+  auto TypeName = Decl->getName();
+  return TypeName.starts_with("__CF") || TypeName.starts_with("__CG") ||
+         TypeName.starts_with("__CM");
+}
+
 std::optional<bool> isUncounted(const CXXRecordDecl* Class)
 {
   // Keep isRefCounted first as it's cheaper.
@@ -230,16 +278,20 @@ std::optional<bool> isUncheckedPtr(const QualType T) {
   return false;
 }
 
-std::optional<bool> isUnsafePtr(const QualType T) {
+std::optional<bool> isUnsafePtr(const QualType T, bool IsArcEnabled) {
   if (T->isPointerType() || T->isReferenceType()) {
     if (auto *CXXRD = T->getPointeeCXXRecordDecl()) {
       auto isUncountedPtr = isUncounted(CXXRD);
       auto isUncheckedPtr = isUnchecked(CXXRD);
-      if (isUncountedPtr && isUncheckedPtr)
-        return *isUncountedPtr || *isUncheckedPtr;
+      auto isUnretainedPtr = isUnretained(T, IsArcEnabled);
+      std::optional<bool> result;
       if (isUncountedPtr)
-        return *isUncountedPtr;
-      return isUncheckedPtr;
+        result = *isUncountedPtr;
+      if (isUncheckedPtr)
+        result = result ? *result || *isUncheckedPtr : *isUncheckedPtr;
+      if (isUnretainedPtr)
+        result = result ? *result || *isUnretainedPtr : *isUnretainedPtr;
+      return result;
     }
   }
   return false;
@@ -263,16 +315,24 @@ std::optional<bool> isGetterOfSafePtr(const CXXMethodDecl *M) {
          method == "impl"))
       return true;
 
+    if (className == "RetainPtr" && method == "get")
+      return true;
+
     // Ref<T> -> T conversion
     // FIXME: Currently allowing any Ref<T> -> whatever cast.
     if (isRefType(className)) {
       if (auto *maybeRefToRawOperator = dyn_cast<CXXConversionDecl>(M))
-        return isUnsafePtr(maybeRefToRawOperator->getConversionType());
+        return isUnsafePtr(maybeRefToRawOperator->getConversionType(), false);
     }
 
     if (isCheckedPtr(className)) {
       if (auto *maybeRefToRawOperator = dyn_cast<CXXConversionDecl>(M))
-        return isUnsafePtr(maybeRefToRawOperator->getConversionType());
+        return isUnsafePtr(maybeRefToRawOperator->getConversionType(), false);
+    }
+
+    if (className == "RetainPtr") {
+      if (auto *maybeRefToRawOperator = dyn_cast<CXXConversionDecl>(M))
+        return isUnsafePtr(maybeRefToRawOperator->getConversionType(), false);
     }
   }
   return false;
@@ -297,6 +357,13 @@ bool isCheckedPtr(const CXXRecordDecl *R) {
   return false;
 }
 
+bool isRetainPtr(const CXXRecordDecl *R) {
+  assert(R);
+  if (auto *TmplR = R->getTemplateInstantiationPattern())
+    return safeGetName(TmplR) == "RetainPtr";
+  return false;
+}
+
 bool isPtrConversion(const FunctionDecl *F) {
   assert(F);
   if (isCtorOfRefCounted(F))
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
index 9adfc0f1f5726..d220b42bbad6d 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
@@ -51,6 +51,9 @@ bool isRefCounted(const clang::CXXRecordDecl *Class);
 /// \returns true if \p Class is a CheckedPtr / CheckedRef, false if not.
 bool isCheckedPtr(const clang::CXXRecordDecl *Class);
 
+/// \returns true if \p Class is a RetainPtr, false if not.
+bool isRetainPtr(const clang::CXXRecordDecl *Class);
+
 /// \returns true if \p Class is ref-countable AND not ref-counted, false if
 /// not, std::nullopt if inconclusive.
 std::optional<bool> isUncounted(const clang::QualType T);
@@ -59,6 +62,10 @@ std::optional<bool> isUncounted(const clang::QualType T);
 /// not, std::nullopt if inconclusive.
 std::optional<bool> isUnchecked(const clang::QualType T);
 
+/// \returns true if \p Class is NS or CF objects AND not retained, false if
+/// not, std::nullopt if inconclusive.
+std::optional<bool> isUnretained(const clang::QualType T, bool IsARCEnabled);
+
 /// \returns true if \p Class is ref-countable AND not ref-counted, false if
 /// not, std::nullopt if inconclusive.
 std::optional<bool> isUncounted(const clang::CXXRecordDecl* Class);
@@ -77,11 +84,14 @@ std::optional<bool> isUncheckedPtr(const clang::QualType T);
 
 /// \returns true if \p T is either a raw pointer or reference to an uncounted
 /// or unchecked class, false if not, std::nullopt if inconclusive.
-std::optional<bool> isUnsafePtr(const QualType T);
+std::optional<bool> isUnsafePtr(const QualType T, bool IsArcEnabled);
 
 /// \returns true if \p T is a RefPtr, Ref, CheckedPtr, CheckedRef, or its
 /// variant, false if not.
-bool isSafePtrType(const clang::QualType T);
+bool isRefOrCheckedPtrType(const clang::QualType T);
+
+/// \returns true if \p T is a RetainPtr, false if not.
+bool isRetainPtrType(const clang::QualType T);
 
 /// \returns true if \p T is a RefPtr, Ref, CheckedPtr, CheckedRef, or
 /// unique_ptr, false if not.
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp
index 56fa72c100ec8..af236f404e2d5 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp
@@ -41,6 +41,8 @@ class RawPtrRefCallArgsChecker
 
   virtual std::optional<bool> isUnsafeType(QualType) const = 0;
   virtual std::optional<bool> isUnsafePtr(QualType) const = 0;
+  virtual bool isSafePtr(const CXXRecordDecl *Record) const = 0;
+  virtual bool isSafePtrType(const QualType type) const = 0;
   virtual const char *ptrKind() const = 0;
 
   void checkASTDecl(const TranslationUnitDecl *TUD, AnalysisManager &MGR,
@@ -141,6 +143,12 @@ class RawPtrRefCallArgsChecker
 
   bool isPtrOriginSafe(const Expr *Arg) const {
     return tryToFindPtrOrigin(Arg, /*StopAtFirstRefCountedObj=*/true,
+                              [&](const clang::CXXRecordDecl *Record) {
+                                return isSafePtr(Record);
+                              },
+                              [&](const clang::QualType T) {
+                                return isSafePtrType(T);
+                              },
                               [&](const clang::Expr *ArgOrigin, bool IsSafe) {
                                 if (IsSafe)
                                   return true;
@@ -315,6 +323,14 @@ class UncountedCallArgsChecker final : public RawPtrRefCallArgsChecker {
     return isUncountedPtr(QT);
   }
 
+  bool isSafePtr(const CXXRecordDecl *Record) const final {
+    return isRefCounted(Record) || isCheckedPtr(Record);
+  }
+
+  bool isSafePtrType(const QualType type) const final {
+    return isRefOrCheckedPtrType(type);
+  }
+
   const char *ptrKind() const final { return "uncounted"; }
 };
 
@@ -332,6 +348,14 @@ class UncheckedCallArgsChecker final : public RawPtrRefCallArgsChecker {
     return isUncheckedPtr(QT);
   }
 
+  bool isSafePtr(const CXXRecordDecl *Record) const final {
+    return isRefCounted(Record) || isCheckedPtr(Record);
+  }
+
+  bool isSafePtrType(const QualType type) const final {
+    return isRefOrCheckedPtrType(type);
+  }
+
   const char *ptrKind() const final { return "unchecked"; }
 };
 
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp
index d586668399502..f7eacdd6b54e0 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp
@@ -15,6 +15,7 @@
 #include "clang/AST/DynamicRecursiveASTVisitor.h"
 #include "clang/AST/ParentMapContext.h"
 #include "clang/Basic/SourceLocation.h"
+#include "clang/Analysis/DomainSpecific/CocoaConventions.h"
 #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
@@ -81,7 +82,7 @@ struct GuardianVisitor : DynamicRecursiveASTVisitor {
   bool VisitCXXMemberCallExpr(CXXMemberCallExpr *MCE) override {
     auto MethodName = safeGetName(MCE->getMethodDecl());
     if (MethodName == "swap" || MethodName == "leakRef" ||
-        MethodName == "releaseNonNull") {
+        MethodName == "releaseNonNull" || MethodName == "clear") {
       auto *ThisArg = MCE->getImplicitObjectArgument()->IgnoreParenCasts();
       if (auto *VarRef = dyn_cast<DeclRefExpr>(ThisArg)) {
         if (VarRef->getDecl() == Guardian)
@@ -173,10 +174,12 @@ class RawPtrRefLocalVarsChecker
       : Bug(this, description, "WebKit coding guidelines") {}
 
   virtual std::optional<bool> isUnsafePtr(const QualType T) const = 0;
+  virtual bool isSafePtr(const CXXRecordDecl *) const = 0;
+  virtual bool isSafePtrType(const QualType) const = 0;
   virtual const char *ptrKind() const = 0;
 
-  void checkASTDecl(const TranslationUnitDecl *TUD, AnalysisManager &MGR,
-                    BugReporter &BRArg) const {
+  virtual void checkASTDecl(const TranslationUnitDecl *TUD,
+                            AnalysisManager &MGR, BugReporter &BRArg) const {
     BR = &BRArg;
 
     // The calls to checkAST* from AnalysisConsumer don't
@@ -263,6 +266,12 @@ class RawPtrRefLocalVarsChecker
     if (IsUncountedPtr && *IsUncountedPtr) {
       if (tryToFindPtrOrigin(
               Value, /*StopAtFirstRefCountedObj=*/false,
+              [&](const clang::CXXRecordDecl *Record) {
+                return isSafePtr(Record);
+              },
+              [&](const clang::QualType Type) {
+                return isSafePtrType(Type);
+              },
               [&](const clang::Expr *InitArgOrigin, bool IsSafe) {
                 if (!InitArgOrigin || IsSafe)
                   return true;
@@ -292,8 +301,7 @@ class RawPtrRefLocalVarsChecker
                           MaybeGuardianArgType->getAsCXXRecordDecl();
                       if (MaybeGuardianArgCXXRecord) {
                         if (MaybeGuardian->isLocalVarDecl() &&
-                            (isRefCounted(MaybeGuardianArgCXXRecord) ||
-                             isCheckedPtr(MaybeGuardianArgCXXRecord) ||
+                            (isSafePtr(MaybeGuardianArgCXXRecord) ||
                              isRefcountedStringsHack(MaybeGuardian)) &&
                             isGuardedScopeEmbeddedInGuardianScope(
                                 V, MaybeGuardian))
@@ -365,6 +373,12 @@ class UncountedLocalVarsChecker final : public RawPtrRefLocalVarsChecker {
   std::optional<bool> isUnsafePtr(const QualType T) const final {
     return isUncountedPtr(T);
   }
+  bool isSafePtr(const CXXRecordDecl *Record) const final {
+    return isRefCounted(Record) || isCheckedPtr(Record);
+  }
+  bool isSafePtrType(const QualType type) const final {
+    return isRefOrCheckedPtrType(type);
+  }
   const char *ptrKind() const final { return "uncounted"; }
 };
 
@@ -376,9 +390,39 @@ class UncheckedLocalVarsChecker final : public RawPtrRefLocalVarsChecker {
   std::optional<bool> isUnsafePtr(const QualType T) const final {
     return isUncheckedPtr(T);
   }
+  bool isSafePtr(const CXXRecordDecl *Record) const final {
+    return isRefCounted(Record) || isCheckedPtr(Record);
+  }
+  bool isSafePtrType(const QualType type) const final {
+    return isRefOrCheckedPtrType(type);
+  }
   const char *ptrKind() const final { return "unchecked"; }
 };
 
+class UnretainedLocalVarsChecker final : public RawPtrRefLocalVarsChecker {
+  mutable bool IsARCEnabled{false};
+public:
+  UnretainedLocalVarsChecker()
+      : RawPtrRefLocalVarsChecker("Unretained raw pointer or reference not "
+                                  "provably backed by a RetainPtr") {}
+  std::optional<bool> isUnsafePtr(const QualType T) const final {
+    return isUnretained(T, IsARCEnabled);
+  }
+  bool isSafePtr(const CXXRecordDecl *Record) const final {
+    return isRetainPtr(Record);
+  }
+  bool isSafePtrType(const QualType type) const final {
+    return isRetainPtrType(type);
+  }
+  const char *ptrKind() const final { return "unretained"; }
+
+  void checkASTDecl(const TranslationUnitDecl *TUD,
+                    AnalysisManager &MGR, BugReporter &BRArg) const final {
+    IsARCEnabled = TUD->getLangOpts().ObjCAutoRefCount;
+    RawPtrRefLocalVarsChecker::checkASTDecl(TUD, MGR, BRArg);
+  }
+};
+
 } // namespace
 
 void ento::registerUncountedLocalVarsChecker(CheckerManager &Mgr) {
@@ -396,3 +440,11 @@ void ento::registerUncheckedLocalVarsChecker(CheckerManager &Mgr) {
 bool ento::shouldRegisterUncheckedLocalVarsChecker(const CheckerManager &) {
   return true;
 }
+
+void ento::registerUnretainedLocalVarsChecker(CheckerManager &Mgr) {
+  Mgr.registerChecker<UnretainedLocalVarsChecker>();
+}
+
+bool ento::shouldRegisterUnretainedLocalVarsChecker(const CheckerManager &) {
+  return true;
+}
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp
index 9527993d0edeb..fd710177e5426 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp
@@ -61,7 +61,7 @@ class UncountedLambdaCapturesChecker
       }
 
       bool shouldCheckThis() {
-        auto result = !ClsType.isNull() ? isUnsafePtr(ClsType) : std::nullopt;
+        auto result = !ClsType.isNull() ? isUnsafePtr(ClsType, false) : std::nullopt;
         r...
[truncated]

github-actions · 2025-02-18T02:28:39Z

✅ With the latest revision this PR passed the C/C++ code formatter.

…typedef for CF*Ref types

clang/docs/analyzer/checkers.rst

t-rasmud · 2025-02-22T22:28:33Z

clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp

+  bool isSafePtrType(const QualType type) const final {
+    return isRefOrCheckedPtrType(type);
+  }
+


Would it be possible to define these two methods in RawPtrRefCallArgsChecker?

These are declared as pure virtual functions in RawPtrRefCallArgsChecker. Do you mean move one of the definitions to the base class so that some of these concrete subclasses won't have to define them??

Ah, I didn't see they're pure virtual functions. Yeah, that was my intention, move them so we don't have to define them repeatedly.

Are you fine with pure virtual in the base class? Or do you think we should move one of the implementation to the base?

I'm fine with them being pure virtual in base class.

Great. Thanks for the review!

t-rasmud

Left some nitpicks. otherwise LGTM.

…iables to NS and CF types. (llvm#127554) This PR adds alpha.webkit.UnretainedLocalVarsChecker by generalizing RawPtrRefLocalVarsChecker. It checks local variables to NS or CF types are guarded with a RetainPtr or not. The new checker is effective for NS and CF types in Objective-C++ code without ARC, and it's effective for CF types in code with ARC.

rniwa requested a review from t-rasmud February 18, 2025 02:25

llvmbot added clang Clang issues not falling into any other category clang:static analyzer labels Feb 18, 2025

rniwa requested a review from haoNoQ February 18, 2025 02:25

rniwa added 5 commits February 17, 2025 18:34

Fix formatting.

e9643fa

Fix documentation

1a98751

Fix the debug assertion failure when self is implicitly captured.

bbcd919

Avoid hard-coding __CF types. Instead, recognize the pointee type in …

62b53c7

…typedef for CF*Ref types

Fix formatting.

c6bc9c5

t-rasmud reviewed Feb 22, 2025

View reviewed changes

t-rasmud approved these changes Feb 22, 2025

View reviewed changes

Add code examples in the documentation

c589057

rniwa merged commit cec3507 into llvm:main Feb 24, 2025
12 checks passed

rniwa deleted the add-unretained-local-vars-checker branch February 24, 2025 20:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[alpha.webkit.UnretainedLocalVarsChecker] Add a checker for local variables to NS and CF types. #127554

[alpha.webkit.UnretainedLocalVarsChecker] Add a checker for local variables to NS and CF types. #127554

rniwa commented Feb 18, 2025

llvmbot commented Feb 18, 2025

llvmbot commented Feb 18, 2025

github-actions bot commented Feb 18, 2025 •

edited

Loading

t-rasmud Feb 22, 2025

rniwa Feb 23, 2025

t-rasmud Feb 24, 2025

rniwa Feb 24, 2025

t-rasmud Feb 24, 2025

rniwa Feb 24, 2025

t-rasmud left a comment

[alpha.webkit.UnretainedLocalVarsChecker] Add a checker for local variables to NS and CF types. #127554

[alpha.webkit.UnretainedLocalVarsChecker] Add a checker for local variables to NS and CF types. #127554

Conversation

rniwa commented Feb 18, 2025

llvmbot commented Feb 18, 2025

llvmbot commented Feb 18, 2025

github-actions bot commented Feb 18, 2025 • edited Loading

t-rasmud Feb 22, 2025

Choose a reason for hiding this comment

rniwa Feb 23, 2025

Choose a reason for hiding this comment

t-rasmud Feb 24, 2025

Choose a reason for hiding this comment

rniwa Feb 24, 2025

Choose a reason for hiding this comment

t-rasmud Feb 24, 2025

Choose a reason for hiding this comment

rniwa Feb 24, 2025

Choose a reason for hiding this comment

t-rasmud left a comment

Choose a reason for hiding this comment

github-actions bot commented Feb 18, 2025 •

edited

Loading