Repository navigation
install: count directories.bin when bin is an empty string - #43145
Conversation
A package.json or a registry manifest with `"bin": ""` and a
`directories.bin` aborts `bun install` on every build, release builds
included:
panic: range end index 516 out of range for slice of length 0
An empty `bin` names no file, so the build pass of each parser reads
`directories.bin` and appends it to the string buffer. The counting pass
stopped at the empty `bin` and reserved nothing for that string, so the
append ran past the end of the buffer.
Both parsers that size their string buffer from a counting pass had the
same mismatch: `PackageManifest::parse` for a registry manifest, and
`Package::parse_with_json_impl` for a package.json. Each counting pass
now reads `bin` the way its build pass does.
|
Status: fix pushed in #43145, waiting for CI. Reproduction (no registry needed). A Release 1.4.3-canary.1 (c6b7fcb) ends with Cause. The build pass of each parser reads Fix. Each counting pass reads Verification. 3 new tests in
The rest of |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review. WalkthroughEmpty string ChangesEmpty
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The empty-bin fallback change has matching counting and resolution updates with coverage for the stated installation paths, so no merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
The review raised no findings, so the diff is unchanged. I added the Windows side of the proof to the status comment above: the three new tests abort on the Windows x64 canary (630e921) and pass on a Windows x64 build of this branch, where the bins link as |
![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.
The two count-pass changes now mirror their build passes exactly (!is_empty() gate, fall-through to directories.bin), and StringBuilder::append in src/bun_core/string/StringBuilder.rs slices dst[..slice.len()], so the uncounted append is a release-build panic, not just a debug assert. Beyond the inline finding, I also checked the npm.rs object-bin arm for the same non-string-value shape — there the extern slice is pre-sized in the count pass and extern_strings_bin_entries_cursor only advances after the loop, so a break 'bin leaves bin unset without stray entries. I also confirmed the manifest builder's doubling slack (npm.rs:2323) stays well under the 523-byte directories.bin for the registry test's small manifest, so that test does exercise the npm.rs half of the fix.
Extended reasoning...
The fix itself is a two-site mirroring of an existing build-pass condition and is correct as written; both Package.rs and npm.rs count passes now skip the empty bin string and fall through to count directories.bin, matching the build passes at Package.rs:2724 and npm.rs:2550. The one inline finding concerns the pre-existing object-bin n arm in Package.rs, which this PR does not touch but which lives in the same 'bin block; the npm.rs sibling of that arm was checked and does not share the lockfile-buffer corruption because its extern slice is pre-sized and the cursor is not advanced on early break. The test's long-path construction was checked against the manifest builder's slack formula and against toBeValidBin/Windows handling; the 523-byte ./-padded path resolves to a short folder name and is not comparable to the skipIf(isWindows) suite at line 3496, which targets the path buffer limit.
One verified lower-impact observation (a convention, logging or cleanup point) was not posted.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/install/lockfile/Package.rs— A package whosebinobject has two or more entries and one non-string value loses every bin after install and leaves empty extern strings in the lockfile. At Package.rs:2690-2696 the build pass grows lockfile.buffers.extern_strings by n*2 default entries before the loop, then Package.rs:2703break 'binon the first non-string value with self.bin still unset. npm links the valid entries. Fix: validate the rows before growing extern_strings, or truncate extern_strings back to current_len on the early break and link the entries that were valid.Extended reasoning...
A folder or workspace package.json has {"bin": {"a": "a.js", "b": 5}}. The count pass at Package.rs:2255-2262 counts 'a', 'a.js', 'b' and breaks. The build pass reaches the
narm at Package.rs:2689. It reserves and grow_default's n*2 = 4 entries in lockfile.buffers.extern_strings at 2692-2696. The loop appends 'a' and 'a.js' into slots 0 and 1, appends 'b' into slot 2, then hitslet Some(v) = v else { break 'bin; }at 2702-2703. self.bin is never assigned, so the package has Tag::None and no bin is linked. extern_strings keeps four entries, the last one default, which are serialized into bun.lockb and bun.lock but referenced by nothing. The npm.rs build pass at 2499-2500 breaks the same way but never advances extern_strings_bin_entries_cursor, so it does not leak slots; the two parsers diverge. This is pre-existing, but it is the same count/build pair the PR audits and the same 'value is not a string' input class. Remedy: on the early break, truncate extern_strings to current_len, or better, skip the non-string entry and still set Tag::Map for the valid pairs as npm does.Verification: pre-existing — the base commit (0d3492e) has byte-identical code for both the object branch of the count pass and the
narm of the build pass; this PR only edits the string branch of the count pass (Package.rs:2264-2272, npm.rs:2184-2191), so it neither introduces, widens nor adds traffic to this path, and it is a different class from the count/build size mismatch the PR fixes (here count and…
|
The finding on the What I ran, with a
One part of the finding I could not confirm. The build pass does grow Why it is not in this PR. This PR fixes a counting pass that reserves less than its build pass appends, which aborts the process. In the object arm the two passes agree on every size and nothing aborts. The defect is a choice about which entries to keep, it is the same in both parsers ( |
2 participants
Problem
package.jsonor a registry manifest with"bin": ""and adirectories.binabortsbun installon every build, release builds included:panic: range end index 516 out of range for slice of length 0.binnames no file, so the build pass readsdirectories.binand appends it. The counting pass stopped at the emptybin(src/install/npm.rs:2184,src/install/lockfile/Package.rs:2264) and reserved nothing for it, so the append ran past the end of the string buffer.Fix
binthe way its build pass does: an empty string falls through todirectories.bin.PackageManifest::parsereads a registry manifest,Package::parse_with_json_implreads apackage.json. Every other reader ofbinappends into a growable buffer and cannot drift.test/cli/install/bun-install-registry.test.ts, one per source of the file. All 3 abort on release 1.4.3-canary (c6b7fcb) and pass here. The rest of that file passes (258 tests), plusbun-install,bun-lock,bun-pmandisolated-install.Background
binnames the executables of a package. A string names one file. An object maps each name to a file.directories.binnames a folder, and every file in it becomes an executable. npm treats an emptybinas absent.npm.rs:2319), so a short uncounted string can fit. The lockfile buffer gets none (lockfile.rs:2671).Notes
Reach.
bun publishsends"bin": ""with no check, so a registry can serve it. The shape also comes from a hand-writtenpackage.json, a workspace, or afile:folder dependency. With the lockfile parser the abort needs only 9 bytes ofdirectories.binpast the slack of the other strings, and the slack is what the rest of thatpackage.jsoncounted. The smallest case I reproduced is a 90-byte rootpackage.jsonwith no dependencies.Tests. Each test uses a
directories.binof 523 bytes, which is longer than any slack. The path is./repeated 256 times plus the folder name, so it resolves to the same folder and the bins still link. Two tests assert the linked bin, one asserts that an install with no dependencies completes.Excluded on purpose.
binclassifier used by both passes of both parsers is the shape that cannot drift again. It needs one abstraction over two JSON value types and two string builders, next toBin::parse_appendinsrc/install/bin.rs. That is a refactor of the bin parsing layer, and it also touches thebun.lockand pnpm readers, so it is not in a crash fix.PackageManifest::parsearound a cursor API. Its counting pass has the same unconditionalcountforbin(npm.rs:3720on that branch). I left a comment there.bun packandbunxreaddirectories.bintoo. Their open reports are pack: ignore bin paths that name no file, strip the trailing slash from directories.bin #38720 and bunx: resolve directories.bin relative to the package #39096. Neither is this under-count.Sibling PR. #43035 removes the debug-only assertions of the same manifest parser. This PR is the release-build crash, and the two do not share a hunk.
no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/cli/install/bun-install-registry.test.ts