From 3128beed67a182ee11a5ea0ed31ddd2c729c14d3 Mon Sep 17 00:00:00 2001 From: Dylan Conway <35280289+dylan-conway@users.noreply.github.com> Date: Sun, 19 Jan 2025 16:07:18 -0800 Subject: [PATCH] fix `fs.mkdir` regression (#16497) --- src/bun.js/node/node_fs.zig | 7 +-- src/c.zig | 2 +- src/cli/upgrade_command.zig | 2 +- src/install/install.zig | 10 +--- src/string_immutable.zig | 83 ++++++++++++++++++++++++++--- src/sys.zig | 13 ++--- test/regression/issue/16474.test.ts | 44 +++++++++++++++ 7 files changed, 134 insertions(+), 27 deletions(-) create mode 100644 test/regression/issue/16474.test.ts diff --git a/src/bun.js/node/node_fs.zig b/src/bun.js/node/node_fs.zig index 50ab3b0802..0cc400cbad 100644 --- a/src/bun.js/node/node_fs.zig +++ b/src/bun.js/node/node_fs.zig @@ -834,7 +834,7 @@ pub fn NewAsyncCpTask(comptime is_shell: bool) type { var buf: bun.OSPathBuffer = undefined; const normdest: bun.OSPathSliceZ = if (Environment.isWindows) - switch (bun.sys.normalizePathWindows(u16, bun.invalid_fd, dest, &buf)) { + switch (bun.sys.normalizePathWindows(u16, bun.invalid_fd, dest, &buf, .{ .add_nt_prefix = false })) { .err => |err| { this.finishConcurrently(.{ .err = err }); return false; @@ -3755,10 +3755,7 @@ pub const NodeFS = struct { pub fn mkdirRecursiveImpl(this: *NodeFS, args: Arguments.Mkdir, comptime Ctx: type, ctx: Ctx) Maybe(Return.Mkdir) { const buf = bun.OSPathBufferPool.get(); defer bun.OSPathBufferPool.put(buf); - const path: bun.OSPathSliceZ = if (Environment.isWindows) - strings.toNTPath(buf, args.path.slice()) - else - args.path.osPath(buf); + const path = args.path.osPath(buf); // TODO: remove and make it always a comptime argument return switch (args.always_return_none) { diff --git a/src/c.zig b/src/c.zig index 1541b9872e..98414cbfbd 100644 --- a/src/c.zig +++ b/src/c.zig @@ -161,7 +161,7 @@ pub fn copyFileZSlowWithHandle(in_handle: bun.FileDescriptor, to_dir: bun.FileDe var buf0: bun.WPathBuffer = undefined; var buf1: bun.WPathBuffer = undefined; - const dest = switch (bun.sys.normalizePathWindows(u8, to_dir, destination, &buf0)) { + const dest = switch (bun.sys.normalizePathWindows(u8, to_dir, destination, &buf0, .{})) { .result => |x| x, .err => |e| return .{ .err = e }, }; diff --git a/src/cli/upgrade_command.zig b/src/cli/upgrade_command.zig index 21219d9a21..9c656a24c2 100644 --- a/src/cli/upgrade_command.zig +++ b/src/cli/upgrade_command.zig @@ -1000,7 +1000,7 @@ pub const upgrade_js_bindings = struct { var buf: bun.WPathBuffer = undefined; const tmpdir_path = fs.FileSystem.RealFS.getDefaultTempDir(); - const path = switch (bun.sys.normalizePathWindows(u8, bun.invalid_fd, tmpdir_path, &buf)) { + const path = switch (bun.sys.normalizePathWindows(u8, bun.invalid_fd, tmpdir_path, &buf, .{})) { .err => return .undefined, .result => |norm| norm, }; diff --git a/src/install/install.zig b/src/install/install.zig index f01403d65b..d3a83a079d 100644 --- a/src/install/install.zig +++ b/src/install/install.zig @@ -1565,11 +1565,7 @@ pub fn NewPackageInstall(comptime kind: PkgInstallKind) type { state.buf[i] = 0; const fullpath = state.buf[0..i :0]; - _ = node_fs_for_package_installer.mkdirRecursiveOSPathImpl(void, {}, fullpath, 0, false).unwrap() catch |err| { - state.cached_package_dir.close(); - state.walker.deinit(); - return Result.fail(err, .copying_files); - }; + _ = node_fs_for_package_installer.mkdirRecursiveOSPathImpl(void, {}, fullpath, 0, false); state.to_copy_buf = state.buf[fullpath.len..]; const cache_path_length = bun.windows.kernel32.GetFinalPathNameByHandleW(state.cached_package_dir.fd, &state.buf2, state.buf2.len, 0); @@ -2286,9 +2282,7 @@ pub fn NewPackageInstall(comptime kind: PkgInstallKind) type { wbuf[i] = 0; const fullpath = wbuf[0..i :0]; - _ = node_fs_for_package_installer.mkdirRecursiveOSPathImpl(void, {}, fullpath, 0, false).unwrap() catch |err| { - return Result.fail(err, .linking_dependency); - }; + _ = node_fs_for_package_installer.mkdirRecursiveOSPathImpl(void, {}, fullpath, 0, false); } const res = strings.copyUTF16IntoUTF8(dest_buf[0..], []const u16, wbuf[0..i], true); diff --git a/src/string_immutable.zig b/src/string_immutable.zig index 4fed847464..a2c2d9dba7 100644 --- a/src/string_immutable.zig +++ b/src/string_immutable.zig @@ -29,6 +29,14 @@ pub inline fn containsChar(self: string, char: u8) bool { return indexOfChar(self, char) != null; } +pub inline fn containsCharT(comptime T: type, self: []const T, char: u8) bool { + return switch (T) { + u8 => containsChar(self, char), + u16 => std.mem.indexOfScalar(u16, self, char) != null, + else => @compileError("invalid type"), + }; +} + pub inline fn contains(self: string, str: string) bool { return containsT(u8, self, str); } @@ -1899,11 +1907,30 @@ pub fn fromWPath(buf: []u8, utf16: []const u16) [:0]const u8 { return buf[0..encode_into_result.written :0]; } +pub fn withoutNTPrefix(path: [:0]const u16) [:0]const u16 { + if (hasPrefixComptimeUTF16(path, &bun.windows.nt_object_prefix_u8)) { + return path[bun.windows.nt_object_prefix.len..]; + } + if (hasPrefixComptimeUTF16(path, &bun.windows.nt_maxpath_prefix_u8)) { + return path[bun.windows.nt_maxpath_prefix.len..]; + } + if (hasPrefixComptimeUTF16(path, &bun.windows.nt_unc_object_prefix_u8)) { + return path[bun.windows.nt_unc_object_prefix.len..]; + } + return path; +} + pub fn toNTPath(wbuf: []u16, utf8: []const u8) [:0]u16 { if (!std.fs.path.isAbsoluteWindows(utf8)) { return toWPathNormalized(wbuf, utf8); } + if (strings.hasPrefixComptime(utf8, &bun.windows.nt_object_prefix_u8) or + strings.hasPrefixComptime(utf8, &bun.windows.nt_unc_object_prefix_u8)) + { + return wbuf[0..toWPathNormalized(wbuf, utf8).len :0]; + } + // UNC absolute path, replace leading '\\' with '\??\UNC\' if (strings.hasPrefixComptime(utf8, "\\\\")) { const prefix = bun.windows.nt_unc_object_prefix; @@ -1916,6 +1943,28 @@ pub fn toNTPath(wbuf: []u16, utf8: []const u8) [:0]u16 { return wbuf[0 .. toWPathNormalized(wbuf[prefix.len..], utf8).len + prefix.len :0]; } +pub fn toNTPath16(wbuf: []u16, path: []const u16) [:0]u16 { + if (!std.fs.path.isAbsoluteWindowsWTF16(path)) { + return toWPathNormalized16(wbuf, path); + } + + if (strings.hasPrefixComptimeUTF16(path, &bun.windows.nt_object_prefix_u8) or + strings.hasPrefixComptimeUTF16(path, &bun.windows.nt_unc_object_prefix_u8)) + { + return wbuf[0..toWPathNormalized16(wbuf, path).len :0]; + } + + if (strings.hasPrefixComptimeUTF16(path, "\\\\")) { + const prefix = bun.windows.nt_unc_object_prefix; + wbuf[0..prefix.len].* = prefix; + return wbuf[0 .. toWPathNormalized16(wbuf[prefix.len..], path[2..]).len + prefix.len :0]; + } + + const prefix = bun.windows.nt_object_prefix; + wbuf[0..prefix.len].* = prefix; + return wbuf[0 .. toWPathNormalized16(wbuf[prefix.len..], path).len + prefix.len :0]; +} + pub fn toNTMaxPath(buf: []u8, utf8: []const u8) [:0]const u8 { if (!std.fs.path.isAbsoluteWindows(utf8) or utf8.len <= 260) { @memcpy(buf[0..utf8.len], utf8); @@ -1978,6 +2027,20 @@ pub fn toWPathNormalized(wbuf: []u16, utf8: []const u8) [:0]u16 { return toWPath(wbuf, path_to_use); } + +pub fn toWPathNormalized16(wbuf: []u16, path: []const u16) [:0]u16 { + var path_to_use = normalizeSlashesOnlyT(u16, wbuf, path, '\\', true); + + // is there a trailing slash? Let's remove it before converting to UTF-16 + if (path_to_use.len > 3 and bun.path.isSepAnyT(u16, path_to_use[path_to_use.len - 1])) { + path_to_use = path_to_use[0 .. path_to_use.len - 1]; + } + + wbuf[path_to_use.len] = 0; + + return wbuf[0..path_to_use.len :0]; +} + pub fn toPathNormalized(buf: []u8, utf8: []const u8) [:0]const u8 { const renormalized = bun.PathBufferPool.get(); defer bun.PathBufferPool.put(renormalized); @@ -1992,21 +2055,29 @@ pub fn toPathNormalized(buf: []u8, utf8: []const u8) [:0]const u8 { return toPath(buf, path_to_use); } -pub fn normalizeSlashesOnly(buf: []u8, utf8: []const u8, comptime desired_slash: u8) []const u8 { +pub fn normalizeSlashesOnlyT(comptime T: type, buf: []T, path: []const T, comptime desired_slash: u8, comptime always_copy: bool) []const T { comptime bun.unsafeAssert(desired_slash == '/' or desired_slash == '\\'); const undesired_slash = if (desired_slash == '/') '\\' else '/'; - if (bun.strings.containsChar(utf8, undesired_slash)) { - @memcpy(buf[0..utf8.len], utf8); - for (buf[0..utf8.len]) |*c| { + if (bun.strings.containsCharT(T, path, undesired_slash)) { + @memcpy(buf[0..path.len], path); + for (buf[0..path.len]) |*c| { if (c.* == undesired_slash) { c.* = desired_slash; } } - return buf[0..utf8.len]; + return buf[0..path.len]; } - return utf8; + if (comptime always_copy) { + @memcpy(buf[0..path.len], path); + return buf[0..path.len]; + } + return path; +} + +pub fn normalizeSlashesOnly(buf: []u8, utf8: []const u8, comptime desired_slash: u8) []const u8 { + return normalizeSlashesOnlyT(u8, buf, utf8, desired_slash, false); } pub fn toWDirNormalized(wbuf: []u16, utf8: []const u8) [:0]const u16 { diff --git a/src/sys.zig b/src/sys.zig index e4ad293710..e32d59f07a 100644 --- a/src/sys.zig +++ b/src/sys.zig @@ -772,7 +772,8 @@ pub fn fstatat(fd: bun.FileDescriptor, path: [:0]const u8) Maybe(bun.Stat) { }; } var stat_ = mem.zeroes(bun.Stat); - if (Maybe(bun.Stat).errnoSysFP(syscall.fstatat(fd.int(), path, &stat_, 0), .fstatat, fd, path)) |err| { + const fd_valid = if (fd == bun.invalid_fd) std.posix.AT.FDCWD else fd.int(); + if (Maybe(bun.Stat).errnoSysFP(syscall.fstatat(fd_valid, path, &stat_, 0), .fstatat, fd, path)) |err| { log("fstatat({}, {s}) = {s}", .{ fd, path, @tagName(err.getErrno()) }); return err; } @@ -889,6 +890,7 @@ pub fn normalizePathWindows( dir_fd: bun.FileDescriptor, path_: []const T, buf: *bun.WPathBuffer, + comptime opts: struct { add_nt_prefix: bool = true }, ) Maybe([:0]const u16) { if (comptime T != u8 and T != u16) { @compileError("normalizePathWindows only supports u8 and u16 character types"); @@ -918,7 +920,7 @@ pub fn normalizePathWindows( } } - const norm = bun.path.normalizeStringGenericTZ(u16, path, buf, .{ .add_nt_prefix = true, .zero_terminate = true }); + const norm = bun.path.normalizeStringGenericTZ(u16, path, buf, .{ .add_nt_prefix = opts.add_nt_prefix, .zero_terminate = true }); return .{ .result = norm }; } @@ -1116,7 +1118,7 @@ fn openDirAtWindowsT( const wbuf = bun.WPathBufferPool.get(); defer bun.WPathBufferPool.put(wbuf); - const norm = switch (normalizePathWindows(T, dirFd, path, wbuf)) { + const norm = switch (normalizePathWindows(T, dirFd, path, wbuf, .{})) { .err => |err| return .{ .err = err }, .result => |norm| norm, }; @@ -1310,7 +1312,7 @@ pub fn openFileAtWindowsT( const wbuf = bun.WPathBufferPool.get(); defer bun.WPathBufferPool.put(wbuf); - const norm = switch (normalizePathWindows(T, dirFd, path, wbuf)) { + const norm = switch (normalizePathWindows(T, dirFd, path, wbuf, .{})) { .err => |err| return .{ .err = err }, .result => |norm| norm, }; @@ -2843,10 +2845,9 @@ pub fn directoryExistsAt(dir: anytype, subpath: anytype) JSC.Maybe(bool) { const wbuf = bun.WPathBufferPool.get(); defer bun.WPathBufferPool.put(wbuf); const path = if (std.meta.Child(@TypeOf(subpath)) == u16) - bun.strings.addNTPathPrefixIfNeeded(wbuf, subpath) + bun.strings.toNTPath16(wbuf, subpath) else bun.strings.toNTPath(wbuf, subpath); - bun.path.dangerouslyConvertPathToWindowsInPlace(u16, path); const path_len_bytes: u16 = @truncate(path.len * 2); var nt_name = w.UNICODE_STRING{ diff --git a/test/regression/issue/16474.test.ts b/test/regression/issue/16474.test.ts new file mode 100644 index 0000000000..29f49fdff0 --- /dev/null +++ b/test/regression/issue/16474.test.ts @@ -0,0 +1,44 @@ +import { mkdirSync, writeFileSync } from "fs"; +import { mkdir } from "fs/promises"; +import { test, expect } from "bun:test"; +import { tmpdirSync } from "harness"; +import { join } from "path"; + +test("fs.mkdir recursive should not error on existing", async () => { + const testDir = tmpdirSync(); + + const dir1 = join(testDir, "test123"); + expect(mkdirSync(dir1, { recursive: true })).toBe(dir1); + expect(mkdirSync(dir1, { recursive: true })).toBeUndefined(); + expect(() => { + mkdirSync(dir1); + }).toThrow("EEXIST: file already exists"); + + // relative + expect(() => { + mkdirSync("123test", { recursive: true }); + mkdirSync("123test", { recursive: true }); + + mkdirSync("123test/456test", { recursive: true }); + mkdirSync("123test/456test", { recursive: true }); + }).not.toThrow(); + + const dir2 = join(testDir, "test456"); + expect(await mkdir(dir2)).toBeUndefined(); + expect(await mkdir(dir2, { recursive: true })).toBeUndefined(); + + // nested + const dir3 = join(testDir, "test789", "nested"); + expect(mkdirSync(dir3, { recursive: true })).toBe(join(testDir, "test789")); + expect(mkdirSync(dir3, { recursive: true })).toBeUndefined(); + + // file + const file = join(testDir, "test789", "file.txt"); + writeFileSync(file, "hi"); + expect(() => { + mkdirSync(file, { recursive: true }); + }).toThrow("EEXIST: file already exists"); + expect(async () => { + await mkdir(file, { recursive: true }); + }).toThrow("EEXIST: file already exists"); +});