Changeset 179753 in webkit


Ignore:
Timestamp:
Feb 6, 2015 12:57:45 PM (9 years ago)
Author:
mark.lam@apple.com
Message:

MachineThreads should be ref counted.
<​https://webkit.org/b/141317>

Reviewed by Filip Pizlo.

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.

  • API/tests/testapi.mm:

(useVMFromOtherThread): - Renamed to be more descriptive.
(useVMFromOtherThreadAndOutliveVM):

  • Added a test that has another thread which uses the VM outlive the VM to confirm that there is no crash.

However, I was not actually able to get the VM to crash without this
patch because I wasn't always able to the thread destructor to be
called. With this patch applied, I did verify with some logging that
the MachineThreads registry is only destructed after all threads
have removed themselves from it.

(threadMain): Deleted.

  • heap/Heap.cpp:

(JSC::Heap::Heap):
(JSC::Heap::~Heap):
(JSC::Heap::gatherStackRoots):

  • heap/Heap.h:

(JSC::Heap::machineThreads):

  • heap/MachineStackMarker.cpp:

(JSC::MachineThreads::Thread::Thread):
(JSC::MachineThreads::addCurrentThread):
(JSC::MachineThreads::removeCurrentThread):

  • heap/MachineStackMarker.h:
Location:
trunk/Source/JavaScriptCore
Files:
6 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/JavaScriptCore/API/tests/testapi.mm

    r179576 r179753  
    473473}
    474474
    475 static void* threadMain(void* contextPtr)
     475static void* useVMFromOtherThread(void* contextPtr)
    476476{
    477477    JSContext *context = (__bridge JSContext*)contextPtr;
    … …  
    480480    TestObject *testObject = [TestObject testObject];
    481481    context[@"testObject"] = testObject;
     482    pthread_exit(nullptr);
     483}
     484
     485struct ThreadArgs {
     486    JSContext* context;
     487    volatile bool* mainThreadIsReadyToJoin;
     488    volatile bool* otherThreadIsDoneWithJSWork;
     489};
     490
     491static void* useVMFromOtherThreadAndOutliveVM(void* data)
     492{
     493    ThreadArgs* args = reinterpret_cast<ThreadArgs*>(data);
     494    volatile bool& mainThreadIsReadyToJoin = *args->mainThreadIsReadyToJoin;
     495    volatile bool& otherThreadIsDoneWithJSWork = *args->otherThreadIsDoneWithJSWork;
     496
     497    @autoreleasepool {
     498        JSContext *context = args->context;
     499
     500        // Do something to enter the VM.
     501        TestObject *testObject = [TestObject testObject];
     502        context[@"testObject"] = testObject;
     503    }
     504    otherThreadIsDoneWithJSWork = true;
     505
     506    while (!mainThreadIsReadyToJoin)
     507        usleep(10000);
    482508    pthread_exit(nullptr);
    483509}
    … …  
    13761402       
    13771403        pthread_t threadID;
    1378         pthread_create(&threadID, NULL, &threadMain, (__bridge void*)context);
     1404        pthread_create(&threadID, NULL, &useVMFromOtherThread, (__bridge void*)context);
    13791405        pthread_join(threadID, nullptr);
    13801406        JSSynchronousGarbageCollectForDebugging([context JSGlobalContextRef]);
    … …  
    13821408        checkResult(@"Did not crash after entering the VM from another thread", true);
    13831409    }
    1384    
     1410
     1411    @autoreleasepool {
     1412        pthread_t threadID;
     1413        volatile bool mainThreadIsReadyToJoin = false;
     1414        volatile bool otherThreadIsDoneWithJSWork = false;
     1415        @autoreleasepool {
     1416            JSContext *context = [[JSContext alloc] init];
     1417            ThreadArgs args = { context, &mainThreadIsReadyToJoin, &otherThreadIsDoneWithJSWork };
     1418
     1419            pthread_create(&threadID, NULL, &useVMFromOtherThreadAndOutliveVM, &args);
     1420            JSSynchronousGarbageCollectForDebugging([context JSGlobalContextRef]);
     1421
     1422            while (!otherThreadIsDoneWithJSWork)
     1423                usleep(10000);
     1424        }
     1425
     1426        mainThreadIsReadyToJoin = true;
     1427        pthread_join(threadID, nullptr);
     1428        checkResult(@"Did not crash if the VM is destructed before another thread using the VM ends", true);
     1429    }
     1430
    13851431    currentThisInsideBlockGetterTest();
    13861432    runDateTests();

  • trunk/Source/JavaScriptCore/ChangeLog

    r179751 r179753  
     12015-02-06  Mark Lam  <mark.lam@apple.com>
     2
     3        MachineThreads should be ref counted.
     4        <https://webkit.org/b/141317>
     5
     6        Reviewed by Filip Pizlo.
     7
     8        The VM's MachineThreads registry object is being referenced from other
     9        threads as a raw pointer.  In a scenario where the VM is destructed on
     10        the main thread, there is no guarantee that another thread isn't still
     11        holding a reference to the registry and will eventually invoke
     12        removeThread() on it on thread exit.  Hence, there's a possible use
     13        after free scenario here.
     14
     15        The fix is to make MachineThreads ThreadSafeRefCounted, and have all
     16        threads that references keep a RefPtr to it to ensure that it stays
     17        alive until the very last thread is done with it.
     18
     19        * API/tests/testapi.mm:
     20        (useVMFromOtherThread): - Renamed to be more descriptive.
     21        (useVMFromOtherThreadAndOutliveVM):
     22        - Added a test that has another thread which uses the VM outlive the
     23          VM to confirm that there is no crash.
     24
     25          However, I was not actually able to get the VM to crash without this
     26          patch because I wasn't always able to the thread destructor to be
     27          called.  With this patch applied, I did verify with some logging that
     28          the MachineThreads registry is only destructed after all threads
     29          have removed themselves from it.
     30
     31        (threadMain): Deleted.
     32
     33        * heap/Heap.cpp:
     34        (JSC::Heap::Heap):
     35        (JSC::Heap::~Heap):
     36        (JSC::Heap::gatherStackRoots):
     37        * heap/Heap.h:
     38        (JSC::Heap::machineThreads):
     39        * heap/MachineStackMarker.cpp:
     40        (JSC::MachineThreads::Thread::Thread):
     41        (JSC::MachineThreads::addCurrentThread):
     42        (JSC::MachineThreads::removeCurrentThread):
     43        * heap/MachineStackMarker.h:
     44
    1452015-02-06  Commit Queue  <commit-queue@webkit.org>
    246

  • trunk/Source/JavaScriptCore/heap/Heap.cpp

    r179728 r179753  
    312312    , m_storageSpace(this)
    313313    , m_extraMemoryUsage(0)
    314     , m_machineThreads(this)
    315314    , m_sharedData(vm)
    316315    , m_slotVisitor(m_sharedData)
    … …  
    341340#endif
    342341{
     342    m_machineThreads = adoptRef(new MachineThreads(this));
    343343    m_storageSpace.init();
    344344    if (Options::verifyHeap())
    … …  
    348348Heap::~Heap()
    349349{
     350    // We need to remove the main thread explicitly here because the main thread
     351    // may not terminate for a while though the Heap (and VM) is being shut down.
     352    m_machineThreads->removeCurrentThread();
    350353}
    351354
    … …  
    588591    GCPHASE(GatherStackRoots);
    589592    m_jitStubRoutines.clearMarks();
    590     m_machineThreads.gatherConservativeRoots(roots, m_jitStubRoutines, m_codeBlocks, dummy, registers);
     593    m_machineThreads->gatherConservativeRoots(roots, m_jitStubRoutines, m_codeBlocks, dummy, registers);
    591594}
    592595

  • trunk/Source/JavaScriptCore/heap/Heap.h

    r179728 r179753  
    120120    VM* vm() const { return m_vm; }
    121121    MarkedSpace& objectSpace() { return m_objectSpace; }
    122     MachineThreads& machineThreads() { return m_machineThreads; }
     122    MachineThreads& machineThreads() { return *m_machineThreads; }
    123123
    124124    const SlotVisitor& slotVisitor() const { return m_slotVisitor; }
    … …  
    356356    std::unique_ptr<HashSet<MarkedArgumentBuffer*>> m_markListSet;
    357357
    358     MachineThreads m_machineThreads;
     358    RefPtr<MachineThreads> m_machineThreads;
    359359   
    360360    GCThreadSharedData m_sharedData;

  • trunk/Source/JavaScriptCore/heap/MachineStackMarker.cpp

    r179666 r179753  
    9292    WTF_MAKE_FAST_ALLOCATED;
    9393public:
    94     Thread(const PlatformThread& platThread, void* base)
     94    Thread(PassRefPtr<MachineThreads> machineThreads, const PlatformThread& platThread, void* base)
    9595        : platformThread(platThread)
    9696        , stackBase(base)
     97        , m_machineThreads(machineThreads)
    9798    {
    9899#if USE(PTHREADS) && !OS(WINDOWS) && !OS(DARWIN) && defined(SA_RESTART)
    … …  
    114115    PlatformThread platformThread;
    115116    void* stackBase;
     117    RefPtr<MachineThreads> m_machineThreads;
    116118};
    117119
    … …  
    121123#if !ASSERT_DISABLED
    122124    , m_heap(heap)
     125    , m_magicNumber(0x1234567890abcdef)
    123126#endif
    124127{
    … …  
    137140        t = next;
    138141    }
     142#if !ASSERT_DISABLED
     143    m_magicNumber = 0xbaddbeefdeadbeef;
     144#endif
    139145}
    140146
    … …  
    171177
    172178    threadSpecificSet(m_threadSpecific, this);
    173     Thread* thread = new Thread(getCurrentPlatformThread(), wtfThreadData().stack().origin());
     179    Thread* thread = new Thread(this, getCurrentPlatformThread(), wtfThreadData().stack().origin());
    174180
    175181    MutexLocker lock(m_registeredThreadsMutex);
    … …  
    181187void MachineThreads::removeThread(void* p)
    182188{
     189    ASSERT(static_cast<MachineThreads*>(p)->m_magicNumber == 0x1234567890abcdef);
    183190    static_cast<MachineThreads*>(p)->removeCurrentThread();
    184191}
    … …  
    188195    PlatformThread currentPlatformThread = getCurrentPlatformThread();
    189196
     197    // This thread could be the last entity that holds a RefPtr to the
     198    // MachineThreads registry. We don't want the registry to be deleted while
     199    // we're deleting the the Thread entry, because:
     200    //
     201    // 1. ~MachineThread() will attempt to lock its m_registeredThreadsMutex
     202    //    and we already hold it here. m_registeredThreadsMutex is not
     203    //    re-entrant.
     204    // 2. The MutexLocker will unlock m_registeredThreadsMutex at the end of
     205    //    this function. We can't let it be destructed until before then.
     206    //
     207    // Using this RefPtr here will defer destructing the registry till after
     208    // MutexLocker releases its m_registeredThreadsMutex.
     209    RefPtr<MachineThreads> retainMachineThreadsRegistryUntilDoneRemovingThread = this;
     210
     211    ASSERT(m_magicNumber == 0x1234567890abcdef);
    190212    MutexLocker lock(m_registeredThreadsMutex);
    191213    if (equalThread(currentPlatformThread, m_registeredThreads->platformThread)) {

  • trunk/Source/JavaScriptCore/heap/MachineStackMarker.h

    r179648 r179753  
    2525#include <setjmp.h>
    2626#include <wtf/Noncopyable.h>
     27#include <wtf/ThreadSafeRefCounted.h>
    2728#include <wtf/ThreadSpecific.h>
    2829#include <wtf/ThreadingPrimitives.h>
    … …  
    3536    class JITStubRoutineSet;
    3637
    37     class MachineThreads {
     38    class MachineThreads : public ThreadSafeRefCounted<MachineThreads> {
    3839        WTF_MAKE_NONCOPYABLE(MachineThreads);
    3940    public:
    … …  
    4748        JS_EXPORT_PRIVATE void addCurrentThread(); // Only needs to be called by clients that can use the same heap from multiple threads.
    4849
     50        void removeCurrentThread();
     51
    4952    private:
    5053        class Thread;
    … …  
    5659
    5760        static void removeThread(void*);
    58         void removeCurrentThread();
    5961
    6062        Mutex m_registeredThreadsMutex;
    … …  
    6365#if !ASSERT_DISABLED
    6466        Heap* m_heap;
     67        uint64_t m_magicNumber; // Only used for detecting use after free.
    6568#endif
    6669    };
Note: See TracChangeset for help on using the changeset viewer.