Bug 141317

Summary: MachineThreads should be ref counted
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, fpizlo, ggaren, mark.lam, mmirman, msaboff, oliver, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
the patch. fpizlo: review+

Description Mark Lam 2015-02-05 18:25:30 PST
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.
Comment 1 Radar WebKit Bug Importer 2015-02-05 18:27:21 PST
<rdar://problem/19739959>
Comment 2 Mark Lam 2015-02-05 20:44:03 PST
Created attachment 246146 [details]
the patch.
Comment 3 Filip Pizlo 2015-02-06 12:41:23 PST
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.
Comment 4 Mark Lam 2015-02-06 12:46:31 PST
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.
Comment 5 Filip Pizlo 2015-02-06 12:47:54 PST
Comment on attachment 246146 [details]
the patch.

r=me but reduce the sleep(1) to something shorter, like a usleep(10000)
Comment 6 Mark Lam 2015-02-06 12:59:53 PST
Thanks for the review.  Reduced the sleep time as suggested.  Landed in r179753: <http://trac.webkit.org/r179753>.
Comment 7 Geoffrey Garen 2015-02-11 15:52:47 PST
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?
Comment 8 Oliver Hunt 2015-02-11 16:17:49 PST
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?
Comment 9 Mark Lam 2015-02-24 15:51:38 PST
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.
Comment 10 Mark Lam 2015-02-24 16:06:09 PST
r179753 was rolled out in r180591: <http://trac.webkit.org/r180591>.