Repository navigation
install: key the lockfile string pool by the bytes, not a stored hash - #41366
Conversation
…lockb name hashes lockfile::StringBuilder::count skips a string whose hash is already in the pool. append_with_hash looked the string up by a hash the caller passed. Package::clone passed the name_hash stored in the lockfile, and Package::from_npm passed hashes stored in the manifest cache. When a stored hash did not match its bytes, count skipped the string and the append wrote it anyway, past the reserved bytes: "range end index N out of range for slice of length M". Remove append_with_hash. append hashes the bytes it is given, so count and append always use the same key, and the ExternalString hash it returns matches the bytes. bun.lockb also copied each package's name_hash from disk. Recompute it from the name on load, as loading bun.lock already does.
|
Reproduced on Bun 1.4.1 in two ways:
Both new tests fail on CI, build 110139: no lane reports a failure in the two new tests. The failures are in code this PR does not change:
Status: ready for review. |
|
Updated 5:17 AM PT - Sep 4th, 2026
❌ @robobun, your commit b3ce903 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 41366That installs a local version of the PR into your bun-41366 --bun |
|
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 (7)
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review. WalkthroughChangesLockfile string construction now computes hashes from string values. Binary lockfile loading recomputes package name hashes. Regression tests cover corrupted cached manifest and lockfile hashes. Lockfile hash integrity
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The lockfile hash handling change has no remaining identified merge-blocking risk. 🚥 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.
I reviewed this PR and didn't find any bugs. Because it changes the string-pool keying invariant that every lockfile clean/clone depends on and adds a recompute pass to the binary lockfile loader, a human familiar with the install subsystem should also take a look.
What was reviewed:
- Confirmed the removed
lockfile::StringBuilder::append_with_hashhas no remaining callers; the survivingappend_with_hashhits are on the separatesemver::string::Buildertype, and every one of those already passesstring_hash(bytes)computed on the line above. - Checked the new recompute loop in
bun.lockb.rsruns afterbuffers::loadpopulatesstring_bytes, soname.slice(string_bytes)reads the loaded buffer. - Verified
package.name_hash = package_name.hashinfrom_npmnow receives the freshly-computed hash viaT::from_pooled(_, hash), not the stale manifest hash. - Both regression tests self-validate their hardcoded binary offsets (asserting the read hash matches before corrupting it) and assert positive outcomes (installed package, saved hash), not merely absence of a panic.
Extended reasoning...
Overview
This PR fixes a lockfile string-builder crash where the two-phase count/append passes could diverge when a caller-supplied hash didn't match the hash of the bytes being appended. lockfile::StringBuilder::append_with_hash is removed and its body folded into append, which now always derives the pool key from SemverStringBuilder::string_hash(slice). Six call sites in CatalogMap.rs, OverrideMap.rs, and Package.rs are migrated. bun.lockb.rs's load gains a post-load pass that recomputes each package's name_hash from its name bytes so a stale stored hash cannot desync package_index or later pooling. Two regression tests reproduce both identified sources of a bad stored hash (a corrupted bun.lockb and a corrupted .npm manifest cache entry) and assert the install succeeds and re-saves the correct hash.
Security risks
None identified. The change tightens an invariant (pool key is always derived from the bytes written) rather than loosening it, and the recompute pass only overwrites a derived field with the value it must have. No new untrusted-input parsing surface is added; the tests operate against the local Verdaccio registry via the existing suite fixtures.
Level of scrutiny
Moderate-to-high. The lockfile string pool is load-bearing for every install — a wrong key here silently produces out-of-bounds writes or mis-pooled strings across the whole dependency graph. The fix itself is structurally simple and clearly correct (make the count key and the append key the same by construction), but the added recompute-on-load loop runs for every package on every bun.lockb open, and the removal of append_with_hash shifts where hashes are computed at several call sites. Someone who owns the install code should confirm the per-load rehash cost is acceptable and that no other consumer of the stored name_hash (e.g. package_index population, verify_data) runs before the recompute point.
Other factors
The PR description is unusually thorough — it names the excluded case (dependency-row name_hash, which is counted/appended by bytes and cannot reach this path), lists suites run against the debug build, and documents that no organic mismatch was observed in lockfiles from five prior Bun releases. The remaining append_with_hash callers in yarn.rs, pnpm.rs, npm_lock.rs, bun.lock.rs, CatalogMap.rs, and OverrideMap.rs are on the separate semver::string::Builder type and every one computes string_hash(bytes) immediately before the call, so they cannot exhibit the count/append divergence. The two changed OverrideMap.rs/CatalogMap.rs sites now hash the same slice twice (once for the local name_hash variable, once inside append); that is a minor redundancy, not a correctness concern. No CODEOWNERS entry covers src/install/, and the timeline shows no outstanding third-party objections.
|
On the two questions for a human reviewer:
|
3 participants
Problem
bun addcan panic inLockfile::clean_with_logger:panic: range end index 391 out of range for slice of length 383. The frames arelockfile::StringBuilder::append_with_hash,Dependency::clone_with_different_buffersandPackage::clone.StringBuilder::count(src/install/lockfile.rs) skips a string whose hash is already pooled.append_with_hashlooked up a hash the caller passed.Package::clonepassed thename_hashstored in the lockfile, andPackage::from_npmpassed hashes stored in the manifest cache. If a stored hash does not match its bytes, the append writes bytes that were never reserved.Fix
append_with_hash.appendhashes the bytes it writes, so the count and the append always use the same key. The six callers move toappend.name_hashfrom its name when abun.lockbloads, as loadingbun.lockalready does. A valid file does not change.bun-lockb.test.tschanges a stored hash inbun.lockb.bun-install-registry.test.tschanges the name hash in a cached manifest. Bun 1.4.1 panics on both. The notes list the other suites.Background
StringBuildercounts the bytes it needs, reserves that much, and then appends.bun.lockbis the binary lockfile, and each.npmfile in the cache is a serialized manifest. Both loaders copy their hashes from disk.Notes
text_lockfilefeature, so that run did not load abun.lock. The origin of its bad hash is not proven. The stored hashes inbun.lockband in the manifest cache are the two sources found, and the tests construct both.bun.lockbfiles written by Bun 1.0.36, 1.1.38, 1.2.0, 1.3.0 and 1.3.14 have none either.bun.lockbwith an out-of-range package id. This change repairsname_hashinstead, because the name fully determines it.package_indexis keyed by it, and the debug build'sverify_dataasserts it.name_hashof dependency rows inbun.lockb. Dependency strings are counted and appended by their bytes, so those rows cannot reach this panic.bun.lockb.rsload. The two changes touch different lines.StringBuilder, which coversfrom_npm, and for the manifest cache test. Both are in this PR.bun-lockb,bun-install-registry,bun-lock,migrate-bun-lockb-v2,bun-add,bun-add-catalog,bun-install,bun-workspaces,overrides,nested-overrides,catalogs,bun-update,migration/.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-lockb.test.ts, test/cli/install/bun-install-registry.test.ts