install: key the lockfile string pool by the bytes, not a stored hash

src/install/lockfile.rs

@@ -2633,12 +2633,9 @@ impl<'a> StringBuilder<'a> {
         Ok(())
     }
 
-    #[inline]
+    /// Keys the pool by the hash of `slice`, as `count` does, so it never writes uncounted bytes.
     pub(crate) fn append<T: StringBuilderType>(&mut self, slice: &[u8]) -> T {
-        self.append_with_hash::<T>(slice, SemverStringBuilder::string_hash(slice))
-    }
-
-    pub(crate) fn append_with_hash<T: StringBuilderType>(&mut self, slice: &[u8], hash: u64) -> T {
+        let hash = SemverStringBuilder::string_hash(slice);
         if SemverString::can_inline(slice) {
             return T::from_init(self.string_bytes.as_slice(), slice, hash);
         }

src/install/lockfile/CatalogMap.rs

@@ -312,7 +312,7 @@ impl CatalogMap {
     ) -> Result<(), AllocError> {
         catalog.try_for_each_property(|dep_name_str, key_loc, value| {
             let dep_name_hash = StringBuilderNs::string_hash(dep_name_str);
-            let dep_name = builder.append_with_hash::<String>(dep_name_str, dep_name_hash);
+            let dep_name = builder.append::<String>(dep_name_str);
 
             let Some(version_str) = value.as_utf8_string_literal() else {
                 return Ok(());

src/install/lockfile/OverrideMap.rs

@@ -1010,9 +1010,7 @@ fn parse_parent(
     selector: &PackageSelector<'_>,
 ) -> Option<Dependency> {
     let name_hash = SemverBuilder::string_hash(selector.name);
-    let name = ctx
-        .builder
-        .append_with_hash::<SemverString>(selector.name, name_hash);
+    let name = ctx.builder.append::<SemverString>(selector.name);
     let version = parse_range(ctx, key_loc, name, name_hash, selector.range)?;
     Some(Dependency {
         name,
@@ -1088,7 +1086,7 @@ fn parse_override_value(
     }
 
     let name_hash = SemverBuilder::string_hash(key);
-    let name = ctx.builder.append_with_hash::<SemverString>(key, name_hash);
+    let name = ctx.builder.append::<SemverString>(key);
 
     // https://docs.npmjs.com/cli/v9/configuring-npm/package-json#overrides
     let literal: SemverString = if value[0] == b'$' {

src/install/lockfile/Package.rs

@@ -552,8 +552,7 @@ impl Package<u64> {
         // `package_index` / `string_bytes` only — none of which the dependency
         // pass mutates — so the reorder is observationally identical.
         let pkg_value = Package {
-            name: builder
-                .append_with_hash::<String>(self.name.slice(old_string_buf), self.name_hash),
+            name: builder.append::<String>(self.name.slice(old_string_buf)),
             bin: self.bin.clone_with_buffers(
                 old_string_buf,
                 old_extern_string_buf,
@@ -714,8 +713,8 @@ impl Package<u64> {
 
         // -- Cloning
         {
-            let package_name: ExternalString = string_builder
-                .append_with_hash::<ExternalString>(manifest.name(), manifest.pkg.name.hash);
+            let package_name: ExternalString =
+                string_builder.append::<ExternalString>(manifest.name());
             package.name_hash = package_name.hash;
             package.name = package_name.value;
             package.resolution =
@@ -772,14 +771,10 @@ impl Package<u64> {
                         }
                     }
 
-                    let name: ExternalString = string_builder.append_with_hash::<ExternalString>(
-                        key.slice(&manifest.string_buf),
-                        key.hash,
-                    );
-                    let dep_version = string_builder.append_with_hash::<String>(
-                        version_string_.slice(&manifest.string_buf),
-                        version_string_.hash,
-                    );
+                    let name: ExternalString =
+                        string_builder.append::<ExternalString>(key.slice(&manifest.string_buf));
+                    let dep_version = string_builder
+                        .append::<String>(version_string_.slice(&manifest.string_buf));
                     // `string_builder` holds the `&mut string_bytes` borrow; read
                     // through it instead of `lockfile.buffers.string_bytes`.
                     let sliced = dep_version.sliced(string_builder.string_bytes.as_slice());

src/install/lockfile/bun.lockb.rs

@@ -458,6 +458,19 @@ pub(crate) fn load(
         return Err(crate::Error::LockfileIsMalformedExpected0AtTheEnd);
     }
 
+    // `name_hash` is derived from `name`. Recompute it, as loading `bun.lock` does.
+    {
+        let string_bytes = lockfile.buffers.string_bytes.as_slice();
+        let package::PackageColumnsMut {
+            name: names,
+            name_hash: name_hashes,
+            ..
+        } = lockfile.packages.split_mut();
+        for (name, name_hash) in names.iter().zip(name_hashes.iter_mut()) {
+            *name_hash = semver::string::Builder::string_hash(name.slice(string_bytes));
+        }
+    }
+
     let has_workspace_name_hashes = false;
     // < Bun v1.0.4 stopped right here when reading the lockfile
     // So we add an extra 8 byte tag to say "hey, there's more data here"

test/cli/install/bun-install-registry.test.ts

@@ -9865,6 +9865,91 @@ test("npm manifest cache entries are only reused for the package name they were
   expect(exitCode).toBe(0);
 });
 
+test("a cached manifest whose name hash disagrees with the name installs with the hash of the name", async () => {
+  const { parseManifest } = npm_manifest_test_helpers;
+  const cacheDir = join(packageDir, ".bun-cache");
+  const lockbPath = join(packageDir, "bun.lockb");
+
+  // bun.lockb stores packages as columns. `name` (8 bytes per package) comes
+  // first, then `name_hash` (8 bytes per package). Package 0 is the root.
+  const nameHashes = async () => {
+    const lockb = Buffer.from(await file(lockbPath).arrayBuffer());
+    const count = Number(lockb.readBigUInt64LE(86));
+    const start = Number(lockb.readBigUInt64LE(110)) + count * 8;
+    return Array.from({ length: count }, (_, i) => lockb.readBigUInt64LE(start + i * 8));
+  };
+
+  // The name must be longer than 8 bytes. Shorter names are stored inline and
+  // never touch the string buffer.
+  await write(
+    packageJson,
+    JSON.stringify({
+      name: "foo",
+      version: "1.0.0",
+      dependencies: {
+        "dep-with-tags": "1.0.0",
+      },
+    }),
+  );
+
+  {
+    await using proc = spawn({
+      cmd: [bunExe(), "install"],
+      cwd: packageDir,
+      stdout: "pipe",
+      stdin: "ignore",
+      stderr: "pipe",
+      env,
+    });
+    const [out, err, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
+    expect(err).toContain("Saved lockfile");
+    expect(out).toContain("+ dep-with-tags@1.0.0");
+    expect(exitCode).toBe(0);
+  }
+
+  const hashes = await nameHashes();
+  expect(hashes).toHaveLength(2);
+  const nameHash = hashes[1];
+
+  const manifestFiles = (await readdirSorted(cacheDir)).filter(name => name.endsWith(".npm"));
+  expect(manifestFiles).toHaveLength(1);
+  const manifestPath = join(cacheDir, manifestFiles[0]);
+
+  // The package record starts at byte 72 (the 49-byte header and the registry
+  // hash and length, aligned to 8). Its name is at byte 32 of the record: a
+  // string (8 bytes), then the hash (8 bytes).
+  const hashOffset = 72 + 32 + 8;
+  const manifest = Buffer.from(await file(manifestPath).arrayBuffer());
+  expect(manifest.readBigUInt64LE(hashOffset)).toBe(nameHash);
+  manifest.writeBigUInt64LE(nameHash ^ 0xffffn, hashOffset);
+  await write(manifestPath, manifest);
+  expect(parseManifest(manifestPath, registryUrl()).name).toBe("dep-with-tags");
+
+  await Promise.all([
+    rm(join(packageDir, "node_modules"), { recursive: true, force: true }),
+    rm(lockbPath, { force: true }),
+  ]);
+
+  // The root's dependency had already pooled the name by its bytes, so the
+  // size pass reserved nothing for it. The append looked the name up by the
+  // cached hash, found nothing, and wrote the name past the reserved bytes:
+  // "panic: range end index N out of range for slice of length M".
+  await using proc = spawn({
+    cmd: [bunExe(), "install", "--prefer-offline"],
+    cwd: packageDir,
+    stdout: "pipe",
+    stdin: "ignore",
+    stderr: "pipe",
+    env,
+  });
+  const [out, err, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
+  expect(err).toContain("Saved lockfile");
+  expect(out).toContain("+ dep-with-tags@1.0.0");
+  expect(exitCode).toBe(0);
+
+  expect(await nameHashes()).toEqual(hashes);
+});
+
 // A manifest cache entry is written to a temporary file in the temporary
 // directory and renamed into the cache directory. Every install on the machine
 // shares the temporary directory, so the temporary file name has to be unique

test/cli/install/bun-lockb.test.ts

@@ -212,6 +212,68 @@ it("recovers from a corrupted binary lockfile instead of panicking", async () =>
   expect(await exists(join(packageDir, "node_modules", "a-dep"))).toBe(true);
 });
 
+it("recomputes a package name hash that disagrees with the name in a binary lockfile", async () => {
+  const { packageDir, packageJson } = await registry.createTestDir({ bunfigOpts: { saveTextLockfile: false } });
+
+  // The name must be longer than 8 bytes. Shorter names are stored inline and
+  // never touch the string buffer.
+  await write(
+    packageJson,
+    JSON.stringify({
+      name: "lockb-name-hash",
+      version: "1.0.0",
+      dependencies: {
+        "dep-with-tags": "1.0.0",
+      },
+    }),
+  );
+
+  await runBunInstall(env, packageDir);
+  const lockbPath = join(packageDir, "bun.lockb");
+
+  // Packages are stored as columns. `name` (8 bytes per package) comes first,
+  // then `name_hash` (8 bytes per package). Package 0 is the root.
+  const nameHashColumn = (lockb: Buffer) => {
+    const count = Number(lockb.readBigUInt64LE(86));
+    return { count, start: Number(lockb.readBigUInt64LE(110)) + count * 8 };
+  };
+  const nameHashes = (lockb: Buffer) => {
+    const { count, start } = nameHashColumn(lockb);
+    return Array.from({ length: count }, (_, i) => lockb.readBigUInt64LE(start + i * 8));
+  };
+  const lockb = Buffer.from(await file(lockbPath).arrayBuffer());
+  const hashes = nameHashes(lockb);
+  expect(hashes).toHaveLength(2);
+  const good = hashes[1];
+  const bad = good ^ 0xffffn;
+  lockb.writeBigUInt64LE(bad, nameHashColumn(lockb).start + 1 * 8);
+  expect(nameHashes(lockb)).toEqual([hashes[0], bad]);
+  await write(lockbPath, lockb);
+
+  // The root's dependency had already pooled the name by its bytes, so the
+  // size pass reserved nothing for it. The append in `Package::clone` looked
+  // the name up by the stored hash, found nothing, and wrote the name past the
+  // reserved bytes: "panic: range end index N out of range for slice of length M".
+  const { stdout, stderr, exited } = spawn({
+    cmd: [bunExe(), "add", "no-deps@1.0.0", "--no-progress"],
+    cwd: packageDir,
+    stdout: "pipe",
+    stderr: "pipe",
+    env,
+  });
+  const [out, err, code] = await Promise.all([stdout.text(), stderr.text(), exited]);
+
+  expect(err).not.toContain("error:");
+  expect(out).toContain("installed no-deps@1.0.0");
+  expect(code).toBe(0);
+
+  // The saved lockfile has the hash of the name again.
+  const saved = nameHashes(Buffer.from(await file(lockbPath).arrayBuffer()));
+  expect(saved).toHaveLength(3);
+  expect(saved).toContain(good);
+  expect(saved).not.toContain(bad);
+});
+
 it("rejects a binary lockfile whose patched-dependency flag byte is out of range", async () => {
   const { packageDir, packageJson } = await registry.createTestDir({ bunfigOpts: { saveTextLockfile: false } });