[clang] [alpha.webkit.UnretainedLocalVarsChecker] Treat the collection of a fast enumeration as the origin of its element (PR #230818)

[via cfe-commits]

llvmorg-github-actions[bot] wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-clang

Author: Ryosuke Niwa (rniwa)

<details>
<summary>Changes</summary>

alpha.webkit.UnretainedLocalVarsChecker reported every element variable of an Objective-C fast enumeration such as "for (T *x in collection)" since the variable has no initializer, even when the collection was kept alive by a RetainPtr local variable.

The element of a fast enumeration is kept alive by its collection, which can't be mutated during the enumeration. So treat the collection as the initial value of the element, as in "T *x = collection;". Because the collection is evaluated before the loop body is entered, a guardian local variable only needs to outlive the loop body; it can be declared in the same scope as the loop as long as the loop body doesn't mutate it.

The collection is a full-expression of its own so a temporary smart pointer in the collection is destroyed before the loop body is entered and isn't considered safe.

Authored with Claude Code.

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


2 Files Affected:

- (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp (+36-3) 
- (modified) clang/test/Analysis/Checkers/WebKit/unretained-local-vars.mm (+85) 


``````````diff
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp
index 232cca1c7a2c4..33935861d153a 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp
@@ -15,6 +15,7 @@
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DynamicRecursiveASTVisitor.h"
 #include "clang/AST/ParentMapContext.h"
+#include "clang/AST/StmtObjC.h"
 #include "clang/Analysis/DomainSpecific/CocoaConventions.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
@@ -214,6 +215,24 @@ bool isGuardedScopeEmbeddedInGuardianScope(const VarDecl *Guarded,
   return false;
 }
 
+// Returns the fast enumeration statement whose element is declared as V as in
+// "for (T *V in Collection)".
+static const ObjCForCollectionStmt *
+findObjCForCollectionStmtOfElement(const VarDecl *V) {
+  ASTContext &Ctx = V->getASTContext();
+  for (auto &VarParent : Ctx.getParents(*V)) {
+    auto *DS = VarParent.get<DeclStmt>();
+    if (!DS || !DS->isSingleDecl())
+      continue;
+    for (auto &DeclStmtParent : Ctx.getParents(*DS)) {
+      auto *FCS = DeclStmtParent.get<ObjCForCollectionStmt>();
+      if (FCS && FCS->getElement() == DS)
+        return FCS;
+    }
+  }
+  return nullptr;
+}
+
 static const VarDecl *findAssignedVar(const Expr *DestExpr) {
   while (DestExpr) {
     DestExpr = DestExpr->IgnoreParenCasts();
@@ -425,6 +444,13 @@ class RawPtrRefLocalVarsChecker
     std::optional<bool> IsUncountedPtr = isUnsafePtr(SinkType);
     if (IsUncountedPtr && *IsUncountedPtr) {
       const Expr *Origin = nullptr;
+      // The element of a fast enumeration is kept alive by the collection,
+      // which can't be mutated during the enumeration. Treat the collection as
+      // the initial value of the element.
+      if (!Value) {
+        if (auto *FCS = findObjCForCollectionStmtOfElement(V))
+          Value = FCS->getCollection();
+      }
       if (Value) {
         if (isPtrOriginSafe(V, Value, DeclWithIssue, Origin))
           return;
@@ -503,9 +529,16 @@ class RawPtrRefLocalVarsChecker
       if (auto *Record = GuardianType->getAsCXXRecordDecl()) {
         if (MaybeGuardian->isLocalVarDecl() &&
             (Model->isSafePtr(Record) ||
-             isRefcountedStringsHack(MaybeGuardian)) &&
-            isGuardedScopeEmbeddedInGuardianScope(V, MaybeGuardian))
-          return true;
+             isRefcountedStringsHack(MaybeGuardian))) {
+          // The collection of a fast enumeration is evaluated before the loop
+          // body is entered so the guardian only needs to outlive the body.
+          if (auto *FCS = findObjCForCollectionStmtOfElement(V)) {
+            GuardianVisitor Visitor(MaybeGuardian);
+            return Visitor.TraverseStmt(const_cast<Stmt *>(FCS->getBody()));
+          }
+          if (isGuardedScopeEmbeddedInGuardianScope(V, MaybeGuardian))
+            return true;
+        }
       }
     }
 
diff --git a/clang/test/Analysis/Checkers/WebKit/unretained-local-vars.mm b/clang/test/Analysis/Checkers/WebKit/unretained-local-vars.mm
index 03946b40ea816..372eb3ee874b7 100644
--- a/clang/test/Analysis/Checkers/WebKit/unretained-local-vars.mm
+++ b/clang/test/Analysis/Checkers/WebKit/unretained-local-vars.mm
@@ -574,6 +574,91 @@ void ns() {
 
 }
 
+namespace fast_enumeration {
+
+NSArray *provideArray();
+
+void iterateLocal() {
+  RetainPtr<NSArray> array = provideArray();
+  for (SomeObj *obj in array.get())
+    [obj doWork];
+}
+
+void iterateLocalInNestedScope() {
+  RetainPtr<NSArray> array = provideArray();
+  {
+    for (SomeObj *obj in array.get()) {
+      for (SomeObj *otherObj in array.get())
+        [otherObj doWork];
+      [obj doWork];
+    }
+  }
+}
+
+void iterateLocalWithRangeBasedFor() {
+  RetainPtr<NSArray> array = provideArray();
+  for (SomeObj *obj : array.get())
+    [obj doWork];
+}
+
+void iterateLocalWithId() {
+  RetainPtr<NSArray> array = provideArray();
+  for (id obj in array.get())
+    [obj doWork];
+}
+
+void iterateLocalClearedInBody() {
+  RetainPtr<NSArray> array = provideArray();
+  for (SomeObj *obj in array.get()) {
+    // expected-warning at -1{{Local variable 'obj' is a raw pointer to RetainPtr-capable type 'SomeObj' [alpha.webkit.UnretainedLocalVarsChecker]}}
+    array = nullptr;
+    [obj doWork];
+  }
+}
+
+void iterateLocalClearedAfterLoop() {
+  RetainPtr<NSArray> array = provideArray();
+  for (SomeObj *obj in array.get())
+    [obj doWork];
+  array = nullptr;
+}
+
+void iterateParam(NSArray *array) {
+  for (SomeObj *obj in array)
+    [obj doWork];
+}
+
+void iterateParamReassigned(NSArray *array) {
+  for (SomeObj *obj in array) {
+    // expected-warning at -1{{Local variable 'obj' is a raw pointer to RetainPtr-capable type 'SomeObj' [alpha.webkit.UnretainedLocalVarsChecker]}}
+    array = provideArray();
+    // expected-warning at -1{{Parameter 'array' is a raw pointer to RetainPtr-capable type 'NSArray' [alpha.webkit.UnretainedLocalVarsChecker]}}
+    [obj doWork];
+  }
+}
+
+void iterateTemporary() {
+  for (SomeObj *obj in retainPtr(provideArray()).get())
+    // expected-warning at -1{{Local variable 'obj' is a raw pointer to RetainPtr-capable type 'SomeObj' [alpha.webkit.UnretainedLocalVarsChecker]}}
+    [obj doWork];
+}
+
+void iterateReturnValue() {
+  for (SomeObj *obj in provideArray())
+    // expected-warning at -1{{Local variable 'obj' is a raw pointer to RetainPtr-capable type 'SomeObj' [alpha.webkit.UnretainedLocalVarsChecker]}}
+    [obj doWork];
+}
+
+void iterateRawLocal() {
+  NSArray *array = provideArray();
+  // expected-warning at -1{{Local variable 'array' is a raw pointer to RetainPtr-capable type 'NSArray' [alpha.webkit.UnretainedLocalVarsChecker]}}
+  for (SomeObj *obj in array)
+    // expected-warning at -1{{Local variable 'obj' is a raw pointer to RetainPtr-capable type 'SomeObj' [alpha.webkit.UnretainedLocalVarsChecker]}}
+    [obj doWork];
+}
+
+} // namespace fast_enumeration
+
 bool doMoreWorkOpaque(OtherObj*);
 SomeObj* provide();
 

``````````

</details>


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