SourceCodeKey: restore source string comparison in operator== - #346
SourceCodeKey: restore source string comparison in operator==#346robobun wants to merge 3 commits into
Conversation
3186362 removed the source string comparison as an optimization, leaving only the 24-bit StringImpl hash + length + flags. That lets two modules with colliding source hashes share one UnlinkedModuleProgramCodeBlock via CodeCache. When Bun hands JSC a JSModuleRecord built from the second module's import/export names, initializeEnvironment links it against the first module's symbol table, producing wrong export values or a null SymbolTableEntry deref in JSModuleNamespaceObject::getOwnPropertySlotCommon (release segfault). Restore upstream's behavior: fast-path on UnlinkedSourceCode equality (provider pointer + offsets), fall back to the source string compare. The string compare runs only on hash-collision bucket probes and legitimate cache hits across distinct providers.
|
Warning Review limit reached
Next review available in: 55 seconds Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — this reverts a Bun-specific divergence back to upstream WebKit's operator==, restoring the source-string comparison needed for correctness.
What was reviewed:
- Confirmed the removed
#if USE(BUN_JSC_ADDITIONS)branch left equality gated only on the 24-bit hash + length/flags/name/host, which is collision-prone as described. - Verified
UnlinkedSourceCode::operator==is the defaulted provider-pointer + offset compare, so the restored fast-path is cheap and thestring()fallback only fires on hash-bucket collisions. - Result is byte-identical to upstream's
SourceCodeKey::operator==; no other Bun-specific paths reference the removed branch.
Extended reasoning...
Overview
Single-file change to Source/JavaScriptCore/parser/SourceCodeKey.h. It deletes a 4-line #if USE(BUN_JSC_ADDITIONS) / #else / #endif block inside SourceCodeKey::operator==, so the Bun build now takes the same final conjunct as upstream: && (m_sourceCode == other.m_sourceCode || string() == other.string()). Previously the Bun branch terminated the comparison after host() == other.host(), meaning two distinct sources with the same length and colliding 24-bit StringImpl hash were treated as equal in CodeCacheMap.
Security risks
None introduced. If anything this closes a correctness/soundness hole: a hash collision could previously cause the wrong UnlinkedModuleProgramCodeBlock to be returned from the code cache, leading to a segfault (release) or assertion (debug), and in the worst case executing one module's code under another module's record. Restoring the full source comparison eliminates that possibility.
Level of scrutiny
Low. This is a pure revert-to-upstream of a hash-key equality operator — no new logic, no new API surface, and the resulting code path is exactly what upstream WebKit ships and has shipped for years. UnlinkedSourceCode::operator== is the compiler-defaulted comparison over RefPtr<SourceProvider> + two int offsets (UnlinkedSourceCode.h:102), so the fast path is a pointer + two-int compare; the StringView compare only runs when providers differ but hash/length already matched.
Other factors
The PR description includes a concrete minimal repro demonstrating the segfault, and the diff strictly reduces divergence from upstream. The only performance concern (the reason the branch was presumably added) is addressed in the description: hash + length filtering means the string compare almost never runs. No outstanding reviewer comments; no bugs flagged by the automated bug hunt.
Preview Builds
|
…e provider The decoded SourceCodeKey is only used for operator== against the runtime key, which does not read sourceType(). Bun encodes bytecode via makeSource() (StringSourceProvider, sourceType Program) but decodes against a Zig::SourceProvider whose sourceType is BunTranspiledModule or Module, so the sourceType check always failed and fell through to the empty-source fallback. That was harmless while operator== skipped the source compare; with it restored (previous commit) the fallback's empty string() causes every disk-cache lookup to miss. Keep the length check so a provider for a different file is still rejected; stale-source detection is the separate m_hash compare.
The restored source string compare in SourceCodeKey::operator== made every compiled-binary bytecode lookup miss: CachedStringSourceProvider only reused the runtime provider when sourceType matched, but bun encodes via a StringSourceProvider (sourceType Program) and decodes against a Zig::SourceProvider (BunTranspiledModule/Module), so the fallback empty-source provider was always used and its empty string() failed the new compare. oven-sh/WebKit#346 now drops the sourceType check; the length check and the separate m_hash compare still reject mismatched or stale source.
There was a problem hiding this comment.
Beyond the two inline nits, I also checked whether dropping the sourceType check reopens the collision on the on-disk cache path — it doesn't: when decoder.provider() is supplied the same pointer lands in both keys so the m_sourceCode == fast-path fires, and when it isn't the empty-source fallback now fails the restored string() compare.
Extended reasoning...
The inline findings are cosmetic only. The one behavioral change here — removing provider->sourceType() == sourceType from the reuse path — is required by the SourceCodeKey.h revert: bun encodes via a StringSourceProvider (Program) but decodes against a Zig::SourceProvider (BunTranspiledModule/Module), so the old check always fell through to the empty-source fallback, which only compared equal because operator== skipped string(). With the string compare restored that fallback would reject every hit, hence the check must go; the length gate plus the outer m_hash compare still prevent substituting a provider for a different file. Not approving because the correctness of that argument depends on Bun-side Zig::SourceProvider behavior outside this repo.
|
@robobun can you address these reviews? |
- Cap all BUN_JSC_ADDITIONS comments at 3 lines. - Remove UNUSED_PARAM(sourceType): it is still passed to StringSourceProvider::create in the fallback path. - Fix the fallback comment: length() still matches (offsets come from the encoded record); rejection relies on string().
There was a problem hiding this comment.
Both earlier nits are addressed in e625133 (spurious UNUSED_PARAM dropped, fallback comment now correctly attributes rejection to string() alone) and this pass found nothing new. Deferring final approval to a human since the CachedTypes.cpp change drops the sourceType gate on the provider-reuse fast path, and its correctness hinges on Bun-side Zig::SourceProvider / disk-cache encode behaviour that isn't visible from this repo.
What was reviewed:
SourceCodeKey::operator==— verified this is a clean revert to upstream's conjunct; strictly narrows equality, cannot introduce false hits.CachedStringSourceProvider::decode— traced that the reused provider only feedsSourceCodeKeyequality, which readsm_flags(encodesSourceCodeType) rather thanprovider->sourceType(), so dropping the gate does not weaken the key comparison.- Fallback path — confirmed
sourceTypeis still consumed byStringSourceProvider::createafter the#endif, so no unused-param warning is introduced.
Extended reasoning...
Overview
Two files: SourceCodeKey.h restores the upstream (m_sourceCode == other.m_sourceCode || string() == other.string()) conjunct in operator== by deleting the USE(BUN_JSC_ADDITIONS) early-terminator, fixing a 24-bit-hash-collision bug that let CodeCacheMap return the wrong module's UnlinkedModuleProgramCodeBlock (segfault or wrong-code execution). CachedTypes.cpp rewrites the encode/decode comments to reflect the restored comparison and drops the provider->sourceType() == sourceType guard on the runtime-provider reuse path (rationale: Bun encodes as Program but decodes against a Zig::SourceProvider, so the guard defeated the fast path and, with the string compare restored, would have caused every disk-cache entry to be rejected via the empty-source fallback).
Since my last review, commit e625133 addressed both nits: the misleading UNUSED_PARAM(sourceType) is gone, and the fallback comment now says only string() mismatches (not length()).
Security risks
None identified. The change strictly tightens equality in the in-memory code cache (adds a source-bytes comparison), which can only reduce false cache hits. The sourceType guard removal loosens the disk-cache provider-reuse condition, but that provider is only used to construct a SourceCodeKey for comparison — and SourceCodeKey compares code type via m_flags, not via the provider's sourceType(), so no comparison dimension is lost.
Level of scrutiny
High. This is the code cache key equality — a false positive here means executing bytecode compiled from a different source file, which is exactly the class of bug this PR fixes. The SourceCodeKey.h half is a low-risk revert-to-upstream. The CachedTypes.cpp half changes disk bytecode-cache validation semantics and its justification ("bun encodes as Program but decodes against a Zig::SourceProvider") depends on encode-side behaviour in the Bun repo that I cannot verify here. That cross-repo dependency is the reason I'm not auto-approving.
Other factors
Preview builds for both 3707e09 and 952c7d4 succeeded. A maintainer (dylan-conway) is already engaged on the thread. The prior inline comments are marked resolved and the follow-up commit matches the suggested fixes.
2 participants
3186362 dropped the source string comparison from
SourceCodeKey::operator==underUSE(BUN_JSC_ADDITIONS), leaving only the 24-bitStringImplhash plus length/flags/name/host. Two modules whose transpiled source has the same length and a colliding 24-bit hash then compare equal, soCodeCacheMap::findCacheAndUpdateAgereturns the first module'sUnlinkedModuleProgramCodeBlockfor the second module.When Bun constructs the second module's
JSModuleRecordfrom its ownModuleInfo(export/local names from the second source) andCyclicModuleRecord::initializeEnvironmentlinks that record against the cached first module'smoduleEnvironmentSymbolTable(),JSModuleNamespaceObject::getOwnPropertySlotCommon → getValuelooks up a local name that is not in the symbol table. In debug this tripsASSERT(iter != symbolTable->end(locker)); in release it readsSymbolTableEntry::scopeOffset()off an end iterator and segfaults. If the local names happen to match, the second module silently evaluates the first module's code instead.Minimal repro (release bun):
Restore upstream's comparison: fast-path on
m_sourceCode == other.m_sourceCode(provider pointer + offsets), fall back to the sourceStringViewcompare. The hash and length checks before this line already reject almost every mismatch, so the string compare only runs on hash-collision bucket probes and on legitimate cache hits across distinct providers.