From efbba60e5b8aed95b2413ff4169632bf3605c963 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?H=C3=BCseyin=20A=C3=A7acak?= <110401522+huseyinacacak-janea@users.noreply.github.com> Date: Sat, 14 Sep 2024 17:56:31 +0300 Subject: [PATCH] path: fix bugs and inconsistencies Fixes: https://github.com/nodejs/node/issues/54025 PR-URL: https://github.com/nodejs/node/pull/54224 Reviewed-By: Yagiz Nizipli Reviewed-By: Luigi Pinca Reviewed-By: James M Snell Reviewed-By: Ruben Bridgewater --- lib/internal/modules/cjs/loader.js | 15 ++- lib/internal/url.js | 9 -- lib/path.js | 124 ++++++++++++++++----- src/path.cc | 61 +++++++--- test/cctest/test_path.cc | 20 ++-- test/parallel/test-fs-utils-get-dirents.js | 2 +- test/parallel/test-path-makelong.js | 6 +- test/parallel/test-path-normalize.js | 2 + test/parallel/test-path-resolve.js | 16 +-- 9 files changed, 185 insertions(+), 70 deletions(-) diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index 451b7c2195e7ad..8d3f59abcf5720 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -702,7 +702,7 @@ Module._findPath = function(request, paths, isMain) { let exts; const trailingSlash = request.length > 0 && - (StringPrototypeCharCodeAt(request, request.length - 1) === CHAR_FORWARD_SLASH || ( + ((StringPrototypeCharCodeAt(request, request.length - 1) === CHAR_FORWARD_SLASH || ( StringPrototypeCharCodeAt(request, request.length - 1) === CHAR_DOT && ( request.length === 1 || @@ -712,7 +712,18 @@ Module._findPath = function(request, paths, isMain) { StringPrototypeCharCodeAt(request, request.length - 3) === CHAR_FORWARD_SLASH )) ) - )); + )) || (isWindows && ( + StringPrototypeCharCodeAt(request, request.length - 1) === CHAR_BACKWARD_SLASH || ( + StringPrototypeCharCodeAt(request, request.length - 1) === CHAR_DOT && + ( + request.length === 1 || + StringPrototypeCharCodeAt(request, request.length - 2) === CHAR_BACKWARD_SLASH || + (StringPrototypeCharCodeAt(request, request.length - 2) === CHAR_DOT && ( + request.length === 2 || + StringPrototypeCharCodeAt(request, request.length - 3) === CHAR_BACKWARD_SLASH + )) + ) + )))); const isRelative = StringPrototypeCharCodeAt(request, 0) === CHAR_DOT && ( diff --git a/lib/internal/url.js b/lib/internal/url.js index 3cb186182947a1..b62766b02987d1 100644 --- a/lib/internal/url.js +++ b/lib/internal/url.js @@ -74,9 +74,7 @@ const { } = require('internal/errors'); const { CHAR_AMPERSAND, - CHAR_BACKWARD_SLASH, CHAR_EQUAL, - CHAR_FORWARD_SLASH, CHAR_LOWERCASE_A, CHAR_LOWERCASE_Z, CHAR_PERCENT, @@ -1569,13 +1567,6 @@ function pathToFileURL(filepath, options = kEmptyObject) { return outURL; } let resolved = (windows ?? isWindows) ? path.win32.resolve(filepath) : path.posix.resolve(filepath); - // path.resolve strips trailing slashes so we must add them back - const filePathLast = StringPrototypeCharCodeAt(filepath, - filepath.length - 1); - if ((filePathLast === CHAR_FORWARD_SLASH || - ((windows ?? isWindows) && filePathLast === CHAR_BACKWARD_SLASH)) && - resolved[resolved.length - 1] !== path.sep) - resolved += '/'; // Call encodePathChars first to avoid encoding % again for ? and #. resolved = encodePathChars(resolved, { windows }); diff --git a/lib/path.js b/lib/path.js index e9b56e89a40ee7..f124a7ed64025d 100644 --- a/lib/path.js +++ b/lib/path.js @@ -190,6 +190,7 @@ const win32 = { let resolvedDevice = ''; let resolvedTail = ''; let resolvedAbsolute = false; + let slashCheck = false; for (let i = args.length - 1; i >= -1; i--) { let path; @@ -221,6 +222,10 @@ const win32 = { } } + if (i === args.length - 1 && + isPathSeparator(StringPrototypeCharCodeAt(path, path.length - 1))) { + slashCheck = true; + } const len = path.length; let rootEnd = 0; let device = ''; @@ -268,10 +273,16 @@ const win32 = { j++; } if (j === len || j !== last) { - // We matched a UNC root - device = - `\\\\${firstPart}\\${StringPrototypeSlice(path, last, j)}`; - rootEnd = j; + if (firstPart !== '.' && firstPart !== '?') { + // We matched a UNC root + device = + `\\\\${firstPart}\\${StringPrototypeSlice(path, last, j)}`; + rootEnd = j; + } else { + // We matched a device root (e.g. \\\\.\\PHYSICALDRIVE0) + device = `\\\\${firstPart}`; + rootEnd = 4; + } } } } @@ -323,9 +334,21 @@ const win32 = { resolvedTail = normalizeString(resolvedTail, !resolvedAbsolute, '\\', isPathSeparator); - return resolvedAbsolute ? - `${resolvedDevice}\\${resolvedTail}` : - `${resolvedDevice}${resolvedTail}` || '.'; + if (!resolvedAbsolute) { + return `${resolvedDevice}${resolvedTail}` || '.'; + } + + if (resolvedTail.length === 0) { + return slashCheck ? `${resolvedDevice}\\` : resolvedDevice; + } + + if (slashCheck) { + return resolvedTail === '\\' ? + `${resolvedDevice}\\` : + `${resolvedDevice}\\${resolvedTail}\\`; + } + + return `${resolvedDevice}\\${resolvedTail}`; }, /** @@ -381,17 +404,22 @@ const win32 = { !isPathSeparator(StringPrototypeCharCodeAt(path, j))) { j++; } - if (j === len) { - // We matched a UNC root only - // Return the normalized version of the UNC root since there - // is nothing left to process - return `\\\\${firstPart}\\${StringPrototypeSlice(path, last)}\\`; - } - if (j !== last) { - // We matched a UNC root with leftovers - device = - `\\\\${firstPart}\\${StringPrototypeSlice(path, last, j)}`; - rootEnd = j; + if (j === len || j !== last) { + if (firstPart === '.' || firstPart === '?') { + // We matched a device root (e.g. \\\\.\\PHYSICALDRIVE0) + device = `\\\\${firstPart}`; + rootEnd = 4; + } else if (j === len) { + // We matched a UNC root only + // Return the normalized version of the UNC root since there + // is nothing left to process + return `\\\\${firstPart}\\${StringPrototypeSlice(path, last)}\\`; + } else { + // We matched a UNC root with leftovers + device = + `\\\\${firstPart}\\${StringPrototypeSlice(path, last, j)}`; + rootEnd = j; + } } } } @@ -1162,6 +1190,7 @@ const posix = { resolve(...args) { let resolvedPath = ''; let resolvedAbsolute = false; + let slashCheck = false; for (let i = args.length - 1; i >= -1 && !resolvedAbsolute; i--) { const path = i >= 0 ? args[i] : posixCwd(); @@ -1171,8 +1200,17 @@ const posix = { if (path.length === 0) { continue; } + if (i === args.length - 1 && + isPosixPathSeparator(StringPrototypeCharCodeAt(path, + path.length - 1))) { + slashCheck = true; + } - resolvedPath = `${path}/${resolvedPath}`; + if (resolvedPath.length !== 0) { + resolvedPath = `${path}/${resolvedPath}`; + } else { + resolvedPath = path; + } resolvedAbsolute = StringPrototypeCharCodeAt(path, 0) === CHAR_FORWARD_SLASH; } @@ -1184,10 +1222,20 @@ const posix = { resolvedPath = normalizeString(resolvedPath, !resolvedAbsolute, '/', isPosixPathSeparator); - if (resolvedAbsolute) { - return `/${resolvedPath}`; + if (!resolvedAbsolute) { + if (resolvedPath.length === 0) { + return '.'; + } + if (slashCheck) { + return `${resolvedPath}/`; + } + return resolvedPath; + } + + if (resolvedPath.length === 0 || resolvedPath === '/') { + return '/'; } - return resolvedPath.length > 0 ? resolvedPath : '.'; + return slashCheck ? `/${resolvedPath}/` : `/${resolvedPath}`; }, /** @@ -1271,11 +1319,35 @@ const posix = { if (from === to) return ''; - const fromStart = 1; - const fromEnd = from.length; + // Trim any leading slashes + let fromStart = 0; + while (fromStart < from.length && + StringPrototypeCharCodeAt(from, fromStart) === CHAR_FORWARD_SLASH) { + fromStart++; + } + // Trim trailing slashes + let fromEnd = from.length; + while ( + fromEnd - 1 > fromStart && + StringPrototypeCharCodeAt(from, fromEnd - 1) === CHAR_FORWARD_SLASH + ) { + fromEnd--; + } const fromLen = fromEnd - fromStart; - const toStart = 1; - const toLen = to.length - toStart; + + // Trim any leading slashes + let toStart = 0; + while (toStart < to.length && + StringPrototypeCharCodeAt(to, toStart) === CHAR_FORWARD_SLASH) { + toStart++; + } + // Trim trailing slashes + let toEnd = to.length; + while (toEnd - 1 > toStart && + StringPrototypeCharCodeAt(to, toEnd - 1) === CHAR_FORWARD_SLASH) { + toEnd--; + } + const toLen = toEnd - toStart; // Compare paths to find the longest common path from root const length = (fromLen < toLen ? fromLen : toLen); diff --git a/src/path.cc b/src/path.cc index fade21c8af9414..628c76549790b8 100644 --- a/src/path.cc +++ b/src/path.cc @@ -97,6 +97,7 @@ std::string PathResolve(Environment* env, std::string resolvedDevice = ""; std::string resolvedTail = ""; bool resolvedAbsolute = false; + bool slashCheck = false; const size_t numArgs = paths.size(); auto cwd = env->GetCwd(env->exec_path()); @@ -126,6 +127,10 @@ std::string PathResolve(Environment* env, } } + if (static_cast(i) == numArgs - 1 && + IsPathSeparator(path[path.length() - 1])) { + slashCheck = true; + } const size_t len = path.length(); int rootEnd = 0; std::string device = ""; @@ -170,9 +175,16 @@ std::string PathResolve(Environment* env, j++; } if (j == len || j != last) { - // We matched a UNC root - device = "\\\\" + firstPart + "\\" + path.substr(last, j - last); - rootEnd = j; + if (firstPart != "." && firstPart != "?") { + // We matched a UNC root + device = + "\\\\" + firstPart + "\\" + path.substr(last, j - last); + rootEnd = j; + } else { + // We matched a device root (e.g. \\\\.\\PHYSICALDRIVE0) + device = "\\\\" + firstPart; + rootEnd = 4; + } } } } @@ -220,15 +232,27 @@ std::string PathResolve(Environment* env, // Normalize the tail path resolvedTail = NormalizeString(resolvedTail, !resolvedAbsolute, "\\"); - if (resolvedAbsolute) { - return resolvedDevice + "\\" + resolvedTail; + if (!resolvedAbsolute) { + if (!resolvedDevice.empty() || !resolvedTail.empty()) { + return resolvedDevice + resolvedTail; + } + return "."; } - if (!resolvedDevice.empty() || !resolvedTail.empty()) { - return resolvedDevice + resolvedTail; + if (resolvedTail.empty()) { + if (slashCheck) { + return resolvedDevice + "\\"; + } + return resolvedDevice; } - return "."; + if (slashCheck) { + if (resolvedTail == "\\") { + return resolvedDevice + "\\"; + } + return resolvedDevice + "\\" + resolvedTail + "\\"; + } + return resolvedDevice + "\\" + resolvedTail; } #else // _WIN32 std::string PathResolve(Environment* env, @@ -237,10 +261,15 @@ std::string PathResolve(Environment* env, bool resolvedAbsolute = false; auto cwd = env->GetCwd(env->exec_path()); const size_t numArgs = paths.size(); + bool slashCheck = false; for (int i = numArgs - 1; i >= -1 && !resolvedAbsolute; i--) { const std::string& path = (i >= 0) ? std::string(paths[i]) : cwd; + if (static_cast(i) == numArgs - 1 && path.back() == '/') { + slashCheck = true; + } + if (!path.empty()) { resolvedPath = std::string(path) + "/" + resolvedPath; @@ -254,15 +283,21 @@ std::string PathResolve(Environment* env, // Normalize the path auto normalizedPath = NormalizeString(resolvedPath, !resolvedAbsolute, "/"); - if (resolvedAbsolute) { - return "/" + normalizedPath; + if (!resolvedAbsolute) { + if (normalizedPath.empty()) { + return "."; + } + if (slashCheck) { + return normalizedPath + "/"; + } + return normalizedPath; } - if (normalizedPath.empty()) { - return "."; + if (normalizedPath.empty() || normalizedPath == "/") { + return "/"; } - return normalizedPath; + return slashCheck ? "/" + normalizedPath + "/" : "/" + normalizedPath; } #endif // _WIN32 diff --git a/test/cctest/test_path.cc b/test/cctest/test_path.cc index 16bd9872f3b035..4e25ffd11024c2 100644 --- a/test/cctest/test_path.cc +++ b/test/cctest/test_path.cc @@ -25,26 +25,28 @@ TEST_F(PathTest, PathResolve) { "d:\\e.exe"); EXPECT_EQ(PathResolve(*env, {"c:/ignore", "c:/some/file"}), "c:\\some\\file"); EXPECT_EQ(PathResolve(*env, {"d:/ignore", "d:some/dir//"}), - "d:\\ignore\\some\\dir"); + "d:\\ignore\\some\\dir\\"); EXPECT_EQ(PathResolve(*env, {"."}), cwd); EXPECT_EQ(PathResolve(*env, {"//server/share", "..", "relative\\"}), - "\\\\server\\share\\relative"); + "\\\\server\\share\\relative\\"); EXPECT_EQ(PathResolve(*env, {"c:/", "//"}), "c:\\"); EXPECT_EQ(PathResolve(*env, {"c:/", "//dir"}), "c:\\dir"); - EXPECT_EQ(PathResolve(*env, {"c:/", "//server/share"}), - "\\\\server\\share\\"); - EXPECT_EQ(PathResolve(*env, {"c:/", "//server//share"}), - "\\\\server\\share\\"); + EXPECT_EQ(PathResolve(*env, {"c:/", "//server/share"}), "\\\\server\\share"); + EXPECT_EQ(PathResolve(*env, {"c:/", "//server//share"}), "\\\\server\\share"); EXPECT_EQ(PathResolve(*env, {"c:/", "///some//dir"}), "c:\\some\\dir"); EXPECT_EQ( PathResolve(*env, {"C:\\foo\\tmp.3\\", "..\\tmp.3\\cycles\\root.js"}), "C:\\foo\\tmp.3\\cycles\\root.js"); + EXPECT_EQ(PathResolve(*env, {"\\\\.\\PHYSICALDRIVE0"}), + "\\\\.\\PHYSICALDRIVE0"); + EXPECT_EQ(PathResolve(*env, {"\\\\?\\PHYSICALDRIVE0"}), + "\\\\?\\PHYSICALDRIVE0"); #else - EXPECT_EQ(PathResolve(*env, {"/var/lib", "../", "file/"}), "/var/file"); - EXPECT_EQ(PathResolve(*env, {"/var/lib", "/../", "file/"}), "/file"); + EXPECT_EQ(PathResolve(*env, {"/var/lib", "../", "file/"}), "/var/file/"); + EXPECT_EQ(PathResolve(*env, {"/var/lib", "/../", "file/"}), "/file/"); EXPECT_EQ(PathResolve(*env, {"a/b/c/", "../../.."}), cwd); EXPECT_EQ(PathResolve(*env, {"."}), cwd); - EXPECT_EQ(PathResolve(*env, {"/some/dir", ".", "/absolute/"}), "/absolute"); + EXPECT_EQ(PathResolve(*env, {"/some/dir", ".", "/absolute/"}), "/absolute/"); EXPECT_EQ(PathResolve(*env, {"/foo/tmp.3/", "../tmp.3/cycles/root.js"}), "/foo/tmp.3/cycles/root.js"); #endif diff --git a/test/parallel/test-fs-utils-get-dirents.js b/test/parallel/test-fs-utils-get-dirents.js index dfc851090b269d..8d19a8730cfb39 100644 --- a/test/parallel/test-fs-utils-get-dirents.js +++ b/test/parallel/test-fs-utils-get-dirents.js @@ -92,7 +92,7 @@ const filename = 'foo'; 'DeprecationWarning', 'dirent.path is deprecated in favor of dirent.parentPath', 'DEP0178'); - assert.deepStrictEqual(dirent.path, Buffer.from(tmpdir.resolve(`${filename}/`))); + assert.deepStrictEqual(dirent.path, Buffer.from(tmpdir.resolve(`${filename}`))); }, )); } diff --git a/test/parallel/test-path-makelong.js b/test/parallel/test-path-makelong.js index 7a4783953c8fde..0c332a32b6830b 100644 --- a/test/parallel/test-path-makelong.js +++ b/test/parallel/test-path-makelong.js @@ -76,10 +76,10 @@ if (common.isWindows) { assert.strictEqual(path.win32.toNamespacedPath('C:\\foo'), '\\\\?\\C:\\foo'); assert.strictEqual(path.win32.toNamespacedPath('C:/foo'), '\\\\?\\C:\\foo'); assert.strictEqual(path.win32.toNamespacedPath('\\\\foo\\bar'), - '\\\\?\\UNC\\foo\\bar\\'); + '\\\\?\\UNC\\foo\\bar'); assert.strictEqual(path.win32.toNamespacedPath('//foo//bar'), - '\\\\?\\UNC\\foo\\bar\\'); -assert.strictEqual(path.win32.toNamespacedPath('\\\\?\\foo'), '\\\\?\\foo\\'); + '\\\\?\\UNC\\foo\\bar'); +assert.strictEqual(path.win32.toNamespacedPath('\\\\?\\foo'), '\\\\?\\foo'); assert.strictEqual(path.win32.toNamespacedPath('\\\\?\\c:\\Windows/System'), '\\\\?\\c:\\Windows\\System'); assert.strictEqual(path.win32.toNamespacedPath(null), null); assert.strictEqual(path.win32.toNamespacedPath(true), true); diff --git a/test/parallel/test-path-normalize.js b/test/parallel/test-path-normalize.js index e1d3b9ce1e6c02..7a3d02cb7fe126 100644 --- a/test/parallel/test-path-normalize.js +++ b/test/parallel/test-path-normalize.js @@ -40,6 +40,8 @@ assert.strictEqual( '..\\..\\..\\..\\baz' ); assert.strictEqual(path.win32.normalize('foo/bar\\baz'), 'foo\\bar\\baz'); +assert.strictEqual(path.win32.normalize('\\\\.\\foo'), '\\\\.\\foo'); +assert.strictEqual(path.win32.normalize('\\\\.\\foo\\'), '\\\\.\\foo\\'); assert.strictEqual(path.posix.normalize('./fixtures///b/../b/c.js'), 'fixtures/b/c.js'); diff --git a/test/parallel/test-path-resolve.js b/test/parallel/test-path-resolve.js index 3fc9b2e3abd90a..52eb773715509e 100644 --- a/test/parallel/test-path-resolve.js +++ b/test/parallel/test-path-resolve.js @@ -23,25 +23,27 @@ const resolveTests = [ [[['c:/blah\\blah', 'd:/games', 'c:../a'], 'c:\\blah\\a'], [['c:/ignore', 'd:\\a/b\\c/d', '\\e.exe'], 'd:\\e.exe'], [['c:/ignore', 'c:/some/file'], 'c:\\some\\file'], - [['d:/ignore', 'd:some/dir//'], 'd:\\ignore\\some\\dir'], + [['d:/ignore', 'd:some/dir//'], 'd:\\ignore\\some\\dir\\'], [['.'], process.cwd()], - [['//server/share', '..', 'relative\\'], '\\\\server\\share\\relative'], + [['//server/share', '..', 'relative\\'], '\\\\server\\share\\relative\\'], [['c:/', '//'], 'c:\\'], [['c:/', '//dir'], 'c:\\dir'], - [['c:/', '//server/share'], '\\\\server\\share\\'], - [['c:/', '//server//share'], '\\\\server\\share\\'], + [['c:/', '//server/share'], '\\\\server\\share'], + [['c:/', '//server//share'], '\\\\server\\share'], [['c:/', '///some//dir'], 'c:\\some\\dir'], [['C:\\foo\\tmp.3\\', '..\\tmp.3\\cycles\\root.js'], 'C:\\foo\\tmp.3\\cycles\\root.js'], + [['\\\\.\\PHYSICALDRIVE0'], '\\\\.\\PHYSICALDRIVE0'], + [['\\\\?\\PHYSICALDRIVE0'], '\\\\?\\PHYSICALDRIVE0'], ], ], [ path.posix.resolve, // Arguments result - [[['/var/lib', '../', 'file/'], '/var/file'], - [['/var/lib', '/../', 'file/'], '/file'], + [[['/var/lib', '../', 'file/'], '/var/file/'], + [['/var/lib', '/../', 'file/'], '/file/'], [['a/b/c/', '../../..'], posixyCwd], [['.'], posixyCwd], - [['/some/dir', '.', '/absolute/'], '/absolute'], + [['/some/dir', '.', '/absolute/'], '/absolute/'], [['/foo/tmp.3/', '../tmp.3/cycles/root.js'], '/foo/tmp.3/cycles/root.js'], ], ],