[5] RegExp bytecode compilation was racing between mutator and compiler threads

MediumJSC RegExp and YARRRace

dcf25ed

Medium, and only that because the window is thin — the commit reports the race needed artificially added thread sleeps to manifest at all. What sits inside the window is a second thread installing an owning pointer that the first may already have handed to the interpreter.

A data race on lazy initialization is one of the oldest shapes in the book: two threads test a null field, both build the object, and one of them destroys the other's work. JavaScriptCore runs a single mutator thread executing JavaScript alongside background compiler threads, which are permitted to inspect heap objects — a RegExp cell, the object that owns a compiled regular expression, among them — but only to read already-materialized state, never to mutate it outside the cell's own lock. A RegExp holds either YARR JIT machine code or an interpreted bytecode pattern, and the compiler-thread entry point is supposed to guarantee it only ever consumes code that already exists.

The angle: script that arranges a regexp with JIT code but no bytecode, then drives it through a compiler-thread fold and a mutator match at once, could have one thread destroy the bytecode pattern the other is currently interpreting.

RegExp::matchConcurrently shouldn't cause any compilation, but bail out if JIT code for the regexp doesn't already exist. However, it is possible that a RegExp has JIT code but no bytecode, in which case matchConcurrently can incorrectly racily attempt to compile bytecode. This PR makes byteCodeCompileIfNecessary threadsafe by taking the cell lock. It also bails out of matchConcurrently if the regexp doesn't already have bytecode when called from the compiler thread. There is no new test because to manifest this race requires artificially adding sleeps to threads. Originally landed as 305413.415@rapid/safari-7624.2.5.110-branch.

Source/JavaScriptCore/runtime/RegExp.cpp

Source/JavaScriptCore/runtime/RegExpInlines.h

Patch Details

Two changes. In RegExp.cpp, RegExp::byteCodeCompileIfNecessary(VM*) now takes Locker locker { cellLock() } as its first statement, ahead of the if (m_regExpBytecode) return; early-out and the subsequent byteCodeCompilePattern() call that stores into m_regExpBytecode and sets m_state.

In RegExpInlines.h, both RegExp::matchInline template overloads — the ovector-filling int version and the MatchResult match-only version — restructure the Yarr::JSRegExpResult::JITCodeFailure punt-to-interpreter path. The unconditional byteCodeCompileIfNecessary(&vm); if (m_state == ParseError) return throwError(); pair is now wrapped in if constexpr (matchFrom == Yarr::MatchFrom::VMThread), so only the mutator may compile bytecode. A new unconditional bail-out follows — if (!m_regExpBytecode) return -1; in the ovector overload and return MatchResult::failed(); in the match-only overload — so a compiler thread reaching the punt path with no already-materialized bytecode reports failure instead of compiling. No test accompanies the patch.

Unsynchronized lazy initialization of an owning pointer that a background thread may both read and install, breaking the single-writer contract readers assume.

Background

JSC's threading model. A JSC VM has one mutator thread that executes JavaScript, plus background compiler threads that optimize functions concurrently. Compiler threads may inspect heap objects, but they operate under a contract limiting what they may touch.

cellLock(). Every JSCell exposes a per-cell lock used to serialize mutations of cell-internal state against concurrent readers such as compiler threads or the GC. WTF::Lock is non-recursive.

Yarr::MatchFrom. A compile-time enum (VMThread / CompilerThread) threaded through RegExp::matchInline as a template parameter, so a single body compiles into two specializations with different permissions. RegExp::matchConcurrently is the compiler-thread entry point that delegates to matchInline<Yarr::MatchFrom::CompilerThread>.

YARR execution tiers. A RegExp is either interpreted from a Yarr::BytecodePattern (m_regExpBytecode, produced by byteCodeCompilePattern, which returns a std::unique_ptr<Yarr::BytecodePattern>) or executed as YARR JIT machine code (m_regExpJITCode). RegExp::m_state records which (NotCompiled / JITCode / ByteCode / ParseError); hasCode() is true for either JITCode or ByteCode, while hasCodeFor(charSize) additionally requires, in the JITCode case under YARR_JIT, that the JIT code exists for that specific char size.

JSRegExpResult::JITCodeFailure. A sentinel returned at execution time by YARR JIT code indicating it could not complete the match, signalling the caller to fall back to the bytecode interpreter for that call.

Lazy initialization and unique_ptr assignment. fooIfNecessary() methods in JSC test a cached field and materialize it on first use; the materializing store publishes a heap object whose constructor ran before the store. Assigning a new value to a std::unique_ptr destroys the object it previously owned.

Analysis

The pre-fix byteCodeCompileIfNecessary was a classic double-checked-without-the-lock routine: it tested m_regExpBytecode, and if null called byteCodeCompilePattern(), stored the result, and updated m_state — none of it under cellLock(). matchInline called it unconditionally from the JITCodeFailure punt path, and matchInline is instantiated for both MatchFrom::VMThread and MatchFrom::CompilerThread.

  Mutator thread                    Compiler thread (concurrent fold)
  ──────────────                    ─────────────────────────────────
  matchInline<VMThread>             matchConcurrently: hasCodeFor() passes
   JIT run -> JITCodeFailure          (m_state == JITCode, bytecode null)
   byteCodeCompileIfNecessary        matchInline<CompilerThread>
    m_regExpBytecode == null          JIT run -> JITCodeFailure
    compile; assign  ────────┐        byteCodeCompileIfNecessary
   interpret(bytecode.get()) │         m_regExpBytecode == null (stale)
        still walking  ◄─────┴──────── compile; assign -> unique_ptr
                                        destroys the pattern in use

The load-bearing precondition is why the compiler thread got to a writing path at all. The commit message describes matchConcurrently as pre-checking that code already exists before delegating — the supplied RegExp.cpp/RegExpInlines.h excerpts are truncated before that function, so the pre-check is relayed from the message. What the supplied header does show is that such a pre-check cannot establish the presence of bytecode: with YARR_JIT enabled, hasCodeFor() requires hasCode() (m_state == JITCode || m_state == ByteCode), and in the JITCode branch it is satisfied by m_regExpJITCode having code for the requested char size while m_regExpBytecode is still null. So the reachable case is a RegExp with JIT code and no bytecode, which satisfies the entry-point precondition — and the JITCodeFailure punt is a runtime-discovered tier downgrade that invalidates that precondition after the fact.

Two distinct memory-safety consequences follow. The first is lost-update destruction: two threads both observe m_regExpBytecode as null, both run byteCodeCompilePattern, and both assign — the second unique_ptr assignment destroys the pattern installed by the first. If the first thread has already passed m_regExpBytecode.get() to Yarr::interpret, which loads the raw pointer once and walks it for the duration of the match, the interpreter continues over a freed BytecodePattern. The second is partial publication: on the reading that the pre-patch stores carried no release/acquire pairing (inferred from the fix's shape, since the body past the early-out is truncated out of the supplied source), a thread could observe a non-null m_regExpBytecode whose pointee's construction is not yet visible, and the unguarded m_state write would race the reader's if (m_state == ParseError) and tier-dispatch decisions.

Reachability is from web content: script chooses the pattern and subject string, and decides which functions get hot enough for DFG/FTL to attempt constant-folding of a regexp operation through matchConcurrently. A trigger sequence would be to construct a RegExp whose pattern YARR compiles to JIT code so m_state == JITCode with bytecode still null; place a match against a literal subject inside a hot function so the optimizer queues it for compile-time folding; arrange for the JIT execution on the compiler thread to return JITCodeFailure (patterns that exhaust the JIT's runtime budget being the lever); then, from the mutator, drive the same RegExp into the identical punt path so both threads race the null test. Neither the specific folding call site nor the JITCodeFailure trigger condition appears in the supplied context, so both are derived rather than read.

Best case for an attacker who wins the race would be a use-after-free on the BytecodePattern graph consumed by Yarr::interpret — an interpreter that dereferences pointers out of the freed structure and indexes its vectors, which under successful reclamation could give a controlled relative read and potentially a controlled write into the ovector. Realising that would require heap grooming so the freed allocation, whose size the attacker shapes through pattern complexity since it holds term and disjunction vectors and character-class tables, is reclaimed by controlled data before the interpreter dereferences it. The weaker and more likely outcome is a torn view of m_state/m_regExpBytecode yielding an unstable crash. The commit message states the race manifests only with artificially added sleeps, so widening the natural window in practice would require driving many compiler-thread folds concurrently with mutator matches on the same cached RegExp.

This vulnerability weakens memory safety inside the WebContent process by breaking the thread-safety contract separating the single mutator from JSC's concurrent compiler threads. The fix restores the invariant that only the mutator writes m_regExpBytecode and that the check-and-install is serialized by cellLock().

The fix is notably belt-and-braces — it both adds the lock and removes the compiler thread as a writer. If matchConcurrently already holds cellLock() across the call, as the commit message's contract comment suggests, the if constexpr gate would be load-bearing rather than redundant, since WTF::Lock is non-recursive and the lock alone would have converted the race into a self-deadlock on the compiler thread; the supplied excerpts stop short of that function, so the reading cannot be checked here. Worth noting too that the ENABLE(YARR_JIT_DEBUG) block a few lines below the patched hunk still calls byteCodeCompileIfNecessary(&vm) without the new gate — debug-build-only code, but the same shape the patch just removed from production.

Audit directions

• A background thread granted read-only access nonetheless reaching a lazy-materialization path, because the entry-point precondition proves a superset of what the inner code requires. The invariant: a boundary check admitting a background thread must prove the exact resource the inner path will consume, not merely that some equivalent resource exists. Narrow: audit the other *IfNecessary members of RegExp — compileIfNecessary and compileIfNecessaryMatchOnly are still called near the top of matchInline regardless of matchFrom, and their compiler-thread safety rests entirely on the entry-point pre-check; also check the ENABLE(YARR_JIT_DEBUG) block, which still calls byteCodeCompileIfNecessary ungated. In review, an if (m_x) return; m_x = build(); body inside a function transitively reachable from a MatchFrom::CompilerThread / ConcurrentJSLocker-style entry point is the tell. Wider: the same class shows up wherever background compilation touches heap-resident derived state — rope resolution on JSString, property-table materialization on Structure, cached string/number conversions — so enumerate the concurrent-folding call sites in DFGAbstractInterpreterInlines.h and DFGConstantFoldingPhase.cpp and check that every fallback branch of each callee is read-only, not just the happy path. Widest: a capability check performed at a boundary must be re-established at each internal tier downgrade — applies to V8's background compile / LocalHeap accesses, SpiderMonkey's offthread parsing, and any permission gate admitting a caller on a coarse hasSomething() predicate. Match tell across codebases: a fast-path/slow-path fork after the permission check where the slow path allocates or installs state.
• Non-atomic publication of a heap object through an owning pointer read by another thread. The invariant: a pointer field any second thread may load must be installed under the same lock that guards the load, or be an atomic with release/acquire semantics. Narrow: grep Source/JavaScriptCore/runtime and Source/JavaScriptCore/yarr for std::unique_ptr members assigned inside a method whose body starts with a null test on that same member, and check whether the enclosing method takes cellLock() or a ConcurrentJSLocker; RegExp::m_rareData and m_ovector are the adjacent fields on this very cell to sanity-check. Wider: the same class appears with any deferred-initialization mechanism — std::once_flag-free memoization, Box/Ref fields installed lazily, and cached-derived-value patterns where an enum discriminant is written separately from the payload pointer, so the two can be observed out of order. In review, two ordinary stores in a row — one to a pointer, one to a state flag — with no lock and no WTF::storeStoreFence is the visual tell. Widest: the classic unsafe-publication class, identical in shape to Java's non-volatile double-checked locking and Rust code needing Arc + OnceLock; if a reader can see the pointer, it must be guaranteed to see everything the constructor wrote.
• Adding a lock to a leaf function without auditing callers that may already hold it. WTF::Lock is non-recursive, so this is a hazard introduced by the standard fix for a race. The invariant: for each lock, the set of functions that acquire it must be an antichain in the call graph. Narrow: trace every caller of RegExp::byteCodeCompileIfNecessary — both matchInline overloads plus the ENABLE(YARR_JIT_DEBUG) path — and confirm none can be on a stack already holding this cell's cellLock(); start by reading RegExp::matchConcurrently, the caller the supplied excerpt does not cover, and verify the debug path under a YARR_JIT_DEBUG build rather than by reading. Wider: the same class covers every commit that adds Locker locker { cellLock() } or a ConcurrentJSLocker to a previously-unlocked helper — check each new acquisition site against its callers, since deadlock, unlike a race, is invisible to review and only surfaces under the exact interleaving the fix was meant to serialize. In review, a Locker added to a function marked ALWAYS_INLINE or otherwise clearly designed as a callee of larger orchestration functions is the tell. Widest: applies to any codebase with non-reentrant mutexes and a habit of pushing locking down into helpers — Chromium's base::Lock, Rust's Mutex. When you move a lock acquisition, you change the lock's call-graph position, and every ancestor becomes a deadlock candidate.