Author: Ryosuke Niwa Date: 2026-05-27T13:52:44-07:00 New Revision: bf005a1227a4822c7c2535dd5f5f3626fbe441b2
URL: https://github.com/llvm/llvm-project/commit/bf005a1227a4822c7c2535dd5f5f3626fbe441b2 DIFF: https://github.com/llvm/llvm-project/commit/bf005a1227a4822c7c2535dd5f5f3626fbe441b2.diff LOG: [alpha.webkit.UnretainedCallArgsChecker] Emit a warning for a non-const RetainPtr member (#184243) This PR fixes a bug in UnretainedCallArgsChecker that it wouldn't emit a warning when calling a function with the return value of a getter of a RetainPtr non-const member variable even if such a member variable could be mutated during such a function call. The bug caused by tryToFindPtrOrigin treating any call of a getter on a smart pointer member variable as safe. Fixed the bug by limiting this to only when the variable is a local variable or a function parameter. In addition, this PR fixes a bug in WebKit checkers that it would erroneously emit a warning when calling a getter on a const RetainPtr member variable beacuse isOwnerPtr was returning false for RetainPtr. This false negative was previously masked / hidden by the false positive fixed in this PR. --------- Co-authored-by: Balázs Benics <[email protected]> Added: clang/test/Analysis/Checkers/WebKit/unretained-call-args-member.mm Modified: clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp clang/test/Analysis/Checkers/WebKit/call-args.cpp clang/test/Analysis/Checkers/WebKit/objc-mock-types.h Removed: ################################################################################ diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp index 8f5f2abf17f02..7b4d231620314 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp @@ -108,9 +108,14 @@ bool tryToFindPtrOrigin( if (auto *decl = memberCall->getMethodDecl()) { std::optional<bool> IsGetterOfRefCt = isGetterOfSafePtr(decl); if (IsGetterOfRefCt && *IsGetterOfRefCt) { - E = memberCall->getImplicitObjectArgument(); - if (StopAtFirstRefCountedObj) { - return callback(E, true); + E = memberCall->getImplicitObjectArgument()->IgnoreParenCasts(); + if (auto *DRE = dyn_cast<DeclRefExpr>(E)) { + if (auto *Decl = dyn_cast_or_null<VarDecl>(DRE->getDecl())) { + if (Decl->isLocalVarDeclOrParm()) { + if (StopAtFirstRefCountedObj) + return callback(E, true); + } + } } continue; } diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp index ab431af8ff391..f1515701cc6f3 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp @@ -139,8 +139,8 @@ bool isCheckedPtr(const std::string &Name) { } bool isOwnerPtr(const std::string &Name) { - return isRefType(Name) || isCheckedPtr(Name) || Name == "unique_ptr" || - Name == "UniqueRef" || Name == "LazyUniqueRef"; + return isRefType(Name) || isCheckedPtr(Name) || isRetainPtrOrOSPtr(Name) || + Name == "unique_ptr" || Name == "UniqueRef" || Name == "LazyUniqueRef"; } static bool isWeakPtrClass(const std::string &Name) { diff --git a/clang/test/Analysis/Checkers/WebKit/call-args.cpp b/clang/test/Analysis/Checkers/WebKit/call-args.cpp index fc829c45da9c5..f15991134c58a 100644 --- a/clang/test/Analysis/Checkers/WebKit/call-args.cpp +++ b/clang/test/Analysis/Checkers/WebKit/call-args.cpp @@ -500,6 +500,48 @@ namespace call_with_adopt_ref { } } +namespace call_on_member { + + class SomeObj { + public: + static Ref<SomeObj> create() { return adoptRef(*new SomeObj); } + + void ref() const; + void deref() const; + + void doWork() { + m_obj->method(); + // expected-warning@-1{{Call argument for 'this' parameter is uncounted and unsafe}} + m_obj.get()->method(); + // expected-warning@-1{{Call argument for 'this' parameter is uncounted and unsafe}} + m_constObj->method(); + } + + void localWork() { + RefPtr obj = provide(); + obj->method(); + obj.get()->method(); + } + + void argWork(RefPtr<RefCountable> arg) { + arg->method(); + arg.get()->method(); + } + + void temporaryWork() { + RefPtr { provide() }->method(); + RefPtr { provide() }.get()->method(); + } + + void work(); + + private: + RefPtr<RefCountable> m_obj; + const RefPtr<RefCountable> m_constObj; + }; + +} + namespace call_with_weak_ptr { class RefCountableWithWeakPtr : public RefCountable, public CanMakeWeakPtr<RefCountableWithWeakPtr> { diff --git a/clang/test/Analysis/Checkers/WebKit/objc-mock-types.h b/clang/test/Analysis/Checkers/WebKit/objc-mock-types.h index 503d11bea3089..ff6c133940ca9 100644 --- a/clang/test/Analysis/Checkers/WebKit/objc-mock-types.h +++ b/clang/test/Analysis/Checkers/WebKit/objc-mock-types.h @@ -447,6 +447,9 @@ template<typename T> static inline void releaseOSObject(T ptr) template<typename T> class OSObjectPtr { public: + using ValueType = typename RemovePointer<T>::Type; + using PtrType = ValueType*; + OSObjectPtr() : m_ptr(nullptr) { @@ -460,6 +463,7 @@ template<typename T> class OSObjectPtr { T get() const { return m_ptr; } + operator PtrType() const { return m_ptr; } explicit operator bool() const { return m_ptr; } bool operator!() const { return !m_ptr; } diff --git a/clang/test/Analysis/Checkers/WebKit/unretained-call-args-member.mm b/clang/test/Analysis/Checkers/WebKit/unretained-call-args-member.mm new file mode 100644 index 0000000000000..8f48927a0c62a --- /dev/null +++ b/clang/test/Analysis/Checkers/WebKit/unretained-call-args-member.mm @@ -0,0 +1,184 @@ +// UNSUPPORTED: target={{.*}}-zos{{.*}}, target={{.*}}-aix{{.*}} +// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.webkit.UnretainedCallArgsChecker -verify %s + +#include "objc-mock-types.h" + +void consume_cf(CFMutableArrayRef); +void consume_obj(SomeObj *); + +namespace call_args_const_retainptr_member { + +class Foo { +public: + Foo(); + void bar(); + +private: + const RetainPtr<SomeObj> m_constObj; + RetainPtr<SomeObj> m_obj; +}; + +void Foo::bar() { + [m_constObj doWork]; // no-warning + [m_obj doWork]; // expected-warning{{Receiver is unretained and unsafe}} +} + +} // namespace call_args_const_retainptr_member + +namespace call_args_const_retainptr_cf_member { + +class Foo { +public: + Foo(); + void bar(); + +private: + const RetainPtr<CFMutableArrayRef> m_cf1; + RetainPtr<CFMutableArrayRef> m_cf2; +}; + +void Foo::bar() { + consume_cf(m_cf1.get()); // no-warning + consume_cf(m_cf2.get()); // expected-warning{{Call argument is unretained and unsafe}} +} + +} // namespace call_args_const_retainptr_cf_member + +namespace call_args_const_retainptr_struct_member { + +struct Bar { + Bar(); + void baz(); + + const RetainPtr<SomeObj> m_constObj; + RetainPtr<SomeObj> m_obj; +}; + +void Bar::baz() { + [m_constObj doWork]; // no-warning + [m_obj doWork]; // expected-warning{{Receiver is unretained and unsafe}} +} + +} // namespace call_args_const_retainptr_struct_member + +namespace call_args_const_retainptr_cf_struct_member { + +struct Bar { + Bar(); + void baz(); + + const RetainPtr<CFMutableArrayRef> m_cf1; + RetainPtr<CFMutableArrayRef> m_cf2; +}; + +void Bar::baz() { + consume_cf(m_cf1.get()); // no-warning + consume_cf(m_cf2.get()); // expected-warning{{Call argument is unretained and unsafe}} +} + +} // namespace call_args_const_retainptr_cf_struct_member + +namespace call_args_const_retainptr_get_as_objc_arg { + +class Foo { +public: + Foo(); + void bar(); + +private: + const RetainPtr<SomeObj> m_constObj; + RetainPtr<SomeObj> m_obj; +}; + +void Foo::bar() { + consume_obj(m_constObj.get()); // no-warning + consume_obj(m_obj.get()); // expected-warning{{Call argument is unretained and unsafe}} +} + +} // namespace call_args_const_retainptr_get_as_objc_arg + +namespace call_args_const_retainptr_implicit_conv_arg { + +class Foo { +public: + Foo(); + void bar(); + +private: + const RetainPtr<SomeObj> m_constObj; + RetainPtr<SomeObj> m_obj; +}; + +void Foo::bar() { + consume_obj(m_constObj); // no-warning + consume_obj(m_obj); // expected-warning{{Call argument is unretained and unsafe}} +} + +} // namespace call_args_const_retainptr_implicit_conv_arg + +namespace call_args_const_osobjectptr_member { + +class Foo { +public: + Foo(); + void bar(); + +private: + const OSObjectPtr<SomeObj *> m_constObj; + OSObjectPtr<SomeObj *> m_obj; +}; + +void Foo::bar() { + consume_obj(m_constObj.get()); // no-warning + consume_obj(m_obj.get()); // expected-warning{{Call argument is unretained and unsafe}} +} + +} // namespace call_args_const_osobjectptr_member + +namespace call_args_const_osobjectptr_receiver { + +class Foo { +public: + Foo(); + void bar(); + +private: + const OSObjectPtr<SomeObj *> m_constObj; + OSObjectPtr<SomeObj *> m_obj; +}; + +void Foo::bar() { + [m_constObj doWork]; // no-warning + [m_obj doWork]; // expected-warning{{Receiver is unretained and unsafe}} +} + +} // namespace call_args_const_osobjectptr_receiver + +namespace call_args_retainptr_local { + +void testLocal(SomeObj *input) { + RetainPtr<SomeObj> localObj = input; + [localObj doWork]; // no-warning + consume_obj(localObj.get()); // no-warning + consume_cf(RetainPtr<CFMutableArrayRef>().get()); // no-warning +} + +} // namespace call_args_retainptr_local + +namespace call_args_retainptr_protected_member { + +class Foo { +public: + Foo(); + void bar(); + +private: + RetainPtr<SomeObj> m_obj; +}; + +void Foo::bar() { + auto protectedObj = m_obj; + [protectedObj doWork]; // no-warning (local copy is safe) +} + +} // namespace call_args_retainptr_protected_member _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
