worker: do not abort a compiled executable on a relative specifier longer than the path buffer - #38422
worker: do not abort a compiled executable on a relative specifier longer than the path buffer#38422robobun wants to merge 1 commit into
Conversation
…nger than the path buffer In a bun build --compile executable, resolve_entry_point_specifier maps ./foo and ../foo Worker specifiers (entry point and preloads) onto the embedded /$bunfs/root/ tree by joining them into a pooled PathBuffer and then appending .js in place. Neither write was bounds checked, so a specifier longer than the buffer, or one whose joined form ends within three bytes of it, panicked with a slice range error and took the whole process down. Use join_abs_string_buf_checked with three bytes held back for the .js suffix, and skip the remap when the specifier does not fit. The resolver below then reports it the same way it reports any other missing module, so the worker fires its error event (or the constructor throws, for a preload) as it does outside of a compiled executable.
|
Warning Review limit reached
Next review available in: 30 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: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Status: fix and tests pushed, waiting on CI. Review so far has no findings to address. Reproduced with bun 1.4.0 on Linux x64: a |
There was a problem hiding this comment.
LGTM — swaps the unchecked join for join_abs_string_buf_checked into a buffer shortened by ".js".len(), so both the join and the later append are bounded; overflow falls through to the resolver exactly as a graph miss already does.
What was reviewed:
- Confirmed
join_abs_string_buf_checked(src/paths/resolve_path.rs:1640) returnsNonewhen the normalized result exceeds the passed slice, and otherwise produces the same bytes as the unchecked variant, so specifiers that fit take the identical path. - Checked that reserving 3 bytes cannot regress a real embedded-module lookup: the only paths it excludes are 1-3 bytes short of
MAX_PATH_BYTES, and those still fall through to the resolver rather than error differently. - Both call sites of
resolve_entry_point_specifier(worker entry inspin, preloads increate) are covered by the two newitBundledtests, which also exercise the.js-append window per platform buffer size.
Extended reasoning...
Overview
The PR changes one block in src/jsc/web_worker.rs::resolve_entry_point_specifier: the standalone-module-graph remap that joins a ./ or ../ worker specifier onto /$bunfs/root/ before looking it up in the embedded graph. It replaces join_abs_string_buf (writes straight into a fixed PathBuffer, panics on overflow) with join_abs_string_buf_checked into pathbuf[..len - 3], and break 'try_from_extension on None. That is the entire native change. Two itBundled tests are added in test/bundler/bundler_compile.test.ts next to the existing worker-relative-path compile tests.
Security risks
None introduced. The change converts a user-input-triggered process abort (slice-index panic on an over-long specifier) into the same recoverable error event / thrown constructor error that plain bun already produces. No new data is trusted; the input space handled is strictly larger, and the fallthrough path is the existing resolver.
Level of scrutiny
Low-to-medium. The diff is nine lines of Rust that substitute one in-tree helper for its bounds-checked sibling — a pattern already used at eight other user-input call sites (Bun.mmap, fs.watch, resolver, lockfile). I read join_abs_string_buf_checked to confirm: fast path defers to the unchecked join when the raw concatenation is under the buffer length; slow path joins into a heap scratch and copies back only if the normalized result fits — so specifiers that fit today produce byte-identical output, and the only new behavior is None where the old code panicked. Reserving ".js".len() up front closes both the join panic and the append-window panic in one guard; the .ts/.tsx arms only shrink, so the reservation is sufficient for every branch.
Other factors
The tests are placed with the sibling compile/WorkerRelativePath* cases, use Buffer.alloc(n, fill) per harness convention, await the actual error event (not a sleep), and cover ./, ../, the per-platform boundary window, and the preload path on the parent thread. The PR description reports USE_SYSTEM_BUN=1 fails both tests with the expected panic and bun bd test passes them. No CODEOWNERS entry covers src/jsc/web_worker.rs. No prior human review comments to address.
|
Updated 5:05 AM PT - Aug 14th, 2026
❌ @robobun, your commit 58210ed has some failures in 🧪 To try this PR locally: bunx bun-pr 38422That installs a local version of the PR into your bun-38422 --bun |
|
Superseded by #40619. That change moved Worker, import() and require() resolution of embedded modules into Verified on main at f189103 with a debug build: The two tests are proposed as regression coverage in #40918. Closing. |
Problem
bun build --compileexecutable,new Worker("./" + "w".repeat(5000))aborts the whole process:panic: range end index 5012 out of range for slice of length 4095. The same script run with plainbunfires the worker'serrorevent and keeps going.../specifiers and thepreloadoption ofWorkertake the same path, so they abort the same way.resolve_entry_point_specifierremaps./footo/$bunfs/root/foo.jsby joining the specifier into a pooledPathBufferwithjoin_abs_string_buf(src/jsc/web_worker.rs:1323on main), which does not check that the normalized result fits, and then writes.jsatpathbuf[base_len..base_len + 3](web_worker.rs:1337), which is not checked either. The specifier is user input of any length..jsappend (range end index 4098 out of range for slice of length 4096).Fix
join_abs_string_buf_checkedintopathbuf[..len - 3], so a successful join always leaves room for the.jsappend, and skip the remap (break 'try_from_extension) when it returnsNone.bunhas today: the worker fireserror(or, for a preload, the constructor throws) and the process continues. Specifiers that fit take exactly the path they took before (join_abs_string_buf_checkedcallsjoin_abs_string_bufwhen the input fits).resolve_entry_point_specifierare covered: the entry point (resolved on the worker thread) and preloads (resolved on the parent thread increate).test/bundler/bundler_compile.test.ts,compile/WorkerRelativePathLongerThanPathBuffer(a compiled binary creates workers for./and../specifiers of 100,000 bytes, plus one./specifier per platform path buffer size (1024, 4096, 98302) whose joined form is one byte short of the buffer, so that on each platform one of them hits the.jsappend window; every one must fireerrorand the binary must exit 0) andcompile/WorkerPreloadLongerThanPathBuffer(a valid embedded worker with a 100,000 byte preload; the constructor must throw and the binary must exit 0).USE_SYSTEM_BUN=1 bun test test/bundler/bundler_compile.test.ts -t LongerThanPathBuffer: both fail withRuntime failed with SIGABRT/panic: range end index 100012 out of range for slice of length 4095.bun bd test test/bundler/bundler_compile.test.ts: 69 of 70 pass; the one failure,compile/HelloWorldWithProcessVersionsBun, fails on any debug build independently of this change and is already covered by test(bundler): fix compile/HelloWorldWithProcessVersionsBun on debug builds #37373.Background
/$bunfs/root/(B:/~BUN/root/on Windows). The bundler renames every embedded module to.js, sonew Worker("./foo")ornew Worker("./foo.ts")has to be remapped to/$bunfs/root/foo.jsbefore it can be looked up in the graph; that remap is what this branch does. When the lookup fails, the specifier falls through to the normal resolver, which resolves it against the working directory.PathBufferis a fixedMAX_PATH_BYTESscratch buffer (4096 bytes on Linux, 1024 on macOS, 98302 on Windows), handed out from a pool.join_abs_string_bufnormalizes the joined path straight into the caller's buffer;join_abs_string_buf_checkedis the variant that returnsNonewhen the normalized result would not fit, and is what the other user-input call sites (Bun.mmap,fs.watch, the resolver's relative path check) already use.Probes against the unfixed binary (bun 1.4.0, Linux x64), one worker per run
main.jscreatesnew Worker(PREFIX + "w".repeat(N))with anonerrorhandler, compiled withbun build --compile. The joined prefix/$bunfs/root/is 13 bytes.With this change, the same binary prints an error event (or the constructor throws, for the preload) and exits 0 for every row.
The same family has one more face that this PR does not touch: a
./specifier this long passed toimport()/require()from inside a compiled executable hits an unchecked join insrc/resolver/resolver.rs(the standalone relative-import branch). That is a separate code path and is being handled separately.