Skip to content

fs: fix cpSync directory iterator exception#62519

Closed
gushiaoke wants to merge 2 commits intonodejs:mainfrom
gushiaoke:fix-cpsync-directory-iterator-exception
Closed

fs: fix cpSync directory iterator exception#62519
gushiaoke wants to merge 2 commits intonodejs:mainfrom
gushiaoke:fix-cpsync-directory-iterator-exception

Conversation

@gushiaoke
Copy link
Copy Markdown

@gushiaoke gushiaoke commented Mar 31, 2026

Fix: fs.cpSync() fails with non-ASCII paths (process abort or operation failure)

πŸ› Problem

fs.cpSync() can cause process abort (not catchable by try-catch) when copying directories with certain non-ASCII characters in paths, especially on Windows with GBK encoding.

Root Cause

In src/node_file.cc line 3741, std::filesystem::directory_iterator is constructed without proper error handling for encoding issues:

for (auto dir_entry : std::filesystem::directory_iterator(src)) {

When the iterator construction fails due to encoding issues (e.g., libc++ bugs handling GBK-encoded paths), it throws std::filesystem::filesystem_error. In some environments (especially Electron with bundled libc++), this uncaught exception triggers __libcpp_verbose_abort(), causing immediate process termination.

Even if the exception is caught, the operation still fails - users cannot copy their files.

Impact

  • JavaScript try-catch blocks cannot catch this error (process abort)
  • Even when error handling is added, the operation fails - files still can't be copied
  • Affects Windows users with Chinese/Japanese/Korean characters in installation paths
  • Electron apps are particularly vulnerable
  • Inconsistent with other Node.js fs operations that use libuv

Real-World Example

// This should work, but instead causes process abort OR throws error:
try {
  fs.cpSync('C:\\η”¨ζˆ·\\AppData\\src', 'C:\\η”¨ζˆ·\\AppData\\dest', { recursive: true });
} catch (err) {
  console.log('Caught error:', err); 
  // On Electron: never reached - process aborts instead
  // On other envs: error thrown, but files still not copied
}

βœ… Solution

Replace std::filesystem::directory_iterator with libuv's uv_fs_scandir and uv_fs_scandir_next, which:

  1. Properly handle path encoding (including GBK on Windows)
  2. Return errors instead of throwing exceptions that can trigger abort()
  3. Are consistent with other Node.js fs operations (e.g., fs.readdirSync)
  4. Actually work - files get copied successfully

Before (Broken)

// ❌ Can throw uncatchable exception β†’ process abort
// ❌ Even if caught, operation fails
for (auto dir_entry : std::filesystem::directory_iterator(src)) {

After (Fixed)

// βœ… Uses libuv like other fs operations
// βœ… Errors are catchable as JS exceptions
// βœ… Actually works with non-ASCII paths
uv_fs_t scandir_req;
int rc = uv_fs_scandir(nullptr, &scandir_req, src_str.c_str(), 0, nullptr);
if (rc < 0) {
  env->ThrowUVException(rc, "scandir", src_str.c_str());
  uv_fs_req_cleanup(&scandir_req);
  return false;
}

uv_dirent_t ent;
while ((rc = uv_fs_scandir_next(&scandir_req, &ent)) != UV_EOF) {
  // Process entries - actually works!
}
uv_fs_req_cleanup(&scandir_req);

πŸ“ Changes

  1. src/node_file.cc: Replace std::filesystem::directory_iterator with uv_fs_scandir

    • Use uv_fs_lstat to get entry type instead of dir_entry.is_*()
    • Add proper cleanup of uv_fs_t requests on all error paths
    • Convert libuv errors to JavaScript exceptions via env->ThrowUVException
  2. test/parallel/test-fs-cpsync-error-handling.js: Add test cases for error handling

πŸ§ͺ Testing

# Run the new test
./node test/parallel/test-fs-cpsync-error-handling.js

# Run existing cpSync tests to ensure no regression
./node test/parallel/test-fs-cp*.js

πŸ“š Related Issues

  • This issue was discovered in Electron applications: TAPD Bug
  • Root cause: libc++ bug in Electron + Node.js using wrong API
  • Similar pattern: fs.readdirSync already uses uv_fs_scandir and works correctly

🎯 Why libuv instead of just fixing the exception?

Option 1 (Previous approach): Just catch the exception

  • βœ… Prevents process abort
  • ❌ Operation still fails - users can't copy files
  • ❌ Still relying on buggy std::filesystem implementation

Option 2 (This PR): Use libuv like other fs operations

  • βœ… Prevents process abort
  • βœ… Actually works - files get copied successfully
  • βœ… Consistent with Node.js fs architecture
  • βœ… Better cross-platform compatibility

πŸ”— Checklist

  • Commit message follows Node.js commit guidelines
  • Tests included for the fix
  • Documentation not required (internal implementation fix, no API changes)
  • Changes are backwards compatible
  • Consistent with other fs operations (uses libuv like fs.readdirSync)

πŸ“Š Diff Summary

src/node_file.cc                                   | 77 ++++++++++++++++++++++++++++++++++++++++-----
test/parallel/test-fs-cpsync-error-handling.js     | 86 +++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 150 insertions(+), 13 deletions(-)

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Mar 31, 2026
Replace std::filesystem::directory_iterator with libuv's uv_fs_scandir
to avoid process abort when copying directories with non-ASCII paths.

Previously, std::filesystem::directory_iterator could throw uncatchable
exceptions when encountering encoding issues (e.g., GBK-encoded Chinese
paths on Windows). In environments with buggy libc++ implementations
(especially Electron), this triggers __libcpp_verbose_abort(), causing
immediate process termination that cannot be caught by JavaScript
try-catch blocks.

This patch uses libuv's uv_fs_scandir and uv_fs_scandir_next instead,
which properly handle path encoding and return errors that can be
converted to JavaScript exceptions. This approach is consistent with
other fs operations in Node.js (e.g., fs.readdirSync) and ensures:

1. No process abort on encoding errors
2. Errors are catchable by JavaScript
3. fs.cpSync() can successfully copy directories with non-ASCII paths
4. Consistent behavior across different C++ standard library implementations

The fix also adds proper cleanup of uv_fs_t requests on all error paths
to prevent resource leaks.
Add test to ensure fs.cpSync properly handles directory iteration errors
without causing process abort.
@gushiaoke gushiaoke force-pushed the fix-cpsync-directory-iterator-exception branch from bb4fdc4 to b2309bc Compare March 31, 2026 05:20
@gushiaoke
Copy link
Copy Markdown
Author

πŸ”„ Updated the fix

I've improved this PR based on a key insight: just catching the exception doesn't solve the problem - the operation still fails and users can't copy their files.

What changed:

Before (v1): Only added error handling for std::filesystem::directory_iterator

  • βœ… Prevents process abort
  • ❌ Still fails - cpSync() throws error and files aren't copied

Now (v2): Use libuv's uv_fs_scandir instead of std::filesystem

  • βœ… Prevents process abort
  • βœ… Actually works - files get copied successfully
  • βœ… Consistent with other Node.js fs operations (fs.readdirSync uses the same approach)

Why libuv?

Node.js already uses libuv for fs.readdirSync() and it correctly handles GBK-encoded paths on Windows. By using the same approach for cpSync(), we ensure:

  1. Correct encoding handling across all platforms
  2. Consistent behavior with other fs operations
  3. No dependency on potentially buggy libc++ implementations

This is a more complete fix that solves both the crash and the functionality issue.

@gushiaoke
Copy link
Copy Markdown
Author

Closing this PR. After further analysis, the core issue (CJK path encoding) was already fixed by #61950, and the remaining edge cases (permissions, symlinks, path length) don't warrant replacing std::filesystem with libuv for directory iteration.

@gushiaoke gushiaoke closed this Mar 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants