Just as we made it safe to ref & deref a Node inside ~Node in the bug 195776, we should make it safe to ref & deref an object which uses ThreadSafeRefCounted inside its destructor.
Created attachment 378281 [details] Patch
Comment on attachment 378281 [details] Patch Attachment 378281 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/13009210 Number of test failures exceeded the failure limit.
Created attachment 378285 [details] Archive of layout-test-results from ews212 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews212 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment on attachment 378281 [details] Patch Attachment 378281 [details] did not pass jsc-ews (mac): Output: https://webkit-queues.webkit.org/results/13009229 New failing tests: wasm.yaml/wasm/references/anyref_globals.js.wasm-collect-continuously stress/arith-trunc-on-various-types.js.default stress/disable-gigacage-strings.js.disable-gigacage stress/regress-170253.js.default stress/get-array-length-concurrently-change-mode.js.ftl-eager stress/op-negate-inline-cache.js.bytecode-cache wasm.yaml/wasm/references/anyref_table.js.wasm-no-cjit-yes-tls-context stress/to-number-throws-correct-exception.js.bytecode-cache wasm.yaml/wasm/js-api/Instance.imports.exports.unicode.js.wasm-collect-continuously stress/super-property-access-exceptions.js.default microbenchmarks/super-get-by-id-with-this-polymorphic.js.default wasm.yaml/wasm/spec-tests/float_exprs.wast.js.wasm-no-cjit-yes-tls-context wasm.yaml/wasm/spec-tests/f32_cmp.wast.js.wasm-no-cjit-yes-tls-context wasm.yaml/wasm/spec-tests/f64_cmp.wast.js.wasm-no-cjit-yes-tls-context stress/arith-floor-on-various-types.js.default stress/spread-optimized-properly.js.ftl-eager wasm.yaml/wasm/references/anyref_globals.js.wasm-slow-memory stress/map-with-nan.js.default stress/test-out-of-memory.js.dfg-eager-no-cjit-validate wasm.yaml/wasm/js-api/wasm-to-wasm-bad-signature.js.wasm-no-air stress/call-apply-exponential-bytecode-size.js.ftl-eager-no-cjit stress/disable-gigacage-typed-arrays.js.disable-gigacage stress/arith-negate-on-various-types.js.default stress/disable-gigacage-arrays.js.disable-gigacage wasm.yaml/wasm/spec-tests/f32.wast.js.wasm-no-cjit-yes-tls-context wasm.yaml/wasm/references/anyref_globals.js.wasm-no-cjit-yes-tls-context stress/test-out-of-memory.js.ftl-eager-no-cjit stress/class-subclassing-function.js.ftl-eager stress/arith-add-on-double-array-with-holes.js.ftl-eager wasm.yaml/wasm/spec-tests/f64.wast.js.wasm-no-tls-context microbenchmarks/eval-cached.js.ftl-eager wasm.yaml/wasm/references/anyref_table.js.wasm-eager-jettison stress/to-number-throws-correct-exception.js.ftl-eager wasm.yaml/wasm/spec-tests/float_misc.wast.js.wasm-no-cjit-yes-tls-context stress/fold-multi-get-by-offset-to-get-by-offset-without-folding-the-structure-check.js.ftl-eager wasm.yaml/wasm/js-api/wasm-to-wasm-bad-signature.js.wasm-eager-jettison stress/to-number-throws-correct-exception.js.default stress/arith-ceil-on-various-types.js.default stress/tagged-template-object-collect.js.dfg-eager-no-cjit-validate stress/test-out-of-memory.js.ftl-eager stress/ftl-gettypedarrayoffset-wasteful.js.ftl-eager-no-cjit wasm.yaml/wasm/references/anyref_table.js.wasm-no-tls-context microbenchmarks/super-get-by-val-with-this-monomorphic.js.bytecode-cache apiTests
Comment on attachment 378281 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378281&action=review > Source/WTF/wtf/ThreadSafeRefCounted.h:86 > + unsigned tempRefCount = m_refCount - 1; > + if (!tempRefCount) { Oh oops, this is obviously not gonna work. Not sure what the heck I was thinking.
Created attachment 378310 [details] Patch
Hm... ThreadSafeRefCounted::deref() is pretty big. We should probably consider putting it into a separate function with NEVER_INLINE at some point.
Comment on attachment 378310 [details] Patch Attachment 378310 [details] did not pass jsc-ews (mac): Output: https://webkit-queues.webkit.org/results/13012033 New failing tests: apiTests
Comment on attachment 378310 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378310&action=review > Source/WTF/wtf/ThreadSafeRefCounted.h:79 > + if (!--m_refCount) { > + m_refCount = 1; This doesn’t seem safe. There’s a window where m_refCount is 0 before it gets changed back to 1.
(In reply to Darin Adler from comment #9) > Comment on attachment 378310 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=378310&action=review > > > Source/WTF/wtf/ThreadSafeRefCounted.h:79 > > + if (!--m_refCount) { > > + m_refCount = 1; > > This doesn’t seem safe. There’s a window where m_refCount is 0 before it > gets changed back to 1. Indeed, it's not most thread-safe code. Here's my reasoning: if m_refCount had reached 0 and subsequently incremented by 1 by another thread, then that thread previously didn't have any Ref / RefPtr reference to this object yet created new one. That's already a bug on its own. While it would be nice to prevent double delete in such a scenario as well, we're kind of hosed in such a situation because such a thread might as well start ref'ing this object long after it had already been deleted. I figured we can address a mitigation for such a case in a different bug since this patch's primary purpose is to prevent ref/deref happening inside a destructor in the same thread from deleting the object twice, not about some other thread ref/deref'ing an object without validating that the object is still live.
Comment on attachment 378310 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378310&action=review >>> Source/WTF/wtf/ThreadSafeRefCounted.h:79 >>> + m_refCount = 1; >> >> This doesn’t seem safe. There’s a window where m_refCount is 0 before it gets changed back to 1. > > Indeed, it's not most thread-safe code. Here's my reasoning: if m_refCount had reached 0 and subsequently incremented by 1 by another thread, then that thread previously didn't have any Ref / RefPtr reference to this object yet created new one. That's already a bug on its own. > > While it would be nice to prevent double delete in such a scenario as well, we're kind of hosed in such a situation because such a thread might as well start ref'ing this object long after it had already been deleted. > > I figured we can address a mitigation for such a case in a different bug since this patch's primary purpose is to prevent ref/deref happening inside a destructor in the same thread from deleting the object twice, not about some other thread ref/deref'ing an object without validating that the object is still live. I think that if there was a comment I would understand this better. To be clear, you are saying that it’s still forbidden to ref/deref inside the destructor? But that we make sure it doesn’t lead to double delete so the implications of the programming mistake are not as horrible.
(In reply to Darin Adler from comment #11) > Comment on attachment 378310 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=378310&action=review > > >>> Source/WTF/wtf/ThreadSafeRefCounted.h:79 > >>> + m_refCount = 1; > >> > >> This doesn’t seem safe. There’s a window where m_refCount is 0 before it gets changed back to 1. > > > > Indeed, it's not most thread-safe code. Here's my reasoning: if m_refCount had reached 0 and subsequently incremented by 1 by another thread, then that thread previously didn't have any Ref / RefPtr reference to this object yet created new one. That's already a bug on its own. > > > > While it would be nice to prevent double delete in such a scenario as well, we're kind of hosed in such a situation because such a thread might as well start ref'ing this object long after it had already been deleted. > > > > I figured we can address a mitigation for such a case in a different bug since this patch's primary purpose is to prevent ref/deref happening inside a destructor in the same thread from deleting the object twice, not about some other thread ref/deref'ing an object without validating that the object is still live. > > I think that if there was a comment I would understand this better. Yeah, I guess we should add a comment to that end. > To be clear, you are saying that it’s still forbidden to ref/deref inside > the destructor? But that we make sure it doesn’t lead to double delete so > the implications of the programming mistake are not as horrible. Yes. The bug 195776 (same work for Node) was motivated by an actual security bug which ended up storing a Node in Ref/RefPtr inside a subclass' destructor.
Comment on attachment 378310 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378310&action=review >>>>> Source/WTF/wtf/ThreadSafeRefCounted.h:79 >>>>> + m_refCount = 1; >>>> >>>> This doesn’t seem safe. There’s a window where m_refCount is 0 before it gets changed back to 1. >>> >>> Indeed, it's not most thread-safe code. Here's my reasoning: if m_refCount had reached 0 and subsequently incremented by 1 by another thread, then that thread previously didn't have any Ref / RefPtr reference to this object yet created new one. That's already a bug on its own. >>> >>> While it would be nice to prevent double delete in such a scenario as well, we're kind of hosed in such a situation because such a thread might as well start ref'ing this object long after it had already been deleted. >>> >>> I figured we can address a mitigation for such a case in a different bug since this patch's primary purpose is to prevent ref/deref happening inside a destructor in the same thread from deleting the object twice, not about some other thread ref/deref'ing an object without validating that the object is still live. >> >> I think that if there was a comment I would understand this better. >> >> To be clear, you are saying that it’s still forbidden to ref/deref inside the destructor? But that we make sure it doesn’t lead to double delete so the implications of the programming mistake are not as horrible. > > Yeah, I guess we should add a comment to that end. Maybe just return early if refcount is zero? auto refCount = m_refCount; // Protect against double destruction inside a destructor. if (!refCount) { ASSERT(...) return; } m_refCount = refCount - 1; return refCount == 1; Also, why do this only in ThreadSafeRefCounted and not RefCounted?
It’s already done in RefCounted apparently.
Created attachment 380078 [details] Updated for ToT
Ping reviewers.
Comment on attachment 380078 [details] Updated for ToT r=me It's clever to --m_refCount and then set back to 1. I think that's the right solution even though it doesn't fix a concurrent race. I'll write down my reasoning here, in case we ever reconsider: (1) Unconditional atomic increment/decrement is free on modern ARM hardware (as measured recently by me on MallocBench, PLT, Speedometer2, and JetStream2); but conditional increment/decrement is not free. It's nice for performance, and therefore our ability to use ThreadSafeRefCounted more, if our approach uses unconditional decrement here. (2) Our goal, as I understand it, is really to defend against re-entrant destruction caused by creating a new RefPtr during destruction. That goal seems a lot more important than protecting against racy raw calls to ref() / deref() because it directly impacts our ability to use RefPtr in more places.
Thanks for the review, and yes, that summaries the reasoning. I'll add to (2) that in those cases where ref() / deref() is called concurrently, such a thread is already doing UAF by the virtue of the fact this thread has already entered the destructor at that point. The fact m_refCount momentarily reached 0 would mean that the offending thread did not have any Ref or RefPtr to this object yet created new Ref / RefPtr while this thread was proceeding with the deletion. In that way, there isn't really a way to prevent the concurrent case because the offending thread's ref() could have happened long after the object had been deleted by this thread regardless of whether we use conditional decrement or not.
Comment on attachment 380078 [details] Updated for ToT Clearing flags on attachment: 380078 Committed r250762: <https://trac.webkit.org/changeset/250762>
All reviewed patches have been landed. Closing bug.
<rdar://problem/56001847>
Oops, this broke JSC tests: perl ./Tools/Scripts/run-javascriptcore-tests --cloop --no-build --no-jsc-stress --no-fail-fast --json-output=jsc_cloop.json --debug ... ASSERTION FAILED: !m_deletionHasBegun /Volumes/Data/slave/highsierra-cloop-debug/build/WebKitBuild/Debug/usr/local/include/wtf/ThreadSafeRefCounted.h(58) : void WTF::ThreadSafeRefCountedBase::ref() const 1 0x10b3b0679 WTFCrash 2 0x10b3b0699 WTFCrashWithSecurityImplication 3 0x10b3c539b WTF::ThreadSafeRefCountedBase::ref() const 4 0x10bae3f51 void WTF::refIfNotNull<JSC::VM>(JSC::VM*) 5 0x10bae3f14 WTF::RefPtr<JSC::VM, WTF::DumbPtrTraits<JSC::VM> >::RefPtr(JSC::VM*) 6 0x10bae3ead WTF::RefPtr<JSC::VM, WTF::DumbPtrTraits<JSC::VM> >::RefPtr(JSC::VM*) 7 0x10bf02649 JSC::JSLock::DropAllLocks::DropAllLocks(JSC::VM*) 8 0x10bf0278d JSC::JSLock::DropAllLocks::DropAllLocks(JSC::VM*) 9 0x10bf028ad JSC::JSLock::DropAllLocks::DropAllLocks(JSC::VM&) 10 0x10b98df7e JSC::Heap::releaseDelayedReleasedObjects() 11 0x10b98dbb6 JSC::Heap::lastChanceToFinalize() 12 0x10c08ef6c JSC::VM::~VM() 13 0x10c092205 JSC::VM::~VM() 14 0x10b6b4e8a WTF::ThreadSafeRefCounted<JSC::VM, (WTF::DestructionThread)0>::deref() const::'lambda'()::operator()() const 15 0x10b69478d WTF::ThreadSafeRefCounted<JSC::VM, (WTF::DestructionThread)0>::deref() const 16 0x10bae3e81 void WTF::derefIfNotNull<JSC::VM>(JSC::VM*) 17 0x10bacb817 WTF::RefPtr<JSC::VM, WTF::DumbPtrTraits<JSC::VM> >::operator=(std::nullptr_t) 18 0x10bf0168a JSC::JSLockHolder::~JSLockHolder() 19 0x10bf01705 JSC::JSLockHolder::~JSLockHolder() 20 0x10b69473b JSContextGroupRelease 21 0x10b606e74 -[JSVirtualMachine dealloc] 22 0x7fff53f0e042 (anonymous namespace)::AutoreleasePoolPage::pop(void*) 23 0x10b2f0d14 testToString() 24 0x10b2ef630 testObjectiveCAPI 25 0x10b2d4ff8 main 26 0x7fff54b2f015 start 27 0x1 This because of the following code: JSLock::DropAllLocks::DropAllLocks(VM* vm) : m_droppedLockCount(0) // 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) {
Re-opened since this is blocked by bug 202609
Created attachment 380721 [details] Fixed JSC tests
Comment on attachment 380721 [details] Fixed JSC tests View in context: https://bugs.webkit.org/attachment.cgi?id=380721&action=review > Source/JavaScriptCore/runtime/JSLock.cpp:-282 > - , m_vm(vm->refCount() ? vm : nullptr) Note that this code was added by https://trac.webkit.org/changeset/165074/webkit.
Comment on attachment 380721 [details] Fixed JSC tests View in context: https://bugs.webkit.org/attachment.cgi?id=380721&action=review > Source/WTF/wtf/ThreadSafeRefCounted.h:89 > + 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; I'm not sure how this achieves what the comment says. https://bugs.webkit.org/show_bug.cgi?id=201576#c0 makes reference to doing the same thing as webkit.org/b/195776, which in turn says to use String's increment/decrement strategy of inc/dec'ing by an s_refCountIncrement of 2. With String, setting refCount = 1 would prevent it from every decrementing to 0 again, thereby preventing a double free. In the case here, m_refCount is being decremented by 1 each time. Setting m_refCount = 1 here doesn't prevent it from being decremented to 0 again. Did you intend to change ThreadSafeRefCountedBase to use a s_refCountIncrement of 2?
Comment on attachment 380721 [details] Fixed JSC tests View in context: https://bugs.webkit.org/attachment.cgi?id=380721&action=review > Source/JavaScriptCore/heap/Heap.cpp:512 > + // 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. > + > // We need to drop locks before calling out to arbitrary code. > - JSLock::DropAllLocks dropAllLocks(m_vm); > + JSLock::DropAllLocks dropAllLocks(m_isShuttingDown ? nullptr : &m_vm); nit: It seems hokey to gave the client of DropAllLocks check if the VM, by proxy of the Heap, is already shutting down in order to do the right thing here. Can we just encapsulate this in DropAllLocks itself? See below ... >> Source/JavaScriptCore/runtime/JSLock.cpp:-282 >> - , m_vm(vm->refCount() ? vm : nullptr) > > Note that this code was added by https://trac.webkit.org/changeset/165074/webkit. I suggest doing the shutdown check here instead like so: , m_vm(vm->heap.isShuttingDown() ? nullptr : vm) >> Source/WTF/wtf/ThreadSafeRefCounted.h:89 >> + m_refCount = 1; > > I'm not sure how this achieves what the comment says. https://bugs.webkit.org/show_bug.cgi?id=201576#c0 makes reference to doing the same thing as webkit.org/b/195776, which in turn says to use String's increment/decrement strategy of inc/dec'ing by an s_refCountIncrement of 2. With String, setting refCount = 1 would prevent it from every decrementing to 0 again, thereby preventing a double free. In the case here, m_refCount is being decremented by 1 each time. Setting m_refCount = 1 here doesn't prevent it from being decremented to 0 again. Did you intend to change ThreadSafeRefCountedBase to use a s_refCountIncrement of 2? I see that this has already been discussed in https://bugs.webkit.org/show_bug.cgi?id=201576#c9, and https://bugs.webkit.org/show_bug.cgi?id=201576#c17. I still don't see how this prevents a double delete. Consider this scenario: 1. m_refCount == 1, and derefBase() is called in ThreadSafeRefCounted::deref(): derefBase() sets m_refCount = 1 and returns true i.e. deletion is executed. 2. a bug occurs, and we call ThreadSafeRefCounted::deref() again: derefBase() decs m_refCount to 0, sets it back to 1, and returns true again i.e. double deletion is executed! Am I missing something here?
(In reply to Mark Lam from comment #27) > Comment on attachment 380721 [details] > Fixed JSC tests > > View in context: > https://bugs.webkit.org/attachment.cgi?id=380721&action=review > > >> Source/WTF/wtf/ThreadSafeRefCounted.h:89 > >> + m_refCount = 1; > > > > I'm not sure how this achieves what the comment says. https://bugs.webkit.org/show_bug.cgi?id=201576#c0 makes reference to doing the same thing as webkit.org/b/195776, which in turn says to use String's increment/decrement strategy of inc/dec'ing by an s_refCountIncrement of 2. With String, setting refCount = 1 would prevent it from every decrementing to 0 again, thereby preventing a double free. In the case here, m_refCount is being decremented by 1 each time. Setting m_refCount = 1 here doesn't prevent it from being decremented to 0 again. Did you intend to change ThreadSafeRefCountedBase to use a s_refCountIncrement of 2? > > I see that this has already been discussed in > https://bugs.webkit.org/show_bug.cgi?id=201576#c9, and > https://bugs.webkit.org/show_bug.cgi?id=201576#c17. I still don't see how > this prevents a double delete. Consider this scenario: > > 1. m_refCount == 1, and derefBase() is called in > ThreadSafeRefCounted::deref(): derefBase() sets m_refCount = 1 and returns > true i.e. deletion is executed. > 2. a bug occurs, and we call ThreadSafeRefCounted::deref() again: > derefBase() decs m_refCount to 0, sets it back to 1, and returns true again > i.e. double deletion is executed! > > Am I missing something here? This patch isn't intending to fix a bug that someone might be calling deref() without ref(), or ref() on a potentially deleted object. This patch's sole purpose is to prevent storing the object to a Ref / RefPtr inside object's destructor not result in double delete. If someone is calling deref() before calling ref() then it would result in UAF at some point regardless (e.g. such a bug could keep deref()'ing an object until it gets deleted even if it had other Ref/RefPtr to it).
Comment on attachment 380721 [details] Fixed JSC tests View in context: https://bugs.webkit.org/attachment.cgi?id=380721&action=review r=me with the DropAllLocks VM shutdown check done in its constructor instead of its client. The pre-existing refCount check there was trying to achieve the same goal, but was unfortunately not effective enough. >>>> Source/WTF/wtf/ThreadSafeRefCounted.h:89 >>>> + m_refCount = 1; >>> >>> I'm not sure how this achieves what the comment says. https://bugs.webkit.org/show_bug.cgi?id=201576#c0 makes reference to doing the same thing as webkit.org/b/195776, which in turn says to use String's increment/decrement strategy of inc/dec'ing by an s_refCountIncrement of 2. With String, setting refCount = 1 would prevent it from every decrementing to 0 again, thereby preventing a double free. In the case here, m_refCount is being decremented by 1 each time. Setting m_refCount = 1 here doesn't prevent it from being decremented to 0 again. Did you intend to change ThreadSafeRefCountedBase to use a s_refCountIncrement of 2? >> >> I see that this has already been discussed in https://bugs.webkit.org/show_bug.cgi?id=201576#c9, and https://bugs.webkit.org/show_bug.cgi?id=201576#c17. I still don't see how this prevents a double delete. Consider this scenario: >> >> 1. m_refCount == 1, and derefBase() is called in ThreadSafeRefCounted::deref(): derefBase() sets m_refCount = 1 and returns true i.e. deletion is executed. >> 2. a bug occurs, and we call ThreadSafeRefCounted::deref() again: derefBase() decs m_refCount to 0, sets it back to 1, and returns true again i.e. double deletion is executed! >> >> Am I missing something here? > > This patch isn't intending to fix a bug that someone might be calling deref() without ref(), or ref() on a potentially deleted object. This patch's sole purpose is to prevent storing the object to a Ref / RefPtr inside object's destructor not result in double delete. If someone is calling deref() before calling ref() then it would result in UAF at some point regardless (e.g. such a bug could keep deref()'ing an object until it gets deleted even if it had other Ref/RefPtr to it). OK, I see what I was missing: in the destructor, with m_refCount set to 1, storing the object in any Ref/RefPtr will necessarily increment m_refCount above 1, thereby ensuring that destruction of said Ref/RefPtr is guaranteed to not decrement m_refCount down to 0. This (the decrement can only happen after an increment) is what I was missing. nit: maybe I was just too dense, or perhaps a little further clarification in the ChangeLog (about how this ensures a non-zero refCount in the destructor because assignment of this object to a Ref/RefPtr always sees an increment before a decrement) may help.
Regarding ChangeLog suggestion, alternatively, change the bug title to “Make a ThreadSafeRefCounted object safe to assign to a Reg/RefPtr inside its destructor”. The current title’s reference to “ref & deref” did not strongly suggest to me that the 2 operations must be paired and done in that order. I misread it as it’s safe to ref & deref arbitrarily in the destructor and still not get double frees.
Comment on attachment 380721 [details] Fixed JSC tests View in context: https://bugs.webkit.org/attachment.cgi?id=380721&action=review >>>>> Source/WTF/wtf/ThreadSafeRefCounted.h:89 >>>>> + m_refCount = 1; >>>> >>>> I'm not sure how this achieves what the comment says. https://bugs.webkit.org/show_bug.cgi?id=201576#c0 makes reference to doing the same thing as webkit.org/b/195776, which in turn says to use String's increment/decrement strategy of inc/dec'ing by an s_refCountIncrement of 2. With String, setting refCount = 1 would prevent it from every decrementing to 0 again, thereby preventing a double free. In the case here, m_refCount is being decremented by 1 each time. Setting m_refCount = 1 here doesn't prevent it from being decremented to 0 again. Did you intend to change ThreadSafeRefCountedBase to use a s_refCountIncrement of 2? >>> >>> I see that this has already been discussed in https://bugs.webkit.org/show_bug.cgi?id=201576#c9, and https://bugs.webkit.org/show_bug.cgi?id=201576#c17. I still don't see how this prevents a double delete. Consider this scenario: >>> >>> 1. m_refCount == 1, and derefBase() is called in ThreadSafeRefCounted::deref(): derefBase() sets m_refCount = 1 and returns true i.e. deletion is executed. >>> 2. a bug occurs, and we call ThreadSafeRefCounted::deref() again: derefBase() decs m_refCount to 0, sets it back to 1, and returns true again i.e. double deletion is executed! >>> >>> Am I missing something here? >> >> This patch isn't intending to fix a bug that someone might be calling deref() without ref(), or ref() on a potentially deleted object. This patch's sole purpose is to prevent storing the object to a Ref / RefPtr inside object's destructor not result in double delete. If someone is calling deref() before calling ref() then it would result in UAF at some point regardless (e.g. such a bug could keep deref()'ing an object until it gets deleted even if it had other Ref/RefPtr to it). > > OK, I see what I was missing: in the destructor, with m_refCount set to 1, storing the object in any Ref/RefPtr will necessarily increment m_refCount above 1, thereby ensuring that destruction of said Ref/RefPtr is guaranteed to not decrement m_refCount down to 0. This (the decrement can only happen after an increment) is what I was missing. > > nit: maybe I was just too dense, or perhaps a little further clarification in the ChangeLog (about how this ensures a non-zero refCount in the destructor because assignment of this object to a Ref/RefPtr always sees an increment before a decrement) may help. Maybe just change "prevents double delete" to "prevents RefPtr creating during destruction from triggering recursive delete".
Committed r251034: <https://trac.webkit.org/changeset/251034>