MachineThreads should be ref counted

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

The VM's MachineThreads registry object is being referenced from other threads as a raw pointer.  In a scenario where the VM is destructed on the main thread, there is no guarantee that another thread isn't still holding a reference to the registry and will eventually invoke removeThread() on it on thread exit.  Hence, there's a possible use after free scenario here.

The fix is to make MachineThreads ThreadSafeRefCounted, and have all threads that references keep a RefPtr to it to ensure that it stays alive until the very last thread is done with it.

[Radar WebKit Bug Importer]

<rdar://problem/19739959>

[Mark Lam]

Created attachment 246146 [details]
the patch.

[Filip Pizlo]

Comment on attachment 246146 [details]
the patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=246146&action=review

> Source/JavaScriptCore/API/tests/testapi.mm:507
> +    while (!mainThreadIsReadyToJoin)
> +        sleep(1);

This feels super shady.

[Mark Lam]

Comment on attachment 246146 [details]
the patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=246146&action=review

> Source/JavaScriptCore/API/tests/testapi.mm:1427
> +        sleep(10);

Eeek ... this is not supposed to be here.  Leftover debugging code.  Will remove.

[Filip Pizlo]

Comment on attachment 246146 [details]
the patch.

r=me but reduce the sleep(1) to something shorter, like a usleep(10000)

[Mark Lam]

Thanks for the review.  Reduced the sleep time as suggested.  Landed in r179753: <http://trac.webkit.org/r179753>.

[Geoffrey Garen]

Comment on attachment 246146 [details]
the patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=246146&action=review

> Source/JavaScriptCore/heap/Heap.cpp:350
> +    // We need to remove the main thread explicitly here because the main thread
> +    // may not terminate for a while though the Heap (and VM) is being shut down.
> +    m_machineThreads->removeCurrentThread();

This is super weird.

(1) The comment claims to be about the main thread, but this code runs on any thread.

(2) A class that never called addCurrentThread calls removeCurrentThread, seemingly over-releasing a resource.

> Source/JavaScriptCore/heap/MachineStackMarker.h:67
> +        uint64_t m_magicNumber; // Only used for detecting use after free.

Why is this better than using MallocScribble or libgmalloc?

[Oliver Hunt]

Comment on attachment 246146 [details]
the patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=246146&action=review

>> Source/JavaScriptCore/heap/MachineStackMarker.h:67
>> +        uint64_t m_magicNumber; // Only used for detecting use after free.
> 
> Why is this better than using MallocScribble or libgmalloc?

Presumably lower cost in a debug build?

[Mark Lam]

After reading more into how pthread_key_delete() and _pthread_tsd_cleanup() works, we determined that this fix is invalid.  Before the fix, there was indeed a race condition with a very small window that could result in a use after free scenario.  I will roll out the patch and implement the fix from scratch with a different approach.

[Mark Lam]

r179753 was rolled out in r180591: <http://trac.webkit.org/r180591>.