Skip to content

Commit

Permalink
Merge r180716 - MachineThreads::Thread clean up has a use after free …
Browse files Browse the repository at this point in the history 
…race condition.

<https://webkit.org/b/141990>

Reviewed by Filip Pizlo.

MachineThreads::Thread clean up relies on the clean up mechanism
implemented in _pthread_tsd_cleanup_key(), which looks like this:

void _pthread_tsd_cleanup_key(pthread_t self, pthread_key_t key)
{
    void (*destructor)(void *);
    if (_pthread_key_get_destructor(key, &destructor)) {
        void **ptr = &self->tsd[key];
        void *value = *ptr;

    // === Start of window for the bug to manifest =================

        // At this point, this thread has cached "destructor" and "value"
        // (which is a MachineThreads*).  If the VM gets destructed (along
        // with its MachineThreads registry) by another thread, then this
        // thread will have no way of knowing that the MachineThreads* is
        // now pointing to freed memory.  Calling the destructor below will
        // therefore result in a use after free scenario when it tries to
        // access the MachineThreads' data members.

        if (value) {
            *ptr = NULL;
            if (destructor) {

    // === End of window for the bug to manifest ==================

                destructor(value);
            }
        }
    }
}

The fix is to add each active MachineThreads to an ActiveMachineThreadsManager,
and always check if the manager still contains that MachineThreads object
before we call removeCurrentThread() on it.  When MachineThreads is destructed,
it will remove itself from the manager.  The add, remove, and checking
operations are all synchronized on the manager's lock, thereby ensuring that
the MachineThreads object, if found in the manager, will remain alive for the
duration of time we call removeCurrentThread() on it.

There's also possible for the MachineThreads object to already be destructed
and another one happened to have been instantiated at the same address.
Hence, we should only remove the exiting thread if it is found in the
MachineThreads object.

There is no test for this issue because this bug requires a race condition
between 2 threads where:
1. Thread B, which had previously used the VM, exiting and
   getting to the bug window shown in _pthread_tsd_cleanup_key() above.
2. Thread A destructing the VM (and its MachineThreads object)
   within that window of time before Thread B calls the destructor.

It is not possible to get a reliable test case without invasively
instrumenting _pthread_tsd_cleanup_key() or MachineThreads::removeCurrentThread()
to significantly increase that window of opportunity.

* heap/MachineStackMarker.cpp:
(JSC::ActiveMachineThreadsManager::Locker::Locker):
(JSC::ActiveMachineThreadsManager::add):
(JSC::ActiveMachineThreadsManager::remove):
(JSC::ActiveMachineThreadsManager::contains):
(JSC::ActiveMachineThreadsManager::ActiveMachineThreadsManager):
(JSC::activeMachineThreadsManager):
(JSC::MachineThreads::MachineThreads):
(JSC::MachineThreads::~MachineThreads):
(JSC::MachineThreads::removeThread):
(JSC::MachineThreads::removeThreadIfFound):
(JSC::MachineThreads::removeCurrentThread): Deleted.
* heap/MachineStackMarker.h:
  • Loading branch information
@carlosgcampos
Mark Lam authored and carlosgcampos committed Mar 1, 2015
1 parent 6b985c8 commit 46cbf41
Show file tree
Hide file tree
Showing 3 changed files with 157 additions and 9 deletions.
77 changes: 77 additions & 0 deletions Source/JavaScriptCore/ChangeLog
View file
@@ -1,3 +1,80 @@
2015-02-26 Mark Lam <mark.lam@apple.com>

MachineThreads::Thread clean up has a use after free race condition.
<https://webkit.org/b/141990>

Reviewed by Filip Pizlo.

MachineThreads::Thread clean up relies on the clean up mechanism
implemented in _pthread_tsd_cleanup_key(), which looks like this:

void _pthread_tsd_cleanup_key(pthread_t self, pthread_key_t key)
{
void (*destructor)(void *);
if (_pthread_key_get_destructor(key, &destructor)) {
void **ptr = &self->tsd[key];
void *value = *ptr;

// === Start of window for the bug to manifest =================

// At this point, this thread has cached "destructor" and "value"
// (which is a MachineThreads*). If the VM gets destructed (along
// with its MachineThreads registry) by another thread, then this
// thread will have no way of knowing that the MachineThreads* is
// now pointing to freed memory. Calling the destructor below will
// therefore result in a use after free scenario when it tries to
// access the MachineThreads' data members.

if (value) {
*ptr = NULL;
if (destructor) {

// === End of window for the bug to manifest ==================

destructor(value);
}
}
}
}

The fix is to add each active MachineThreads to an ActiveMachineThreadsManager,
and always check if the manager still contains that MachineThreads object
before we call removeCurrentThread() on it. When MachineThreads is destructed,
it will remove itself from the manager. The add, remove, and checking
operations are all synchronized on the manager's lock, thereby ensuring that
the MachineThreads object, if found in the manager, will remain alive for the
duration of time we call removeCurrentThread() on it.

There's also possible for the MachineThreads object to already be destructed
and another one happened to have been instantiated at the same address.
Hence, we should only remove the exiting thread if it is found in the
MachineThreads object.

There is no test for this issue because this bug requires a race condition
between 2 threads where:
1. Thread B, which had previously used the VM, exiting and
getting to the bug window shown in _pthread_tsd_cleanup_key() above.
2. Thread A destructing the VM (and its MachineThreads object)
within that window of time before Thread B calls the destructor.

It is not possible to get a reliable test case without invasively
instrumenting _pthread_tsd_cleanup_key() or MachineThreads::removeCurrentThread()
to significantly increase that window of opportunity.

* heap/MachineStackMarker.cpp:
(JSC::ActiveMachineThreadsManager::Locker::Locker):
(JSC::ActiveMachineThreadsManager::add):
(JSC::ActiveMachineThreadsManager::remove):
(JSC::ActiveMachineThreadsManager::contains):
(JSC::ActiveMachineThreadsManager::ActiveMachineThreadsManager):
(JSC::activeMachineThreadsManager):
(JSC::MachineThreads::MachineThreads):
(JSC::MachineThreads::~MachineThreads):
(JSC::MachineThreads::removeThread):
(JSC::MachineThreads::removeThreadIfFound):
(JSC::MachineThreads::removeCurrentThread): Deleted.
* heap/MachineStackMarker.h:

2015-02-26 Csaba Osztrogonác <ossy@webkit.org>

Add calleeSaveRegisters() implementation for ARM Traditional
Expand Down
85 changes: 77 additions & 8 deletions Source/JavaScriptCore/heap/MachineStackMarker.cpp
View file
@@ -1,5 +1,5 @@
/*
* Copyright (C) 2003, 2004, 2005, 2006, 2007, 2008, 2009 Apple Inc. All rights reserved.
* Copyright (C) 2003-2009, 2015 Apple Inc. All rights reserved.
* Copyright (C) 2007 Eric Seidel <eric@webkit.org>
* Copyright (C) 2009 Acision BV. All rights reserved.
*
Expand Down Expand Up @@ -88,6 +88,65 @@ static void pthreadSignalHandlerSuspendResume(int)
#endif
#endif

class ActiveMachineThreadsManager;
static ActiveMachineThreadsManager& activeMachineThreadsManager();

class ActiveMachineThreadsManager {
WTF_MAKE_NONCOPYABLE(ActiveMachineThreadsManager);
public:

class Locker {
public:
Locker(ActiveMachineThreadsManager& manager)
: m_locker(manager.m_lock)
{
}

private:
MutexLocker m_locker;
};

void add(MachineThreads* machineThreads)
{
MutexLocker managerLock(m_lock);
m_set.add(machineThreads);
}

void remove(MachineThreads* machineThreads)
{
MutexLocker managerLock(m_lock);
auto recordedMachineThreads = m_set.take(machineThreads);
RELEASE_ASSERT(recordedMachineThreads = machineThreads);
}

bool contains(MachineThreads* machineThreads)
{
return m_set.contains(machineThreads);
}

private:
typedef HashSet<MachineThreads*> MachineThreadsSet;

ActiveMachineThreadsManager() { }

Mutex m_lock;
MachineThreadsSet m_set;

friend ActiveMachineThreadsManager& activeMachineThreadsManager();
};

static ActiveMachineThreadsManager& activeMachineThreadsManager()
{
static std::once_flag initializeManagerOnceFlag;
static ActiveMachineThreadsManager* manager = nullptr;

std::call_once(initializeManagerOnceFlag, [] {
manager = new ActiveMachineThreadsManager();
});
return *manager;
}


class MachineThreads::Thread {
WTF_MAKE_FAST_ALLOCATED;
public:
Expand Down Expand Up @@ -124,10 +183,12 @@ MachineThreads::MachineThreads(Heap* heap)
{
UNUSED_PARAM(heap);
threadSpecificKeyCreate(&m_threadSpecific, removeThread);
activeMachineThreadsManager().add(this);
}

MachineThreads::~MachineThreads()
{
activeMachineThreadsManager().remove(this);
threadSpecificKeyDelete(m_threadSpecific);

MutexLocker registeredThreadsLock(m_registeredThreadsMutex);
Expand Down Expand Up @@ -180,29 +241,37 @@ void MachineThreads::addCurrentThread()

void MachineThreads::removeThread(void* p)
{
static_cast<MachineThreads*>(p)->removeCurrentThread();
auto& manager = activeMachineThreadsManager();
ActiveMachineThreadsManager::Locker lock(manager);
auto machineThreads = static_cast<MachineThreads*>(p);
if (manager.contains(machineThreads)) {
// There's a chance that the MachineThreads registry that this thread
// was registered with was already destructed, and another one happened
// to be instantiated at the same address. Hence, this thread may or
// may not be found in this MachineThreads registry. We only need to
// do a removal if this thread is found in it.
machineThreads->removeThreadIfFound(getCurrentPlatformThread());
}
}

void MachineThreads::removeCurrentThread()
template<typename PlatformThread>
void MachineThreads::removeThreadIfFound(PlatformThread platformThread)
{
PlatformThread currentPlatformThread = getCurrentPlatformThread();

MutexLocker lock(m_registeredThreadsMutex);
if (equalThread(currentPlatformThread, m_registeredThreads->platformThread)) {
if (equalThread(platformThread, m_registeredThreads->platformThread)) {
Thread* t = m_registeredThreads;
m_registeredThreads = m_registeredThreads->next;
delete t;
} else {
Thread* last = m_registeredThreads;
Thread* t;
for (t = m_registeredThreads->next; t; t = t->next) {
if (equalThread(t->platformThread, currentPlatformThread)) {
if (equalThread(t->platformThread, platformThread)) {
last->next = t->next;
break;
}
last = t;
}
ASSERT(t); // If t is NULL, we never found ourselves in the list.
delete t;
}
}
Expand Down
4 changes: 3 additions & 1 deletion Source/JavaScriptCore/heap/MachineStackMarker.h
View file
Expand Up @@ -55,7 +55,9 @@ namespace JSC {
bool tryCopyOtherThreadStacks(MutexLocker&, void*, size_t capacity, size_t*);

static void removeThread(void*);
void removeCurrentThread();

template<typename PlatformThread>
void removeThreadIfFound(PlatformThread);

Mutex m_registeredThreadsMutex;
Thread* m_registeredThreads;
Expand Down

0 comments on commit 46cbf41

Please sign in to comment.