[alpha.webkit.RawPtrRefMemberChecker] The checker doesn't warn Objective-C types in ivars. #132833

rniwa · 2025-03-24T21:24:49Z

This PR fixes the bug that we weren't generating warnings when a raw poiner is used to point to a NS type in Objective-C ivars. Also fix the bug that we weren't suppressing this warning in system headers.

…ive-C types in ivars. This PR fixes the bug that we weren't generating warnings when a raw poiner is used to point to a NS type in Objective-C ivars. Also fix the bug that we weren't suppressing this warning in system headers.

llvmbot · 2025-03-24T21:25:25Z

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

Author: Ryosuke Niwa (rniwa)

Changes

This PR fixes the bug that we weren't generating warnings when a raw poiner is used to point to a NS type in Objective-C ivars. Also fix the bug that we weren't suppressing this warning in system headers.

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

3 Files Affected:

(modified) clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefMemberChecker.cpp (+10)
(modified) clang/test/Analysis/Checkers/WebKit/mock-system-header.h (+11)
(modified) clang/test/Analysis/Checkers/WebKit/unretained-members.mm (+11-1)

diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefMemberChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefMemberChecker.cpp
index dc4e2c7d168fb..6d20869043358 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefMemberChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefMemberChecker.cpp
@@ -111,6 +111,8 @@ class RawPtrRefMemberChecker
   }
 
   void visitObjCDecl(const ObjCContainerDecl *CD) const {
+    if (BR->getSourceManager().isInSystemHeader(CD->getLocation()))
+      return;
     if (auto *ID = dyn_cast<ObjCInterfaceDecl>(CD)) {
       for (auto *Ivar : ID->ivars())
         visitIvarDecl(CD, Ivar);
@@ -133,6 +135,14 @@ class RawPtrRefMemberChecker
       std::optional<bool> IsCompatible = isPtrCompatible(QT, IvarCXXRD);
       if (IsCompatible && *IsCompatible)
         reportBug(Ivar, IvarType, IvarCXXRD, CD);
+    } else {
+      std::optional<bool> IsCompatible = isPtrCompatible(QT, nullptr);
+      auto *PointeeType = IvarType->getPointeeType().getTypePtrOrNull();
+      if (IsCompatible && *IsCompatible) {
+        auto *Desugared = PointeeType->getUnqualifiedDesugaredType();
+        if (auto *ObjCType = dyn_cast_or_null<ObjCInterfaceType>(Desugared))
+          reportBug(Ivar, IvarType, ObjCType->getDecl(), CD);
+      }
     }
   }
 
diff --git a/clang/test/Analysis/Checkers/WebKit/mock-system-header.h b/clang/test/Analysis/Checkers/WebKit/mock-system-header.h
index e993fd697ffab..b377f5098c002 100644
--- a/clang/test/Analysis/Checkers/WebKit/mock-system-header.h
+++ b/clang/test/Analysis/Checkers/WebKit/mock-system-header.h
@@ -29,3 +29,14 @@ enum os_log_type_t : uint8_t {
 typedef struct os_log_s *os_log_t;
 os_log_t os_log_create(const char *subsystem, const char *category);
 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;
+
+#ifdef __OBJC__
+@class NSString;
+@interface SystemObject : NSObject {
+  NSString *ns_string;
+  CFStringRef cf_string;
+}
+@end
+#endif
diff --git a/clang/test/Analysis/Checkers/WebKit/unretained-members.mm b/clang/test/Analysis/Checkers/WebKit/unretained-members.mm
index e068a583c18c5..79f7a05caa1be 100644
--- a/clang/test/Analysis/Checkers/WebKit/unretained-members.mm
+++ b/clang/test/Analysis/Checkers/WebKit/unretained-members.mm
@@ -1,7 +1,8 @@
 // RUN: %clang_analyze_cc1 -analyzer-checker=alpha.webkit.NoUnretainedMemberChecker -verify %s
 
 #include "objc-mock-types.h"
-
+#include "mock-system-header.h"
+#if 0
 namespace members {
 
   struct Foo {
@@ -58,3 +59,12 @@ void forceTmplToInstantiate(FooTmpl<SomeObj, CFMutableArrayRef>) {}
 
   void forceTmplToInstantiate(RefPtr<SomeObj>) {}
 }
+#endif
+
+@interface AnotherObject : NSObject {
+  NSString *ns_string;
+  // expected-warning@-1{{Instance variable 'ns_string' in 'AnotherObject' is a raw pointer to retainable type 'NSString'; member variables must be a RetainPtr}}
+  CFStringRef cf_string;
+  // expected-warning@-1{{Instance variable 'cf_string' in 'AnotherObject' is a retainable type 'CFStringRef'; member variables must be a RetainPtr}}
+}
+@end

llvmbot · 2025-03-24T21:25:25Z

@llvm/pr-subscribers-clang

Author: Ryosuke Niwa (rniwa)

Changes

This PR fixes the bug that we weren't generating warnings when a raw poiner is used to point to a NS type in Objective-C ivars. Also fix the bug that we weren't suppressing this warning in system headers.

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

3 Files Affected:

(modified) clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefMemberChecker.cpp (+10)
(modified) clang/test/Analysis/Checkers/WebKit/mock-system-header.h (+11)
(modified) clang/test/Analysis/Checkers/WebKit/unretained-members.mm (+11-1)

diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefMemberChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefMemberChecker.cpp
index dc4e2c7d168fb..6d20869043358 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefMemberChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefMemberChecker.cpp
@@ -111,6 +111,8 @@ class RawPtrRefMemberChecker
   }
 
   void visitObjCDecl(const ObjCContainerDecl *CD) const {
+    if (BR->getSourceManager().isInSystemHeader(CD->getLocation()))
+      return;
     if (auto *ID = dyn_cast<ObjCInterfaceDecl>(CD)) {
       for (auto *Ivar : ID->ivars())
         visitIvarDecl(CD, Ivar);
@@ -133,6 +135,14 @@ class RawPtrRefMemberChecker
       std::optional<bool> IsCompatible = isPtrCompatible(QT, IvarCXXRD);
       if (IsCompatible && *IsCompatible)
         reportBug(Ivar, IvarType, IvarCXXRD, CD);
+    } else {
+      std::optional<bool> IsCompatible = isPtrCompatible(QT, nullptr);
+      auto *PointeeType = IvarType->getPointeeType().getTypePtrOrNull();
+      if (IsCompatible && *IsCompatible) {
+        auto *Desugared = PointeeType->getUnqualifiedDesugaredType();
+        if (auto *ObjCType = dyn_cast_or_null<ObjCInterfaceType>(Desugared))
+          reportBug(Ivar, IvarType, ObjCType->getDecl(), CD);
+      }
     }
   }
 
diff --git a/clang/test/Analysis/Checkers/WebKit/mock-system-header.h b/clang/test/Analysis/Checkers/WebKit/mock-system-header.h
index e993fd697ffab..b377f5098c002 100644
--- a/clang/test/Analysis/Checkers/WebKit/mock-system-header.h
+++ b/clang/test/Analysis/Checkers/WebKit/mock-system-header.h
@@ -29,3 +29,14 @@ enum os_log_type_t : uint8_t {
 typedef struct os_log_s *os_log_t;
 os_log_t os_log_create(const char *subsystem, const char *category);
 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;
+
+#ifdef __OBJC__
+@class NSString;
+@interface SystemObject : NSObject {
+  NSString *ns_string;
+  CFStringRef cf_string;
+}
+@end
+#endif
diff --git a/clang/test/Analysis/Checkers/WebKit/unretained-members.mm b/clang/test/Analysis/Checkers/WebKit/unretained-members.mm
index e068a583c18c5..79f7a05caa1be 100644
--- a/clang/test/Analysis/Checkers/WebKit/unretained-members.mm
+++ b/clang/test/Analysis/Checkers/WebKit/unretained-members.mm
@@ -1,7 +1,8 @@
 // RUN: %clang_analyze_cc1 -analyzer-checker=alpha.webkit.NoUnretainedMemberChecker -verify %s
 
 #include "objc-mock-types.h"
-
+#include "mock-system-header.h"
+#if 0
 namespace members {
 
   struct Foo {
@@ -58,3 +59,12 @@ void forceTmplToInstantiate(FooTmpl<SomeObj, CFMutableArrayRef>) {}
 
   void forceTmplToInstantiate(RefPtr<SomeObj>) {}
 }
+#endif
+
+@interface AnotherObject : NSObject {
+  NSString *ns_string;
+  // expected-warning@-1{{Instance variable 'ns_string' in 'AnotherObject' is a raw pointer to retainable type 'NSString'; member variables must be a RetainPtr}}
+  CFStringRef cf_string;
+  // expected-warning@-1{{Instance variable 'cf_string' in 'AnotherObject' is a retainable type 'CFStringRef'; member variables must be a RetainPtr}}
+}
+@end

t-rasmud · 2025-03-26T22:06:01Z

clang/test/Analysis/Checkers/WebKit/unretained-members.mm

+  CFStringRef cf_string;
+  // expected-warning@-1{{Instance variable 'cf_string' in 'AnotherObject' is a retainable type 'CFStringRef'; member variables must be a RetainPtr}}
+}
+@end


This is more of a question for my understanding: Do we expect the same behavior for raw pointers that are properties?

@interface AnotherObject : NSObject @property(nonatomic,strong) NSString *prop_string; @end

I think so.

Can we add a test case for properties?

Turns out I have to enumerate properties separately from ivars. Added a test with code fix.

t-rasmud

LGTM!

rniwa · 2025-03-28T18:58:38Z

Thanks for the review!

rniwa · 2025-03-28T19:07:19Z

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

+      return;
+    if (auto *PropCXXRD = PropType->getPointeeCXXRecordDecl()) {
+      std::optional<bool> IsCompatible = isPtrCompatible(QT, PropCXXRD);
+      fprintf(stderr, "IsCompatible=%d\n", IsCompatible ? *IsCompatible : -1);


Oh oops, I forgot to remove this.

…ive-C types in ivars. (llvm#132833) This PR fixes the bug that we weren't generating warnings when a raw poiner is used to point to a NS type in Objective-C ivars. Also fix the bug that we weren't suppressing this warning in system headers.

rniwa requested review from haoNoQ and t-rasmud March 24, 2025 21:24

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

Revert erroneous #if 0 ~ #endif

d72d243

t-rasmud reviewed Mar 26, 2025

View reviewed changes

Also add the support for detecting raw pointer property with a test.

c3796e7

t-rasmud approved these changes Mar 28, 2025

View reviewed changes

rniwa commented Mar 28, 2025

View reviewed changes

rniwa added 2 commits March 28, 2025 12:31

Remove fprintf logging for debugging purposes

a46ce85

Fix uncounted-obj-arg.mm

800f4ca

rniwa merged commit 304b3c5 into llvm:main Mar 29, 2025
11 checks passed

rniwa deleted the fix-RawPtrRefMemberChecker-retainable-ivar branch March 29, 2025 00:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[alpha.webkit.RawPtrRefMemberChecker] The checker doesn't warn Objective-C types in ivars. #132833

[alpha.webkit.RawPtrRefMemberChecker] The checker doesn't warn Objective-C types in ivars. #132833

Uh oh!

rniwa commented Mar 24, 2025

Uh oh!

llvmbot commented Mar 24, 2025

Uh oh!

llvmbot commented Mar 24, 2025

Uh oh!

t-rasmud Mar 26, 2025

Uh oh!

rniwa Mar 26, 2025

Uh oh!

t-rasmud Mar 27, 2025

Uh oh!

rniwa Mar 27, 2025

Uh oh!

t-rasmud left a comment

Uh oh!

rniwa commented Mar 28, 2025

Uh oh!

rniwa Mar 28, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

[alpha.webkit.RawPtrRefMemberChecker] The checker doesn't warn Objective-C types in ivars. #132833

[alpha.webkit.RawPtrRefMemberChecker] The checker doesn't warn Objective-C types in ivars. #132833

Uh oh!

Conversation

rniwa commented Mar 24, 2025

Uh oh!

llvmbot commented Mar 24, 2025

Uh oh!

llvmbot commented Mar 24, 2025

Uh oh!

t-rasmud Mar 26, 2025

Choose a reason for hiding this comment

Uh oh!

rniwa Mar 26, 2025

Choose a reason for hiding this comment

Uh oh!

t-rasmud Mar 27, 2025

Choose a reason for hiding this comment

Uh oh!

rniwa Mar 27, 2025

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 Mar 28, 2025

Uh oh!

rniwa Mar 28, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants