Repository navigation
Conversation
3186362 ("Micro-optimize SourceCodeKey") dropped the final `m_sourceCode == other.m_sourceCode || string() == other.string()` term from SourceCodeKey::operator== under USE(BUN_JSC_ADDITIONS). What is left of the key is the provider's StringImpl hash (24 bits), the length, the parse flags, the function name and the origin host, so any two sources of the same length whose text hashes collide compare equal in the CodeCache and the second one to be compiled is handed the UnlinkedCodeBlock of the first: a module, program, eval or Function body silently runs some other source's code. With a 24-bit hash a graph of a few thousand same-length modules hits this routinely; the WTF switch from WyHash to RapidHash only changed which pairs collide. Restore the upstream comparison. The UnlinkedSourceCode equality check in front of it keeps the common hit (same provider re-evaluated, or a bytecode cache entry decoded against its own provider) free of any string comparison; the text is only compared when two different providers produce the same hash and length. JSTests/modules/code-cache-key-compares-source.js imports two 31-byte modules whose sources both hash to 0xd438ab; without the fix the second module evaluates to the first module's value.
|
Warning Review limit reached
Next review available in: 11 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 (4)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — reverts a broken Bun-specific micro-optimization back to upstream's correct behavior.
What was reviewed:
SourceCodeKey::operator==: removing the#if USE(BUN_JSC_ADDITIONS)branch restores them_sourceCode == other.m_sourceCode || string() == other.string()term, making the header byte-identical to upstream WebKit.- Confirmed the 24-bit
StringImplhash + length is insufficient as a key equality proxy; the restoredUnlinkedSourceCodepointer-equality short-circuit keeps the common same-provider path cheap. - New regression test:
shouldBeimport resolves against the existingJSTests/modules/resources/assert.js; the two 31-byte collision fixtures and the test comment explaining how to regenerate them look right.
Extended reasoning...
Overview
This PR touches one production file, Source/JavaScriptCore/parser/SourceCodeKey.h, deleting a 4-line #if USE(BUN_JSC_ADDITIONS) block so that SourceCodeKey::operator== once again ends with && (m_sourceCode == other.m_sourceCode || string() == other.string()). The result is byte-identical to upstream WebKit. It also adds a 3-file regression test under JSTests/modules/ with two hand-crafted 31-byte modules whose source text collides on the 24-bit StringImpl hash.
Security risks
The bug being fixed is itself the security-relevant surface: a 24-bit-hash + length collision in the per-VM CodeCache caused one module to execute another module's compiled bytecode. The fix strictly tightens the equality check by adding a term back, so it cannot introduce new false positives. The added StringView comparison is a read-only memcmp over already-held source text; no new allocation, parsing, or untrusted input handling is introduced. No auth/crypto/permissions code is touched.
Level of scrutiny
Low-to-moderate. Although CodeCache is on a hot path for every top-level compile, the change is a pure revert to upstream's long-standing, well-tested comparison. There is no new logic to reason about — the diff removes a divergence rather than adding one. The performance concern that motivated the original micro-optimization is addressed by the m_sourceCode == other.m_sourceCode short-circuit (upstream 5fdba4d), so the string comparison only runs on distinct-provider hash+length collisions.
Other factors
- The regression test imports
shouldBefrom./resources/assert.js, which exists and exports that function. - The test comment documents that the fixture files must remain byte-for-byte and how to regenerate a colliding pair, which protects against accidental reformatting breaking the test.
- No outstanding reviewer comments; the only timeline entry is a CodeRabbit rate-limit notice.
- The bug hunting system found no issues.
Preview Builds
|
|
Closing as a duplicate of #346, which has been open for this same bug since July and is the more complete change. #346 restores the same The Bun side of #346 is oven-sh/bun#35778. The module test added here (two 31-byte sources hashing to |
Problem
.tsfiles whose 226-byte transpiled outputs both hash to0xc8c7c0fails withReferenceError: d1427 is not definedinside the file that never mentionsd1427; the second module is running the first module's bytecode. A minimal pair is in the new test:first.js/second.jsareexport const value = 10001215;andexport const value = 10006987;, both 31 bytes hashing to0xd438ab, and importing both yields10001215twice.SourceCodeKey::operator==underUSE(BUN_JSC_ADDITIONS)(Source/JavaScriptCore/parser/SourceCodeKey.h). The key now compares equal whenever hash, length, flags, name and origin host match. The hash isSourceProvider::hash(), i.e. the 24-bitStringImplhash of the text, soCodeCacheMap::findCacheAndUpdateAgereturns theUnlinkedModuleProgramCodeBlock(or program / eval / Function code block) that was compiled for a different source.jsc -mfrom the current prebuilt (autobuild-f0f60fd...) and Bun reproduce it;--useCodeCache=falsemakes both produce the right values. Bun 1.3.14 shows the same failure with a pair that collides under the previous WyHash-basedStringHasher(10002063/10002789), so this has been latent since 2024 and the RapidHash switch only changed which pairs collide.Fix
&& (m_sourceCode == other.m_sourceCode || string() == other.string()). The header is now identical to upstream, so future merges no longer carry this hunk.UnlinkedSourceCodeequality in front of the text comparison is what upstream added in 5fdba4d; a hit for the same provider (re-evaluating the sameSourceCode,--isolatereuse of one provider, or a bytecode cache entry, whose decoded key points at the live provider) short-circuits before touching the text. TheStringViewcomparison only runs when two distinct providers agree on hash and length, which is either identical text from a second provider (a memcmp, far cheaper than the compile it saves) or the collision this PR is about.runtime/CodeCache.cppandruntime/CachedTypes.cppwith the fixed header against thee2f13c6prebuilt libraries and relinking thejscshell: the new test and the Bun reproducer pass on that shell and fail on the unmodified one;caching-should-not-make-ambiguous.jsanddynamic-import-loaded-modules-cache.jsstill pass.JSTests/modules/code-cache-key-compares-source.js. On the unfixedjscit fails withError: bad value: 10001215on thesecondassertion and passes with--useCodeCache=false. The two module files must stay byte-for-byte as committed; the test file says how to find a replacement pair if that ever changes.Background
CodeCacheis the per-VM cache of unlinked code blocks for top-level code (modules, programs,eval,new Function). It is a hash map keyed bySourceCodeKey, consulted on every top-level compile, so a false-positive key equality substitutes one source's compiled code for another's.StringImpl::hash()keeps only 24 bits of the string hash (8 bits of the word are flags), so same-length collisions among a few thousand strings are expected, not rare; the hash is only meant to pick a bucket, never to stand in for equality.