[WebKit Checkers] Treat a NS/CF global defined in a system header as a safe pointer origin #161146

rniwa · 2025-09-29T08:16:00Z

No description provided.

llvmbot · 2025-09-29T08:16:39Z

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

@llvm/pr-subscribers-clang

Author: Ryosuke Niwa (rniwa)

Changes

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

7 Files Affected:

(modified) clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp (+7-2)
(modified) clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h (+1)
(modified) clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp (+8-1)
(modified) clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp (+7-1)
(modified) clang/test/Analysis/Checkers/WebKit/mock-system-header.h (+6)
(modified) clang/test/Analysis/Checkers/WebKit/unretained-local-vars.mm (+26)
(added) clang/test/Analysis/Checkers/WebKit/unretained-obj-arg.mm (+18)

diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
index 00a1b8b6e7e89..ab9a5b85cf241 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
@@ -26,6 +26,7 @@ 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::Decl *)> isSafeGlobalDecl,
     std::function<bool(const clang::Expr *, bool)> callback) {
   while (E) {
     if (auto *DRE = dyn_cast<DeclRefExpr>(E)) {
@@ -34,6 +35,8 @@ bool tryToFindPtrOrigin(
         if (VD->hasGlobalStorage() && QT.isConstQualified()) {
           return callback(E, true);
         }
+        if (VD->hasGlobalStorage() && isSafeGlobalDecl(VD))
+          return callback(E, true);
       }
     }
     if (auto *tempExpr = dyn_cast<MaterializeTemporaryExpr>(E)) {
@@ -71,9 +74,11 @@ bool tryToFindPtrOrigin(
     }
     if (auto *Expr = dyn_cast<ConditionalOperator>(E)) {
       return tryToFindPtrOrigin(Expr->getTrueExpr(), StopAtFirstRefCountedObj,
-                                isSafePtr, isSafePtrType, callback) &&
+                                isSafePtr, isSafePtrType, isSafeGlobalDecl,
+                                callback) &&
              tryToFindPtrOrigin(Expr->getFalseExpr(), StopAtFirstRefCountedObj,
-                                isSafePtr, isSafePtrType, callback);
+                                isSafePtr, isSafePtrType, isSafeGlobalDecl,
+                                callback);
     }
     if (auto *cast = dyn_cast<CastExpr>(E)) {
       if (StopAtFirstRefCountedObj) {
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h
index 3a009d65efea6..9fff456b7e8b8 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h
@@ -56,6 +56,7 @@ 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::Decl *)> isSafeGlobalDecl,
     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/RawPtrRefCallArgsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp
index 9585ceb40f95e..791e70998477f 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp
@@ -29,12 +29,12 @@ namespace {
 class RawPtrRefCallArgsChecker
     : public Checker<check::ASTDecl<TranslationUnitDecl>> {
   BugType Bug;
-  mutable BugReporter *BR;
 
   TrivialFunctionAnalysis TFA;
   EnsureFunctionAnalysis EFA;
 
 protected:
+  mutable BugReporter *BR;
   mutable std::optional<RetainTypeChecker> RTC;
 
 public:
@@ -46,6 +46,7 @@ class RawPtrRefCallArgsChecker
   virtual bool isSafePtr(const CXXRecordDecl *Record) const = 0;
   virtual bool isSafePtrType(const QualType type) const = 0;
   virtual bool isSafeExpr(const Expr *) const { return false; }
+  virtual bool isSafeDecl(const Decl *) const { return false; }
   virtual const char *ptrKind() const = 0;
 
   void checkASTDecl(const TranslationUnitDecl *TUD, AnalysisManager &MGR,
@@ -214,6 +215,7 @@ class RawPtrRefCallArgsChecker
         Arg, /*StopAtFirstRefCountedObj=*/true,
         [&](const clang::CXXRecordDecl *Record) { return isSafePtr(Record); },
         [&](const clang::QualType T) { return isSafePtrType(T); },
+        [&](const clang::Decl *D) { return isSafeDecl(D); },
         [&](const clang::Expr *ArgOrigin, bool IsSafe) {
           if (IsSafe)
             return true;
@@ -479,6 +481,11 @@ class UnretainedCallArgsChecker final : public RawPtrRefCallArgsChecker {
            isa<ObjCMessageExpr>(E);
   }
 
+  bool isSafeDecl(const Decl *D) const final {
+    // Treat NS/CF globals in system header as immortal.
+    return BR->getSourceManager().isInSystemHeader(D->getLocation());
+  }
+
   const char *ptrKind() const final { return "unretained"; }
 };
 
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp
index dd9701fbbb017..c13df47920f72 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp
@@ -166,10 +166,10 @@ bool isGuardedScopeEmbeddedInGuardianScope(const VarDecl *Guarded,
 class RawPtrRefLocalVarsChecker
     : public Checker<check::ASTDecl<TranslationUnitDecl>> {
   BugType Bug;
-  mutable BugReporter *BR;
   EnsureFunctionAnalysis EFA;
 
 protected:
+  mutable BugReporter *BR;
   mutable std::optional<RetainTypeChecker> RTC;
 
 public:
@@ -180,6 +180,7 @@ class RawPtrRefLocalVarsChecker
   virtual bool isSafePtr(const CXXRecordDecl *) const = 0;
   virtual bool isSafePtrType(const QualType) const = 0;
   virtual bool isSafeExpr(const Expr *) const { return false; }
+  virtual bool isSafeDecl(const Decl *) const { return false; }
   virtual const char *ptrKind() const = 0;
 
   void checkASTDecl(const TranslationUnitDecl *TUD, AnalysisManager &MGR,
@@ -288,6 +289,7 @@ class RawPtrRefLocalVarsChecker
                 return isSafePtr(Record);
               },
               [&](const clang::QualType Type) { return isSafePtrType(Type); },
+              [&](const clang::Decl *D) { return isSafeDecl(D); },
               [&](const clang::Expr *InitArgOrigin, bool IsSafe) {
                 if (!InitArgOrigin || IsSafe)
                   return true;
@@ -443,6 +445,10 @@ class UnretainedLocalVarsChecker final : public RawPtrRefLocalVarsChecker {
     return ento::cocoa::isCocoaObjectRef(E->getType()) &&
            isa<ObjCMessageExpr>(E);
   }
+  bool isSafeDecl(const Decl *D) const final {
+    // Treat NS/CF globals in system header as immortal.
+    return BR->getSourceManager().isInSystemHeader(D->getLocation());
+  }
   const char *ptrKind() const final { return "unretained"; }
 };
 
diff --git a/clang/test/Analysis/Checkers/WebKit/mock-system-header.h b/clang/test/Analysis/Checkers/WebKit/mock-system-header.h
index 1e44de8eb62ad..d55b3abd34f4c 100644
--- a/clang/test/Analysis/Checkers/WebKit/mock-system-header.h
+++ b/clang/test/Analysis/Checkers/WebKit/mock-system-header.h
@@ -34,6 +34,8 @@ void os_log_msg(os_log_t oslog, os_log_type_t type, const char *msg, ...);
 
 typedef const struct __attribute__((objc_bridge(NSString))) __CFString * CFStringRef;
 
+extern CFStringRef const kCFURLTagNamesKey;
+
 #ifdef __OBJC__
 @class NSString;
 @interface SystemObject {
@@ -41,4 +43,8 @@ typedef const struct __attribute__((objc_bridge(NSString))) __CFString * CFStrin
   CFStringRef cf_string;
 }
 @end
+
+typedef NSString *NSNotificationName;
+extern "C" NSNotificationName NSApplicationDidBecomeActiveNotification;
+
 #endif
diff --git a/clang/test/Analysis/Checkers/WebKit/unretained-local-vars.mm b/clang/test/Analysis/Checkers/WebKit/unretained-local-vars.mm
index 307a4d03fe101..cdd36790854f1 100644
--- a/clang/test/Analysis/Checkers/WebKit/unretained-local-vars.mm
+++ b/clang/test/Analysis/Checkers/WebKit/unretained-local-vars.mm
@@ -1,8 +1,11 @@
 // RUN: %clang_analyze_cc1 -analyzer-checker=alpha.webkit.UnretainedLocalVarsChecker -verify %s
 
 #import "objc-mock-types.h"
+#import "mock-system-header.h"
 
 void someFunction();
+extern "C" CFStringRef LocalGlobalCFString;
+extern "C" NSString *LocalGlobalNSString;
 
 namespace raw_ptr {
 void foo() {
@@ -535,6 +538,29 @@ unsigned foo() {
 
 } // namespace ns_retained_return_value
 
+namespace ns_global {
+
+void consumeCFString(CFStringRef);
+void consumeNSString(NSString *);
+
+void cf() {
+  auto *str = kCFURLTagNamesKey;
+  consumeCFString(str);
+  auto *localStr = LocalGlobalCFString;
+  // expected-warning@-1{{Local variable 'localStr' is unretained and unsafe [alpha.webkit.UnretainedLocalVarsChecker]}}
+  consumeCFString(localStr);
+}
+
+void ns() {
+  auto *str = NSApplicationDidBecomeActiveNotification;
+  consumeNSString(str);
+  auto *localStr = LocalGlobalNSString;
+  // expected-warning@-1{{Local variable 'localStr' is unretained and unsafe [alpha.webkit.UnretainedLocalVarsChecker]}}
+  consumeNSString(localStr);
+}
+
+}
+
 bool doMoreWorkOpaque(OtherObj*);
 SomeObj* provide();
 
diff --git a/clang/test/Analysis/Checkers/WebKit/unretained-obj-arg.mm b/clang/test/Analysis/Checkers/WebKit/unretained-obj-arg.mm
new file mode 100644
index 0000000000000..5c78b21d6c94f
--- /dev/null
+++ b/clang/test/Analysis/Checkers/WebKit/unretained-obj-arg.mm
@@ -0,0 +1,18 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.webkit.UnretainedCallArgsChecker -verify %s
+
+#import "mock-types.h"
+#import "mock-system-header.h"
+
+void consumeCFString(CFStringRef);
+extern "C" CFStringRef LocalGlobalCFString;
+void consumeNSString(NSString *);
+extern "C" NSString *LocalGlobalNSString;
+
+void foo() {
+  consumeCFString(kCFURLTagNamesKey);
+  consumeCFString(LocalGlobalCFString);
+    // expected-warning@-1{{Call argument is unretained and unsafe}}
+  consumeNSString(NSApplicationDidBecomeActiveNotification);
+  consumeNSString(LocalGlobalNSString);
+    // expected-warning@-1{{Call argument is unretained and unsafe}}
+}

t-rasmud

LGTM!

…a safe pointer origin

t-rasmud

LGTM!

rniwa · 2025-10-14T23:52:09Z

I don't think the test failures are related to this PR.

…a safe pointer origin (llvm#161146)

…a safe pointer origin (llvm#161146) (cherry picked from commit 1127dd7)

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

rniwa requested a review from t-rasmud September 29, 2025 08:17

csaavedra mentioned this pull request Sep 30, 2025

Address safer CPP warnings in WebViewImpl.mm WebKit/WebKit#51562

Merged

t-rasmud approved these changes Oct 14, 2025

View reviewed changes

[WebKit Checkers] Treat a NS/CF global defined in a system header as …

31e0e4a

…a safe pointer origin

rniwa force-pushed the fix-webkit-treat-ns-cf-globals-as-immortal branch from 91c92d8 to 31e0e4a Compare October 14, 2025 22:29

t-rasmud approved these changes Oct 14, 2025

View reviewed changes

rniwa merged commit 1127dd7 into llvm:main Oct 14, 2025
10 of 14 checks passed

rniwa deleted the fix-webkit-treat-ns-cf-globals-as-immortal branch October 14, 2025 23:53

rniwa added a commit to rniwa/llvm-project that referenced this pull request Oct 14, 2025

[WebKit Checkers] Treat a NS/CF global defined in a system header as …

8cd08a7

…a safe pointer origin (llvm#161146)

rniwa added a commit to rniwa/llvm-project that referenced this pull request Oct 16, 2025

[WebKit Checkers] Treat a NS/CF global defined in a system header as …

aabc74a

…a safe pointer origin (llvm#161146) (cherry picked from commit 1127dd7)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[WebKit Checkers] Treat a NS/CF global defined in a system header as a safe pointer origin #161146

[WebKit Checkers] Treat a NS/CF global defined in a system header as a safe pointer origin #161146

Uh oh!

rniwa commented Sep 29, 2025

Uh oh!

llvmbot commented Sep 29, 2025 •

edited

Loading

Uh oh!

t-rasmud left a comment

Uh oh!

t-rasmud left a comment

Uh oh!

rniwa commented Oct 14, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

[WebKit Checkers] Treat a NS/CF global defined in a system header as a safe pointer origin #161146

[WebKit Checkers] Treat a NS/CF global defined in a system header as a safe pointer origin #161146

Uh oh!

Conversation

rniwa commented Sep 29, 2025

Uh oh!

llvmbot commented Sep 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

t-rasmud left a comment

Choose a reason for hiding this comment

Uh oh!

t-rasmud left a comment

Choose a reason for hiding this comment

Uh oh!

rniwa commented Oct 14, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

llvmbot commented Sep 29, 2025 •

edited

Loading