WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
141317
MachineThreads should be ref counted
https://bugs.webkit.org/show_bug.cgi?id=141317
Summary
MachineThreads should be ref counted
Mark Lam
Reported
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.
Attachments
the patch.
(12.04 KB, patch)
2015-02-05 20:44 PST
,
Mark Lam
fpizlo
: review+
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2015-02-05 18:27:21 PST
<
rdar://problem/19739959
>
Mark Lam
Comment 2
2015-02-05 20:44:03 PST
Created
attachment 246146
[details]
the patch.
Filip Pizlo
Comment 3
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.
Mark Lam
Comment 4
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.
Filip Pizlo
Comment 5
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)
Mark Lam
Comment 6
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
>.
Geoffrey Garen
Comment 7
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?
Oliver Hunt
Comment 8
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?
Mark Lam
Comment 9
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.
Mark Lam
Comment 10
2015-02-24 16:06:09 PST
r179753
was rolled out in
r180591
: <
http://trac.webkit.org/r180591
>.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug