Skip to content
Permalink
Browse files
[JSC] Fix race condition in Atomics.{wait,waitAsync}
https://bugs.webkit.org/show_bug.cgi?id=248631

Reviewed by Yusuke Suzuki.

At the moment, this is manifesting as intermittent timeouts in the arm32 linux
test runner, for example: https://build.webkit.org/#/builders/24/builds/17430

After investigation, this is the test `stress/waitasync-waiter-list-order.js`
hanging; this manifests as some of the workers hanging on the initial
`Atomic.wait` call in that test--using a debugger to examine the memory being
waited on reveals that they should all have woken up and moved on.

The implementation of both `Atomics.wait` and `Atomics.waitAsync` begin by
atomically reading from the array and comparing the result to the expected
value, then obtaining a lock on the list of waiters and proceeding from
there--this is contrary to the spec [1], which requires that the atomic read
happens while the list of waiters is exclusively locked and allows a race where
the following interleaving results in A hanging indefinitely if no other calls
to Atomic.notify on that address occur:

    thread A: begin Atomic.wait(buffer, idx, expected_value)
    thread A: read buffer[idx], get expected_value

    thread B: perform Atomic.store(buffer, idx, unexpected_value)
    thread B: perform Atomic.notify(buffer, idx) [nothing is notified since the list of waiters is empty]

    thread A: lock list of waiters, wait on condition variable, hang waiting for next notify

----

This is fixed by delaying the atomic read until the lock on the list of waiters
is held

[1] https://tc39.es/ecma262/multipage/structured-data.html#sec-atomics.wait

* Source/JavaScriptCore/runtime/AtomicsObject.cpp:
(JSC::atomicsWaitImpl):
* Source/JavaScriptCore/runtime/WaiterListManager.cpp:
(JSC::WaiterListManager::waitImpl):
(JSC::WaiterListManager::wait):
(JSC::WaiterListManager::timeoutAsyncWaiter):
(JSC::WaiterListManager::addAsyncWaiter): Deleted.
* Source/JavaScriptCore/runtime/WaiterListManager.h:

Canonical link: https://commits.webkit.org/257423@main
  • Loading branch information
jjgriego authored and Constellation committed Dec 6, 2022
1 parent 1e50668 commit bb3be5c8f260f4dbdece34885f9de06dd5254b6d
Show file tree
Hide file tree
Showing 4 changed files with 70 additions and 58 deletions.
@@ -1,5 +1,3 @@
//@ skip
// Temporarily skip the test until https://github.com/WebKit/WebKit/pull/7048 fixes the race condition.
const TOTAL_WAITER_COUNT = 10;
const WAIT_INDEX = TOTAL_WAITER_COUNT * 2 - 1;

@@ -455,8 +455,7 @@ JSValue atomicsWaitImpl(JSGlobalObject* globalObject, JSArrayType* typedArray, u
return { };
}

AtomicsWaitValidation validation = WTF::atomicLoad(ptr) == expectedValue ? AtomicsWaitValidation::Pass : AtomicsWaitValidation::Fail;
return WaiterListManager::singleton().wait(globalObject, vm, ptr, validation, timeout, type);
return WaiterListManager::singleton().wait(globalObject, vm, ptr, expectedValue, timeout, type);
}

JSC_DEFINE_HOST_FUNCTION(atomicsFuncWait, (JSGlobalObject* globalObject, CallFrame* callFrame))
@@ -50,38 +50,56 @@ WaiterListManager& WaiterListManager::singleton()
return manager;
}

JSValue WaiterListManager::wait(JSGlobalObject* globalObject, VM& vm, void* ptr, AtomicsWaitValidation validation, Seconds timeout, AtomicsWaitType type)
template <typename ValueType>
JSValue WaiterListManager::waitImpl(JSGlobalObject* globalObject, VM& vm, ValueType* ptr, ValueType expectedValue, Seconds timeout, AtomicsWaitType type)
{
if (type == AtomicsWaitType::Async) {
JSObject* object = constructEmptyObject(globalObject);

bool isAsync = false;
JSValue value;
if (validation == AtomicsWaitValidation::Fail)
value = vm.smallStrings.notEqualString();
else if (!timeout)
value = vm.smallStrings.timedOutString();
else {
isAsync = true;
JSPromise* promise = JSPromise::create(vm, globalObject->promiseStructure());
addAsyncWaiter(ptr, promise, timeout);
value = promise;

Ref<WaiterList> list = findOrCreateList(ptr);
JSPromise* promise = JSPromise::create(vm, globalObject->promiseStructure());

{
Locker listLocker { list->lock };
if (WTF::atomicLoad(ptr) != expectedValue)
value = vm.smallStrings.notEqualString();
else if (!timeout)
value = vm.smallStrings.timedOutString();
else {
isAsync = true;

Ref<Waiter> waiter = adoptRef(*new Waiter(promise));
list->addLast(listLocker, waiter);

if (timeout != Seconds::infinity()) {
Ref<RunLoop::DispatchTimer> timer = RunLoop::current().dispatchAfter(timeout, [this, ptr, waiter = waiter.copyRef()]() mutable {
timeoutAsyncWaiter(ptr, WTFMove(waiter));
});
waiter->setTimer(listLocker, WTFMove(timer));
dataLogLnIf(WaiterListsManagerInternal::verbose, "WaiterListManager added a new AsyncWaiter ", RawPointer(waiter.ptr()), " to a waiterList for ptr ", RawPointer(ptr));
}

value = promise;
}
}

object->putDirect(vm, vm.propertyNames->async, jsBoolean(isAsync));
object->putDirect(vm, vm.propertyNames->value, value);
return object;
}

if (validation == AtomicsWaitValidation::Fail)
return vm.smallStrings.notEqualString();

Ref<Waiter> syncWaiter = vm.syncWaiter();
Ref<WaiterList> list = findOrCreateList(ptr);
MonotonicTime time = MonotonicTime::now() + timeout;

{
Locker listLocker { list->lock };
if (WTF::atomicLoad(ptr) != expectedValue)
return vm.smallStrings.notEqualString();

list->addLast(listLocker, syncWaiter);
dataLogLnIf(WaiterListsManagerInternal::verbose, "WaiterListManager added a new SyncWaiter ", RawPointer(&syncWaiter), " to a waiterList for ptr ", RawPointer(ptr));

@@ -101,49 +119,44 @@ JSValue WaiterListManager::wait(JSGlobalObject* globalObject, VM& vm, void* ptr,
}
}

void WaiterListManager::addAsyncWaiter(void* ptr, JSPromise* promise, Seconds timeout)
JSValue WaiterListManager::wait(JSGlobalObject* globalObject, VM& vm, int32_t* ptr, int32_t expected, Seconds timeout, AtomicsWaitType waitType)
{
Ref<WaiterList> list = findOrCreateList(ptr);
Ref<Waiter> waiter = adoptRef(*new Waiter(promise));
return waitImpl(globalObject, vm, ptr, expected, timeout, waitType);
}

{
Locker listLocker { list->lock };
list->addLast(listLocker, waiter);

if (timeout != Seconds::infinity()) {
Ref<RunLoop::DispatchTimer> timer = RunLoop::current().dispatchAfter(timeout, [this, ptr, waiter = waiter.copyRef()]() mutable {
if (RefPtr<WaiterList> list = findList(ptr)) {
// All cases:
// 1. Find a list for ptr.
Locker listLocker { list->lock };
if (waiter->ticket(listLocker)) {
if (waiter->isOnList()) {
// 1.1. The list contains the waiter which must be in the list and hasn't been notified.
// It should have a ticket, then notify it with timeout.
bool didGetDequeued = list->findAndRemove(listLocker, waiter);
ASSERT_UNUSED(didGetDequeued, didGetDequeued);
}
// 1.2. The list doesn't contain the waiter.
// 1.2.1 It's a new list, then the waiter must be removed from a list which is destructed.
// Then, the waiter may (notify it if it does) or may not have a ticket.
notifyWaiterImpl(listLocker, WTFMove(waiter), ResolveResult::Timeout);
return;
}
// 1.2.2 It's the list the waiter used to belong. Then it must be notified by other thread and ignore it.
}
JSValue WaiterListManager::wait(JSGlobalObject* globalObject, VM& vm, int64_t* ptr, int64_t expected, Seconds timeout, AtomicsWaitType waitType)
{
return waitImpl(globalObject, vm, ptr, expected, timeout, waitType);
}

// 2. Doesn't find a list for ptr, then the waiter must be removed from the list.
// 2.1. The waiter has a ticket, then notify it.
// 2.2. The waiter doesn't has a ticket, then it's notified and ignore it.
ASSERT(!waiter->isOnList());
if (waiter->ticket(NoLockingNecessary))
notifyWaiterImpl(NoLockingNecessary, WTFMove(waiter), ResolveResult::Timeout);
});
waiter->setTimer(listLocker, WTFMove(timer));
void WaiterListManager::timeoutAsyncWaiter(void* ptr, Ref<Waiter>&& waiter)
{
if (RefPtr<WaiterList> list = findList(ptr)) {
// All cases:
// 1. Find a list for ptr.
Locker listLocker { list->lock };
if (waiter->ticket(listLocker)) {
if (waiter->isOnList()) {
// 1.1. The list contains the waiter which must be in the list and hasn't been notified.
// It should have a ticket, then notify it with timeout.
bool didGetDequeued = list->findAndRemove(listLocker, waiter);
ASSERT_UNUSED(didGetDequeued, didGetDequeued);
}
// 1.2. The list doesn't contain the waiter.
// 1.2.1 It's a new list, then the waiter must be removed from a list which is destructed.
// Then, the waiter may (notify it if it does) or may not have a ticket.
notifyWaiterImpl(listLocker, WTFMove(waiter), ResolveResult::Timeout);
return;
}
// 1.2.2 It's the list the waiter used to belong. Then it must be notified by other thread and ignore it.
}

dataLogLnIf(WaiterListsManagerInternal::verbose, "WaiterListManager added a new AsyncWaiter ", RawPointer(waiter.ptr()), " to a waiterList for ptr ", RawPointer(ptr));
// 2. Doesn't find a list for ptr, then the waiter must be removed from the list.
// 2.1. The waiter has a ticket, then notify it.
// 2.2. The waiter doesn't has a ticket, then it's notified and ignore it.
ASSERT(!waiter->isOnList());
if (waiter->ticket(NoLockingNecessary))
notifyWaiterImpl(NoLockingNecessary, WTFMove(waiter), ResolveResult::Timeout);
}

unsigned WaiterListManager::notifyWaiter(void* ptr, unsigned count)
@@ -208,9 +208,8 @@ class WaiterListManager {
public:
static WaiterListManager& singleton();

JS_EXPORT_PRIVATE JSValue wait(JSGlobalObject*, VM&, void* ptr, AtomicsWaitValidation, Seconds timeout, AtomicsWaitType);

void addAsyncWaiter(void* ptr, JSPromise*, Seconds timeout);
JS_EXPORT_PRIVATE JSValue wait(JSGlobalObject*, VM&, int32_t* ptr, int32_t expected, Seconds timeout, AtomicsWaitType);
JS_EXPORT_PRIVATE JSValue wait(JSGlobalObject*, VM&, int64_t* ptr, int64_t expected, Seconds timeout, AtomicsWaitType);

enum class ResolveResult : uint8_t { Ok, Timeout };
unsigned notifyWaiter(void* ptr, unsigned count);
@@ -222,9 +221,12 @@ class WaiterListManager {
void unregisterSharedArrayBuffer(uint8_t* arrayPtr, size_t);

private:
template <typename ValueType>
JSValue waitImpl(JSGlobalObject*, VM&, ValueType* ptr, ValueType expectedValue, Seconds timeout, AtomicsWaitType);

void notifyWaiterImpl(const AbstractLocker&, Ref<Waiter>&&, const ResolveResult);

void timeoutAsyncWaiter(void* ptr, Waiter* target);
void timeoutAsyncWaiter(void* ptr, Ref<Waiter>&&);

void cancelAsyncWaiter(const AbstractLocker&, Waiter*);

0 comments on commit bb3be5c

Please sign in to comment.