From acd361855ab67be451a92b80c38ce8bfc899d275 Mon Sep 17 00:00:00 2001 From: "Alex Lam S.L" Date: Wed, 15 Mar 2023 17:49:52 +0200 Subject: [PATCH] [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 | 36 +++++++++++++--------------- src/resolver/resolver.zig | 2 +- test/cli/install/bun-install.test.ts | 22 ++++++++--------- test/cli/install/dummy.registry.ts | 13 +++++----- test/js/deno/harness/test.ts | 18 +++++++------- test/js/deno/harness/util.ts | 5 +--- test/js/deno/scripts/postinstall.ts | 2 +- 7 files changed, 48 insertions(+), 50 deletions(-) diff --git a/src/install/lockfile.zig b/src/install/lockfile.zig index 095cbae3c60b2..8d8892736699a 100644 --- a/src/install/lockfile.zig +++ b/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(); diff --git a/src/resolver/resolver.zig b/src/resolver/resolver.zig index c2e00c22ccc7a..493685ca7d467 100644 --- a/src/resolver/resolver.zig +++ b/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), diff --git a/test/cli/install/bun-install.test.ts b/test/cli/install/bun-install.test.ts index ee515280e495c..d722a15fb42e3 100644 --- a/test/cli/install/bun-install.test.ts +++ b/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,7 +14,7 @@ import { requested, root_url, setHandler, -} from "./dummy.registry"; +} from "./dummy.registry.js"; beforeAll(dummyBeforeAll); afterAll(dummyAfterAll); @@ -22,7 +22,7 @@ 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": {}, diff --git a/test/cli/install/dummy.registry.ts b/test/cli/install/dummy.registry.ts index d3fdc2d7f0f5f..b77d79167e465 100644 --- a/test/cli/install/dummy.registry.ts +++ b/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; +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 { return results; } -export function setHandler(newHandler) { +export function setHandler(newHandler: RequestHandler) { handler = newHandler; } diff --git a/test/js/deno/harness/test.ts b/test/js/deno/harness/test.ts index 8e31c3d4d5556..cbe73c52d9289 100644 --- a/test/js/deno/harness/test.ts +++ b/test/js/deno/harness/test.ts @@ -2,10 +2,12 @@ import { test as bunTest } from "bun:test"; type Fn = () => void | Promise; 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 { diff --git a/test/js/deno/harness/util.ts b/test/js/deno/harness/util.ts index ecd8a6953322c..422d6b34c610a 100644 --- a/test/js/deno/harness/util.ts +++ b/test/js/deno/harness/util.ts @@ -27,10 +27,7 @@ export function deferred() { return Object.assign(promise, methods); } -export function delay( - ms: number, - options: { signal?: AbortSignal } = {}, -): Promise { +export function delay(ms: number, options: { signal?: AbortSignal } = {}): Promise { const { signal } = options; if (signal?.aborted) { return Promise.reject(new DOMException("Delay was aborted.", "AbortError")); diff --git a/test/js/deno/scripts/postinstall.ts b/test/js/deno/scripts/postinstall.ts index a5e885dc87e5f..624fb9fad2786 100644 --- a/test/js/deno/scripts/postinstall.ts +++ b/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";