Skip to content

Commit c13321a

Browse files
geoffreygarengeoffreygaren
committed
Added a compile time assertion that forbids classes that offer WeakPtr
with no corresponding RefPtr or CheckedPtr https://bugs.webkit.org/show_bug.cgi?id=273569 rdar://127296969 Reviewed by Ryosuke Niwa. An object that offers WeakPtr with no corresponding RefPtr or CheckedPtr is a dangerous contradiction. On the one hand, we know that it can be deleted at any time. On the other hand, when we go to use it, we do nothing to ensure its lifetime. This has been a source of use after free / security bugs. This patch adds a compile time assertion against future uses of this anti-pattern. An explicit template specialization allow list maintains existing uses. The allow list is a todo list for future deployment of RefPtr or CheckedPtr. This patch does not enforce correct usage of RefPtr or CheckedPtr; it only enforces a rule that a class must at least offer RefPtr or CheckedPtr if it offers WeakPtr. In order to facilitate the allow list, I needed to * Move some nested classes out-of-line, since C++ does not support forward declaration or template specialization of nested classes * Move some destructors out-of-line, since our compile time assertion requires a complete type definition, and I didn't want to increase #includes in headers Canonical link: https://commits.webkit.org/278224@main
1 parent 1d96c31 commit c13321a

569 files changed

Lines changed: 4731 additions & 1370 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎Source/JavaScriptCore/inspector/InspectorTarget.h‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,15 @@
2929
#include <wtf/WeakPtr.h>
3030
#include <wtf/text/WTFString.h>
3131

32+
namespace Inspector {
33+
class InspectorTarget;
34+
}
35+
36+
namespace WTF {
37+
template<typename T> struct IsDeprecatedWeakRefSmartPointerException;
38+
template<> struct IsDeprecatedWeakRefSmartPointerException<Inspector::InspectorTarget> : std::true_type { };
39+
}
40+
3241
namespace Inspector {
3342

3443
// FIXME: Add DedicatedWorker Inspector Targets

‎Source/JavaScriptCore/runtime/ConsoleClient.h‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,15 @@
2929
#include <wtf/Forward.h>
3030
#include <wtf/WeakPtr.h>
3131

32+
namespace JSC {
33+
class ConsoleClient;
34+
}
35+
36+
namespace WTF {
37+
template<typename T> struct IsDeprecatedWeakRefSmartPointerException;
38+
template<> struct IsDeprecatedWeakRefSmartPointerException<JSC::ConsoleClient> : std::true_type { };
39+
}
40+
3241
namespace Inspector {
3342
class ScriptArguments;
3443
}

‎Source/JavaScriptCore/runtime/JSGlobalObject.cpp‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3158,6 +3158,11 @@ void JSGlobalObject::setConsoleClient(WeakPtr<ConsoleClient>&& consoleClient)
31583158
m_consoleClient = WTFMove(consoleClient);
31593159
}
31603160

3161+
WeakPtr<ConsoleClient> JSGlobalObject::consoleClient() const
3162+
{
3163+
return m_consoleClient;
3164+
}
3165+
31613166
void JSGlobalObject::setDebugger(Debugger* debugger)
31623167
{
31633168
m_debugger = debugger;

‎Source/JavaScriptCore/runtime/JSGlobalObject.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -894,7 +894,7 @@ class JSGlobalObject : public JSSegmentedVariableObject {
894894
unsigned* addressOfGlobalLexicalBindingEpoch() { return &m_globalLexicalBindingEpoch; }
895895

896896
JS_EXPORT_PRIVATE void setConsoleClient(WeakPtr<ConsoleClient>&&);
897-
WeakPtr<ConsoleClient> consoleClient() const { return m_consoleClient; }
897+
JS_EXPORT_PRIVATE WeakPtr<ConsoleClient> consoleClient() const;
898898

899899
void setName(const String&);
900900
const String& name() const { return m_name; }

‎Source/WTF/wtf/CancellableTask.h‎

Lines changed: 39 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -29,37 +29,55 @@
2929
#include <wtf/UniqueRef.h>
3030
#include <wtf/WeakPtr.h>
3131

32+
namespace WTF {
33+
class TaskCancellationGroup;
34+
class TaskCancellationGroupImpl;
35+
}
36+
37+
namespace WTF {
38+
template<typename T> struct IsDeprecatedWeakRefSmartPointerException;
39+
template<> struct IsDeprecatedWeakRefSmartPointerException<WTF::TaskCancellationGroup> : std::true_type { };
40+
template<> struct IsDeprecatedWeakRefSmartPointerException<WTF::TaskCancellationGroupImpl> : std::true_type { };
41+
}
42+
3243
namespace WTF {
3344

3445
class CancellableTask;
3546

47+
class TaskCancellationGroupImpl : public CanMakeWeakPtr<TaskCancellationGroupImpl> {
48+
WTF_MAKE_FAST_ALLOCATED;
49+
public:
50+
void cancel() { weakPtrFactory().revokeAll(); }
51+
bool hasPendingTask() const { return weakPtrFactory().weakPtrCount(); }
52+
};
53+
54+
class TaskCancellationGroupHandle {
55+
public:
56+
bool isCancelled() const { return !m_impl; }
57+
void clear() { m_impl = nullptr; }
58+
private:
59+
friend class TaskCancellationGroup;
60+
explicit TaskCancellationGroupHandle(TaskCancellationGroupImpl& impl)
61+
: m_impl(impl)
62+
{
63+
}
64+
WeakPtr<TaskCancellationGroupImpl> m_impl;
65+
};
66+
3667
class TaskCancellationGroup : public CanMakeWeakPtr<TaskCancellationGroup> {
3768
public:
38-
TaskCancellationGroup() : m_impl(makeUniqueRef<Impl>()) { }
69+
TaskCancellationGroup()
70+
: m_impl(makeUniqueRef<TaskCancellationGroupImpl>())
71+
{
72+
}
3973
void cancel() { m_impl->cancel(); }
4074
bool hasPendingTask() const { return m_impl->hasPendingTask(); }
4175

4276
private:
4377
friend class CancellableTask;
44-
class Impl : public CanMakeWeakPtr<Impl> {
45-
WTF_MAKE_FAST_ALLOCATED;
46-
public:
47-
void cancel() { weakPtrFactory().revokeAll(); }
48-
bool hasPendingTask() const { return weakPtrFactory().weakPtrCount(); }
49-
};
50-
51-
class Handle {
52-
public:
53-
bool isCancelled() const { return !m_impl; }
54-
void clear() { m_impl = nullptr; }
55-
private:
56-
friend class TaskCancellationGroup;
57-
explicit Handle(Impl& impl) : m_impl(impl) { }
58-
WeakPtr<Impl> m_impl;
59-
};
60-
Handle createHandle() { return Handle { m_impl }; }
61-
62-
UniqueRef<Impl> m_impl;
78+
TaskCancellationGroupHandle createHandle() { return TaskCancellationGroupHandle { m_impl }; }
79+
80+
UniqueRef<TaskCancellationGroupImpl> m_impl;
6381
};
6482

6583
class CancellableTask {
@@ -68,7 +86,7 @@ class CancellableTask {
6886
void operator()();
6987

7088
private:
71-
TaskCancellationGroup::Handle m_cancellationGroup;
89+
TaskCancellationGroupHandle m_cancellationGroup;
7290
Function<void()> m_task;
7391
};
7492

‎Source/WTF/wtf/ListHashSet.h‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,19 @@
2727
#include <wtf/WeakPtr.h>
2828
#endif
2929

30+
namespace WTF {
31+
template<typename Value, typename HashFunctions> class ListHashSet;
32+
template<typename ValueArg> struct ListHashSetNode;
33+
template<typename ValueArg, typename HashArg> class ListHashSetConstIterator;
34+
}
35+
36+
namespace WTF {
37+
template<typename T> struct IsDeprecatedWeakRefSmartPointerException;
38+
template<typename Value, typename HashFunctions> struct IsDeprecatedWeakRefSmartPointerException<WTF::ListHashSet<Value, HashFunctions>> : std::true_type { };
39+
template<typename ValueArg> struct IsDeprecatedWeakRefSmartPointerException<WTF::ListHashSetNode<ValueArg>> : std::true_type { };
40+
template<typename ValueArg, typename HashArg> struct IsDeprecatedWeakRefSmartPointerException<WTF::ListHashSetConstIterator<ValueArg, HashArg>> : std::true_type { };
41+
}
42+
3043
namespace WTF {
3144

3245
// ListHashSet: Just like HashSet, this class provides a Set

‎Source/WTF/wtf/NativePromise.h‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,15 @@
5151
#include <wtf/Vector.h>
5252
#include <wtf/WeakPtr.h>
5353

54+
namespace WTF {
55+
class NativePromiseRequest;
56+
}
57+
58+
namespace WTF {
59+
template<typename T> struct IsDeprecatedWeakRefSmartPointerException;
60+
template<> struct IsDeprecatedWeakRefSmartPointerException<WTF::NativePromiseRequest> : std::true_type { };
61+
}
62+
5463
namespace WTF {
5564

5665
/*

‎Source/WTF/wtf/Observer.h‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,15 @@
2929
#include <wtf/Noncopyable.h>
3030
#include <wtf/WeakPtr.h>
3131

32+
namespace WTF {
33+
template<typename> class Observer;
34+
}
35+
36+
namespace WTF {
37+
template<typename T> struct IsDeprecatedWeakRefSmartPointerException;
38+
template<typename Out, typename... In> struct IsDeprecatedWeakRefSmartPointerException<WTF::Observer<Out(In...)>> : std::true_type { };
39+
}
40+
3241
namespace WTF {
3342

3443
template<typename> class Observer;

‎Source/WTF/wtf/TypeTraits.h‎

Lines changed: 33 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,14 @@
3636
#include <wtf/Ref.h>
3737
#include <wtf/RefPtr.h>
3838

39+
// SFINAE depends on overload resolution. We indicate the overload we'd prefer
40+
// (if it can compile) using a higher priorty type (int), and the overload
41+
// to fall back to using a lower priority type (long). 0 can convert to int
42+
// or long, so we can trigger overload resolution using 0. C++ is awesome!
43+
#define SFINAE_OVERLOAD 0
44+
#define SFINAE_OVERLOAD_DEFAULT long
45+
#define SFINAE_OVERLOAD_PREFERRED int
46+
3947
namespace WTF {
4048

4149
namespace detail {
@@ -92,34 +100,49 @@ struct RemoveSmartPointerHelper<T, Ref<Pointee>> {
92100
template<typename T>
93101
struct RemoveSmartPointer : detail::RemoveSmartPointerHelper<T, std::remove_cv_t<T>> { };
94102

95-
// HasRefCountMethods implementation
103+
// HasRefPtrMethods implementation
96104
namespace detail {
97105

98106
template<typename>
99107
struct SFINAE1True : std::true_type { };
100108

101109
template<class T>
102-
static auto HasRefCountMethodsTest(int) -> SFINAE1True<decltype(std::declval<T>().ref(), std::declval<T>().deref())>;
110+
static auto HasRefPtrMethodsTest(SFINAE_OVERLOAD_PREFERRED) -> SFINAE1True<decltype(&T::ref, &T::deref)>;
111+
template<class>
112+
static auto HasRefPtrMethodsTest(SFINAE_OVERLOAD_DEFAULT) -> std::false_type;
113+
114+
} // namespace detail
115+
116+
template<class T>
117+
struct HasRefPtrMethods : decltype(detail::HasRefPtrMethodsTest<T>(SFINAE_OVERLOAD)) { };
118+
119+
// HasCheckedPtrMethods implementation
120+
namespace detail {
121+
122+
template<class T>
123+
static auto HasCheckedPtrMethodsTest(SFINAE_OVERLOAD_PREFERRED) -> SFINAE1True<decltype(&T::incrementPtrCount, &T::decrementPtrCount)>;
103124
template<class>
104-
static auto HasRefCountMethodsTest(long) -> std::false_type;
125+
static auto HasCheckedPtrMethodsTest(SFINAE_OVERLOAD_DEFAULT) -> std::false_type;
105126

106127
} // namespace detail
107128

108129
template<class T>
109-
struct HasRefCountMethods : decltype(detail::HasRefCountMethodsTest<T>(0)) { };
130+
struct HasCheckedPtrMethods : decltype(detail::HasCheckedPtrMethodsTest<T>(SFINAE_OVERLOAD)) { };
110131

111132
// HasIsolatedCopy()
112133
namespace detail {
113134

135+
// FIXME: This test is incorrectly false for RefCounted objects because
136+
// substitution for std::declval<T>() fails when the constructor is private.
114137
template<class T>
115-
static auto HasIsolatedCopyTest(int) -> SFINAE1True<decltype(std::declval<T>().isolatedCopy())>;
138+
static auto HasIsolatedCopyTest(SFINAE_OVERLOAD_PREFERRED) -> SFINAE1True<decltype(std::declval<T>().isolatedCopy())>;
116139
template<class>
117-
static auto HasIsolatedCopyTest(long) -> std::false_type;
140+
static auto HasIsolatedCopyTest(SFINAE_OVERLOAD_DEFAULT) -> std::false_type;
118141

119142
} // namespace detail
120143

121144
template<class T>
122-
struct HasIsolatedCopy : decltype(detail::HasIsolatedCopyTest<T>(0)) { };
145+
struct HasIsolatedCopy : decltype(detail::HasIsolatedCopyTest<T>(SFINAE_OVERLOAD)) { };
123146

124147
// LooksLikeRCSerialDispatcher implementation
125148
namespace detail {
@@ -128,16 +151,16 @@ template <bool b, typename>
128151
struct SFINAE1If : std::integral_constant<bool, b> { };
129152

130153
template <bool b, class T>
131-
static auto LooksLikeRCSerialDispatcherTest(int)
154+
static auto LooksLikeRCSerialDispatcherTest(SFINAE_OVERLOAD_PREFERRED)
132155
-> SFINAE1If<b, decltype(std::declval<T>().ref(), std::declval<T>().deref())>;
133156

134157
template <bool, typename>
135-
static auto LooksLikeRCSerialDispatcherTest(long) -> std::false_type;
158+
static auto LooksLikeRCSerialDispatcherTest(SFINAE_OVERLOAD_DEFAULT) -> std::false_type;
136159

137160
} // namespace detail
138161

139162
template <class T>
140-
struct LooksLikeRCSerialDispatcher : decltype(detail::LooksLikeRCSerialDispatcherTest<std::is_base_of_v<SerialFunctionDispatcher, T>, T>(0)) { };
163+
struct LooksLikeRCSerialDispatcher : decltype(detail::LooksLikeRCSerialDispatcherTest<std::is_base_of_v<SerialFunctionDispatcher, T>, T>(SFINAE_OVERLOAD)) { };
141164

142165
class NativePromiseBase;
143166
class ConvertibleToNativePromise;

‎Source/WTF/wtf/WeakPtr.h‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,13 @@ template<typename T, typename WeakPtrImpl, typename PtrTraits> class WeakPtr {
8383
{
8484
}
8585

86+
~WeakPtr()
87+
{
88+
static_assert(
89+
HasRefPtrMethods<T>::value || HasCheckedPtrMethods<T>::value || IsDeprecatedWeakRefSmartPointerException<std::remove_const_t<T>>::value,
90+
"Classes that offer weak pointers should also offer RefPtr or CheckedPtr.");
91+
}
92+
8693
RefPtr<WeakPtrImpl, PtrTraits> releaseImpl() { return WTFMove(m_impl); }
8794

8895
T* get() const

0 commit comments

Comments
 (0)