Conversation
… the join buffer PackageJSON::parse joins every "sideEffects" array entry onto the package directory through resolve_path::join, which writes into a fixed 4096-byte thread-local buffer without a bounds check. An entry whose joined path did not fit panicked with "range end index N out of range for slice of length 4095" and took down the process, both at runtime (require/import of the package) and in bun build, since both go through the same parse. Use join_spill, which falls back to a heap buffer when the joined path does not fit. The three branches only hash (exact entries) or copy (glob entries) the joined pattern, so borrowing it from the spill buffer is enough. Long entries keep their normal meaning: a long glob such as a big brace group still matches, and a long exact entry simply never matches.
|
Status: reproduced and fixed, waiting on CI. Reproduced on the 1.4.0 canary (Linux x64 and Windows x64) with a package.json whose |
|
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 (3)
WalkthroughThe resolver now uses a caller-provided spill buffer for long ChangesSide-effects path handling
Suggested reviewers: 🚥 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 — swaps the fixed-buffer join for the existing join_spill helper with a hoisted spill Vec, so oversized sideEffects entries no longer panic.
What was reviewed:
- Confirmed
join_spillinresolve_path.rsfalls back to the thread-local buffer when the result fits and only growsspillotherwise, matching the PR description. - Verified
StringHashMapUnownedKey::initonly stores(hash, len)and the glob branch copies viato_vec()/normalize_path_for_glob, so re-borrowingspilleach iteration is sound. - Checked the six new tests cover all three parse branches (exact/glob/mixed) at both runtime and
bun build, and that the glob cases require the long pattern to actually match rather than merely not crash.
Extended reasoning...
Overview
The PR fixes a panic in src/resolver/package_json.rs when a package.json sideEffects array entry, joined onto the package directory, exceeds the 4096-byte thread-local join buffer. The FileSystemPackageJsonExt::join shim is changed to take a spill Vec<u8> and forward to the existing bun_paths::resolve_path::join_spill, and one Vec is hoisted above the three sideEffects loops (exact, glob, mixed). Six tests are added: three itBundled cases in test/bundler/esbuild/dce.test.ts and three subprocess cases in test/js/bun/resolve/resolve.test.ts.
Security risks
None. This is a crash fix in the resolver's package.json parser. The input (a sideEffects array entry) was already user/package-controlled and already reached this code path; the change replaces a bounds panic with heap allocation. No new trust boundary is crossed, no auth/crypto/permissions code is touched, and the joined pattern is consumed synchronously (hashed or copied) before the buffer is reused.
Level of scrutiny
Low-to-medium. The Rust change is a mechanical substitution of one in-tree helper for its spill-capable sibling, mirroring the shape already used in GlobWalker::bun_join and elsewhere. I verified join_spill in src/paths/resolve_path.rs computes join_needed up front, uses the thread-local buffer when it fits, and resizes the spill Vec otherwise — so the common case is unchanged. I also confirmed StringHashMapUnownedKey::init only stores (hash, len) (no borrow of the pattern bytes) and the glob branches copy into owned Vecs, so the single hoisted spill buffer can be safely re-borrowed each iteration.
Other factors
The tests follow repo conventions closely: backend: "cli" contains the abort in a subprocess, Buffer.alloc(n, fill).toString() avoids slow .repeat() in debug builds, the runtime tests use it.concurrent.each with tempDir/bunEnv/drained pipes and assert a combined {stdout, stderr, exitCode} object, and the bundler tests assert the tree-shaking outcome (the long brace pattern is the only thing marking effects/effect.js, so it must be stored and matched, not silently dropped). The 100 KB segment exceeds the largest platform path buffer (Windows 32767*3+1), so the tests exercise the spill path everywhere. No outstanding reviewer comments; only a robobun status note in the timeline.
|
Updated 9:00 AM PT - Aug 11th, 2026
✅ @robobun, your commit ca68af267d0f8aba7b7b747ff53884997f041267 passed in 🧪 To try this PR locally: bunx bun-pr 37529That installs a local version of the PR into your bun-37529 --bun |
This branch has not been deployed
1 participant
Repro
Any package.json the resolver parses (project root or a dependency) whose
"sideEffects"array has an entry that, joined onto the package directory, is about 4096 bytes or longer:(exit 134; expected output is
3). Same abort for a glob entry and forbun buildof anything importing the package, since both go throughPackageJSON::parse. Reproduced with the 1.4.0 canary on Linux x64 and Windows x64.Cause
The
sideEffectsblock insrc/resolver/package_json.rsbuilds each pattern with the localFileSystemPackageJsonExt::joinshim, which forwards tobun_paths::resolve_path::join. That writes the normalized join into the fixed 4096-byte thread-localJOIN_BUF(the output starts at offset 1, hence the 4095) with no length check, so a long entry panics in every one of the three branches (mixed, globs only, exact only), on every platform.Fix
The shim now takes a spill
Vecand forwards to the existingjoin_spill, which uses the thread-local buffer when the result fits and grows theVecotherwise. OneVecis hoisted above the three loops, the same shape asGlobWalker::bun_joinand the recursivereaddir. Each branch either hashes the joined pattern (StringHashMapUnownedKey::init) or copies it into the glob list before the next iteration, so borrowing it from the spill buffer is sufficient and noresolve_path.rschanges are needed.Keeping the entry, rather than skipping it or failing the parse, is the behaviour the field's consumers expect: the length of an entry says nothing about what it matches. A long glob (for example a large brace group) can match ordinary files in the package and is honoured exactly like a short one, and a long exact entry keeps its usual meaning of matching nothing while leaving the other entries of the array in effect. Webpack and esbuild accept such entries as well; only the fixed buffer made them fatal here.
Tests
test/bundler/esbuild/dce.test.ts: threeitBundledcases next to the othersideEffectsglob tests, one per branch (exact only, globs only, mixed). They usebackend: "cli"so the abort is contained to thebun buildchild, and they check the tree-shaking result: the normal entries still apply, and in the glob and mixed cases only the 100 KB brace pattern markseffects/effect.js, so it has to be stored and matched rather than dropped.test/js/bun/resolve/resolve.test.ts:require()of a package with an oversized exact, glob, and mixedsideEffectsarray prints3and exits 0.All six fail on the unmodified binary with the panic above (
SIGABRT/ exit 134) and pass with this change against a debug build on Linux; theresolve.test.tscases were also checked against a debug build on Windows x64 (theitBundledsuite does not currently run there). The fulldce.test.tsandresolve.test.tsfiles pass against the debug build.