Make it safe to store a ThreadSafeRefCounted object in Ref & RefPtr safe inside its destructor
https://bugs.webkit.org/show_bug.cgi?id=201576
Source/JavaScriptCore:
<rdar://problem/56001847>
Reviewed by Geoffrey Garen and Mark Lam.
Made DropAllLocks::DropAllLocks check Heap::isShuttingDown instead of VM's refCount being 0 to detect
when VM is getting destroyed.
* runtime/JSLock.cpp:
(JSC::JSLock::DropAllLocks::DropAllLocks):
Source/WTF:
Reviewed by Geoffrey Garen and Mark Lam.
This patch leaves m_refCount 1 inside the last deref call to ThreadSafeRefCounted
so that storing an instance of this object in Ref or RefPtr would not trigger a recursive delete.
Note: this patch does not try to fix a race condition by which another thread tries to ref()
this object after the "last" call to deref had happened since such a code would ref() this object
long after it had been removed (UAF), nor some code calling deref() before calling ref() inside
a destructor since there is no way to defend against unpaired calls to ref() & deref() like that.
Also added m_deletionHasBegun like RefCounted.
* wtf/ThreadSafeRefCounted.h:
(WTF::ThreadSafeRefCountedBase::ref const):
(WTF::ThreadSafeRefCountedBase::hasOneRef const):
(WTF::ThreadSafeRefCountedBase::derefBase const):
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@251034 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
index 13c6cd3..b6fcc65 100644
--- a/Source/JavaScriptCore/ChangeLog
+++ b/Source/JavaScriptCore/ChangeLog
@@ -1,3 +1,17 @@
+2019-10-10 Ryosuke Niwa <rniwa@webkit.org>
+
+ Make it safe to store a ThreadSafeRefCounted object in Ref & RefPtr safe inside its destructor
+ https://bugs.webkit.org/show_bug.cgi?id=201576
+ <rdar://problem/56001847>
+
+ Reviewed by Geoffrey Garen and Mark Lam.
+
+ Made DropAllLocks::DropAllLocks check Heap::isShuttingDown instead of VM's refCount being 0 to detect
+ when VM is getting destroyed.
+
+ * runtime/JSLock.cpp:
+ (JSC::JSLock::DropAllLocks::DropAllLocks):
+
2019-10-11 Keith Miller <keith_miller@apple.com>
Wasm B3IRGenerator should use arguments for control data.
diff --git a/Source/JavaScriptCore/runtime/JSLock.cpp b/Source/JavaScriptCore/runtime/JSLock.cpp
index daf0f77..0427fdd 100644
--- a/Source/JavaScriptCore/runtime/JSLock.cpp
+++ b/Source/JavaScriptCore/runtime/JSLock.cpp
@@ -279,7 +279,7 @@
// If the VM is in the middle of being destroyed then we don't want to resurrect it
// by allowing DropAllLocks to ref it. By this point the JSLock has already been
// released anyways, so it doesn't matter that DropAllLocks is a no-op.
- , m_vm(vm->refCount() ? vm : nullptr)
+ , m_vm(vm->heap.isShuttingDown() ? nullptr : vm)
{
if (!m_vm)
return;
diff --git a/Source/WTF/ChangeLog b/Source/WTF/ChangeLog
index 07f2469..ef97fb1 100644
--- a/Source/WTF/ChangeLog
+++ b/Source/WTF/ChangeLog
@@ -1,3 +1,25 @@
+2019-10-10 Ryosuke Niwa <rniwa@webkit.org>
+
+ Make it safe to store a ThreadSafeRefCounted object in Ref & RefPtr safe inside its destructor
+ https://bugs.webkit.org/show_bug.cgi?id=201576
+
+ Reviewed by Geoffrey Garen and Mark Lam.
+
+ This patch leaves m_refCount 1 inside the last deref call to ThreadSafeRefCounted
+ so that storing an instance of this object in Ref or RefPtr would not trigger a recursive delete.
+
+ Note: this patch does not try to fix a race condition by which another thread tries to ref()
+ this object after the "last" call to deref had happened since such a code would ref() this object
+ long after it had been removed (UAF), nor some code calling deref() before calling ref() inside
+ a destructor since there is no way to defend against unpaired calls to ref() & deref() like that.
+
+ Also added m_deletionHasBegun like RefCounted.
+
+ * wtf/ThreadSafeRefCounted.h:
+ (WTF::ThreadSafeRefCountedBase::ref const):
+ (WTF::ThreadSafeRefCountedBase::hasOneRef const):
+ (WTF::ThreadSafeRefCountedBase::derefBase const):
+
2019-10-11 Alex Christensen <achristensen@webkit.org>
Only use CFNetwork SPI for metrics where needed
diff --git a/Source/WTF/wtf/ThreadSafeRefCounted.h b/Source/WTF/wtf/ThreadSafeRefCounted.h
index 8a624b2..0561f20 100644
--- a/Source/WTF/wtf/ThreadSafeRefCounted.h
+++ b/Source/WTF/wtf/ThreadSafeRefCounted.h
@@ -32,19 +32,39 @@
namespace WTF {
+#if defined(NDEBUG) && !ENABLE(SECURITY_ASSERTIONS)
+#define CHECK_THREAD_SAFE_REF_COUNTED_LIFECYCLE 0
+#else
+#define CHECK_THREAD_SAFE_REF_COUNTED_LIFECYCLE 1
+#endif
+
class ThreadSafeRefCountedBase {
WTF_MAKE_NONCOPYABLE(ThreadSafeRefCountedBase);
WTF_MAKE_FAST_ALLOCATED;
public:
ThreadSafeRefCountedBase() = default;
+#if CHECK_THREAD_SAFE_REF_COUNTED_LIFECYCLE
+ ~ThreadSafeRefCountedBase()
+ {
+ // When this ThreadSafeRefCounted object is a part of another object, derefBase() is never called on this object.
+ m_deletionHasBegun = true;
+ }
+#endif
+
void ref() const
{
+#if CHECK_THREAD_SAFE_REF_COUNTED_LIFECYCLE
+ ASSERT_WITH_SECURITY_IMPLICATION(!m_deletionHasBegun);
+#endif
++m_refCount;
}
bool hasOneRef() const
{
+#if CHECK_THREAD_SAFE_REF_COUNTED_LIFECYCLE
+ ASSERT(!m_deletionHasBegun);
+#endif
return refCount() == 1;
}
@@ -57,11 +77,31 @@
// Returns whether the pointer should be freed or not.
bool derefBase() const
{
- return !--m_refCount;
+ ASSERT(m_refCount);
+
+#if CHECK_THREAD_SAFE_REF_COUNTED_LIFECYCLE
+ ASSERT_WITH_SECURITY_IMPLICATION(!m_deletionHasBegun);
+#endif
+
+ if (UNLIKELY(!--m_refCount)) {
+ // Setting m_refCount to 1 here prevents double delete within the destructor but not from another thread
+ // since such a thread could have ref'ed this object long after it had been deleted. See webkit.org/b/201576.
+ m_refCount = 1;
+#if CHECK_THREAD_SAFE_REF_COUNTED_LIFECYCLE
+ m_deletionHasBegun = true;
+#endif
+ return true;
+ }
+
+ return false;
}
private:
mutable std::atomic<unsigned> m_refCount { 1 };
+
+#if CHECK_THREAD_SAFE_REF_COUNTED_LIFECYCLE
+ mutable std::atomic<bool> m_deletionHasBegun { false };
+#endif
};
enum class DestructionThread { Any, Main, MainRunLoop };
