speculationFromCell: reject misaligned StringImpl* from concurrent zap residue#285
speculationFromCell: reject misaligned StringImpl* from concurrent zap residue#285robobun wants to merge 1 commit into
Conversation
…p residue
On weak-memory-order architectures (observed on Windows arm64 in Bun CI),
the DFG compiler thread can read a JSString from a ValueProfile bucket
before the main thread's m_fiber store becomes visible. With no release
fence between construction and the bucket store (mutatorFence() is a no-op
outside concurrent marking), the compiler thread observes the slot's
previous bytes: HeapCell::zap(StopAllocating) residue.
zap() writes cellWords[2] = ZapReason (= 2 for StopAllocating) and leaves
cellWords[3] intact, so m_fiber reads as {low32=2, high32=prev-hi-bits},
e.g. 0x0000021700000002. isSanePointer() passes (below 48 bits, above
page 0), tryGetValueImpl() sees bit 0 clear so returns it as a StringImpl*,
and impl->isAtom() segfaults at [impl + 0x10].
StringImpl is at least 8-byte aligned (pointer member), so rejecting
misaligned impl pointers catches the zap residue without touching real
values. Extends the existing rdar://69036888 band-aid to cover the case
isSanePointer lets through.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe ChangesString pointer validation
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs — the added alignment check is strictly defensive and cannot reject a valid StringImpl*. Deferring to a human because this is core JIT speculation code and the underlying arm64 memory-ordering analysis (and the band-aid-vs-fence tradeoff) is worth a second pair of eyes.
What was reviewed:
- Verified
alignof(StringImpl) >= 8(inheritsStringImplShapewith pointer members), so real pointers always pass. - Confirmed
ZapReason::StopAllocating == 2sets bit 1, caught by the mask;Destruction == 1already handled viaisRopeInPointer. - Fallback to
SpecNoneis the existing conservative path;ASSERT_NOT_REACHEDmatches the pre-existingisSanePointerpattern.
Extended reasoning...
Overview
Single-line change to speculationFromCell in Source/JavaScriptCore/bytecode/SpeculatedType.cpp: adds || (reinterpret_cast<uintptr_t>(impl) & (alignof(StringImpl) - 1)) to the existing isSanePointer guard before dereferencing a concurrently-read StringImpl* from JSString::m_fiber.
Security risks
None introduced. The change only widens a rejection condition on a pointer read from a racy profiling bucket. A valid heap-allocated StringImpl is always at least pointer-aligned (it derives from StringImplShape, which holds a const char*/const char16_t* union), so no legitimate pointer is rejected. Rejecting misaligned garbage that would otherwise be dereferenced is strictly a hardening improvement.
Level of scrutiny
High. This is DFG type speculation — a hot, correctness-critical path in the JIT — and the rationale rests on a detailed arm64 weak-memory-ordering race between the mutator, baseline JIT prologue, and the DFG compiler thread. The code change itself is trivially safe (purely additive guard, conservative fallback), but the root-cause analysis and the decision to extend the rdar://69036888 band-aid rather than add a release fence are architectural judgments a JSC maintainer should sign off on.
Other factors
- The crash was seen once in CI and is not locally reproducible, so there is no test.
ASSERT_NOT_REACHED()will still fire in debug builds when this race occurs, consistent with the existingisSanePointerbranch; that is pre-existing behavior, not a regression.- No prior reviews or comments on the PR.
Preview Builds
|
…ointers https://bugs.webkit.org/show_bug.cgi?id=319215 Reviewed by Yusuke Suzuki. A sane pointer to a T must be aligned like a T. Add a typed overload that checks this on top of the existing canonical-address check. The void* overload keeps its old behavior for callers probing arbitrary addresses (e.g. the disassembler); all typed call sites pick up the stricter check without change. This catches the value seen in a real crash: a concurrent DFG thread read HeapCell::zap() residue (0x21700000002) out of a freshly reused JSString and dereferenced it as a StringImpl*, because it passed every existing check in speculationFromCell() (oven-sh#285). The reordering race itself is fixed in bug 319213; this hardens the checks that bug 216638 added as a mitigation for bad pointers from profiling data. * Source/JavaScriptCore/tools/Integrity.h: (JSC::Integrity::isSanePointer): Canonical link: https://commits.webkit.org/317118@main
|
Another occurrence in Bun CI, Windows arm64, build 74427 on Disassembly at the faulting IP (
|
|
Two more sightings of this on Bun CI's Windows arm64 lane this week, on unrelated PRs:
Both symbolicate to the same stack ( Sentry shows this is the same crash users are hitting in production. Across the four grouped issues (BUN-2V2R, BUN-2X1W, BUN-3JDB, BUN-3K59) there are ~1,200 events since April, effectively 100% on aarch64 (predominantly macOS, also Linux and Windows). The fault address distribution is dominated by The branch is 12 commits behind oven-sh/WebKit main now (bun currently pins |
1 participant
Crash
Seen in Bun CI on Windows arm64 (build #72279), segfault at
0x21700000012:Faulting instruction:
ldrb w8, [x9, #0x10]readingStringImpl::m_hashAndFlagsforimpl->isAtom(), withx9 = 0x21700000002loaded fromJSString::m_fiberat[cell+8].Cause
0x0000021700000002isHeapCell::zap(StopAllocating)residue:So bytes 8-15 as u64 LE =
(0x00000217 << 32) | 0x00000002.The race, on arm64:
stopAllocating()zaps free cells instringSpace(JSString hasneedsDestruction).m_fiber = StringImpl*, passes it as an argument to a JS call.ValueProfilebucket (plain store).decodeConcurrent= plain load on JSVALUE64) and thencell->m_fiber(fiberConcurrently()= plain load).mutatorFence()is a no-op outside concurrent marking, so there is no release fence between step 2 and step 3. arm64 can reorder the two stores; the DFG thread observes the cell pointer before them_fiberstore and reads zap residue.cell->isString()passes because byte 5 is preserved through zap.isSanePointer(0x21700000002)passes (below 48 bits, abovelowestAccessibleAddress).tryGetValueImpl()sees bit 0 clear so returns it as aStringImpl*.impl->isAtom()dereferences[impl + 0x10]and segfaults.Not reproducible locally (0/120 runs with the exact CI binary and environment), not seen on recent main builds.
Fix
StringImplis at least 8-byte aligned (it has a pointer member). The zap residue hasZapReason(1 or 2) in the low word, so a misalignedimpldistinguishes it from any real pointer.ZapReason::Destruction(1) is already handled (bit 0 =isRopeInPointer,tryGetValueImpl()returns null); this change catchesStopAllocating(2).This extends the existing
rdar://69036888band-aid rather than adding fences to the hot path.