From e448c4cc3bf43fc67e39799d34c4ed70b8037509 Mon Sep 17 00:00:00 2001 From: Dylan Conway <35280289+dylan-conway@users.noreply.github.com> Date: Wed, 16 Oct 2024 18:55:49 -0700 Subject: [PATCH] `fs.mkdir` empty string bugfix (#14510) --- src/bun.js/node/node_fs.zig | 27 ++++++------- src/sys.zig | 1 - test/cli/run/log-test.test.ts | 3 +- test/js/node/fs/fs.test.ts | 71 ++++++++++++++++++++--------------- 4 files changed, 54 insertions(+), 48 deletions(-) diff --git a/src/bun.js/node/node_fs.zig b/src/bun.js/node/node_fs.zig index 5d8260c69f925b..f3f434e029a24f 100644 --- a/src/bun.js/node/node_fs.zig +++ b/src/bun.js/node/node_fs.zig @@ -4857,21 +4857,11 @@ pub const NodeFS = struct { pub fn mkdirRecursiveImpl(this: *NodeFS, args: Arguments.Mkdir, comptime flavor: Flavor, comptime Ctx: type, ctx: Ctx) Maybe(Return.Mkdir) { _ = flavor; var buf: bun.OSPathBuffer = undefined; - const path: bun.OSPathSliceZ = if (!Environment.isWindows) - args.path.osPath(&buf) - else brk: { - // TODO(@paperdave): clean this up a lot. - var joined_buf: bun.PathBuffer = undefined; - if (std.fs.path.isAbsolute(args.path.slice())) { - const utf8 = PosixToWinNormalizer.resolveCWDWithExternalBufZ(&joined_buf, args.path.slice()) catch - return .{ .err = .{ .errno = @intFromEnum(C.SystemErrno.ENOMEM), .syscall = .getcwd } }; - break :brk strings.toWPath(&buf, utf8); - } else { - var cwd_buf: bun.PathBuffer = undefined; - const cwd = std.posix.getcwd(&cwd_buf) catch return .{ .err = .{ .errno = @intFromEnum(C.SystemErrno.ENOMEM), .syscall = .getcwd } }; - break :brk strings.toWPath(&buf, bun.path.joinAbsStringBuf(cwd, &joined_buf, &.{args.path.slice()}, .windows)); - } - }; + const path: bun.OSPathSliceZ = if (Environment.isWindows) + strings.toNTPath(&buf, args.path.slice()) + else + args.path.osPath(&buf); + // TODO: remove and make it always a comptime argument return switch (args.always_return_none) { inline else => |always_return_none| this.mkdirRecursiveOSPathImpl(Ctx, ctx, path, args.mode, !always_return_none), @@ -4921,7 +4911,12 @@ pub const NodeFS = struct { return .{ .result = .{ .none = {} } }; }, // continue - .NOENT => {}, + .NOENT => { + if (len == 0) { + // no path to copy + return .{ .err = err }; + } + }, } }, .result => { diff --git a/src/sys.zig b/src/sys.zig index c31f67d4a4e096..667bd4de3c9b41 100644 --- a/src/sys.zig +++ b/src/sys.zig @@ -729,7 +729,6 @@ pub fn mkdirOSPath(file_path: bun.OSPathSliceZ, flags: bun.Mode) Maybe(void) { return switch (Environment.os) { else => mkdir(file_path, flags), .windows => { - assertIsValidWindowsPath(bun.OSPathChar, file_path); const rc = kernel32.CreateDirectoryW(file_path, null); if (Maybe(void).errnoSys( diff --git a/test/cli/run/log-test.test.ts b/test/cli/run/log-test.test.ts index dd684b8e28e91e..c23fb2c1ed4fc7 100644 --- a/test/cli/run/log-test.test.ts +++ b/test/cli/run/log-test.test.ts @@ -2,7 +2,7 @@ import { spawnSync } from "bun"; import { expect, it } from "bun:test"; import * as fs from "fs"; import { bunEnv, bunExe } from "harness"; -import { dirname, join } from "path"; +import { dirname, join, resolve } from "path"; it("should not log .env when quiet", async () => { writeDirectoryTree("/tmp/log-test-silent", { @@ -36,6 +36,7 @@ it("should log .env by default", async () => { }); function writeDirectoryTree(base: string, paths: Record) { + base = resolve(base); for (const path of Object.keys(paths)) { const content = paths[path]; const joined = join(base, path); diff --git a/test/js/node/fs/fs.test.ts b/test/js/node/fs/fs.test.ts index 33a57af66cd461..81570275e9a9af 100644 --- a/test/js/node/fs/fs.test.ts +++ b/test/js/node/fs/fs.test.ts @@ -62,6 +62,19 @@ function mkdirForce(path: string) { if (!existsSync(path)) mkdirSync(path, { recursive: true }); } +function tmpdirTestMkdir(): string { + const now = Date.now().toString(); + const tempdir = `${tmpdir()}/fs.test.ts/${now}/1234/hi`; + expect(existsSync(tempdir)).toBe(false); + const res = mkdirSync(tempdir, { recursive: true }); + if (!res?.includes(now)) { + expect(res).toInclude("fs.test.ts"); + } + expect(res).not.toInclude("1234"); + expect(existsSync(tempdir)).toBe(true); + return tempdir; +} + it("fs.writeFile(1, data) should work when its inherited", async () => { expect([join(import.meta.dir, "fs-writeFile-1-fixture.js"), "1"]).toRun(); }); @@ -315,9 +328,7 @@ it("writeFileSync NOT in append SHOULD truncate the file", () => { describe("copyFileSync", () => { it("should work for files < 128 KB", () => { - const tempdir = `${tmpdir()}/fs.test.js/${Date.now()}/1234/hi`; - expect(existsSync(tempdir)).toBe(false); - expect(tempdir.includes(mkdirSync(tempdir, { recursive: true })!)).toBe(true); + const tempdir = tmpdirTestMkdir(); // that don't exist copyFileSync(import.meta.path, tempdir + "/copyFileSync.js"); @@ -333,9 +344,7 @@ describe("copyFileSync", () => { }); it("should work for files > 128 KB ", () => { - const tempdir = `${tmpdir()}/fs.test.js/${Date.now()}-1/1234/hi`; - expect(existsSync(tempdir)).toBe(false); - expect(tempdir.includes(mkdirSync(tempdir, { recursive: true })!)).toBe(true); + const tempdir = tmpdirTestMkdir(); var buffer = new Int32Array(128 * 1024); for (let i = 0; i < buffer.length; i++) { buffer[i] = i % 256; @@ -362,9 +371,7 @@ describe("copyFileSync", () => { }); it("FICLONE option does not error ever", () => { - const tempdir = `${tmpdir()}/fs.test.js/${Date.now()}.FICLONE/1234/hi`; - expect(existsSync(tempdir)).toBe(false); - expect(tempdir.includes(mkdirSync(tempdir, { recursive: true })!)).toBe(true); + const tempdir = tmpdirTestMkdir(); // that don't exist copyFileSync(import.meta.path, tempdir + "/copyFileSync.js", fs.constants.COPYFILE_FICLONE); @@ -373,9 +380,7 @@ describe("copyFileSync", () => { }); it("COPYFILE_EXCL works", () => { - const tempdir = `${tmpdir()}/fs.test.js/${Date.now()}.COPYFILE_EXCL/1234/hi`; - expect(existsSync(tempdir)).toBe(false); - expect(tempdir.includes(mkdirSync(tempdir, { recursive: true })!)).toBe(true); + const tempdir = tmpdirTestMkdir(); // that don't exist copyFileSync(import.meta.path, tempdir + "/copyFileSync.js", fs.constants.COPYFILE_EXCL); @@ -387,9 +392,7 @@ describe("copyFileSync", () => { if (process.platform === "linux") { describe("should work when copyFileRange is not available", () => { it("on large files", () => { - const tempdir = `${tmpdir()}/fs.test.js/${Date.now()}-1/1234/large`; - expect(existsSync(tempdir)).toBe(false); - expect(tempdir.includes(mkdirSync(tempdir, { recursive: true })!)).toBe(true); + const tempdir = tmpdirTestMkdir(); var buffer = new Int32Array(128 * 1024); for (let i = 0; i < buffer.length; i++) { buffer[i] = i % 256; @@ -421,9 +424,7 @@ describe("copyFileSync", () => { }); it("on small files", () => { - const tempdir = `${tmpdir()}/fs.test.js/${Date.now()}-1/1234/small`; - expect(existsSync(tempdir)).toBe(false); - expect(tempdir.includes(mkdirSync(tempdir, { recursive: true })!)).toBe(true); + const tempdir = tmpdirTestMkdir(); var buffer = new Int32Array(1 * 1024); for (let i = 0; i < buffer.length; i++) { buffer[i] = i % 256; @@ -460,12 +461,22 @@ describe("copyFileSync", () => { describe("mkdirSync", () => { it("should create a directory", () => { - const tempdir = `${tmpdir()}/fs.test.js/${Date.now()}.mkdirSync/1234/hi`; + const now = Date.now().toString(); + const base = join(now, ".mkdirSync", "1234", "hi"); + const tempdir = `${tmpdir()}/${base}`; expect(existsSync(tempdir)).toBe(false); - expect(tempdir.includes(mkdirSync(tempdir, { recursive: true })!)).toBe(true); + + const res = mkdirSync(tempdir, { recursive: true }); + expect(res).toInclude(now); + expect(res).not.toInclude(".mkdirSync"); expect(existsSync(tempdir)).toBe(true); }); + it("should throw ENOENT for empty string", () => { + expect(() => mkdirSync("", { recursive: true })).toThrow("No such file or directory"); + expect(() => mkdirSync("")).toThrow("No such file or directory"); + }); + it("throws for invalid options", () => { const path = `${tmpdir()}/${Date.now()}.rm.dir2/foo/bar`; @@ -1091,10 +1102,10 @@ describe("readSync", () => { closeSync(fd); }); - it("works with invalid fd but zero length",()=>{ + it("works with invalid fd but zero length", () => { expect(readSync(2147483640, Buffer.alloc(0))).toBe(0); expect(readSync(2147483640, Buffer.alloc(10), 0, 0, 0)).toBe(0); - }) + }); }); it("writevSync", () => { @@ -2074,7 +2085,7 @@ describe("fs.ReadStream", () => { describe("createWriteStream", () => { it("simple write stream finishes", async () => { - const path = `${tmpdir()}/fs.test.js/${Date.now()}.createWriteStream.txt`; + const path = `${tmpdir()}/fs.test.ts/${Date.now()}.createWriteStream.txt`; const stream = createWriteStream(path); stream.write("Test file written successfully"); stream.end(); @@ -2092,7 +2103,7 @@ describe("createWriteStream", () => { }); it("writing null throws ERR_STREAM_NULL_VALUES", async () => { - const path = `${tmpdir()}/fs.test.js/${Date.now()}.createWriteStreamNulls.txt`; + const path = `${tmpdir()}/fs.test.ts/${Date.now()}.createWriteStreamNulls.txt`; const stream = createWriteStream(path); try { stream.write(null); @@ -2103,7 +2114,7 @@ describe("createWriteStream", () => { }); it("writing null throws ERR_STREAM_NULL_VALUES (objectMode: true)", async () => { - const path = `${tmpdir()}/fs.test.js/${Date.now()}.createWriteStreamNulls.txt`; + const path = `${tmpdir()}/fs.test.ts/${Date.now()}.createWriteStreamNulls.txt`; const stream = createWriteStream(path, { // @ts-ignore-next-line objectMode: true, @@ -2117,7 +2128,7 @@ describe("createWriteStream", () => { }); it("writing false throws ERR_INVALID_ARG_TYPE", async () => { - const path = `${tmpdir()}/fs.test.js/${Date.now()}.createWriteStreamFalse.txt`; + const path = `${tmpdir()}/fs.test.ts/${Date.now()}.createWriteStreamFalse.txt`; const stream = createWriteStream(path); try { stream.write(false); @@ -2128,7 +2139,7 @@ describe("createWriteStream", () => { }); it("writing false throws ERR_INVALID_ARG_TYPE (objectMode: true)", async () => { - const path = `${tmpdir()}/fs.test.js/${Date.now()}.createWriteStreamFalse.txt`; + const path = `${tmpdir()}/fs.test.ts/${Date.now()}.createWriteStreamFalse.txt`; const stream = createWriteStream(path, { // @ts-ignore-next-line objectMode: true, @@ -2142,7 +2153,7 @@ describe("createWriteStream", () => { }); it("writing in append mode should not truncate the file", async () => { - const path = `${tmpdir()}/fs.test.js/${Date.now()}.createWriteStreamAppend.txt`; + const path = `${tmpdir()}/fs.test.ts/${Date.now()}.createWriteStreamAppend.txt`; const stream = createWriteStream(path, { // @ts-ignore-next-line flags: "a", @@ -2233,7 +2244,7 @@ describe("fs/promises", () => { }); it("writeFile", async () => { - const path = `${tmpdir()}/fs.test.js/${Date.now()}.writeFile.txt`; + const path = `${tmpdir()}/fs.test.ts/${Date.now()}.writeFile.txt`; await writeFile(path, "File written successfully"); expect(readFileSync(path, "utf8")).toBe("File written successfully"); }); @@ -2595,7 +2606,7 @@ it("fstat on a large file", () => { var dest: string = "", fd; try { - dest = `${tmpdir()}/fs.test.js/${Math.trunc(Math.random() * 10000000000).toString(32)}.stat.txt`; + dest = `${tmpdir()}/fs.test.ts/${Math.trunc(Math.random() * 10000000000).toString(32)}.stat.txt`; mkdirSync(dirname(dest), { recursive: true }); const bigBuffer = new Uint8Array(1024 * 1024 * 1024); fd = openSync(dest, "w");