From f8d5b2e6e2c1774d3ee3045187ca15d7f80eefb4 Mon Sep 17 00:00:00 2001 From: Ciro Spaciari Date: Wed, 30 Oct 2024 22:06:21 -0700 Subject: [PATCH] Fix module resolution cache keys (#14901) Co-authored-by: dave caruso Co-authored-by: cirospaciari --- src/bun.js/javascript.zig | 8 ++-- src/bun.js/node/types.zig | 9 ++-- src/bundler.zig | 2 +- src/fs.zig | 2 +- src/resolver/resolve_path.zig | 36 ++++++++++++---- src/resolver/resolver.zig | 19 +++++---- src/string_immutable.zig | 65 ++++++++++++++--------------- src/sys.zig | 5 +++ test/js/bun/resolve/resolve.test.ts | 24 +++++++++++ 9 files changed, 111 insertions(+), 59 deletions(-) diff --git a/src/bun.js/javascript.zig b/src/bun.js/javascript.zig index ab2d9ec90d..20adca6a23 100644 --- a/src/bun.js/javascript.zig +++ b/src/bun.js/javascript.zig @@ -2343,7 +2343,7 @@ pub const VirtualMachine = struct { const buster_name = name: { if (std.fs.path.isAbsolute(normalized_specifier)) { if (std.fs.path.dirname(normalized_specifier)) |dir| { - // Normalized with trailing slash + // Normalized without trailing slash break :name bun.strings.normalizeSlashesOnly(&specifier_cache_resolver_buf, dir, std.fs.path.sep); } } @@ -2363,7 +2363,7 @@ pub const VirtualMachine = struct { }; // Only re-query if we previously had something cached. - if (jsc_vm.bundler.resolver.bustDirCache(buster_name)) { + if (jsc_vm.bundler.resolver.bustDirCache(bun.strings.withoutTrailingSlashWindowsPath(buster_name))) { continue; } @@ -4398,7 +4398,7 @@ pub fn NewHotReloader(comptime Ctx: type, comptime EventLoopType: type, comptime // on windows we receive file events for all items affected by a directory change // so we only need to clear the directory cache. all other effects will be handled // by the file events - _ = this.ctx.bustDirCache(strings.pathWithoutTrailingSlashOne(file_path)); + _ = this.ctx.bustDirCache(strings.withoutTrailingSlashWindowsPath(file_path)); continue; } var affected_buf: [128][]const u8 = undefined; @@ -4448,7 +4448,7 @@ pub fn NewHotReloader(comptime Ctx: type, comptime EventLoopType: type, comptime } } - _ = this.ctx.bustDirCache(strings.pathWithoutTrailingSlashOne(file_path)); + _ = this.ctx.bustDirCache(strings.withoutTrailingSlashWindowsPath(file_path)); if (entries_option) |dir_ent| { var last_file_hash: GenericWatcher.HashType = std.math.maxInt(GenericWatcher.HashType); diff --git a/src/bun.js/node/types.zig b/src/bun.js/node/types.zig index 5f90346105..9b7b7d9ecc 100644 --- a/src/bun.js/node/types.zig +++ b/src/bun.js/node/types.zig @@ -2093,9 +2093,12 @@ pub const Process = struct { fs.top_level_dir = fs.top_level_dir_buf[0..into_cwd_buf.len]; const len = fs.top_level_dir.len; - fs.top_level_dir_buf[len] = std.fs.path.sep; - fs.top_level_dir_buf[len + 1] = 0; - fs.top_level_dir = fs.top_level_dir_buf[0 .. len + 1]; + // Ensure the path ends with a slash + if (fs.top_level_dir_buf[len - 1] != std.fs.path.sep) { + fs.top_level_dir_buf[len] = std.fs.path.sep; + fs.top_level_dir_buf[len + 1] = 0; + fs.top_level_dir = fs.top_level_dir_buf[0 .. len + 1]; + } const withoutTrailingSlash = if (Environment.isWindows) strings.withoutTrailingSlashWindowsPath else strings.withoutTrailingSlash; var str = bun.String.createUTF8(withoutTrailingSlash(fs.top_level_dir)); return str.transferToJS(globalObject); diff --git a/src/bundler.zig b/src/bundler.zig index 97f19cfce4..c6cd1eeb26 100644 --- a/src/bundler.zig +++ b/src/bundler.zig @@ -449,7 +449,7 @@ pub const Bundler = struct { }; // Only re-query if we previously had something cached. - if (bundler.resolver.bustDirCache(buster_name)) { + if (bundler.resolver.bustDirCache(bun.strings.withoutTrailingSlashWindowsPath(buster_name))) { if (_resolveEntryPoint(bundler, entry_point)) |result| return result else |_| { diff --git a/src/fs.zig b/src/fs.zig index d172588fc1..d14c3f676a 100644 --- a/src/fs.zig +++ b/src/fs.zig @@ -1033,7 +1033,7 @@ pub const FileSystem = struct { comptime Iterator: type, iterator: Iterator, ) !*EntriesOption { - var dir = bun.strings.pathWithoutTrailingSlashOne(dir_maybe_trail_slash); + var dir = bun.strings.withoutTrailingSlashWindowsPath(dir_maybe_trail_slash); bun.resolver.Resolver.assertValidCacheKey(dir); var cache_result: ?allocators.Result = null; diff --git a/src/resolver/resolve_path.zig b/src/resolver/resolve_path.zig index 4bbf4e773c..07909bba74 100644 --- a/src/resolver/resolve_path.zig +++ b/src/resolver/resolve_path.zig @@ -613,6 +613,8 @@ fn windowsVolumeNameLenT(comptime T: type, path: []const T) struct { usize, usiz } } } + + return .{ path.len, 0 }; } else { if (bun.strings.indexAnyComptimeT(T, path[3..], strings.literal(T, "/\\"))) |idx| { // TODO: handle input "//abc//def" should be picked up as a unc path @@ -624,6 +626,7 @@ fn windowsVolumeNameLenT(comptime T: type, path: []const T) struct { usize, usiz } } } + return .{ path.len, 0 }; } } return .{ 0, 0 }; @@ -678,6 +681,7 @@ pub fn windowsFilesystemRootT(comptime T: type, path: []const T) []const T { return path[0..2]; } } + // UNC if (path.len >= 5 and Platform.windows.isSeparatorT(T, path[0]) and @@ -685,13 +689,14 @@ pub fn windowsFilesystemRootT(comptime T: type, path: []const T) []const T { !Platform.windows.isSeparatorT(T, path[2]) and path[2] != '.') { - if (bun.strings.indexAnyComptimeT(T, path[3..], "/\\")) |idx| { - if (bun.strings.indexAnyComptimeT(T, path[4 + idx ..], "/\\")) |idx_second| { + if (bun.strings.indexOfAnyT(T, path[3..], "/\\")) |idx| { + if (bun.strings.indexOfAnyT(T, path[4 + idx ..], "/\\")) |idx_second| { return path[0 .. idx + idx_second + 4 + 1]; // +1 to skip second separator } - return path[0..]; } + return path[0..]; } + if (isSepAnyT(T, path[0])) return path[0..1]; return path[0..0]; } @@ -793,12 +798,18 @@ pub fn normalizeStringGenericTZ( } else { @memcpy(buf[buf_i .. buf_i + 2], strings.literal(T, sep_str ++ sep_str)); } - @memcpy(buf[buf_i + 2 .. buf_i + indexOfThirdUNCSlash + 1], path_[2 .. indexOfThirdUNCSlash + 1]); - buf[buf_i + indexOfThirdUNCSlash] = options.separator; - @memcpy( - buf[buf_i + indexOfThirdUNCSlash + 1 .. buf_i + volLen], - path_[indexOfThirdUNCSlash + 1 .. volLen], - ); + if (indexOfThirdUNCSlash > 0) { + // we have the ending slash + @memcpy(buf[buf_i + 2 .. buf_i + indexOfThirdUNCSlash + 1], path_[2 .. indexOfThirdUNCSlash + 1]); + buf[buf_i + indexOfThirdUNCSlash] = options.separator; + @memcpy( + buf[buf_i + indexOfThirdUNCSlash + 1 .. buf_i + volLen], + path_[indexOfThirdUNCSlash + 1 .. volLen], + ); + } else { + // we dont have the ending slash + @memcpy(buf[buf_i + 2 .. buf_i + volLen], path_[2..volLen]); + } buf[buf_i + volLen] = options.separator; buf_i += volLen + 1; path_begin = volLen + 1; @@ -1923,9 +1934,11 @@ pub const PosixToWinNormalizer = struct { @memcpy(buf[source_root.len..][0 .. maybe_posix_path.len - 1], maybe_posix_path[1..]); const res = buf[0 .. source_root.len + maybe_posix_path.len - 1]; assert(!bun.strings.isWindowsAbsolutePathMissingDriveLetter(u8, res)); + assert(std.fs.path.isAbsoluteWindows(res)); return res; } } + assert(!bun.strings.isWindowsAbsolutePathMissingDriveLetter(u8, maybe_posix_path)); } return maybe_posix_path; } @@ -1948,9 +1961,11 @@ pub const PosixToWinNormalizer = struct { @memcpy(buf[source_root.len..][0 .. maybe_posix_path.len - 1], maybe_posix_path[1..]); const res = buf[0 .. source_root.len + maybe_posix_path.len - 1]; assert(!bun.strings.isWindowsAbsolutePathMissingDriveLetter(u8, res)); + assert(std.fs.path.isAbsoluteWindows(res)); return res; } } + assert(!bun.strings.isWindowsAbsolutePathMissingDriveLetter(u8, maybe_posix_path)); } return maybe_posix_path; @@ -1975,9 +1990,12 @@ pub const PosixToWinNormalizer = struct { buf[source_root.len + maybe_posix_path.len - 1] = 0; const res = buf[0 .. source_root.len + maybe_posix_path.len - 1 :0]; assert(!bun.strings.isWindowsAbsolutePathMissingDriveLetter(u8, res)); + assert(std.fs.path.isAbsoluteWindows(res)); return res; } } + + assert(!bun.strings.isWindowsAbsolutePathMissingDriveLetter(u8, maybe_posix_path)); } @memcpy(buf.ptr, maybe_posix_path); diff --git a/src/resolver/resolver.zig b/src/resolver/resolver.zig index fea49fe255..438d16b9b6 100644 --- a/src/resolver/resolver.zig +++ b/src/resolver/resolver.zig @@ -1550,8 +1550,8 @@ pub const Resolver = struct { /// But drive roots MUST have a trailing slash ('/' and 'C:\') /// UNC paths, even if the root, must not have the trailing slash. /// - /// The helper function bun.strings.pathWithoutTrailingSlashOne can be used to remove - /// the trailing slash from a path, but also note it will only remove a SINGLE slash. + /// The helper function bun.strings.withoutTrailingSlashWindowsPath can be used + /// to remove the trailing slash from a path pub fn assertValidCacheKey(path: []const u8) void { if (Environment.allow_assert) { if (path.len > 1 and strings.charIsAnySlash(path[path.len - 1]) and !if (Environment.isWindows) @@ -2048,7 +2048,7 @@ pub const Resolver = struct { ) !?*DirInfo { assert(r.package_manager != null); - const dir_path = strings.pathWithoutTrailingSlashOne(dir_path_maybe_trail_slash); + const dir_path = strings.withoutTrailingSlashWindowsPath(dir_path_maybe_trail_slash); assertValidCacheKey(dir_path); var dir_cache_info_result = r.dir_cache.getOrPut(dir_path) catch bun.outOfMemory(); @@ -2545,7 +2545,7 @@ pub const Resolver = struct { assert(std.fs.path.isAbsolute(input_path)); - const path_without_trailing_slash = strings.pathWithoutTrailingSlashOne(input_path); + const path_without_trailing_slash = strings.withoutTrailingSlashWindowsPath(input_path); assertValidCacheKey(path_without_trailing_slash); const top_result = try r.dir_cache.getOrPut(path_without_trailing_slash); if (top_result.status != .unknown) { @@ -2567,7 +2567,7 @@ pub const Resolver = struct { .status = .not_found, }; const root_path = if (Environment.isWindows) - bun.strings.pathWithoutTrailingSlashOne(ResolvePath.windowsFilesystemRoot(path)) + bun.strings.withoutTrailingSlashWindowsPath(ResolvePath.windowsFilesystemRoot(path)) else // we cannot just use "/" // we will write to the buffer past the ptr len so it must be a non-const buffer @@ -3661,7 +3661,8 @@ pub const Resolver = struct { } } - const dir_path = bun.strings.pathWithoutTrailingSlashOne(Dirname.dirname(path)); + const dir_path = bun.strings.withoutTrailingSlashWindowsPath(Dirname.dirname(path)); + bun.strings.assertIsValidWindowsPath(u8, dir_path); const dir_entry: *Fs.FileSystem.RealFS.EntriesOption = rfs.readDirectory( dir_path, @@ -4116,7 +4117,11 @@ pub const Dirname = struct { if (Environment.isWindows) { const root = ResolvePath.windowsFilesystemRoot(path); assert(root.len > 0); - break :brk root; + + // Preserve the trailing slash for UNC paths. + // Going from `\\server\share\folder` should end up + // at `\\server\share\`, not `\\server\share` + break :brk if (root.len >= 5 and path.len > root.len) path[0 .. root.len + 1] else root; } break :brk "/"; }; diff --git a/src/string_immutable.zig b/src/string_immutable.zig index 594bf3386e..5745bd2b91 100644 --- a/src/string_immutable.zig +++ b/src/string_immutable.zig @@ -135,6 +135,7 @@ pub fn indexOfAny16(self: []const u16, comptime str: anytype) ?OptionalUsize { pub fn indexOfAnyT(comptime T: type, str: []const T, comptime chars: anytype) ?OptionalUsize { if (T == u8) return indexOfAny(str, chars); + for (str, 0..) |c, i| { inline for (chars) |a| { if (c == a) { @@ -741,34 +742,24 @@ pub fn withoutTrailingSlash(this: string) []const u8 { return href; } -/// Does not strip the C:\ -pub fn withoutTrailingSlashWindowsPath(this: string) []const u8 { - if (this.len < 3 or - this[1] != ':') return withoutTrailingSlash(this); +/// Does not strip the device root (C:\ or \\Server\Share\ portion off of the path) +pub fn withoutTrailingSlashWindowsPath(input: string) []const u8 { + if (Environment.isPosix or input.len < 3 or input[1] != ':') + return withoutTrailingSlash(input); - var href = this; - while (href.len > 3 and (switch (href[href.len - 1]) { + const root_len = bun.path.windowsFilesystemRoot(input).len + 1; + + var path = input; + while (path.len > root_len and (switch (path[path.len - 1]) { '/', '\\' => true, else => false, })) { - href.len -= 1; + path.len -= 1; } - return href; -} + bun.assert(!isWindowsAbsolutePathMissingDriveLetter(u8, path)); -/// This will remove ONE trailing slash at the end of a string, -/// but on Windows it will not remove the \ in "C:\" -pub fn pathWithoutTrailingSlashOne(str: []const u8) []const u8 { - return if (str.len > 0 and charIsAnySlash(str[str.len - 1])) - if (Environment.isWindows and str.len == 3 and str[1] == ':') - // Preserve "C:\" - str - else - // Remove one slash - str[0 .. str.len - 1] - else - str; + return path; } pub fn withoutLeadingSlash(this: string) []const u8 { @@ -1828,25 +1819,21 @@ pub fn utf16Codepoint(comptime Type: type, input: Type) UTF16Replacement { } } -/// Checks if a path is missing a windows drive letter. Not a perfect check, -/// but it is good enough for most cases. For windows APIs, this is used for -/// an assertion, and PosixToWinNormalizer can help make an absolute path -/// contain a drive letter. +/// Checks if a path is missing a windows drive letter. For windows APIs, +/// this is used for an assertion, and PosixToWinNormalizer can help make +/// an absolute path contain a drive letter. pub fn isWindowsAbsolutePathMissingDriveLetter(comptime T: type, chars: []const T) bool { bun.unsafeAssert(bun.path.Platform.windows.isAbsoluteT(T, chars)); bun.unsafeAssert(chars.len > 0); // 'C:\hello' -> false + // This is the most common situation, so we check it first if (!(chars[0] == '/' or chars[0] == '\\')) { bun.unsafeAssert(chars.len > 2); bun.unsafeAssert(chars[1] == ':'); return false; } - // '\\hello' -> false (probably a UNC path) - if (chars.len > 1 and - (chars[1] == '/' or chars[1] == '\\')) return false; - if (chars.len > 4) { // '\??\hello' -> false (has the NT object prefix) if (chars[1] == '?' and @@ -1861,9 +1848,13 @@ pub fn isWindowsAbsolutePathMissingDriveLetter(comptime T: type, chars: []const return false; } - // oh no, '/hello/world' - // where is the drive letter! - return true; + // A path starting with `/` can be a UNC path with forward slashes, + // or actually just a posix path. + // + // '\\Server\Share' -> false (unc) + // '\\Server\\Share' -> true (not unc because extra slashes) + // '\Server\Share' -> true (posix path) + return bun.path.windowsFilesystemRootT(T, chars).len == 1; } pub fn fromWPath(buf: []u8, utf16: []const u16) [:0]const u8 { @@ -1978,13 +1969,19 @@ pub fn toWPath(wbuf: []u16, utf8: []const u8) [:0]const u16 { pub fn toWDirPath(wbuf: []u16, utf8: []const u8) [:0]const u16 { return toWPathMaybeDir(wbuf, utf8, true); } - +fn isUNCPath(comptime T: type, path: []const T) bool { + return path.len >= 3 and + bun.path.Platform.windows.isSeparatorT(T, path[0]) and + bun.path.Platform.windows.isSeparatorT(T, path[1]) and + !bun.path.Platform.windows.isSeparatorT(T, path[2]) and + path[2] != '.'; +} pub fn assertIsValidWindowsPath(comptime T: type, path: []const T) void { if (Environment.allow_assert and Environment.isWindows) { if (bun.path.Platform.windows.isAbsoluteT(T, path) and isWindowsAbsolutePathMissingDriveLetter(T, path) and // is it a null device path? that's not an error. it's just a weird file path. - !eqlComptimeT(T, path, "\\\\.\\NUL") and !eqlComptimeT(T, path, "\\\\.\\nul") and !eqlComptimeT(T, path, "\\nul") and !eqlComptimeT(T, path, "\\NUL")) + !eqlComptimeT(T, path, "\\\\.\\NUL") and !eqlComptimeT(T, path, "\\\\.\\nul") and !eqlComptimeT(T, path, "\\nul") and !eqlComptimeT(T, path, "\\NUL") and !isUNCPath(T, path)) { std.debug.panic("Internal Error: Do not pass posix paths to Windows APIs, was given '{s}'" ++ if (Environment.isDebug) " (missing a root like 'C:\\', see PosixToWinNormalizer for why this is an assertion)" else ". Please open an issue on GitHub with a reproduction.", .{ if (T == u8) path else bun.fmt.utf16(path), diff --git a/src/sys.zig b/src/sys.zig index e229a04916..e2ea08f83a 100644 --- a/src/sys.zig +++ b/src/sys.zig @@ -958,6 +958,11 @@ fn openDirAtWindowsT( .result => |norm| norm, }; + if (comptime T == u8) { + log("openDirAtWindows({s}) = {s}", .{ path, bun.fmt.utf16(norm) }); + } else { + log("openDirAtWindowsT({s}) = {s}", .{ bun.fmt.utf16(path), bun.fmt.utf16(norm) }); + } return openDirAtWindowsNtPath(dirFd, norm, options); } diff --git a/test/js/bun/resolve/resolve.test.ts b/test/js/bun/resolve/resolve.test.ts index 6491d75c2a..d2fa6fbb97 100644 --- a/test/js/bun/resolve/resolve.test.ts +++ b/test/js/bun/resolve/resolve.test.ts @@ -1,3 +1,7 @@ +import { it, expect } from "bun:test"; +import { mkdirSync, writeFileSync } from "fs"; +import { join } from "path"; +import { bunExe, bunEnv, tempDirWithFiles, isWindows } from "harness"; import { pathToFileURL } from "bun"; import { expect, it } from "bun:test"; import { mkdirSync, writeFileSync } from "fs"; @@ -311,3 +315,23 @@ it.todo("import override to bun:test", async () => { // @ts-expect-error expect(await import("#bun_test")).toBeDefined(); }); + +it.if(isWindows)("directory cache key computation", () => { + expect(import(`${process.cwd()}\\\\doesnotexist.ts`)).rejects.toThrow(); + expect(import(`${process.cwd()}\\\\\\doesnotexist.ts`)).rejects.toThrow(); + expect(import(`\\\\Test\\\\doesnotexist.ts\\` as any)).rejects.toThrow(); + expect(import(`\\\\Test\\\\doesnotexist.ts\\\\` as any)).rejects.toThrow(); + expect(import(`\\\\Test\\\\doesnotexist.ts\\\\\\` as any)).rejects.toThrow(); + expect(import(`\\\\Test\\\\\\doesnotexist.ts` as any)).rejects.toThrow(); + expect(import(`\\\\Test\\\\\\\\doesnotexist.ts` as any)).rejects.toThrow(); + expect(import(`\\\\Test\\doesnotexist.ts` as any)).rejects.toThrow(); + expect(import(`\\\\\\Test\\doesnotexist.ts` as any)).rejects.toThrow(); + expect(import(`\\\\Test\\\\\\doesnotexist.ts\\` as any)).rejects.toThrow(); + expect(import(`\\\\Test\\\\\\\\doesnotexist.ts\\` as any)).rejects.toThrow(); + expect(import(`\\\\Test\\doesnotexist.ts\\` as any)).rejects.toThrow(); + expect(import(`\\\\\\Test\\doesnotexist.ts\\` as any)).rejects.toThrow(); + expect(import(`\\\\Test\\\\\\doesnotexist.ts\\\\` as any)).rejects.toThrow(); + expect(import(`\\\\Test\\\\\\\\doesnotexist.ts\\\\` as any)).rejects.toThrow(); + expect(import(`\\\\Test\\doesnotexist.ts\\\\` as any)).rejects.toThrow(); + expect(import(`\\\\\\Test\\doesnotexist.ts\\\\` as any)).rejects.toThrow(); +});