Conversation
|
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)
WalkthroughAdds ChangesCSS Invalid UTF-8 Handling
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 6:26 AM PT - Jun 29th, 2026
❌ @robobun, your commit 9789336 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 32795That installs a local version of the PR into your bun-32795 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any issues — the fix is well-scoped and thoroughly tested — but it touches the bundler's core parse path and changes BabyString::in's panic semantics, so it's worth a human look.
Extended reasoning...
Overview
This PR fixes a panic: unreachable when bundling CSS containing invalid UTF-8 bytes. It adds bun_css::replace_invalid_utf8 (SIMD-validated fast path, arena-allocated U+FFFD substitution on the cold path) and calls it at the two CSS entry points (ParseTask::parse_worker and Transpiler::parse). It also hardens BabyString::in to return an empty span instead of panicking when the needle isn't found in the formatted parent message. Four new tests cover CLI, Bun.build, at-rule url(), and non-import invalid bytes.
Security risks
None identified. This is input sanitization that brings CSS in line with the JS/HTML loaders, which already replace invalid UTF-8. The change reduces attack surface by eliminating a panic on malformed input rather than introducing one.
Level of scrutiny
Moderate-to-high. The logic itself is small and the common-case path is zero-cost (is_valid_utf8 returns the original slice unchanged), but the call sites are in the bundler's per-file parse pipeline (ParseTask.rs ~line 2300, transpiler.rs), which is production-critical and runs on every CSS file. The BabyString::in change also affects error reporting for all loaders, not just CSS — turning an expect("unreachable") into a silent empty-span fallback is a deliberate trade-off (don't abort a build while reporting an error) that a maintainer should ratify.
Other factors
- The PR description is unusually thorough, with stack trace, root-cause analysis, and explicit reasoning for each change.
- The sanitization is correctly placed before
sourceis built in ParseTask, so token positions / error line text / source maps all index the same buffer — this is the subtle part and it looks right. bump(the per-task arena) is in scope at the ParseTask call site and outlives the parse, so the arena allocation in the cold path is sound.- No CODEOWNERS match the touched files.
- The bug-hunting system found no issues.
Deferring because the bundler parse path and a cross-loader error-reporting helper are core enough that a human familiar with the arena/lifetime story should confirm.
|
CI status for the current head (9789336, build 66729): 282 jobs passed, the Windows lanes are green, and every lane that runs the new tests (including the ASAN shards) passes. The red lanes are unrelated to this diff:
The diff is limited to the CSS loader path, |
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
It needs to use the arena lifetime.
|
Fixed in 2d63fb9: the replacement buffer is now built in the parse arena ( |
|
The missing decode step has a second, uglier symptom than the panic: silent output corruption through the escape decoder. This PR already fixes it. Writing it up here so the case gets pinned by a test. The escape decoder eats source bytes after an ill-formed sequence
On current main, all three build with exit 0 and no diagnostics: printf '.a { content: "\\\xc3x AFTER"; }' > in.css && bun build in.css
# .a { content: "\uFFFDAFTER"; } the `x ` after the escape is gone
printf '.a { content: "\\\xc3"; color: green; }' > in.css && bun build in.css
# .a { content: "\uFFFD color: green; }"; } the closing quote was eaten, so the
# rest of the rule is now string text
printf '.x\\\xc3yz { color: red; }' > in.css && bun build in.css
# .x\uFFFD { color: red; } the class name lost its `yz`The last two change which rules exist and which selectors they target, with no warning. Why this PR covers it
I also looked at whether the tokenizer needs its own fix on top of this (advance by the source length rather than by Suggested testThe escape case is the one shape where the tokenizer decodes the bad bytes instead of slicing past them, and the only one that corrupts structure rather than content, so it is worth a case in test.concurrent("escaped invalid byte does not swallow the bytes after it", async () => {
using dir = tempDir("css-invalid-utf8-escape", {});
// `\` followed by a lone 0xC3 lead byte. The escape must decode to U+FFFD and
// consume exactly that byte; it used to consume three (the UTF-8 length of
// U+FFFD), eating ident characters, string content, or the closing quote.
writeFileSync(
join(String(dir), "in.css"),
Buffer.concat([
Buffer.from(".x\\"),
Buffer.from([0xc3]),
Buffer.from('yz { content: "\\'),
Buffer.from([0xc3]),
Buffer.from('x AFTER"; }\n.a { content: "\\'),
Buffer.from([0xc3]),
Buffer.from('"; color: green; }\n'),
]),
);
await using proc = Bun.spawn({
cmd: [bunExe(), "build", "./in.css", "--outdir=out"],
env: bunEnv,
cwd: String(dir),
stdout: "pipe",
stderr: "pipe",
});
const [stderr, exitCode] = await Promise.all([proc.stderr.text(), proc.exited]);
expect(stderr).not.toContain("error:");
expect(exitCode).toBe(0);
const out = await Bun.file(join(String(dir), "out", "in.css")).text();
// Selector: `yz` used to be eaten out of the class name.
expect(out).toContain(".x\uFFFDyz");
// String content: the `x ` after the escape used to be eaten.
expect(out).toContain('content: "\uFFFDx AFTER"');
// The closing quote used to be eaten, absorbing `; color: green; }` into the string.
expect(out).toContain('content: "\uFFFD";');
expect(out).toContain("color: green;");
});On main all four |
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
Use the existing code to do this string with replacement. Do not roll your own.
|
Done in 2f5e509. The replacement now goes through the existing converters: |
8547dc7 to
9f14380
Compare
The CSS tokenizer hands out raw sub-slices of the source as token values and assumes byte positions are char boundaries, but the source bytes were never validated. An @import specifier containing an invalid byte reached the bundler's resolve error formatter, which expects to find the raw specifier inside the lossily formatted message text and panicked (unreachable) when it was not there. Decode CSS sources before parsing, replacing invalid sequences with U+FFFD the way a browser decodes a stylesheet, and make the error formatter degrade to an empty specifier instead of panicking.
Build the replacement buffer with ArenaVec::with_capacity_in and return it with leak(), instead of filling a heap Vec and copying it into the arena afterwards.
Use strings::to_utf16_alloc (which substitutes U+FFFD for invalid sequences when fail_if_invalid is false, the same call TextDecoder and Blob.text use) followed by strings::to_utf8_alloc, instead of a local utf8_chunks loop.
strings::replace_invalid_utf8 takes the arena and returns an arena-lifetime slice: the input when it is already valid, else an arena copy with each ill-formed sequence replaced by U+FFFD. The CSS parse sites call it directly. Add an escape-decoder case to the test: an escaped ill-formed byte used to advance the tokenizer by the encoded width of U+FFFD, dropping the two source bytes after it.
7d43702 to
038075c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/bun_core/string/immutable.rs`:
- Around line 1566-1572: The allocation in the invalid UTF-8 repair path only
reserves space for one replacement marker, so repeated invalid chunks in the
utf8_chunks loop can force reallocations; update the sizing logic in the string
repair routine to compute the full repaired length up front and pass that exact
capacity to ArenaVec::with_capacity_in. Use the existing invalid-chunk scan in
the code path around bytes.utf8_chunks() and UNICODE_REPLACEMENT_STR to
determine the total number of replacements before building out.
In `@test/bundler/css/invalid-utf8.test.ts`:
- Around line 109-115: In the success-path assertions in invalid-utf8.test.ts,
move the proc.exited/exitCode check to the end so the emitted CSS/file content
assertions run first and surface the more specific diff on failure. Update the
affected test blocks around the stdout/stderr checks and Bun.file(out/in.css)
validation, and apply the same ordering to the other matching case noted in the
review so exitCode is always asserted last.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ed721635-92f7-4dec-aed9-d10d69760b53
📒 Files selected for processing (6)
src/ast/lib.rssrc/bun_core/lib.rssrc/bun_core/string/immutable.rssrc/bundler/ParseTask.rssrc/bundler/transpiler.rstest/bundler/css/invalid-utf8.test.ts
|
Two more reproducers for the missing decode step came out of fuzzing raw bytes into the CSS path. They hit a different symptom than the resolve-error panic this PR started from: on an asserts build, a UTF-8 continuation byte at a token start (lead bytes like the 0xE2/0xE9 used in this PR's tests happen to be tolerated by the tokenizer) aborts on the char-boundary assertions in printf '.a { color: red \xAF}\n' > bad1.css && bun build ./bad1.css
printf '@property --x { syntax: "<length>"; inherits: false; initial-value:\xBD1px; }\n' > bad2.css && bun build ./bad2.cssI verified both against this branch at 9789336: the first now builds with U+FFFD in the output and the second reports the normal
|
|
One more way to reach the char-boundary assertion this PR fixes, in case it is useful when this gets rebased: The fuzz inputs themselves are valid UTF-8. What happens is that the fuzz tests run past their timeouts and keep rewriting the shared printf '.test{color:red;;;}\xbf\xbd"}' > x.css && bun build x.css
# panic: assertion failed: strings::is_on_char_boundary(self.src, self.position)
# Tokenizer::get_position (css_parser.rs:4154) <- Parser::state <- StyleSheetParser::nextSame shape as the |
|
Superseded by #41801, which ports this change onto current main. This branch is about 1960 commits behind and no longer merges: |
2 participants
What
bun build(CLI andBun.build) crashes withpanic: unreachableon a CSS file containing a single invalid UTF-8 byte inside an@import:Same crash from
Bun.build({ entrypoints: ["x.css"], throw: false }), from a JS or HTML entry that imports the CSS file, and from@-x url(a\xE2b) tok;(aurl()token in an at-rule prelude). A file like this comes from a truncated or wrong-encoding asset; the build should report an error, not abort.Top of the stack:
Why
The CSS tokenizer takes
&[u8]and hands out raw sub-slices of the source as token values (upstream rust-cssparser takes&str, so it never sees ill-formed input). The raw specifier bytes land in anImportRecord, resolution fails, and the resolve error formatter renders the message withbstr::BStr's lossyDisplay(invalid bytes become U+FFFD) whileBabyString::insearches for the original raw bytes inside that lossy text andexpect()s the hit.The JS lexer, the HTML loader, and the runtime resolver already sanitize their specifiers to U+FFFD. CSS was the only producer of raw bytes.
Fix
bun_core::strings::replace_invalid_utf8(bytes, arena): SIMD-validate the source (simdutf) and, only when it is invalid, copy it into the parse arena with each ill-formed sequence replaced by U+FFFD, returning the arena-lifetime slice. This is the decode step from css-syntax 3.2 and matches what a browser does with the same stylesheet.ParseTask(before theSourceis built, sosource.contents, token positions, error line text, and source maps all index the same buffer) and the non-bundling transpile path.BabyString::indegrades to an empty specifier instead of panicking when the needle is not found. After the loader fix every current producer is valid UTF-8, so this is hardening of the error path rather than the fix itself; a miss there should never abort a build.Tests
test/bundler/css/invalid-utf8.test.ts, five cases: the CLI repro, theBun.buildAPI form, the at-ruleurl()form, an invalid byte outside any import (asserting the emitted CSS is well-formed UTF-8, an input that also used to crash), and an escaped invalid byte (the escape decoder used to advance by the encoded width of U+FFFD and silently drop the two source bytes after it, changing selectors and eating closing quotes). All five fail on the current build and pass with the fix.Not changed here:
ResolveMessage's JS getters decode non-ASCII message text as Latin-1 (log.specifieris"./café.js"forimport "./café.js"today, no invalid UTF-8 involved). That is a separate pre-existing issue, so the API test asserts the ASCII affixes of the specifier.