[install] sanitise against malformed bun.lockb (#2397)

* [install] sanitise against malformed `bun.lockb`

fixes #2392

* fix `prettier` checks in unrelated files

src/install/lockfile.zig

@@ -98,7 +98,6 @@ buffers: Buffers = .{},
 /// name -> PackageID || [*]PackageID
 /// Not for iterating.
 package_index: PackageIndex.Map,
-unique_packages: Bitset,
 string_pool: StringPool,
 allocator: Allocator,
 scratch: Scratch = .{},
@@ -667,10 +666,6 @@ pub fn clean(old: *Lockfile, updates: []PackageManager.UpdateRequest) !*Lockfile
         invalid_package_id,
     );
     var clone_queue_ = PendingResolutions.init(old.allocator);
-    new.unique_packages = try Bitset.initEmpty(
-        old.allocator,
-        old.unique_packages.bit_length,
-    );
     var cloner = Cloner{
         .old = old,
         .lockfile = new,
@@ -1449,7 +1444,6 @@ pub fn initEmpty(this: *Lockfile, allocator: Allocator) !void {
         .packages = .{},
         .buffers = .{},
         .package_index = PackageIndex.Map.initContext(allocator, .{}),
-        .unique_packages = try Bitset.initFull(allocator, 0),
         .string_pool = StringPool.init(allocator),
         .allocator = allocator,
         .scratch = Scratch.init(allocator),
@@ -1503,18 +1497,13 @@ pub fn getPackageID(
 }
 
 pub fn getOrPutID(this: *Lockfile, id: PackageID, name_hash: PackageNameHash) !void {
-    if (this.unique_packages.capacity() < this.packages.len) try this.unique_packages.resize(this.allocator, this.packages.len, true);
     var gpe = try this.package_index.getOrPut(name_hash);
 
     if (gpe.found_existing) {
         var index: *PackageIndex.Entry = gpe.value_ptr;
 
-        this.unique_packages.unset(id);
-
         switch (index.*) {
             .PackageID => |single| {
-                this.unique_packages.unset(single);
-
                 var ids = try PackageIDList.initCapacity(this.allocator, 8);
                 ids.appendAssumeCapacity(single);
                 ids.appendAssumeCapacity(id);
@@ -1528,7 +1517,6 @@ pub fn getOrPutID(this: *Lockfile, id: PackageID, name_hash: PackageNameHash) !v
         }
     } else {
         gpe.value_ptr.* = .{ .PackageID = id };
-        this.unique_packages.set(id);
     }
 }
 
@@ -3227,7 +3215,6 @@ pub const Package = extern struct {
 pub fn deinit(this: *Lockfile) void {
     this.buffers.deinit(this.allocator);
     this.packages.deinit(this.allocator);
-    this.unique_packages.deinit(this.allocator);
     this.string_pool.deinit();
     this.scripts.deinit(this.allocator);
     this.workspace_paths.deinit(this.allocator);
@@ -3414,12 +3401,18 @@ const Buffers = struct {
         }
     }
 
-    pub fn legacyPackageToDependencyID(this: Buffers, package_id: PackageID) !DependencyID {
+    pub fn legacyPackageToDependencyID(this: Buffers, dependency_visited: ?*Bitset, package_id: PackageID) !DependencyID {
         switch (package_id) {
             0 => return Tree.root_dep_id,
             invalid_package_id => return invalid_package_id,
             else => for (this.resolutions.items, 0..) |pkg_id, dep_id| {
-                if (pkg_id == package_id) return @truncate(DependencyID, dep_id);
+                if (pkg_id == package_id) {
+                    if (dependency_visited) |visited| {
+                        if (visited.isSet(dep_id)) continue;
+                        visited.set(dep_id);
+                    }
+                    return @truncate(DependencyID, dep_id);
+                }
             },
         }
         return error.@"Lockfile is missing resolution data";
@@ -3490,14 +3483,20 @@ const Buffers = struct {
 
         // Legacy tree structure stores package IDs instead of dependency IDs
         if (this.trees.items.len > 0 and this.trees.items[0].dependency_id != Tree.root_dep_id) {
+            var visited = try Bitset.initEmpty(allocator, this.dependencies.items.len);
             for (this.trees.items) |*tree| {
-                const dependency_id = tree.dependency_id;
-                tree.dependency_id = try this.legacyPackageToDependencyID(dependency_id);
+                const package_id = tree.dependency_id;
+                tree.dependency_id = try this.legacyPackageToDependencyID(&visited, package_id);
             }
+            visited.setRangeValue(.{
+                .start = 0,
+                .end = this.dependencies.items.len,
+            }, false);
             for (this.hoisted_dependencies.items) |*package_id| {
                 const pid = package_id.*;
-                package_id.* = try this.legacyPackageToDependencyID(pid);
+                package_id.* = try this.legacyPackageToDependencyID(&visited, pid);
             }
+            visited.deinit(allocator);
         }
 
         return this;
@@ -3577,7 +3576,6 @@ pub const Serializer = struct {
 
         {
             lockfile.package_index = PackageIndex.Map.initContext(allocator, .{});
-            lockfile.unique_packages = try Bitset.initFull(allocator, lockfile.packages.len);
             lockfile.string_pool = StringPool.initContext(allocator, .{});
             try lockfile.package_index.ensureTotalCapacity(@truncate(u32, lockfile.packages.len));
             const slice = lockfile.packages.slice();

src/resolver/resolver.zig

@@ -1688,7 +1688,7 @@ pub const Resolver = struct {
                                 if (st == .extract)
                                     manager.enqueuePackageForDownload(
                                         esm.name,
-                                        manager.lockfile.buffers.legacyPackageToDependencyID(resolved_package_id) catch unreachable,
+                                        manager.lockfile.buffers.legacyPackageToDependencyID(null, resolved_package_id) catch unreachable,
                                         resolved_package_id,
                                         resolution.value.npm.version,
                                         manager.lockfile.str(&resolution.value.npm.url),

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

@@ -1,4 +1,4 @@
-import { file, listen, spawn } from "bun";
+import { file, listen, Socket, spawn } from "bun";
 import { afterAll, afterEach, beforeAll, beforeEach, expect, it } from "bun:test";
 import { bunExe, bunEnv as env } from "harness";
 import { access, mkdir, readlink, writeFile } from "fs/promises";
@@ -14,15 +14,15 @@ import {
   requested,
   root_url,
   setHandler,
-} from "./dummy.registry";
+} from "./dummy.registry.js";
 
 beforeAll(dummyBeforeAll);
 afterAll(dummyAfterAll);
 beforeEach(dummyBeforeEach);
 afterEach(dummyAfterEach);
 
 it("should report connection errors", async () => {
-  function end(socket) {
+  function end(socket: Socket) {
     socket.end();
   }
   const server = listen({
@@ -62,7 +62,7 @@ registry = "http://localhost:${server.port}/"
   });
   expect(stderr).toBeDefined();
   const err = await new Response(stderr).text();
-  expect(err.split(/\r?\n/)).toContain("error: ConnectionRefused downloading package manifest bar");
+  expect(err.split(/\r?\n/)).toContain("error: ConnectionClosed downloading package manifest bar");
   expect(stdout).toBeDefined();
   expect(await new Response(stdout).text()).toBe("");
   expect(await exited).toBe(1);
@@ -998,7 +998,7 @@ it("should prefer latest-tagged dependency", async () => {
 });
 
 it("should handle dependency aliasing", async () => {
-  const urls = [];
+  const urls: string[] = [];
   setHandler(
     dummyRegistry(urls, {
       "0.0.3": {
@@ -1166,7 +1166,7 @@ it("should handle dependency aliasing (dist-tagged)", async () => {
 });
 
 it("should not reinstall aliased dependencies", async () => {
-  const urls = [];
+  const urls: string[] = [];
   setHandler(
     dummyRegistry(urls, {
       "0.0.3": {
@@ -1264,7 +1264,7 @@ it("should not reinstall aliased dependencies", async () => {
 });
 
 it("should handle aliased & direct dependency references", async () => {
-  const urls = [];
+  const urls: string[] = [];
   setHandler(
     dummyRegistry(urls, {
       "0.0.3": {
@@ -1344,7 +1344,7 @@ it("should handle aliased & direct dependency references", async () => {
 });
 
 it("should not hoist if name collides with alias", async () => {
-  const urls = [];
+  const urls: string[] = [];
   setHandler(
     dummyRegistry(urls, {
       "0.0.2": {},
@@ -1958,7 +1958,7 @@ it("should handle GitHub URL in dependencies (git+https://github.com/user/repo.g
 });
 
 it("should consider peerDependencies during hoisting", async () => {
-  const urls = [];
+  const urls: string[] = [];
   setHandler(
     dummyRegistry(urls, {
       "0.0.3": {
@@ -2651,7 +2651,7 @@ it("should de-duplicate committish in Git URLs", async () => {
 });
 
 it("should prefer optionalDependencies over dependencies of the same name", async () => {
-  const urls = [];
+  const urls: string[] = [];
   setHandler(
     dummyRegistry(urls, {
       "0.0.3": {},
@@ -2704,7 +2704,7 @@ it("should prefer optionalDependencies over dependencies of the same name", asyn
 });
 
 it("should prefer dependencies over peerDependencies of the same name", async () => {
-  const urls = [];
+  const urls: string[] = [];
   setHandler(
     dummyRegistry(urls, {
       "0.0.3": {},

test/cli/install/dummy.registry.ts

@@ -1,13 +1,14 @@
-import { file } from "bun";
+import { file, Server } from "bun";
 import { expect } from "bun:test";
 import { mkdtemp, readdir, realpath, rm, writeFile } from "fs/promises";
 import { tmpdir } from "os";
 import { basename, join } from "path";
 
-let handler, server;
-export let package_dir, requested, root_url;
+type RequestHandler = (request: Request) => Response | Promise<Response>;
+let handler: RequestHandler, server: Server;
+export let package_dir: string, requested: number, root_url: string;
 
-export function dummyRegistry(urls, info: any = { "0.0.2": {} }) {
+export function dummyRegistry(urls: string[], info: any = { "0.0.2": {} }): RequestHandler {
   return async request => {
     urls.push(request.url);
     expect(request.method).toBe("GET");
@@ -20,7 +21,7 @@ export function dummyRegistry(urls, info: any = { "0.0.2": {} }) {
     expect(request.headers.get("npm-auth-type")).toBe(null);
     expect(await request.text()).toBe("");
     const name = request.url.slice(request.url.indexOf("/", root_url.length) + 1);
-    const versions = {};
+    const versions: any = {};
     let version;
     for (version in info) {
       if (!/^[0-9]/.test(version)) continue;
@@ -51,7 +52,7 @@ export async function readdirSorted(path: PathLike): Promise<string[]> {
   return results;
 }
 
-export function setHandler(newHandler) {
+export function setHandler(newHandler: RequestHandler) {
   handler = newHandler;
 }
 

test/js/deno/harness/test.ts

@@ -2,10 +2,12 @@ import { test as bunTest } from "bun:test";
 
 type Fn = () => void | Promise<unknown>;
 type Options = {
-  permissions?: "none" | {
-    net?: boolean;
-    read?: boolean;
-  };
+  permissions?:
+    | "none"
+    | {
+        net?: boolean;
+        read?: boolean;
+      };
   ignore?: boolean;
 };
 
@@ -14,10 +16,10 @@ export function test(arg0: Fn | Options, arg1?: Fn): void {
     bunTest(arg0.name, arg0);
   } else if (typeof arg1 === "function") {
     if (
-      arg0?.ignore === true
-      || arg0?.permissions === "none"
-      || arg0?.permissions?.net === false
-      || arg0?.permissions?.read === false
+      arg0?.ignore === true ||
+      arg0?.permissions === "none" ||
+      arg0?.permissions?.net === false ||
+      arg0?.permissions?.read === false
     ) {
       bunTest.skip(arg1.name, arg1);
     } else {

test/js/deno/harness/util.ts

@@ -27,10 +27,7 @@ export function deferred<T>() {
   return Object.assign(promise, methods);
 }
 
-export function delay(
-  ms: number,
-  options: { signal?: AbortSignal } = {},
-): Promise<void> {
+export function delay(ms: number, options: { signal?: AbortSignal } = {}): Promise<void> {
   const { signal } = options;
   if (signal?.aborted) {
     return Promise.reject(new DOMException("Delay was aborted.", "AbortError"));

test/js/deno/scripts/postinstall.ts

@@ -96,7 +96,7 @@ function visitTest(item: CallExpression): void {
     if (argument.expression.type === "FunctionExpression") {
       const fn = argument.expression.identifier?.value;
       if (fn) {
-        const pattern = new RegExp(tests.flatMap((test) => test.skip ?? []).join("|"), "i");
+        const pattern = new RegExp(tests.flatMap(test => test.skip ?? []).join("|"), "i");
         if (pattern.test(fn)) {
           // @ts-ignore
           item.callee.property.value = "test.ignore";