Title: [251034] trunk/Source
- Revision
- 251034
- Author
- [email protected]
- Date
- 2019-10-11 19:19:34 -0700 (Fri, 11 Oct 2019)
Log Message
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/_javascript_Core:
<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):
Modified Paths
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (251033 => 251034)
--- trunk/Source/_javascript_Core/ChangeLog 2019-10-12 01:27:57 UTC (rev 251033)
+++ trunk/Source/_javascript_Core/ChangeLog 2019-10-12 02:19:34 UTC (rev 251034)
@@ -1,3 +1,17 @@
+2019-10-10 Ryosuke Niwa <[email protected]>
+
+ 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 <[email protected]>
Wasm B3IRGenerator should use arguments for control data.
Modified: trunk/Source/_javascript_Core/runtime/JSLock.cpp (251033 => 251034)
--- trunk/Source/_javascript_Core/runtime/JSLock.cpp 2019-10-12 01:27:57 UTC (rev 251033)
+++ trunk/Source/_javascript_Core/runtime/JSLock.cpp 2019-10-12 02:19:34 UTC (rev 251034)
@@ -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;
Modified: trunk/Source/WTF/ChangeLog (251033 => 251034)
--- trunk/Source/WTF/ChangeLog 2019-10-12 01:27:57 UTC (rev 251033)
+++ trunk/Source/WTF/ChangeLog 2019-10-12 02:19:34 UTC (rev 251034)
@@ -1,3 +1,25 @@
+2019-10-10 Ryosuke Niwa <[email protected]>
+
+ 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 <[email protected]>
Only use CFNetwork SPI for metrics where needed
Modified: trunk/Source/WTF/wtf/ThreadSafeRefCounted.h (251033 => 251034)
--- trunk/Source/WTF/wtf/ThreadSafeRefCounted.h 2019-10-12 01:27:57 UTC (rev 251033)
+++ trunk/Source/WTF/wtf/ThreadSafeRefCounted.h 2019-10-12 02:19:34 UTC (rev 251034)
@@ -32,6 +32,12 @@
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;
@@ -38,13 +44,27 @@
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 };
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes
