From ee46d2297c648dc6cc8cbc0327c453514b878294 Mon Sep 17 00:00:00 2001 From: Aviv Keller Date: Mon, 21 Oct 2024 03:10:47 -0400 Subject: [PATCH] Revert "path: fix bugs and inconsistencies" This reverts commit efbba60e5b8aed95b2413ff4169632bf3605c963. PR-URL: https://github.com/nodejs/node/pull/55414 Reviewed-By: Claudio Wunder Reviewed-By: Richard Lau Reviewed-By: Luigi Pinca --- lib/internal/modules/cjs/loader.js | 15 +-- lib/internal/url.js | 11 +- 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, 71 insertions(+), 186 deletions(-) diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index 9e57b9a3554..7d8e6948e25 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -700,7 +700,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 || @@ -710,18 +710,7 @@ 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 dbddadfc8b4..64f1b0b6cc0 100644 --- a/lib/internal/url.js +++ b/lib/internal/url.js @@ -74,7 +74,9 @@ const { } = require('internal/errors'); const { CHAR_AMPERSAND, + CHAR_BACKWARD_SLASH, CHAR_EQUAL, + CHAR_FORWARD_SLASH, CHAR_LOWERCASE_A, CHAR_LOWERCASE_Z, CHAR_PERCENT, @@ -1597,7 +1599,14 @@ function pathToFileURL(filepath, options = kEmptyObject) { ); return outURL; } - const resolved = windows ? path.win32.resolve(filepath) : path.posix.resolve(filepath); + let resolved = windows ? 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 += '/'; return new URL(`file://${encodePathChars(resolved, { windows })}`); } diff --git a/lib/path.js b/lib/path.js index ef85d2e0b58..eefa9bc5db0 100644 --- a/lib/path.js +++ b/lib/path.js @@ -190,7 +190,6 @@ const win32 = { let resolvedDevice = ''; let resolvedTail = ''; let resolvedAbsolute = false; - let slashCheck = false; for (let i = args.length - 1; i >= -1; i--) { let path; @@ -222,10 +221,6 @@ const win32 = { } } - if (i === args.length - 1 && - isPathSeparator(StringPrototypeCharCodeAt(path, path.length - 1))) { - slashCheck = true; - } const len = path.length; let rootEnd = 0; let device = ''; @@ -273,16 +268,10 @@ const win32 = { j++; } if (j === len || j !== last) { - 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; - } + // We matched a UNC root + device = + `\\\\${firstPart}\\${StringPrototypeSlice(path, last, j)}`; + rootEnd = j; } } } @@ -334,21 +323,9 @@ const win32 = { resolvedTail = normalizeString(resolvedTail, !resolvedAbsolute, '\\', isPathSeparator); - if (!resolvedAbsolute) { - return `${resolvedDevice}${resolvedTail}` || '.'; - } - - if (resolvedTail.length === 0) { - return slashCheck ? `${resolvedDevice}\\` : resolvedDevice; - } - - if (slashCheck) { - return resolvedTail === '\\' ? - `${resolvedDevice}\\` : - `${resolvedDevice}\\${resolvedTail}\\`; - } - - return `${resolvedDevice}\\${resolvedTail}`; + return resolvedAbsolute ? + `${resolvedDevice}\\${resolvedTail}` : + `${resolvedDevice}${resolvedTail}` || '.'; }, /** @@ -404,22 +381,17 @@ const win32 = { !isPathSeparator(StringPrototypeCharCodeAt(path, j))) { 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; - } + 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; } } } @@ -1190,7 +1162,6 @@ const posix = { resolve(...args) { let resolvedPath = ''; let resolvedAbsolute = false; - let slashCheck = false; for (let i = args.length - 1; i >= 0 && !resolvedAbsolute; i--) { const path = args[i]; @@ -1200,17 +1171,8 @@ const posix = { if (path.length === 0) { continue; } - if (i === args.length - 1 && - isPosixPathSeparator(StringPrototypeCharCodeAt(path, - path.length - 1))) { - slashCheck = true; - } - if (resolvedPath.length !== 0) { - resolvedPath = `${path}/${resolvedPath}`; - } else { - resolvedPath = path; - } + resolvedPath = `${path}/${resolvedPath}`; resolvedAbsolute = StringPrototypeCharCodeAt(path, 0) === CHAR_FORWARD_SLASH; } @@ -1229,20 +1191,10 @@ const posix = { resolvedPath = normalizeString(resolvedPath, !resolvedAbsolute, '/', isPosixPathSeparator); - if (!resolvedAbsolute) { - if (resolvedPath.length === 0) { - return '.'; - } - if (slashCheck) { - return `${resolvedPath}/`; - } - return resolvedPath; + if (resolvedAbsolute) { + return `/${resolvedPath}`; } - - if (resolvedPath.length === 0 || resolvedPath === '/') { - return '/'; - } - return slashCheck ? `/${resolvedPath}/` : `/${resolvedPath}`; + return resolvedPath.length > 0 ? resolvedPath : '.'; }, /** @@ -1326,35 +1278,11 @@ const posix = { if (from === to) return ''; - // 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 fromStart = 1; + const fromEnd = from.length; const fromLen = fromEnd - fromStart; - - // 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; + const toStart = 1; + const toLen = to.length - 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 df6a11644b4..4068e1d892d 100644 --- a/src/path.cc +++ b/src/path.cc @@ -97,7 +97,6 @@ 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()); @@ -127,10 +126,6 @@ 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 = ""; @@ -175,16 +170,9 @@ std::string PathResolve(Environment* env, j++; } if (j == len || j != last) { - 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; - } + // We matched a UNC root + device = "\\\\" + firstPart + "\\" + path.substr(last, j - last); + rootEnd = j; } } } @@ -232,27 +220,15 @@ std::string PathResolve(Environment* env, // Normalize the tail path resolvedTail = NormalizeString(resolvedTail, !resolvedAbsolute, "\\"); - if (!resolvedAbsolute) { - if (!resolvedDevice.empty() || !resolvedTail.empty()) { - return resolvedDevice + resolvedTail; - } - return "."; + if (resolvedAbsolute) { + return resolvedDevice + "\\" + resolvedTail; } - if (resolvedTail.empty()) { - if (slashCheck) { - return resolvedDevice + "\\"; - } - return resolvedDevice; + if (!resolvedDevice.empty() || !resolvedTail.empty()) { + return resolvedDevice + resolvedTail; } - if (slashCheck) { - if (resolvedTail == "\\") { - return resolvedDevice + "\\"; - } - return resolvedDevice + "\\" + resolvedTail + "\\"; - } - return resolvedDevice + "\\" + resolvedTail; + return "."; } #else // _WIN32 std::string PathResolve(Environment* env, @@ -261,16 +237,11 @@ 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 (!path.empty()) { - if (static_cast(i) == numArgs - 1 && path.back() == '/') { - slashCheck = true; - } - resolvedPath = std::string(path) + "/" + resolvedPath; if (path.front() == '/') { @@ -283,21 +254,15 @@ std::string PathResolve(Environment* env, // Normalize the path auto normalizedPath = NormalizeString(resolvedPath, !resolvedAbsolute, "/"); - if (!resolvedAbsolute) { - if (normalizedPath.empty()) { - return "."; - } - if (slashCheck) { - return normalizedPath + "/"; - } - return normalizedPath; + if (resolvedAbsolute) { + return "/" + normalizedPath; } - if (normalizedPath.empty() || normalizedPath == "/") { - return "/"; + if (normalizedPath.empty()) { + return "."; } - return slashCheck ? "/" + normalizedPath + "/" : "/" + normalizedPath; + return normalizedPath; } #endif // _WIN32 diff --git a/test/cctest/test_path.cc b/test/cctest/test_path.cc index 4e25ffd1102..16bd9872f3b 100644 --- a/test/cctest/test_path.cc +++ b/test/cctest/test_path.cc @@ -25,28 +25,26 @@ 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 8d19a8730cf..dfc851090b2 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 0c332a32b68..7a4783953c8 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 7a3d02cb7fe..e1d3b9ce1e6 100644 --- a/test/parallel/test-path-normalize.js +++ b/test/parallel/test-path-normalize.js @@ -40,8 +40,6 @@ 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 52eb7737155..3fc9b2e3abd 100644 --- a/test/parallel/test-path-resolve.js +++ b/test/parallel/test-path-resolve.js @@ -23,27 +23,25 @@ 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'], ], ],