aed1fddc0b  [JSC] JSLock m_hasOwnerThread has concurrency issue

The bug at a glance

OBSERVED: the patch converts JSLock::m_hasOwnerThread from a plain bool guarded by a one-sided storeStoreFence into a std::atomic<bool> accessed with release stores and acquire loads, and the commit describes a real cross-thread data race in currentThreadIsHoldingLock / ownerThread / ownerThreadUID. INFERRED: JSLock ownership is queried from threads other than the owner (notably during VM/thread suspension and heap-access coordination), so a thread could observe m_hasOwnerThread==true while the paired m_ownerThread store is not yet visible, reading a stale/torn owner. That can produce incorrect ‘we already hold the lock’ decisions and use of a stale Thread pointer, a memory-safety-adjacent concurrency bug in a core VM primitive. Rated medium: it is a genuine data race with a plausible path to misbehavior, but the patch is a hardening/ordering fix and no concrete corruption primitive is demonstrated.

JSLock kept two fields (m_hasOwnerThread and m_ownerThread) that must stay in sync, but published m_hasOwnerThread with only a store-store fence on the write side and no barrier on the read side. Cross-thread readers (currentThreadIsHoldingLock, ownerThread, ownerThreadUID) could see the flag set before the owner-thread pointer became visible.

Root cause

JSLock (Source/JavaScriptCore/runtime/JSLock.{h,cpp}) is the recursive lock that guards a VM. Besides the underlying m_lock, it caches ownership state in two fields that must be mutually consistent: m_hasOwnerThread (a bool) and m_ownerThread (a RefPtr<Thread>). A source comment explicitly notes that m_hasOwnerThread exists as a separate flag (rather than making m_ownerThread optional) because currentThreadIsHoldingLock() may be called from a different thread and an optional is vulnerable to races (referencing bug 169042#c6).

OBSERVED (write side): JSLock::lock() set m_ownerThread = &Thread::currentSingleton(), then executed WTF::storeStoreFence(), then wrote m_hasOwnerThread = true. The store-store fence only orders the two stores on the writing CPU; it guarantees that if a reader sees m_hasOwnerThread==true it will (on the writer’s side) have ordered the m_ownerThread store first. unlock() cleared m_hasOwnerThread = false with no ordering relative to the subsequent m_lock.unlock().

OBSERVED (read side): ownerThread(), ownerThreadUID(), and currentThreadIsHoldingLock() all read m_hasOwnerThread as a plain bool with no acquire barrier, then read m_ownerThread. Per the commit message: ’loading this is not having a barrier … CPU can freely change the visibility of the other thread’s store to them. We may see a state that m_hasOwnerThread is true, but m_ownerThread is not stored yet.’ A one-sided fence on the writer is insufficient without a matching acquire on every reader; the plain-bool load creates a data race under the C++ memory model regardless.

INFERRED: these queries are made from threads that are not the lock owner, in particular around thread suspension (ownerThreadUID is documented as used to avoid deadlock with thread suspension) and heap-access / safepoint coordination. A racing reader could observe m_hasOwnerThread==true paired with a stale or not-yet-published m_ownerThread, or observe the flag with reordered visibility. currentThreadIsHoldingLock() combines the flag with m_ownerThread.get() == &Thread::currentSingleton(); a torn/stale read there can make a thread wrongly conclude it (or another thread) holds the lock, corrupting the recursive-lock bookkeeping and dereferencing a stale Thread. The patch replaces the storeStoreFence+bool with m_hasOwnerThread.store(true/false, memory_order_release) on writes and m_hasOwnerThread.load(memory_order_acquire) on all three readers, establishing a proper release/acquire happens-before so that seeing the flag set guarantees the paired m_ownerThread store is visible.

Key code

Reader-side acquire loads that now pair with the release stores (JSLock.h)

    std::optional<RefPtr<Thread>> ownerThread() const
    {
        if (m_hasOwnerThread.load(std::memory_order_acquire))
            return m_ownerThread;
        return std::nullopt;
    }

    std::optional<uint64_t> ownerThreadUID() const
    {
        if (!m_hasOwnerThread.load(std::memory_order_acquire))
            return std::nullopt;
        return m_ownerThread->uid();
    }

    bool currentThreadIsHoldingLock() { return m_hasOwnerThread.load(std::memory_order_acquire) && m_ownerThread.get() == &Thread::currentSingleton(); }

Patch walkthrough

• Source/JavaScriptCore/runtime/JSLock.cpp — In lock(), removes WTF::storeStoreFence() and the plain ’m_hasOwnerThread = true’, replacing them with m_hasOwnerThread.store(true, std::memory_order_release) after m_ownerThread is assigned, so the owner-thread store is published-before the flag. In unlock(), changes ’m_hasOwnerThread = false’ to m_hasOwnerThread.store(false, std::memory_order_release).
• Source/JavaScriptCore/runtime/JSLock.h — Changes the member declaration from ‘bool m_hasOwnerThread { false }’ to ‘std::atomic<bool> m_hasOwnerThread { false }’. Updates the three cross-thread readers ownerThread(), ownerThreadUID(), and currentThreadIsHoldingLock() to load the flag via m_hasOwnerThread.load(std::memory_order_acquire), giving each reader the acquire barrier that pairs with the release store and guarantees a non-stale m_ownerThread.

Background

JSLock — JSC’s recursive lock protecting a VM. Caches owner state in m_hasOwnerThread and m_ownerThread so ownership can be queried cheaply, including from non-owner threads.

m_hasOwnerThread vs m_ownerThread — Two fields kept in sync deliberately (a comment cites bug 169042#c6): a separate bool flag rather than an optional, because currentThreadIsHoldingLock may run on another thread and optionals race.

storeStoreFence — WTF one-sided barrier ordering prior stores before later stores on the issuing CPU. It does not provide the acquire side a reader needs, and a plain-bool load remains a data race under the C++ memory model.

release/acquire ordering — A release store publishes all prior writes to any thread that performs an acquire load observing that store, establishing happens-before. This is the pairing the patch installs on m_hasOwnerThread.

ownerThreadUID and thread suspension — ownerThreadUID() is documented as used to avoid deadlock with thread suspension; it is a canonical cross-thread reader of the owner state and thus exposed to the race.

Vulnerability window

1. Legacy design — JSLock stores owner state as bool + RefPtr, publishing the flag with a store-store fence and reading it as a plain bool.
2. Latent race — Cross-thread readers (currentThreadIsHoldingLock, ownerThread, ownerThreadUID) load the flag without an acquire barrier, so m_ownerThread visibility is not guaranteed to accompany the flag.
3. Diagnosis — Bug 311431 / rdar://173797266 identifies that a thread can see m_hasOwnerThread==true while m_ownerThread is stale or not yet stored.
4. Fix landed on branch — Originally landed as 305413.611 on a safari-7624 branch (rdar://176061322), converting the flag to atomic release/acquire.
5. Mainline — Merged as 314150@main, changing writes to release stores and all three readers to acquire loads.
6. State after fix — Observing the flag set now guarantees the paired m_ownerThread store is visible, so ownership queries no longer read a stale Thread pointer.

Triggering

No test accompanies the patch (a data race on a lock primitive is not deterministically reproducible from script). Conceptual trigger: run JS on a VM thread that repeatedly acquires/releases the JSLock while a second thread (e.g. the sampling profiler, a Web Worker/VM coordinator, or a thread performing suspension) repeatedly calls currentThreadIsHoldingLock()/ownerThreadUID() on that JSLock, under TSan or on a weakly-ordered CPU (arm64). TSan reports the data race on m_hasOwnerThread/m_ownerThread on an unpatched build; on hardware, a reader can transiently observe the flag set with a stale owner pointer.

Exploitation

1. Concurrency setup — Requires two threads interacting with the same JSLock: the owner toggling ownership in lock()/unlock() and a non-owner querying ownership (suspension/heap-access/profiler paths).
2. Windowing the race — On weakly-ordered CPUs the non-owner can observe m_hasOwnerThread==true before m_ownerThread’s store is visible, i.e. a stale or torn owner read.
3. Effect — Incorrect ownership decisions in currentThreadIsHoldingLock and use of a stale Thread pointer in ownerThread/ownerThreadUID, corrupting recursive-lock bookkeeping.
4. Escalation assessment — No memory-corruption primitive is demonstrated by the patch; this is a hardening fix for a real data race whose exploitability beyond misbehavior/crash is unestablished and hardware-dependent.

Detection & hunting

For defenders and SOC / detection engineers:

• ThreadSanitizer report — TSan on JSC will flag the data race on m_hasOwnerThread/m_ownerThread when a non-owner thread queries ownership concurrently with lock()/unlock() on an unpatched build.
• Stale owner-thread crashes — Crashes dereferencing m_ownerThread (e.g. in ownerThreadUID->uid()) or wrong currentThreadIsHoldingLock results on arm64 under thread suspension are consistent with this race.
• One-sided fence pattern — Audit signal: a storeStoreFence paired publish with a plain (non-atomic, non-acquire) read on the consumer side.

Audit directions

• Other flag+pointer sync pairs in JSC — Search for the pattern of a bool flag published with storeStoreFence guarding a separately-stored pointer that is read cross-thread; apply the same release/acquire fix.
• All JSLock ownership callers — Enumerate callers of currentThreadIsHoldingLock/ownerThread/ownerThreadUID and confirm none assume atomicity beyond what the flag now provides, especially in suspension and safepoint code.
• WTF one-sided fence uses — Review remaining storeStoreFence/loadLoadFence sites in JSC for missing paired barriers on the opposite side.
• Thread suspension interactions — Audit the suspension/heap-access machinery that queries lock ownership to ensure the newly-strengthened ordering is sufficient there and no additional lost-update windows remain.