From 6ff73c6f07ac211a225a8573806c6451993316e8 Mon Sep 17 00:00:00 2001 From: ChrisChan0668 <192564531+ChrisChan0668@users.noreply.github.com> Date: Thu, 18 Jun 2026 16:25:54 +0800 Subject: [PATCH] fs: handle cpSync copyDir filesystem errors The native fs.cpSync() copyDir fast path is used for recursive copies when no filter option is provided. Some std::filesystem calls in that path used throwing overloads instead of overloads that report failures through std::error_code. When directory iteration, path canonicalization, or file type checks failed, those C++ exceptions could bypass Node's normal error conversion. Instead of reporting a JavaScript exception that callers can catch, the process could terminate in the native layer. Use non-throwing std::filesystem overloads throughout the copyDir implementation and convert each failure with ThrowStdErrException. Also add a Windows regression test that denies access to a copied directory and verifies cpSync throws in the child process instead of aborting. Fixes: https://github.com/nodejs/node/issues/63970 Signed-off-by: ChrisChan0668 <192564531+ChrisChan0668@users.noreply.github.com> --- src/node_file.cc | 234 +++++++++++++----- ...test-fs-cp-sync-inaccessible-dir-error.mjs | 75 ++++++ 2 files changed, 252 insertions(+), 57 deletions(-) create mode 100644 test/parallel/test-fs-cp-sync-inaccessible-dir-error.mjs diff --git a/src/node_file.cc b/src/node_file.cc index d93f213202ec43..d99a213f5e849d 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -3677,9 +3677,11 @@ static void CpSyncOverrideFile(const FunctionCallbackInfo& args) { } std::vector normalizePathToArray( - const std::filesystem::path& path) { + const std::filesystem::path& path, std::error_code& error) { std::vector parts; - std::filesystem::path absPath = std::filesystem::absolute(path); + std::filesystem::path absPath = std::filesystem::absolute(path, error); + if (error) return parts; + for (const auto& part : absPath) { if (!part.empty()) parts.push_back(part.string()); } @@ -3687,9 +3689,14 @@ std::vector normalizePathToArray( } bool isInsideDir(const std::filesystem::path& src, - const std::filesystem::path& dest) { - auto srcArr = normalizePathToArray(src); - auto destArr = normalizePathToArray(dest); + const std::filesystem::path& dest, + std::error_code& error) { + auto srcArr = normalizePathToArray(src, error); + if (error) return false; + + auto destArr = normalizePathToArray(dest, error); + if (error) return false; + if (srcArr.size() > destArr.size()) return false; return std::equal(srcArr.begin(), srcArr.end(), destArr.begin()); } @@ -3744,13 +3751,31 @@ static void CpSyncCopyDir(const FunctionCallbackInfo& args) { error_on_exist, dereference, &isolate](std::filesystem::path src, - std::filesystem::path dest) { + std::filesystem::path dest) { + auto throw_fs_error = [&env](const std::error_code& error, + const std::filesystem::path& path) { + auto path_str = ConvertPathToUTF8(path); + env->ThrowStdErrException(error, "cp", path_str.c_str()); + return false; + }; + std::error_code error; - for (auto dir_entry : std::filesystem::directory_iterator(src)) { + std::filesystem::directory_iterator dir_iter(src, error); + if (error) { + return throw_fs_error(error, src); + } + + for (std::filesystem::directory_iterator end; dir_iter != end;) { + const auto& dir_entry = *dir_iter; auto dest_file_path = dest / dir_entry.path().filename(); auto dest_str = ConvertPathToUTF8(dest); - if (dir_entry.is_symlink()) { + bool entry_is_symlink = dir_entry.is_symlink(error); + if (error) { + return throw_fs_error(error, dir_entry.path()); + } + + if (entry_is_symlink) { if (verbatim_symlinks) { std::filesystem::copy_symlink( dir_entry.path(), dest_file_path, error); @@ -3766,8 +3791,19 @@ static void CpSyncCopyDir(const FunctionCallbackInfo& args) { return false; } - if (std::filesystem::exists(dest_file_path)) { - if (std::filesystem::is_symlink((dest_file_path.c_str()))) { + bool dest_exists = std::filesystem::exists(dest_file_path, error); + if (error) { + return throw_fs_error(error, dest_file_path); + } + + if (dest_exists) { + bool dest_is_symlink = + std::filesystem::is_symlink(dest_file_path, error); + if (error) { + return throw_fs_error(error, dest_file_path); + } + + if (dest_is_symlink) { auto current_dest_symlink_target = std::filesystem::read_symlink(dest_file_path.c_str(), error); if (error) { @@ -3775,26 +3811,63 @@ static void CpSyncCopyDir(const FunctionCallbackInfo& args) { return false; } - if (!dereference && - std::filesystem::is_directory(symlink_target) && - isInsideDir(symlink_target, current_dest_symlink_target)) { - static constexpr const char* message = - "Cannot copy %s to a subdirectory of self %s"; - THROW_ERR_FS_CP_EINVAL( - env, message, symlink_target, current_dest_symlink_target); - return false; + if (!dereference) { + bool symlink_target_is_directory = + std::filesystem::is_directory(symlink_target, error); + if (error) { + return throw_fs_error(error, symlink_target); + } + + if (symlink_target_is_directory) { + bool symlink_target_is_inside_dest = + isInsideDir(symlink_target, + current_dest_symlink_target, + error); + if (error) { + return throw_fs_error(error, symlink_target); + } + + if (symlink_target_is_inside_dest) { + static constexpr const char* message = + "Cannot copy %s to a subdirectory of self %s"; + THROW_ERR_FS_CP_EINVAL(env, + message, + symlink_target, + current_dest_symlink_target); + return false; + } + } } // Prevent copy if src is a subdir of dest since unlinking // dest in this case would result in removing src contents // and therefore a broken symlink would be created. - if (std::filesystem::is_directory(dest_file_path) && - isInsideDir(current_dest_symlink_target, symlink_target)) { - static constexpr const char* message = - "cannot overwrite %s with %s"; - THROW_ERR_FS_CP_SYMLINK_TO_SUBDIRECTORY( - env, message, current_dest_symlink_target, symlink_target); - return false; + bool dest_is_directory = + std::filesystem::is_directory(dest_file_path, error); + if (error) { + return throw_fs_error(error, dest_file_path); + } + + if (dest_is_directory) { + bool dest_is_inside_symlink_target = + isInsideDir(current_dest_symlink_target, + symlink_target, + error); + if (error) { + return throw_fs_error(error, current_dest_symlink_target); + } + + if (dest_is_inside_symlink_target) { + static constexpr const char* message = + "cannot overwrite %s with %s"; + const auto& dest_symlink_target = + current_dest_symlink_target; + THROW_ERR_FS_CP_SYMLINK_TO_SUBDIRECTORY(env, + message, + dest_symlink_target, + symlink_target); + return false; + } } // symlinks get overridden by cp even if force: false, this is @@ -3805,20 +3878,43 @@ static void CpSyncCopyDir(const FunctionCallbackInfo& args) { env->ThrowStdErrException(error, "cp", dest_str.c_str()); return false; } - } else if (std::filesystem::is_regular_file(dest_file_path)) { - if (!dereference || (!force && error_on_exist)) { - auto dest_file_path_str = ConvertPathToUTF8(dest_file_path); - env->ThrowStdErrException( - std::make_error_code(std::errc::file_exists), - "cp", - dest_file_path_str.c_str()); - return false; + } else { + bool dest_is_regular_file = + std::filesystem::is_regular_file(dest_file_path, error); + if (error) { + return throw_fs_error(error, dest_file_path); + } + + if (dest_is_regular_file) { + if (!dereference || (!force && error_on_exist)) { + auto dest_file_path_str = ConvertPathToUTF8(dest_file_path); + env->ThrowStdErrException( + std::make_error_code(std::errc::file_exists), + "cp", + dest_file_path_str.c_str()); + return false; + } } } } - auto symlink_target_absolute = std::filesystem::weakly_canonical( - std::filesystem::absolute(src / symlink_target)); - if (dir_entry.is_directory()) { + auto absolute_symlink_target = + std::filesystem::absolute(src / symlink_target, error); + if (error) { + return throw_fs_error(error, src / symlink_target); + } + + auto symlink_target_absolute = + std::filesystem::weakly_canonical(absolute_symlink_target, error); + if (error) { + return throw_fs_error(error, absolute_symlink_target); + } + + bool entry_is_directory = dir_entry.is_directory(error); + if (error) { + return throw_fs_error(error, dir_entry.path()); + } + + if (entry_is_directory) { std::filesystem::create_directory_symlink( symlink_target_absolute, dest_file_path, error); } else { @@ -3830,33 +3926,57 @@ static void CpSyncCopyDir(const FunctionCallbackInfo& args) { return false; } } - } else if (dir_entry.is_directory()) { - auto entry_dir_path = src / dir_entry.path().filename(); - std::filesystem::create_directory(dest_file_path); - auto success = copy_dir_contents(entry_dir_path, dest_file_path); - if (!success) { - return false; - } - } else if (dir_entry.is_regular_file()) { - std::filesystem::copy_file( - dir_entry.path(), dest_file_path, file_copy_opts, error); + } else { + bool entry_is_directory = dir_entry.is_directory(error); if (error) { - if (error.value() == EEXIST) { - THROW_ERR_FS_CP_EEXIST(isolate, - "[ERR_FS_CP_EEXIST]: Target already exists: " - "cp returned EEXIST (%s already exists)", - dest_file_path); + return throw_fs_error(error, dir_entry.path()); + } + + if (entry_is_directory) { + auto entry_dir_path = src / dir_entry.path().filename(); + std::filesystem::create_directory(dest_file_path, error); + if (error) { + return throw_fs_error(error, dest_file_path); + } + + auto success = copy_dir_contents(entry_dir_path, dest_file_path); + if (!success) { return false; } - env->ThrowStdErrException(error, "cp", dest_str.c_str()); - return false; - } + } else { + bool entry_is_regular_file = dir_entry.is_regular_file(error); + if (error) { + return throw_fs_error(error, dir_entry.path()); + } + + if (entry_is_regular_file) { + std::filesystem::copy_file( + dir_entry.path(), dest_file_path, file_copy_opts, error); + if (error) { + if (error.value() == EEXIST) { + THROW_ERR_FS_CP_EEXIST( + isolate, + "[ERR_FS_CP_EEXIST]: Target already exists: " + "cp returned EEXIST (%s already exists)", + dest_file_path); + return false; + } + env->ThrowStdErrException(error, "cp", dest_str.c_str()); + return false; + } - if (preserve_timestamps && - !CopyUtimes(dir_entry.path(), dest_file_path, env)) { - return false; + if (preserve_timestamps && + !CopyUtimes(dir_entry.path(), dest_file_path, env)) { + return false; + } + } } } + + dir_iter.increment(error); + if (error) { + return throw_fs_error(error, src); + } } return true; }; diff --git a/test/parallel/test-fs-cp-sync-inaccessible-dir-error.mjs b/test/parallel/test-fs-cp-sync-inaccessible-dir-error.mjs new file mode 100644 index 00000000000000..d2aa12c3844d02 --- /dev/null +++ b/test/parallel/test-fs-cp-sync-inaccessible-dir-error.mjs @@ -0,0 +1,75 @@ +// This tests that cpSync converts native directory enumeration failures into +// JavaScript exceptions. +import * as common from '../common/index.mjs'; +import { nextdir } from '../common/fs.js'; +import assert from 'node:assert'; +import { execFileSync, spawnSync } from 'node:child_process'; +import { cpSync, existsSync, mkdirSync, rmSync, writeFileSync } from 'node:fs'; +import { join } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import tmpdir from '../common/tmpdir.js'; + +if (!common.isWindows) { + common.skip('Windows ACL specific test'); +} + +function run(command, args) { + return execFileSync(command, args, { + encoding: 'utf8', + stdio: ['ignore', 'pipe', 'pipe'], + }); +} + +function currentWindowsUser() { + return run('whoami', []).trim(); +} + +function restrictDirectory(dir) { + const user = currentWindowsUser(); + run('icacls', [dir, '/deny', `${user}:(OI)(CI)(RX)`]); +} + +function restoreDirectory(dir) { + if (!existsSync(dir)) return; + + const user = currentWindowsUser(); + run('icacls', [dir, '/remove:d', user]); +} + +if (process.argv[2] === 'child') { + const src = process.argv[3]; + const dest = process.argv[4]; + + assert.throws( + () => cpSync(src, dest, { recursive: true }), + { name: 'Error' } + ); + process.exit(0); +} + +tmpdir.refresh(); + +const root = nextdir(); +const src = join(root, 'src'); +const dest = join(root, 'dest'); +const restrictedDir = join(src, 'restricted'); +mkdirSync(restrictedDir, { recursive: true }); +writeFileSync(join(src, 'readable.txt'), 'readable\n'); +writeFileSync(join(restrictedDir, 'blocked.txt'), 'blocked\n'); + +restrictDirectory(restrictedDir); + +try { + const filename = fileURLToPath(import.meta.url); + const child = spawnSync(process.execPath, + [filename, 'child', src, dest], + { encoding: 'utf8' }); + assert.strictEqual( + child.status, + 0, + `child exited with ${child.status}\nstdout:\n${child.stdout}\nstderr:\n${child.stderr}` + ); +} finally { + restoreDirectory(restrictedDir); + rmSync(root, { recursive: true, force: true }); +}