From 50bec782e114112fa56c60fba2684c8b13bf5189 Mon Sep 17 00:00:00 2001 From: Phail' Sharkaev Date: Mon, 7 Sep 2026 10:07:40 +0500 Subject: [PATCH 1/2] src: optimize NormalizeString to cut allocations and copies Rewrite the parent-dir ("..") handling in NormalizeString() to use a write-position rewind with a "dotdot" floor index, the approach used by Go's path/filepath.Clean, and build segments with in-place appends instead of temporary-string concatenation. Previously each ".." did `res = res.substr(0, idx)`, which allocated a new string and copied the whole surviving prefix (O(n^2) copy volume on deep-backtrack paths), plus two find_last_of() scans. Each normal segment built two or three temporary std::strings, and res was never reserve()d. Now ".." rewinds the write position to the previous separator and resize()s, so there is no allocation, no prefix copy and no find_last_of call. Segments are written with push_back() and append(). The output is byte for byte identical to the previous implementation. Signed-off-by: Phail' Sharkaev --- src/path.cc | 63 +++++++++++++++++++--------------------- test/cctest/test_path.cc | 41 ++++++++++++++++++++++++++ 2 files changed, 71 insertions(+), 33 deletions(-) diff --git a/src/path.cc b/src/path.cc index f4b8d4577bd1..1bb93d6180b1 100644 --- a/src/path.cc +++ b/src/path.cc @@ -21,8 +21,15 @@ constexpr bool IsPathSeparator(const char c) noexcept { std::string NormalizeString(const std::string_view path, bool allowAboveRoot, const std::string_view separator) { + const char separator_char = separator[0]; std::string res; - int lastSegmentLength = 0; + res.reserve(path.size()); + // `dotdot` is the floor in `res` below which a `..` segment cannot + // backtrack: it sits just past any leading `..` run that could not be + // resolved. This lets `..` rewind the write position to the preceding + // separator without rescanning or reallocating the whole prefix. Same + // approach as Go's path/filepath.Clean. + int dotdot = 0; int lastSlash = -1; int dots = 0; char code = 0; @@ -37,45 +44,35 @@ std::string NormalizeString(const std::string_view path, if (IsPathSeparator(code)) { if (lastSlash == static_cast(i - 1) || dots == 1) { - // NOOP + // NOOP: empty segment (e.g. `//`) or a `.` segment. } else if (dots == 2) { - int len = res.length(); - if (len < 2 || lastSegmentLength != 2 || res[len - 1] != '.' || - res[len - 2] != '.') { - if (len > 2) { - auto lastSlashIndex = res.find_last_of(separator); - if (lastSlashIndex == std::string::npos) { - res = ""; - lastSegmentLength = 0; - } else { - res = res.substr(0, lastSlashIndex); - len = res.length(); - lastSegmentLength = len - 1 - res.find_last_of(separator); - } - lastSlash = i; - dots = 0; - continue; - } else if (len != 0) { - res = ""; - lastSegmentLength = 0; - lastSlash = i; - dots = 0; - continue; + int w = static_cast(res.length()); + if (w > dotdot) { + // Drop the previous segment by rewinding the write position to the + // separator that precedes it. + w--; + while (w > dotdot && res[w] != separator_char) { + w--; } - } - - if (allowAboveRoot) { - res += res.length() > 0 ? std::string(separator) + ".." : ".."; - lastSegmentLength = 2; + res.resize(static_cast(w)); + lastSlash = i; + dots = 0; + continue; + } else if (allowAboveRoot) { + // Cannot backtrack past the floor; keep the `..` and raise the floor. + if (!res.empty()) { + res.push_back(separator_char); + } + res.append("..", 2); + dotdot = static_cast(res.length()); } } else { if (!res.empty()) { - res += std::string(separator) + - std::string(path.substr(lastSlash + 1, i - (lastSlash + 1))); + res.push_back(separator_char); + res.append(path.data() + lastSlash + 1, i - (lastSlash + 1)); } else { - res = path.substr(lastSlash + 1, i - (lastSlash + 1)); + res.assign(path.data() + lastSlash + 1, i - (lastSlash + 1)); } - lastSegmentLength = i - lastSlash - 1; } lastSlash = i; dots = 0; diff --git a/test/cctest/test_path.cc b/test/cctest/test_path.cc index 1fd991340452..289699243795 100644 --- a/test/cctest/test_path.cc +++ b/test/cctest/test_path.cc @@ -9,6 +9,7 @@ using node::BufferValue; using node::NormalizeFileURLOrPath; +using node::NormalizeString; using node::PathResolve; using node::ToNamespacedPath; @@ -44,6 +45,13 @@ TEST_F(PathTest, PathResolve) { "\\\\.\\PHYSICALDRIVE0"); EXPECT_EQ(PathResolve(*env, {"\\\\?\\PHYSICALDRIVE0"}), "\\\\?\\PHYSICALDRIVE0"); + // Backtracking past the drive root stays clamped at the drive root. + EXPECT_EQ(PathResolve(*env, {"c:/a/b/c", "..\\..\\..\\.."}), "c:\\"); + // UNC root is preserved when backtracking past it. The UNC share + // \\server\share is the root, so "..","..","x" cannot escape it and the + // remaining segment "x" is appended to the share root. + EXPECT_EQ(PathResolve(*env, {"//server/share", "..", "..", "x"}), + "\\\\server\\share\\x"); #else EXPECT_EQ(PathResolve(*env, {"/var/lib", "../", "file/"}), "/var/file"); EXPECT_EQ(PathResolve(*env, {"/var/lib", "/../", "file/"}), "/file"); @@ -52,9 +60,42 @@ TEST_F(PathTest, PathResolve) { 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"); + // Backtracking past the root stays clamped at the root. + EXPECT_EQ(PathResolve(*env, {"/a/b/c/d/e", "../../../../.."}), "/"); + EXPECT_EQ(PathResolve(*env, {"/a/b/c", "../../../../.."}), "/"); + // Mixed current-dir and parent-dir segments. + EXPECT_EQ(PathResolve(*env, {"/a/./b/../c/./d"}), "/a/c/d"); + // Collapsing of repeated separators. + EXPECT_EQ(PathResolve(*env, {"/a//b///c"}), "/a/b/c"); + // Single parent-dir traversal. + EXPECT_EQ(PathResolve(*env, {"/a/../b"}), "/b"); + EXPECT_EQ(PathResolve(*env, {"/a/b/../../c"}), "/c"); + // Trailing separator is stripped. + EXPECT_EQ(PathResolve(*env, {"/a/b/c/"}), "/a/b/c"); + // Single absolute segment. + EXPECT_EQ(PathResolve(*env, {"/single"}), "/single"); #endif } +TEST_F(PathTest, NormalizeString) { + // allowAboveRoot = false (absolute context): ".." that cannot be resolved is + // dropped, "." segments and repeated/trailing separators are collapsed. + EXPECT_EQ(NormalizeString("a/b/../../../c", false, "/"), "c"); + EXPECT_EQ(NormalizeString("a/b/c/d/e/../../../../..", false, "/"), ""); + EXPECT_EQ(NormalizeString("a/./b//c/", false, "/"), "a/b/c"); + EXPECT_EQ(NormalizeString("./foo/./bar/", false, "/"), "foo/bar"); + // allowAboveRoot = true (relative context): leading ".." is preserved. + EXPECT_EQ(NormalizeString("a/b/../../../c", true, "/"), "../c"); + EXPECT_EQ(NormalizeString("../../a", true, "/"), "../../a"); + EXPECT_EQ(NormalizeString("foo/..", true, "/"), ""); + EXPECT_EQ(NormalizeString("foo/../..", true, "/"), ".."); +#ifdef _WIN32 + // The Windows separator is handled the same way. + EXPECT_EQ(NormalizeString("a\\b\\..\\..\\..\\c", false, "\\"), "c"); + EXPECT_EQ(NormalizeString("..\\..\\a", true, "\\"), "..\\..\\a"); +#endif // _WIN32 +} + TEST_F(PathTest, ToNamespacedPath) { const v8::HandleScope handle_scope(isolate_); Argv argv; From 6a2778da21a420e5b89bdc1f73ab950466e8321a Mon Sep 17 00:00:00 2001 From: Phail' Sharkaev Date: Mon, 7 Sep 2026 10:07:40 +0500 Subject: [PATCH 2/2] benchmark: add permission fs.read path shape benchmark process.permission.has() resolves the reference natively through PathResolve() and NormalizeString() in src/path.cc before the granted tree lookup, and string references are passed through from JS unchanged, so it is a JS-reachable entry point for the native path normalization on all platforms. The new benchmark feeds it paths of different shapes: already normalized, `.` segments with repeated separators, a few `..` segments, many segments without backtracking, and a deep descent followed by an equally deep backtrack. This makes changes to NormalizeString() measurable with benchmark/compare.js. Signed-off-by: Phail' Sharkaev --- .../permission-processhas-fs-read-paths.js | 63 +++++++++++++++++++ 1 file changed, 63 insertions(+) create mode 100644 benchmark/permission/permission-processhas-fs-read-paths.js diff --git a/benchmark/permission/permission-processhas-fs-read-paths.js b/benchmark/permission/permission-processhas-fs-read-paths.js new file mode 100644 index 000000000000..b84c683280c0 --- /dev/null +++ b/benchmark/permission/permission-processhas-fs-read-paths.js @@ -0,0 +1,63 @@ +'use strict'; +const common = require('../common.js'); +const path = require('path'); + +// process.permission.has() resolves the reference path natively (PathResolve +// -> NormalizeString in src/path.cc) before the granted-tree lookup, and string +// references are passed through from JS unchanged. This benchmark feeds it +// paths of different shapes so that the native normalization cost is visible. +const configs = { + n: [1e5], + pathType: [ + 'normalized', + 'dot-segments', + 'dotdot', + 'deep', + 'dotdot-heavy', + ], +}; + +const rootPath = path.resolve(__dirname, '../../..'); + +const options = { + flags: [ + '--permission', + `--allow-fs-read=${rootPath}`, + '--allow-child-process', + '--no-warnings', + ], +}; + +const bench = common.createBenchmark(main, configs, options); + +function makePath(pathType) { + switch (pathType) { + case 'normalized': + // Already-normalized absolute path (the common case). + return `${rootPath}/benchmark/permission/valid-file`; + case 'dot-segments': + // `.` segments and repeated separators that are collapsed. + return `${rootPath}/./benchmark//./permission/./valid-file`; + case 'dotdot': + // A few `..` segments backtracking over the previous segments. + return `${rootPath}/a/b/c/d/e/../../../../../valid-file`; + case 'deep': + // Many segments, no backtracking. + return `${rootPath}${'/segment'.repeat(200)}/valid-file`; + case 'dotdot-heavy': + // Deep descent followed by an equally deep backtrack. + return `${rootPath}${'/a'.repeat(50)}${'/..'.repeat(50)}/valid-file`; + default: + throw new Error(`Unknown pathType: ${pathType}`); + } +} + +function main({ n, pathType }) { + const reference = makePath(pathType); + + bench.start(); + for (let i = 0; i < n; i++) { + process.permission.has('fs.read', reference); + } + bench.end(n); +}