Skip to content

[WebKit checkers] Recognize adoptRef as a safe function #119846

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Dec 19, 2024
Merged

[WebKit checkers] Recognize adoptRef as a safe function #119846

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6b4a6b8
[WebKit checkers] Recognize adoptRef as a safe function
rniwa Dec 13, 2024
0cdeb56
Fix the mock RefPtr
rniwa Dec 13, 2024
bc318db
Added a comment regarding dummy struct for rdar://141692212
rniwa Dec 18, 2024
89004f9
Fix the tests
rniwa Dec 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,9 +125,8 @@ bool isCtorOfRefCounted(const clang::FunctionDecl *F) {
assert(F);
const std::string &FunctionName = safeGetName(F);

return isRefType(FunctionName) || FunctionName == "makeRef" ||
FunctionName == "makeRefPtr" || FunctionName == "UniqueRef" ||
FunctionName == "makeUniqueRef" ||
return isRefType(FunctionName) || FunctionName == "adoptRef" ||
FunctionName == "UniqueRef" || FunctionName == "makeUniqueRef" ||
FunctionName == "makeUniqueRefWithoutFastMallocCheck"

|| FunctionName == "String" || FunctionName == "AtomString" ||
Expand Down
18 changes: 18 additions & 0 deletions clang/test/Analysis/Checkers/WebKit/call-args.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -365,3 +365,21 @@ namespace call_with_explicit_temporary_obj {
RefPtr { provide() }->method();
}
}

namespace call_with_adopt_ref {
class Obj {
public:
void ref() const;
void deref() const;
void method();
};

// This is needed due to rdar://141692212.
struct dummy {
RefPtr<Obj> any;
Copy link
Contributor Author

@rniwa rniwa Dec 13, 2024

Choose a reason for hiding this comment

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

For whatever reason, this must exist for the bug to reproduce.

Copy link
Contributor

@t-rasmud t-rasmud Dec 18, 2024

Choose a reason for hiding this comment

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

@rniwa I ran this test case under the debugger and here's what I found: The call to IsPtrOriginSafe returns true in the absence of struct dummy when visiting the CallExpr causing the checker to not issue a warning:

CXXMemberCallExpr 0x147945af8 'void'
`-MemberExpr 0x147945ac8 '<bound member function type>' ->method 0x14793ccb0
  `-CXXOperatorCallExpr 0x147945988 'class call_with_adopt_ref::Obj *' '->'
    |-ImplicitCastExpr 0x147945970 'class call_with_adopt_ref::Obj *(*)(void) const' <FunctionToPointerDecay>
    | `-DeclRefExpr 0x1479458e8 'class call_with_adopt_ref::Obj *(void) const' lvalue CXXMethod 0x147944388 'operator->' 'class call_with_adopt_ref::Obj *(void) const'
    `-ImplicitCastExpr 0x1479458d0 'const struct RefPtr<class call_with_adopt_ref::Obj>' lvalue <NoOp>
      `-MaterializeTemporaryExpr 0x1479458b8 'RefPtr<Obj>':'struct RefPtr<class call_with_adopt_ref::Obj>' lvalue
        `-CXXBindTemporaryExpr 0x147945898 'RefPtr<Obj>':'struct RefPtr<class call_with_adopt_ref::Obj>' (CXXTemporary 0x147945898)
          `-CallExpr 0x1479407e8 'RefPtr<Obj>':'struct RefPtr<class call_with_adopt_ref::Obj>'
            |-ImplicitCastExpr 0x1479407d0 'RefPtr<Obj> (*)(class call_with_adopt_ref::Obj *)' <FunctionToPointerDecay>
            | `-DeclRefExpr 0x147940740 'RefPtr<Obj> (class call_with_adopt_ref::Obj *)' lvalue Function 0x14793fe00 'adoptRef' 'RefPtr<Obj> (class call_with_adopt_ref::Obj *)' (FunctionTemplate 0x14792ff48 'adoptRef')
            `-CXXNewExpr 0x14793f408 'Obj *' Function 0x14793d0c8 'operator new' 'void *(unsigned long)'
              `-CXXConstructExpr 0x14793f3e0 'Obj':'class call_with_adopt_ref::Obj' 'void (void) noexcept'

Does this give any clue for the unexpected behavior?

Copy link
Contributor Author

@rniwa rniwa Dec 18, 2024

Choose a reason for hiding this comment

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

It's still unclear to me why the presence of struct dummy affects the semantics of seemingly unrelated code.

Copy link
Contributor

@t-rasmud t-rasmud Dec 18, 2024

Choose a reason for hiding this comment

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

Looks like there is some bug in AST generation. The return type of callee is different for both the versions at

llvm-project/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp

Line 103 in fbc18b8

if (isSafePtrType(callee->getReturnType()))

With dummy struct:

(lldb) p callee->getReturnType()->dump()
ElaboratedType 0x1309723b0 'RefPtr<Obj>' sugar
`-RecordType 0x130971240 'struct RefPtr<class call_with_adopt_ref::Obj>'
  `-ClassTemplateSpecialization 0x130971160 'RefPtr'

Without the dummy struct:

(lldb) p callee->getReturnType()->dump()
ElaboratedType 0x12c973230 'RefPtr<Obj>' sugar
`-TemplateSpecializationType 0x12c9731e0 'RefPtr<class call_with_adopt_ref::Obj>' sugar
  |-name: 'RefPtr' qualified
  | `-ClassTemplateDecl 0x12c95ee30 prev 0x12c95e690  RefPtr
  |-TemplateArgument type 'class call_with_adopt_ref::Obj'
  | `-SubstTemplateTypeParmType 0x12c9730a0 'class call_with_adopt_ref::Obj' sugar typename depth 0 index 0 T
  |   |-FunctionTemplate 0x12c95ec18 'adoptRef'
  |   `-RecordType 0x12c970560 'class call_with_adopt_ref::Obj'
  |     `-CXXRecord 0x12c9704d0 'Obj'
  `-RecordType 0x12c9731c0 'struct RefPtr<class call_with_adopt_ref::Obj>'
    `-ClassTemplateSpecialization 0x12c9730e0 'RefPtr'

Maybe create a separate radar for this issue and add the radar ID to the comment in the test case?
Otherwise, LGTM!

Copy link
Contributor Author

@rniwa rniwa Dec 18, 2024

Choose a reason for hiding this comment

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

Filed rdar://141692212. Will add a comment to that end. Thanks for the review!

};

void foo() {
adoptRef(new Obj)->method();
}
}
36 changes: 35 additions & 1 deletion clang/test/Analysis/Checkers/WebKit/mock-types.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,10 @@ template<typename T> struct DefaultRefDerefTraits {
template <typename T, typename PtrTraits = RawPtrTraits<T>, typename RefDerefTraits = DefaultRefDerefTraits<T>> struct Ref {
typename PtrTraits::StorageType t;

enum AdoptTag { Adopt };

Ref() : t{} {};
Ref(T &t, AdoptTag) : t(&t) { }
Ref(T &t) : t(&RefDerefTraits::ref(t)) { }
Ref(const Ref& o) : t(RefDerefTraits::refIfNotNull(PtrTraits::unwrap(o.t))) { }
Ref(Ref&& o) : t(o.leakRef()) { }
Expand All @@ -73,10 +76,19 @@ template <typename T, typename PtrTraits = RawPtrTraits<T>, typename RefDerefTra
T* leakRef() { return PtrTraits::exchange(t, nullptr); }
};

template <typename T> Ref<T> adoptRef(T& t) {
using Ref = Ref<T>;
return Ref(t, Ref::Adopt);
}

template<typename T> class RefPtr;
template<typename T> RefPtr<T> adoptRef(T*);

template <typename T> struct RefPtr {
T *t;

RefPtr() : t(new T) {}
RefPtr() : t(nullptr) { }

RefPtr(T *t)
: t(t) {
if (t)
Expand All @@ -85,6 +97,17 @@ template <typename T> struct RefPtr {
RefPtr(Ref<T>&& o)
: t(o.leakRef())
{ }
RefPtr(RefPtr&& o)
: t(o.t)
{
o.t = nullptr;
}
RefPtr(const RefPtr& o)
: t(o.t)
{
if (t)
t->ref();
}
~RefPtr() {
if (t)
t->deref();
Expand All @@ -110,8 +133,19 @@ template <typename T> struct RefPtr {
return *this;
}
operator bool() const { return t; }

private:
friend RefPtr adoptRef<T>(T*);

// call_with_adopt_ref in call-args.cpp requires this method to be private.
enum AdoptTag { Adopt };
RefPtr(T *t, AdoptTag) : t(t) { }
};

template <typename T> RefPtr<T> adoptRef(T* t) {
return RefPtr<T>(t, RefPtr<T>::Adopt);
}

template <typename T> bool operator==(const RefPtr<T> &, const RefPtr<T> &) {
return false;
}
Expand Down
8 changes: 0 additions & 8 deletions clang/test/Analysis/Checkers/WebKit/ref-cntbl-crtp-base-no-virtual-dtor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,14 +61,6 @@ template<typename Out, typename... In> Function<Out(In...)> adopt(Detail::Callab
return Function<Out(In...)>(impl, Function<Out(In...)>::Adopt);
}

template<typename T, typename PtrTraits = RawPtrTraits<T>, typename RefDerefTraits = DefaultRefDerefTraits<T>> Ref<T, PtrTraits, RefDerefTraits> adoptRef(T&);

template<typename T, typename _PtrTraits, typename RefDerefTraits>
inline Ref<T, _PtrTraits, RefDerefTraits> adoptRef(T& reference)
{
return Ref<T, _PtrTraits, RefDerefTraits>(reference);
}

enum class DestructionThread : unsigned char { Any, Main, MainRunLoop };
void ensureOnMainThread(Function<void()>&&); // Sync if called on main thread, async otherwise.
void ensureOnMainRunLoop(Function<void()>&&); // Sync if called on main run loop, async otherwise.
Expand Down
Loading