SourceCodeKey: restore source string comparison in operator== - #346
SourceCodeKey: restore source string comparison in operator==#346robobun wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 3 minutes 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
|
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? |
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.
e625133 to
e73097b
Compare
There was a problem hiding this comment.
Re-reviewed after the latest push — both earlier nits are addressed (the fallback comment now cites only string() for the rejection, and UNUSED_PARAM(sourceType) is gone), and no new issues were found. Given this touches CodeCache key equality and the bytecode decode fast-path, a human sign-off still makes sense.
What was reviewed:
SourceCodeKey::operator==— confirmed the restored conjunct is byte-for-byte upstream's; the Bun divergence is fully removed.CachedStringSourceProvider::decode— dropping thesourceTypecheck leaves only the length guard before reusing the runtime provider;operator==never readssourceType, so a mismatched-type provider with matching length still compares by pointer/string correctly.- Fallback path callers (
isCachedBytecodeStillValid,decodeSourceCodeKey) — with no provider, the empty-source key is now rejected via the restoredstring()compare, not silently accepted.
Extended reasoning...
Overview
Two files: SourceCodeKey.h deletes the USE(BUN_JSC_ADDITIONS) divergence in operator==, restoring upstream's (m_sourceCode == other.m_sourceCode || string() == other.string()) conjunct. CachedTypes.cpp rewrites three comment blocks in CachedStringSourceProvider and drops the provider->sourceType() == sourceType check from the decode fast-path so Bun's Zig::SourceProvider (whose sourceType differs from the encode-time Program) is reused rather than falling through to the empty-source fallback.
Since my previous review the author pushed a revision that (1) rewrote the fallback comment to attribute rejection solely to the string() mismatch, and (2) removed the stray UNUSED_PARAM(sourceType). Both prior inline comments are resolved and the current diff reflects those fixes. The bug hunting system found nothing on this revision.
Security risks
None identified. The change strictly tightens SourceCodeKey equality (from hash+length+flags to additionally requiring source-byte equality), which closes a wrong-code-execution / segfault path rather than opening one. Dropping the sourceType check in decode() only affects which SourceProvider object is attached to the decoded key; it does not weaken the key comparison itself, and operator== does not consult sourceType.
Level of scrutiny
High. SourceCodeKey is the hash-map key for every CodeCache lookup (modules, programs, indirect eval, new Function), so a false-positive equality means executing the wrong source's bytecode. The SourceCodeKey.h half is a pure revert-to-upstream and is very low risk on its own. The CachedTypes.cpp half is small but subtle: it depends on the invariant that Bun's decode path always supplies a provider whose source() matches the runtime key's, so the pointer fast-path in operator== fires and the (unstored) source bytes are never compared. That reasoning checks out, but it is exactly the kind of cross-component invariant a maintainer familiar with Bun's --compile --bytecode pipeline should confirm.
Other factors
A maintainer is already engaged on the thread and asked the author to address the review comments, so human review is clearly in progress. Regression tests covering module/require/new Function/vm.Script/indirect-eval collisions live on the Bun side (oven-sh/bun#35778), and preview builds for the latest commit succeeded. Net: the change looks correct and my earlier feedback is fully addressed; deferring only because the code path is correctness-critical and a human is already reviewing.
…lisions Under USE(BUN_JSC_ADDITIONS), SourceCodeKey::operator== dropped the source string comparison and relied only on the 24-bit StringImpl hash plus length/flags/host. Two distinct modules whose transpiled source has the same length and colliding 24-bit hash then share one cached UnlinkedModuleProgramCodeBlock. When bun feeds JSC a JSModuleRecord built from the second module's export names and CyclicModuleRecord::initializeEnvironment links it against the first module's moduleEnvironmentSymbolTable, the second module silently evaluates the first module's code, or (when the exported function name differs) getValue() in JSModuleNamespaceObject hits 'ASSERTION FAILED: iter != symbolTable->end(locker)' in debug and derefs a null SymbolTableEntry in release. The WebKit-side fix (oven-sh/WebKit#346) restores upstream's behavior: fast-path on UnlinkedSourceCode equality, fall back to a source string compare. This commit bumps WEBKIT_VERSION to the preview build of that PR and adds tests for both observable failure modes.
|
Rebased onto current main as a single commit (e73097b); the two inline comments from the earlier round were already addressed and resolved. Preview |
3186362 dropped the source string comparison from SourceCodeKey::operator== under USE(BUN_JSC_ADDITIONS), leaving only the 24-bit StringImpl hash plus length/flags/name/host. Two sources of equal length whose hashes collide then compare equal, so CodeCache hands the second one the first one's unlinked code block. This affects every CodeCache consumer: module programs (where it also segfaults, because Bun's JSModuleRecord carries the second module's export names and JSModuleNamespaceObject::getValue derefs an end() symbol table iterator), programs (require of CommonJS, vm.Script), indirect eval, and new Function all return the first colliding source's code. Restore upstream's comparison: fast-path on UnlinkedSourceCode equality (provider pointer + offsets), fall back to the source string compare. The hash and length checks ahead of it reject almost every mismatch, so the string compare only runs on hash-collision bucket probes and on legitimate cache hits across distinct providers. CachedStringSourceProvider::decode reused the runtime provider for the decoded key only when sourceType matched. Bun encodes bytecode through makeSource() (a StringSourceProvider, sourceType Program) and decodes against a Zig::SourceProvider (BunTranspiledModule or Module), so that check always failed and fell through to the empty-source fallback. That was harmless while operator== skipped the source compare; with it restored, the fallback's empty string() made every disk-cache lookup miss. operator== does not read sourceType, so drop that check and keep the length guard; the callers that pass no provider (isCachedBytecodeStillValid, decodeSourceCodeKey) still get the conservative rejection via the string compare.
e73097b to
83153fb
Compare
…lisions Under USE(BUN_JSC_ADDITIONS), SourceCodeKey::operator== dropped the source string comparison and relied only on the 24-bit StringImpl hash plus length/flags/host. Two distinct modules whose transpiled source has the same length and colliding 24-bit hash then share one cached UnlinkedModuleProgramCodeBlock. When bun feeds JSC a JSModuleRecord built from the second module's export names and CyclicModuleRecord::initializeEnvironment links it against the first module's moduleEnvironmentSymbolTable, the second module silently evaluates the first module's code, or (when the exported function name differs) getValue() in JSModuleNamespaceObject hits 'ASSERTION FAILED: iter != symbolTable->end(locker)' in debug and derefs a null SymbolTableEntry in release. The WebKit-side fix (oven-sh/WebKit#346) restores upstream's behavior: fast-path on UnlinkedSourceCode equality, fall back to a source string compare. This commit bumps WEBKIT_VERSION to the preview build of that PR and adds tests for both observable failure modes.
There was a problem hiding this comment.
I reviewed this PR and found no issues; the two earlier nits (misleading length() claim in the fallback comment, stray UNUSED_PARAM(sourceType)) are both addressed in the current diff. Because it changes core CodeCache key equality and relaxes the bytecode-decode provider check, a human sanity-check on the --compile --bytecode path would still be worthwhile.
What was reviewed:
SourceCodeKey::operator==— confirmed the restored conjunct is byte-identical to upstream;UnlinkedSourceCode::operator==is defaulted (provider ptr + offsets), so the pointer fast-path holds.CachedStringSourceProvider::decode— dropping thesourceTypecheck is safe: the decoded key is only used forSourceCodeKey::operator==, which never reads sourceType, and the restoredstring()compare backstops a wrong-provider length collision.- No-provider fallback — verified
CachedUnlinkedSourceCodeShape::decoderestores encoded offsets, so rejection relies onstring()(notlength()), matching the corrected comment.
Extended reasoning...
Overview
Two files touched. SourceCodeKey.h deletes the #if USE(BUN_JSC_ADDITIONS) branch in operator== that skipped the final (m_sourceCode == other.m_sourceCode || string() == other.string()) conjunct — a pure revert to upstream WebKit behaviour. CachedTypes.cpp rewrites three comment blocks in CachedStringSourceProvider to reflect that source bytes are compared again, and drops the provider->sourceType() == sourceType guard from the decode-time provider-reuse fast path (keeping the length guard).
Security risks
None introduced. The change closes a correctness/safety hole: the prior 24-bit-hash-only comparison allowed hash-colliding sources to share an UnlinkedCodeBlock, which the PR description shows can segfault via an end-iterator read in SymbolTableEntry. Restoring the source-byte comparison is strictly stricter. Dropping the sourceType guard cannot admit a wrong provider into a cache hit because the decoded key still flows through operator==, which now compares source bytes; a length-colliding wrong provider fails the string() compare.
Level of scrutiny
Moderate-to-high. SourceCodeKey is the key for every CodeCache consumer (modules, programs, indirect eval, new Function), so a mistake here affects all JS execution. That said, the SourceCodeKey.h half is a mechanical revert-to-upstream and carries essentially zero risk. The CachedTypes.cpp half is a one-condition relaxation on a Bun-only bytecode-cache path (--compile --bytecode), with a clear rationale (encode via StringSourceProvider/Program, decode via Zig::SourceProvider) and CI coverage in the paired oven-sh/bun#35778 (bundler_compile.test.ts, new code-cache-collision.test.ts). The remaining question — whether any Bun caller depends on the sourceType mismatch to reject a stale cache entry — is best confirmed by someone familiar with the Bun-side Decoder::create call sites.
Other factors
My two earlier inline nits were addressed: the fallback comment now correctly attributes rejection to string() alone, and the misleading UNUSED_PARAM(sourceType) is gone (sourceType is still consumed by the fallback StringSourceProvider::create at line 1694). The two subsequent pushes were pure rebases per the author's comments. I traced UnlinkedSourceCode::operator== (defaulted, so provider-pointer + offsets) and CachedUnlinkedSourceCodeShape::decode (restores encoded offsets regardless of provider) to confirm the comment claims and fast-path reasoning are accurate. Deferring rather than approving because this is core-engine cache correctness with a semantic relaxation, not a purely mechanical change.
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
This pages the entire source code string into memory. Isn't the issue here more so that the source hash isn't sufficiently unique? This scenario should only be possible in an adversarial scenario designed to specifically trigger this condition.
3 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 sources of equal length whose hashes collide then compare equal, andCodeCacheMap::findCacheAndUpdateAgereturns the first source's unlinked code block for the second.SourceCodeKeyis the key for everyCodeCacheconsumer, so on current Bun all of these return the first colliding source's code (node returns the distinct values):UnlinkedModuleProgramCodeBlock): wrong exports, or a segfault when the local names differ. Bun builds the second module'sJSModuleRecordfrom its ownModuleInfo,CyclicModuleRecord::initializeEnvironmentlinks it against the cached first module'smoduleEnvironmentSymbolTable(), andJSModuleNamespaceObject::getOwnPropertySlotCommon → getValuelooks up a local name that is not in the table. Debug tripsASSERT(iter != symbolTable->end(locker)); release readsSymbolTableEntry::scopeOffset()off an end iterator and segfaults at0x0.UnlinkedProgramCodeBlock):require()of two colliding CommonJS files,vm.Script.UnlinkedEvalCodeBlock).new Function(getUnlinkedGlobalFunctionExecutable).Minimal module repro (release bun):
Fix
SourceCodeKey::operator==: restore upstream's comparison: fast-path onm_sourceCode == other.m_sourceCode(provider pointer + offsets), fall back to the sourceStringViewcompare. The hash and length checks ahead of it reject almost every mismatch, so the string compare only runs on hash-collision bucket probes and on legitimate cache hits across distinct providers.CachedStringSourceProvider::decode: it reused the runtime provider for the decoded key only whensourceTypematched. Bun encodes bytecode throughmakeSource()(aStringSourceProvider,sourceType Program) and decodes against aZig::SourceProvider(BunTranspiledModuleorModule), so the check always failed and fell through to the empty-source fallback. Harmless whileoperator==skipped the source compare; with it restored, the fallback's emptystring()made every--compile --bytecodelookup miss (caught bybundler_compile.test.tsin jsc: fix CodeCache hash collisions running the wrong source's code (modules segfault) bun#35778 CI).operator==does not readsourceType, so drop that check; the length guard stays, and the callers that pass no provider (isCachedBytecodeStillValid,decodeSourceCodeKey) still get the conservative rejection via the string compare.Rebased onto current
main(past #405, which touched the adjacentCachedSourceProviderswitch; no overlap). Bun side with regression tests for the module,require,new Function,vm.Scriptand indirect eval paths: oven-sh/bun#35778.