Skip to content

Conversation

rniwa
Copy link
Contributor

@rniwa rniwa commented Sep 9, 2025

This PR adds the support for treating a function return value to be safe if the function is annotated with NS_RETURNS_RETAINED or CF_RETURNS_RETAINED.

This PR adds the support for treating a function return value to be safe
if the function is annotated with NS_RETURNS_RETAINED or CF_RETURNS_RETAINED.
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:static analyzer labels Sep 9, 2025
@llvmbot
Copy link
Member

llvmbot commented Sep 9, 2025

@llvm/pr-subscribers-clang

Author: Ryosuke Niwa (rniwa)

Changes

This PR adds the support for treating a function return value to be safe if the function is annotated with NS_RETURNS_RETAINED or CF_RETURNS_RETAINED.


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

3 Files Affected:

  • (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp (+7)
  • (modified) clang/test/Analysis/Checkers/WebKit/unretained-call-args.mm (+14)
  • (modified) clang/test/Analysis/Checkers/WebKit/unretained-local-vars.mm (+15)
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
index 478bd85177143..f43e5ac9af198 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
@@ -91,6 +91,13 @@ bool tryToFindPtrOrigin(
       continue;
     }
     if (auto *call = dyn_cast<CallExpr>(E)) {
+      if (auto *Callee = call->getCalleeDecl()) {
+        if (Callee->hasAttr<CFReturnsRetainedAttr>() ||
+            Callee->hasAttr<NSReturnsRetainedAttr>()) {
+          return callback(E, true);
+        }
+      }
+
       if (auto *memberCall = dyn_cast<CXXMemberCallExpr>(call)) {
         if (auto *decl = memberCall->getMethodDecl()) {
           std::optional<bool> IsGetterOfRefCt = isGetterOfSafePtr(decl);
diff --git a/clang/test/Analysis/Checkers/WebKit/unretained-call-args.mm b/clang/test/Analysis/Checkers/WebKit/unretained-call-args.mm
index c69113c48806d..c44f209d80a4e 100644
--- a/clang/test/Analysis/Checkers/WebKit/unretained-call-args.mm
+++ b/clang/test/Analysis/Checkers/WebKit/unretained-call-args.mm
@@ -438,6 +438,20 @@ void use_const_local() {
 
 } // namespace const_global
 
+namespace ns_retained_return_value {
+
+NSString *provideNS() NS_RETURNS_RETAINED;
+CFDictionaryRef provideCF() CF_RETURNS_RETAINED;
+void consumeNS(NSString *);
+void consumeCF(CFDictionaryRef);
+
+void foo() {
+  consumeNS(provideNS());
+  consumeCF(provideCF());
+}
+
+} // namespace ns_retained_return_value
+
 @interface TestObject : NSObject
 - (void)doWork:(NSString *)msg, ...;
 - (void)doWorkOnSelf;
diff --git a/clang/test/Analysis/Checkers/WebKit/unretained-local-vars.mm b/clang/test/Analysis/Checkers/WebKit/unretained-local-vars.mm
index 10f7c9acb7a3c..0ad8f707e254c 100644
--- a/clang/test/Analysis/Checkers/WebKit/unretained-local-vars.mm
+++ b/clang/test/Analysis/Checkers/WebKit/unretained-local-vars.mm
@@ -408,6 +408,21 @@ void use_const_local() {
 
 } // namespace const_global
 
+namespace ns_retained_return_value {
+
+NSString *provideNS() NS_RETURNS_RETAINED;
+CFDictionaryRef provideCF() CF_RETURNS_RETAINED;
+void consumeNS(NSString *);
+void consumeCF(CFDictionaryRef);
+
+unsigned foo() {
+  auto *string = provideNS();
+  auto *dictionary = provideCF();
+  return string.length + CFDictionaryGetCount(dictionary);
+}
+
+} // namespace ns_retained_return_value
+
 bool doMoreWorkOpaque(OtherObj*);
 SomeObj* provide();
 

@llvmbot
Copy link
Member

llvmbot commented Sep 9, 2025

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

Author: Ryosuke Niwa (rniwa)

Changes

This PR adds the support for treating a function return value to be safe if the function is annotated with NS_RETURNS_RETAINED or CF_RETURNS_RETAINED.


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

3 Files Affected:

  • (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp (+7)
  • (modified) clang/test/Analysis/Checkers/WebKit/unretained-call-args.mm (+14)
  • (modified) clang/test/Analysis/Checkers/WebKit/unretained-local-vars.mm (+15)
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
index 478bd85177143..f43e5ac9af198 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
@@ -91,6 +91,13 @@ bool tryToFindPtrOrigin(
       continue;
     }
     if (auto *call = dyn_cast<CallExpr>(E)) {
+      if (auto *Callee = call->getCalleeDecl()) {
+        if (Callee->hasAttr<CFReturnsRetainedAttr>() ||
+            Callee->hasAttr<NSReturnsRetainedAttr>()) {
+          return callback(E, true);
+        }
+      }
+
       if (auto *memberCall = dyn_cast<CXXMemberCallExpr>(call)) {
         if (auto *decl = memberCall->getMethodDecl()) {
           std::optional<bool> IsGetterOfRefCt = isGetterOfSafePtr(decl);
diff --git a/clang/test/Analysis/Checkers/WebKit/unretained-call-args.mm b/clang/test/Analysis/Checkers/WebKit/unretained-call-args.mm
index c69113c48806d..c44f209d80a4e 100644
--- a/clang/test/Analysis/Checkers/WebKit/unretained-call-args.mm
+++ b/clang/test/Analysis/Checkers/WebKit/unretained-call-args.mm
@@ -438,6 +438,20 @@ void use_const_local() {
 
 } // namespace const_global
 
+namespace ns_retained_return_value {
+
+NSString *provideNS() NS_RETURNS_RETAINED;
+CFDictionaryRef provideCF() CF_RETURNS_RETAINED;
+void consumeNS(NSString *);
+void consumeCF(CFDictionaryRef);
+
+void foo() {
+  consumeNS(provideNS());
+  consumeCF(provideCF());
+}
+
+} // namespace ns_retained_return_value
+
 @interface TestObject : NSObject
 - (void)doWork:(NSString *)msg, ...;
 - (void)doWorkOnSelf;
diff --git a/clang/test/Analysis/Checkers/WebKit/unretained-local-vars.mm b/clang/test/Analysis/Checkers/WebKit/unretained-local-vars.mm
index 10f7c9acb7a3c..0ad8f707e254c 100644
--- a/clang/test/Analysis/Checkers/WebKit/unretained-local-vars.mm
+++ b/clang/test/Analysis/Checkers/WebKit/unretained-local-vars.mm
@@ -408,6 +408,21 @@ void use_const_local() {
 
 } // namespace const_global
 
+namespace ns_retained_return_value {
+
+NSString *provideNS() NS_RETURNS_RETAINED;
+CFDictionaryRef provideCF() CF_RETURNS_RETAINED;
+void consumeNS(NSString *);
+void consumeCF(CFDictionaryRef);
+
+unsigned foo() {
+  auto *string = provideNS();
+  auto *dictionary = provideCF();
+  return string.length + CFDictionaryGetCount(dictionary);
+}
+
+} // namespace ns_retained_return_value
+
 bool doMoreWorkOpaque(OtherObj*);
 SomeObj* provide();
 

Copy link
Contributor

@t-rasmud t-rasmud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a small nit in the test case. Otherwise, LGTM!

}

} // namespace ns_retained_return_value

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fairly certain this works, but can we have a test case with inheritance (sub class calls provideNS defined in super class) for the sake of completion?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean provideNS returns a subclass of a class consumeNS takes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or are you talking about adding a test case where provideNS is defined in a C++ class then a subclass of it calls consumeNS(provideNS()) in some member function?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or are you talking about adding a test case where provideNS is defined in a C++ class then a subclass of it calls consumeNS(provideNS()) in some member function?

I was thinking of this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, added.

Define a function which returns a retained value in a base class and call that in a derived class
@rniwa
Copy link
Contributor Author

rniwa commented Sep 11, 2025

Thanks for the review!

@rniwa rniwa merged commit b64ed9d into llvm:main Sep 11, 2025
9 checks passed
@rniwa rniwa deleted the fix-webkit-recognize-ns-cf-returns-retained branch September 11, 2025 20:32
rniwa added a commit to rniwa/llvm-project that referenced this pull request Sep 11, 2025
…ED. (llvm#157629)

This PR adds the support for treating a function return value to be safe
if the function is annotated with NS_RETURNS_RETAINED or
CF_RETURNS_RETAINED.
adrian-prantl pushed a commit to swiftlang/llvm-project that referenced this pull request Sep 14, 2025
…ED. (llvm#157629)

This PR adds the support for treating a function return value to be safe
if the function is annotated with NS_RETURNS_RETAINED or
CF_RETURNS_RETAINED.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:static analyzer clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants