Repository navigation
Conversation
mapping::parse refuses a map when a segment names a source index that is not below the count its caller passes. Two of its four callers passed a wrong count. parse_json passed i32::MAX, which turned the check off. A mapping that names a source past `sources` stayed in the map, and its stack frame got the built file name with the original line and column. It now passes sources.len(). Such a map is refused with InvalidSourceIndexValue, as a map with a negative source index is. The dev server passed paths.len(). Source 0 of its map is the HMR runtime and file i is source i + 1, so the decoder refused the map when the last file of a bundle had mappings, and /_bun/report_error remapped no frame. It now passes paths.len() + 1. The range guard in Lookup::get_source_code cannot fire with the bound in place, so it is deleted.
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes.
 |
| Layer / File(s) | Summary |
|---|---|
Source-index parsing and validation src/sourcemap/Mapping.rs, src/sourcemap/lib.rs, test/js/bun/sourcemap/negative-vlq-mapping.test.ts |
mapping::parse receives the source-path count. Lookup no longer checks the source index before indexing external_source_names. Tests cover valid and invalid source indexes in scalar and SIMD decoding, plus external-sourcemap bundle output. |
HMR source counts and reported frames src/runtime/bake/dev_server/source_map_store.rs, test/bake/dev/html.test.ts |
The bake source-map parser includes the HMR runtime in its source count. Tests report client-bundle and HMR-chunk frames and check returned file positions. |
Suggested reviewers: alii, jarred-sumner
Merge Risk: 
6c7b5
-
No actionable merge-blocking issue remains. Source-index validation and HMR source counts are consistent with their consumers; the change is mergeable subject to normal checks.
Pre-merge checks: 
-
Check name Status Explanation Title check The title clearly and concisely describes the main change: passing the real source count to the mappings decoder. Description check The description is detailed and covers the change, verification steps, test results, tradeoffs, and follow-up work. It does not use the exact template headings, but it provides the required informatio… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Description check
-
Explanation
The description is detailed and covers the change, verification steps, test results, tradeoffs, and follow-up work. It does not use the exact template headings, but it provides the required information under equivalent sections.
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
|
Reproduced on a release build of main (11c3671).
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I checked the deleted range guard in Lookup::get_source_code: external_source_names is only ever filled by parse_json (same length as the sources_count it now passes) and by the standalone graph (which always takes the branch that keeps its own bound check), the sole caller is gated on is_external(), both decoders refuse the whole parse on an out-of-range index (the Highway kernel bails before committing the row), so the indexing cannot go out of bounds. The dev-server paths.len() + 1 is exact: paths/files are pushed in lockstep and the reader already uses file_paths[index - 1] behind its own bound check.
Extended reasoning...
The change is three one-line edits in the sourcemap decoder's callers plus one deleted guard, with two new test files covering both decoders, at-limit/one-past boundaries, inline and external maps, and the dev-server error-report endpoint. It touches no auth, crypto, or injection surface; the untrusted input is a sourcemap's VLQ string, and the new bound tightens validation. Deferring rather than approving because the whole-map refusal (versus per-frame un-mapping as node does) is a user-visible behavior tradeoff the inline finding records, and a maintainer should weigh it.
| mappings_vlq, | ||
| None, | ||
| i32::MAX, | ||
| i32::try_from(sources_paths.items().len()).unwrap_or(i32::MAX), |
There was a problem hiding this comment.
🟡 (optional) Users running a prebuilt // @ bun file whose map has one segment past sources now lose remapping for every frame in that file, with a warn: line, where main remapped the in-range frames. src/sourcemap/lib.rs:954 passes sources.len() to mapping::parse, and Mapping.rs:582 refuses the whole map on the first out-of-range index, so get_source_map_impl returns None at lib.rs:552 and SavedSourceMap.rs:329 drops the entry for good. Node leaves only the bad frame unmapped. Fix: match node per frame: let parse keep the map and have Lookup::display_source_url_if_needed/get_source_code return None for an out-of-range source_index (the guard this PR deletes at Mapping.rs:386), or at least keep accepting sources: [] maps.
Why this was flagged
Trigger: any JSON map reachable through parse_json (lib.rs:447 inline, :468 dev-server provider, :493 registered, :538 <file>.map on disk) in which at least one 4- or 5-field segment names an index >= sources.length, or whose sources is [] while mappings carries 4-field segments. On the base commit parse_json passed i32::MAX (lib.rs:954 old), the map parsed, in-range frames were remapped to their original file and only the bad frame printed a mixed position. After this change Mapping.rs:582 returns InvalidSourceIndexValue for the first bad segment, lib.rs:545-552 warns Could not decode sourcemap in '<file>': InvalidSourceIndexValue and returns None, and SavedSourceMap.rs:329-338 removes the entry so the file is never retried: every stack trace, Bun.inspect(error) preview, and bun test --coverage lookup for that file stays at built positions for the life of the process. A per-frame guard (the one deleted at Mapping.rs:386, plus the kept one at Mapping.rs:321) would reject only the bad frame as node does.
Verification: src/sourcemap/lib.rs:954 now passes i32::try_from(sources_paths.items().len()).unwrap_or(i32::MAX) where the base passed i32::MAX. src/sourcemap/Mapping.rs:582-589 returns Err(ParseFail { err: InvalidSourceIndexValue }) on the first out-of-range index, and src/jsc/SavedSourceMap.rs:327-333 removes the table entry, so no frame in that file is remapped for the rest of the process.
There was a problem hiding this comment.
Not changed. The guard you suggest is what main does today, and it is the cause of the mixed frame. display_source_url_if_needed returns None for an index past the names (Mapping.rs:321). The three remap sites in VirtualMachine.rs then keep the built URL and still copy the original line and column. That prints entry.js:5:3.
A verdict for each frame needs more than that guard. Every reader of a lookup has to treat the mapping as absent: the three remap sites, coverage, and the content pick in parse_json. Or the decoder has to mark the row. Both add a branch to each lookup, for every map.
The whole-map refusal is what this decoder already does for a negative source index, a negative line or column, and a bad VLQ. new module.SourceMap passes its real count to the same decoder and throws.
Reach: 0 of 542 bun-built maps from npm packages have such a row, and 0 of 192 maps from bun build. 2 of 8,111 .map files in the checkout have one. Both are invalid-map fixtures of the tc39 suite.
For sources: [] with 4-field mappings, every frame of that file is a mixed frame on main. Node leaves those frames unmapped. With this PR they are unmapped too, with one warning.
The cost is the first Downsides bullet: the in-range frames of such a map are no longer remapped. I leave this thread open so that a maintainer can decide.
Problem
mapping::parsedecodesmappings. It refuses a map whose source index reaches its caller's count. For the map of a built file,parse_jsonpassesi32::MAX(src/sourcemap/lib.rs:954).sourcesprints the built file name with the original line:entry.js:5:3.src/runtime/bake/dev_server/source_map_store.rs:677). Since Fix source index bounds check in sourcemap decoder #24145,/_bun/report_errorremaps nothing when a bundle's last file has mappings.Fix
parse_jsonpassessources.len(). Such a map giveswarn: Could not decode sourcemap in '<file>': InvalidSourceIndexValue, as a negative index does.paths.len() + 1. This revives fix(bake): correct source count in sourcemap parsing for error remapping #25240, closed by a stale sweep.Lookup::get_source_codecannot fire now. It is deleted.test/js/bun/sourcemap/negative-vlq-mapping.test.ts,test/bake/dev/html.test.ts(5 new tests, main fails 4). Self-reviewed: 19 concerns raised, 10 addressed, 9 belong to sourcemap: accept a source map whosesourcesContentis absent or null #36582.Background
// @bun. Bun reads its map on the first stack trace.sourcesContentis absent or null #36582 accepts more maps. Land this before or with it.Downsides
sourcesnow un-maps its whole file, with one warning. Main and node remap the rest. 0 of 542 bun-built maps from npm have one.parse_jsongrows 51 bytes,get_source_code23 (crate codegen, no ThinLTO). No linked release binary was measured.Notes
Repro for the source index.
entry.js:entry.js.map(one source, and a mapping to source index 1):{"version":3,"sources":["original.ts"],"sourcesContent":[null],"names":[],"mappings":";;ECIE;"}at t (entry.js:5:3), no warning. Line 5 and column 3 are the original position,entry.jsis the built file.at t (entry.js:3:18)andwarn: Could not decode sourcemap in 'entry.js': InvalidSourceIndexValue.--enable-source-maps:at t (entry.js:3:9).A map with
"sources":[]and a 4-field mapping prints the same mixed frame on main. Here it is refused too.Why the whole map. The decoder already refuses the whole map for a negative source index, a negative line or column, and a bad VLQ.
new module.SourceMapandbun pm diffpass their real count to the same decoder.parse_jsonwas the one caller that turned the check off. Node decides for each frame: it leaves a frame with an unknown source unmapped and remaps the others. In a probe of 10 maps with 2 frames each, this PR leaves 1 frame at its built position that main remaps (the in-range frame of the map above, in a second function).Probe of 10 maps, release build of main (11c3671) against this PR.
sourcesContentorsources, and this PR does not change that)The dev server. Its map has the HMR runtime as source 0 and file
iof the bundle as sourcei + 1(join_vlq,source_map_store.rs:278).get_parsed_source_mappassedpaths.len(). #24145 changed the decoder's check from>to>=. From then on the index of the last file was out of range and the parse failed. The reader of the result already expectsfile_paths[index - 1](error_report_request.rs:242).hmr-<id>.js): main leaves it, this PR remaps it./is made relative. The test compares base names. It was not run on Windows here.Maps that the new bound refuses. Decoded with
new module.SourceMap, which uses the same decoder withsources.length. A map counts only if it decodes oncesourcesis padded, so a negative index (refused on main already) is not counted.bun build: 102 packages oftest/node_modulesbundled with--target=bun, external and inline, up to 2,113 sources in one map..mapfiles in the checkout. Both are invalid-map fixtures of the tc39 suite (invalid-mapping-segment-source-index-out-of-bounds,invalid-mapping-bad-separator).Cost.
parse_json/mapping::parse/mapping::List::find, gdb hit counts on the debug build of this PR): load only 0/0/0, one caught stack 1/1/2, 1000 caught stacks 1/1/2000, an uncaught error 2/1/2, 1000 stacks in a transpiled.ts0/0/0. A release build of main gives the same counts for the first two functions (List::findis inlined there). The diff adds no call, no allocation and no file read.bun_sourcemapcrate (opt-level 3, machine code from the crate's bitcode withclang -O3 -march=nehalem, no ThinLTO):parse_json2413 to 2464 bytes and 498 to 504 instructions.Lookup::get_source_code789 to 812 bytes: the deleted guard was also what let the compiler drop the bounds check of the index.mapping::parse(5393 bytes) andList::find(412) do not change. The crate's.textgoes from 53,569 to 53,633 bytes.bun_runtime(the dev server change is one+ 1for each error report). A cold debug build takes about 2 hours on the test machine.perf,valgrind,straceandbloatyare not installed there.Suites on the debug build of this PR.
test/js/bun/sourcemap/(370),test/js/node/module/sourcemap.test.js(14),test/js/node/module/sourcemap-simd.test.ts(24),test/js/bun/util/inspect-error.test.js(46),test/cli/test/coverage.test.ts(43),test/regression/issue/22003.test.ts(1),test/bundler/bundler_edgecase.test.ts -t EmitInvalidSourceMap(4), and intest/bake/dev:html(17),sourcemap(2),server-sourcemap(5),production(21),esm(18),bundle(54),hot(12),css(18).Overlap.
sourcesContentis absent or null #36582 makessourcesContentoptional inparse_json. Every map it starts to accept reaches this check, so a mixed frame becomes possible for more maps until this bound is in.onLoadreturned #46148 and Apply input //# sourceMappingURL to stack traces #36625 edit other lines ofparse_jsonand keepi32::MAX.entry.source_slot_count() + 1inside a larger change. fix(bake): correct source count in sourcemap parsing for error remapping #25240 made the same one-line fix in the Zig source and had no test.Left for later.
//# sourceMappingURL=line anywhere in a built file is read as its inline map, also when it is text inside a string. See sourcemap: ignore a sourceMappingURL line inside a string or a block comment #47347.new module.SourceMapthrows for a source index pastsources. Node returns an entry withoutoriginalSource.InternalSourceMap::from_vlqtakes no source count. It reads only maps that the bundler made.