Repository navigation
Don't panic generating a sourcemap for a source ending in a truncated UTF-8 sequence - #32774
Conversation
…ence LineOffsetTable::generate_in computed each codepoint's width from the lead byte's declared length. The decode already clamped that width to the bytes remaining, but the advance and the SIMD-skip offset did not, so a source whose final bytes are a truncated multi-byte sequence panicked: panic: range start index 4 out of range for slice of length 1 Compute the clamped width once and use it for the decode slice, the skip offset, and the advance, matching the clamped_width idiom already used by the string escapers.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughSource-map generation now limits UTF-8 multibyte decoding to the remaining input bytes, and a new roundtrip test verifies that truncated or invalid trailing UTF-8 sequences still produce the same sourcemap output. ChangesUTF-8 sourcemap truncation
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — minimal, well-scoped clamp fix matching the idiom used everywhere else, with regression tests for both panic sites.
Extended reasoning...
Overview
This PR fixes a panic in LineOffsetTable::generate_in (src/sourcemap/LineOffsetTable.rs) when a source file ends in a truncated multi-byte UTF-8 sequence. The fix hoists the already-present .min(remaining.len()) clamp — previously applied only to the decode slice — so that the same clamped width cp_len is used for all three consumers: the decode slice, the SIMD-skip offset passed to index_of_newline_or_non_ascii_check_start, and the loop advance remaining = &remaining[cp_len..]. A new test.each block in internal-sourcemap-roundtrip.test.ts covers six truncated-tail variants that exercise both panic sites and asserts the exact emitted mappings.
Security risks
None. This converts a slice-index panic on malformed input into graceful handling. No unsafe is touched, no new parsing surface is exposed, and for any well-formed input cp_len == len_ so behavior is byte-for-byte identical. The clamp can only differ from the declared width within the final len_ - 1 bytes of the source.
Level of scrutiny
Low-to-moderate. The Rust diff is effectively a 3-line refactor: move the min(remaining.len()) computation up and reuse the result. I verified that index_of_newline_or_non_ascii_check_start does &slice_[offset as usize..] internally (so the unclamped offset was a real second panic site), that decode_wtf8_rune_t still receives the declared len_ (correct — the decoder needs the lead-byte width; truncation is handled by the zero-padded buffer, same as before), and that the len_ == 1 ASCII fast path is unaffected since the loop guard guarantees remaining.len() >= 1. The PR description accurately notes that every other caller of wtf8_byte_sequence_length_with_invalid already uses this clamped-width idiom.
Other factors
No CODEOWNERS apply to these paths. No prior human review comments are outstanding. The bug-hunting system found nothing. The added tests are deterministic, spawn child processes (so a regression would fail the test rather than crash the runner), and pin the exact mappings output to catch any unintended sourcemap drift.
There was a problem hiding this comment.
LGTM — minimal, correct clamp of the declared UTF-8 width to bytes-remaining, matching the idiom already used elsewhere in the tree, with thorough regression tests.
Extended reasoning...
Overview
This PR fixes a panic in LineOffsetTable::generate_in (src/sourcemap/LineOffsetTable.rs) when a source file ends in a truncated multi-byte UTF-8 sequence. The fix hoists the existing .min(remaining.len()) clamp — which was already applied to the decode slice — up to the definition of cp_len, so the same clamped width is used for the decode, the SIMD-skip offset passed to index_of_newline_or_non_ascii_check_start, and the loop advance remaining = &remaining[cp_len..]. Net code change is ~6 lines in one function plus a comment. A new test.each block in test/js/bun/sourcemap/internal-sourcemap-roundtrip.test.ts covers six truncated-tail variants spanning both former panic sites and asserts exit code 0 plus exact sources/mappings output.
Security risks
None. This strictly tightens a slice bound: previously an out-of-bounds slice index produced a Rust panic (safe abort, no UB); now it advances by the bytes actually available. There is no unsafe, no new I/O, and no change to what data is read or emitted for well-formed input.
Level of scrutiny
Low. The diff is tiny and mechanical: it moves an existing .min(remaining.len()) two lines earlier and reuses the result. For any non-truncated input cp_len == len_ so behavior is byte-identical; the two only diverge within the final len_ - 1 bytes of the file. The loop guard while !remaining.is_empty() guarantees cp_len >= 1, so the advance still makes progress. The decode call still passes the unclamped len_ to decode_wtf8_rune_t over a zero-padded 4-byte buffer, which is unchanged from before.
Other factors
The PR description is unusually thorough — it identifies both panic sites, explains why Chunk::update_generated_line_and_column_slow (the only other unclamped caller) is unreachable for this input, and notes that every other caller of wtf8_byte_sequence_length_with_invalid already uses this clamp idiom. The new tests are end-to-end (spawn bun build --sourcemap=external) and assert concrete output, not just absence-of-crash. The bug-hunting system found no issues, and there are no outstanding human review comments. No CODEOWNERS entry covers this path.
|
CI status: the diff is green. Every failure on this PR's two builds is unrelated infrastructure or an external breakage. First run (65054): 4 failures, all infrastructure. An npm registry mid-publish on react's experimental dist-tags broke the Retriggered run (65072): 2 failures.
This PR's own test (all six variants in |
|
Heads up: the expect(received).toMatchObject(expected)
"sources": [
- "../in.js",
+ "..\in.js",It fails on all retries, so it isn't flake. First hit on windows-2019-x64 in build 65191, a branch with main merged in. Since 990be52 is on main and nothing has changed this file since, main's Windows lanes and every PR's Windows lanes will fail the same way until it's addressed. I'm not touching it from my PR (#32733, glob, unrelated; I only hit this by merging main) because the right fix needs a call that belongs here: normalize |
Fixes #14769 ### Repro `bun build --sourcemap` writes the host path separator into the `sources` entries of the emitted `.js.map`, so on Windows a source one directory above the out dir comes back as `..\in.js` instead of `../in.js`. Source map `sources` are URLs resolved against the map's location, where `\` is not a path separator; esbuild, which this code is ported from, explicitly converts to forward slashes here ("Make sure to always use forward slashes, even on Windows"). This is the bug reported in #14769: `bun build src/main.mjs --sourcemap=linked --outdir dist` on Windows produces a map whose `sources` all carry a `..\src\` prefix, and Chrome devtools shows the raw `..\src\main.mjs:5` in the console instead of resolving the frame back to `main.mjs:5`. It is also what turned the Windows test lanes red. #32774 added the first test in the suite to pin an exact `sources` value, and it fails deterministically on all three Windows `test-bun` lanes (2019 x64, 2019 x64-baseline, 11 aarch64). From build 65191: ``` error: expect(received).toMatchObject(expected) { "sources": [ - "../in.js", + "..\in.js", ], } at internal-sourcemap-roundtrip.test.ts:443:17 ``` All six `sourcemap of a source with a truncated trailing UTF-8 sequence` variants fail the same way, on every retry. Main has not completed a Windows build since #32774 merged (each one since was superseded before the test lanes finished), so its next completed build, and every PR that merges main, hits this too. ### Cause `LinkerContext::generate_source_map_for_chunk` computes each file source's relative path with `bun_paths::resolve_path::relative_alloc`, which uses the host separator (`platform::Auto`), and writes the result straight into the `sources` array. Everywhere else an output-facing path is built, the tree already enforces forward slashes: `Path::pretty` carries a Windows debug assertion that it contains no backslashes (`assert_pretty_is_valid`), `dupe_alloc_fix_pretty` upholds it with `platform_to_posix_in_place`, and the dev server's source map writer plus the standalone module graph both normalize before emitting. The bundler's chunk source map writer is the one place that overrides `pretty` with a raw native relative path and skips the step. The non-file branch (`path.pretty`, virtual and plugin sources) already holds the invariant and is untouched. ### Fix A new `source_map_relative_path` helper computes the relative path and calls `bun_paths::resolve_path::platform_to_posix_in_place` on it; both `sources` call sites (the first entry and the `source_indices[1..]` loop) go through it. The normalization is a compile-time no-op when the host separator is `/`, so POSIX output is byte-identical; on Windows the only change is `\` to `/` inside file-namespace `sources` entries. ### Verification `test/js/bun/sourcemap/internal-sourcemap-roundtrip.test.ts` gains a test that builds an entry in a nested directory with one import, so the two `sources` entries each cross multiple separators and between them cover both call sites, and pins them exactly: ```ts expect(map.sources).toEqual(["../src/dep.js", "../src/nested/in.js"]); ``` Because the fixed function is a no-op on POSIX by construction, the failure is only observable on a Windows host. The three red Windows lanes in build 65191 above are the fail-before; the `sources: ["../in.js"]` assertions from #32774 and the new test are the regression coverage. The full roundtrip file, `bun-build-compile-sourcemap.test.ts`, `compile-sourcemap-internal.test.ts`, and the `snapshotSourceMap` bundler edge cases all pass with the change. The `expectBundled` harness masks exactly this (`parsed.sources.map(a => a.replaceAll("\\", "/"))` at `expectBundled.ts:1617`), which is why no `test/bundler/` source map test ever caught it. Left in place since it also covers non-file `pretty` sources, which this change does not touch.
2 participants
Repro
A source file whose last bytes are a truncated multi-byte UTF-8 sequence (a lead byte that declares more bytes than the file has left) panics every sourcemap-printing path:
bun build --sourcemap=inline|external, plainbun <file>(the runtime transpiler builds the same line table for stack-trace remapping), and--hotrebuilds.at
LineOffsetTable::generate, viaChunk::add_source_mapping/js_printer::print_stmt.bun buildwithout--sourcemaphandles the same file fine, since the lexer already treats a truncated trailing sequence as EOF. Only the sourcemap line table is unguarded. Files ending mid-sequence show up in practice: truncated copies or downloads, an editor or CI job dying mid-write, or a file being rewritten while a build is in flight.Other truncated tails hit the same bug:
\xe2,\xc3,\xc1,\xe0\x81,\xf0\x9f\x92.Cause
LineOffsetTable::generate_in(src/sourcemap/LineOffsetTable.rs) walks the source one codepoint at a time. For each lead byte it gets the declared sequence width fromwtf8_byte_sequence_length_with_invalid. The codepoint decode already clamps that width to the bytes remaining (.min(remaining.len())), but two other uses of it do not:remaining = &remaining[cp_len..], so a lone0xF0at EOF indexes[4..]of a 1-byte slice (this is the reported panic)index_of_newline_or_non_ascii_check_start, which does&slice[offset..]internally; this is the panic site for overlong truncated leads such as0xC1and0xE0 0x81, whose zero-padded decode lands in the ASCII fast pathEvery other caller of
wtf8_byte_sequence_length_with_invalidin the tree already clamps both the decode and the advance: the lexer steppers and URL percent-encoder inbun_core::strings, the string escapers injs_printer, and the markdown ANSI renderer. (Chunk::update_generated_line_and_column_slowhas a superficially similar unclamped advance, but it walks the printer output, which only grows by whole codepoints; the lexer strips a truncated trailing sequence from the source before anything reaches the printer, so it is not reachable and is left unchanged.)Fix
Compute the clamped width once,
cp_len = (len_ as usize).min(remaining.len()), and use it for all three: the decode slice, the skip offset, and the advance. This is the sameclamped_widthidiom the string escapers injs_printerandbun_core::stringalready use.For any non-truncated input
cp_len == len_, so behavior is unchanged; the two can only differ withinlen_ - 1bytes of the end of the source.Verification
test/js/bun/sourcemap/internal-sourcemap-roundtrip.test.tsgains atest.eachover six truncated tails (0xF0,0xE2,0xC3,0xC1,0xE0 0x81,0xF0 0x9F 0x92), chosen so the set covers both panic sites. Each spawnsbun build --sourcemap=externalon the malformed source and asserts exit code 0 plus the exact emittedsourcesandmappings. The mappings are identical across every variant because theconsole.log(1);payload is byte-identical and the truncated bytes live in a stripped comment.All six fail on an unpatched build with the panic above; all pass with the fix, and the rest of the file still passes.