fs: fix repeated copy of directory with symlinks

lib/internal/fs/cp/cp-sync.js

@@ -53,6 +53,7 @@ function cpSyncFn(src, dest, opts) {
     if (!shouldCopy) return;
   }
 
+  opts = { ...opts, destRoot: dest };
   fsBinding.cpSyncCheckPaths(src, dest, opts.dereference, opts.recursive);
 
   return getStats(src, dest, opts);
@@ -71,7 +72,7 @@ function getStats(src, dest, opts) {
            srcStat.isBlockDevice()) {
     return onFile(srcStat, destStat, src, dest, opts);
   } else if (srcStat.isSymbolicLink()) {
-    return onLink(destStat, src, dest, opts.verbatimSymlinks);
+    return onLink(destStat, src, dest, opts.verbatimSymlinks, opts.destRoot);
   }
 
   // It is not possible to get here because all possible cases are handled above.
@@ -186,7 +187,7 @@ function copyDir(src, dest, opts, mkDir, srcMode) {
 }
 
 // TODO(@anonrig): Move this function to C++.
-function onLink(destStat, src, dest, verbatimSymlinks) {
+function onLink(destStat, src, dest, verbatimSymlinks, destRoot) {
   let resolvedSrc = readlinkSync(src);
   if (!verbatimSymlinks && !isAbsolute(resolvedSrc)) {
     resolvedSrc = resolve(dirname(src), resolvedSrc);
@@ -211,7 +212,13 @@ function onLink(destStat, src, dest, verbatimSymlinks) {
   }
   const srcIsDir = fsBinding.internalModuleStat(src) === 1;
 
-  if (srcIsDir && isSrcSubdir(resolvedSrc, resolvedDest)) {
+  // A symlink that resolves to the same target as the destination symlink
+  // is not a self-copy, unless the target is the destination directory
+  // itself or one of its ancestors.
+  const sameTarget = resolvedSrc === resolvedDest &&
+    !isSrcSubdir(resolvedSrc, resolve(destRoot));
+
+  if (srcIsDir && isSrcSubdir(resolvedSrc, resolvedDest) && !sameTarget) {
     throw new ERR_FS_CP_EINVAL({
       message: `cannot copy ${resolvedSrc} to a subdirectory of self ` +
           `${resolvedDest}`,
@@ -224,7 +231,8 @@ function onLink(destStat, src, dest, verbatimSymlinks) {
   // 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 (statSync(dest).isDirectory() && isSrcSubdir(resolvedDest, resolvedSrc)) {
+  if (statSync(dest).isDirectory() && isSrcSubdir(resolvedDest, resolvedSrc) &&
+      !sameTarget) {
     throw new ERR_FS_CP_SYMLINK_TO_SUBDIRECTORY({
       message: `cannot overwrite ${resolvedDest} with ${resolvedSrc}`,
       path: dest,

lib/internal/fs/cp/cp.js

@@ -63,6 +63,7 @@ async function cpFn(src, dest, opts) {
       'node is not recommended';
     process.emitWarning(warning, 'TimestampPrecisionWarning');
   }
+  opts = { ...opts, destRoot: dest };
   const stats = await checkPaths(src, dest, opts);
   const { srcStat, destStat, skipped } = stats;
   if (skipped) return;
@@ -357,7 +358,13 @@ async function onLink(destStat, src, dest, opts) {
 
   const srcIsDir = fsBinding.internalModuleStat(src) === 1;
 
-  if (srcIsDir && isSrcSubdir(resolvedSrc, resolvedDest)) {
+  // A symlink that resolves to the same target as the destination symlink
+  // is not a self-copy, unless the target is the destination directory
+  // itself or one of its ancestors.
+  const sameTarget = resolvedSrc === resolvedDest &&
+    !isSrcSubdir(resolvedSrc, resolve(opts.destRoot));
+
+  if (srcIsDir && isSrcSubdir(resolvedSrc, resolvedDest) && !sameTarget) {
     throw new ERR_FS_CP_EINVAL({
       message: `cannot copy ${resolvedSrc} to a subdirectory of self ` +
             `${resolvedDest}`,
@@ -371,7 +378,8 @@ async function onLink(destStat, src, dest, opts) {
   // dest in this case would result in removing src contents
   // and therefore a broken symlink would be created.
   const srcStat = await stat(src);
-  if (srcStat.isDirectory() && isSrcSubdir(resolvedDest, resolvedSrc)) {
+  if (srcStat.isDirectory() && isSrcSubdir(resolvedDest, resolvedSrc) &&
+      !sameTarget) {
     throw new ERR_FS_CP_SYMLINK_TO_SUBDIRECTORY({
       message: `cannot overwrite ${resolvedDest} with ${resolvedSrc}`,
       path: dest,

src/node_file.cc

@@ -3755,6 +3755,7 @@ static void CpSyncCopyDir(const FunctionCallbackInfo<Value>& args) {
                        force,
                        error_on_exist,
                        dereference,
+                       dest_path,
                        &isolate](std::filesystem::path src,
                                  std::filesystem::path dest) {
     std::error_code error;
@@ -3778,6 +3779,15 @@ static void CpSyncCopyDir(const FunctionCallbackInfo<Value>& args) {
             return false;
           }
 
+          auto symlink_target_absolute = std::filesystem::weakly_canonical(
+              std::filesystem::absolute(src / symlink_target));
+#ifdef _WIN32
+          auto wstr = symlink_target_absolute.wstring();
+          if (wstr.starts_with(L"\\\\?\\")) {
+            symlink_target_absolute = std::filesystem::path(wstr.substr(4));
+          }
+#endif
+
           if (std::filesystem::exists(dest_file_path)) {
             if (std::filesystem::is_symlink((dest_file_path.c_str()))) {
               auto current_dest_symlink_target =
@@ -3787,9 +3797,29 @@ static void CpSyncCopyDir(const FunctionCallbackInfo<Value>& args) {
                 return false;
               }
 
+              // A symlink that resolves to the same target as the destination
+              // symlink is not a self-copy, unless the target is the
+              // destination directory itself or one of its ancestors.
+              auto current_dest_symlink_target_absolute =
+                  std::filesystem::weakly_canonical(
+                      std::filesystem::absolute(dest_file_path.parent_path() /
+                                                current_dest_symlink_target));
+#ifdef _WIN32
+              auto wstr2 = current_dest_symlink_target_absolute.wstring();
+              if (wstr2.starts_with(L"\\\\?\\")) {
+                current_dest_symlink_target_absolute =
+                    std::filesystem::path(wstr2.substr(4));
+              }
+#endif
+              bool same_target =
+                  symlink_target_absolute ==
+                      current_dest_symlink_target_absolute &&
+                  !isInsideDir(symlink_target_absolute, dest_path);
+
               if (!dereference &&
                   std::filesystem::is_directory(symlink_target) &&
-                  isInsideDir(symlink_target, current_dest_symlink_target)) {
+                  isInsideDir(symlink_target, current_dest_symlink_target) &&
+                  !same_target) {
                 static constexpr const char* message =
                     "Cannot copy %s to a subdirectory of self %s";
                 THROW_ERR_FS_CP_EINVAL(
@@ -3801,7 +3831,8 @@ static void CpSyncCopyDir(const FunctionCallbackInfo<Value>& args) {
               // 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)) {
+                  isInsideDir(current_dest_symlink_target, symlink_target) &&
+                  !same_target) {
                 static constexpr const char* message =
                     "cannot overwrite %s with %s";
                 THROW_ERR_FS_CP_SYMLINK_TO_SUBDIRECTORY(
@@ -3828,14 +3859,6 @@ static void CpSyncCopyDir(const FunctionCallbackInfo<Value>& args) {
               }
             }
           }
-          auto symlink_target_absolute = std::filesystem::weakly_canonical(
-              std::filesystem::absolute(src / symlink_target));
-#ifdef _WIN32
-          auto wstr = symlink_target_absolute.wstring();
-          if (wstr.starts_with(L"\\\\?\\")) {
-            symlink_target_absolute = std::filesystem::path(wstr.substr(4));
-          }
-#endif
           if (dir_entry.is_directory()) {
             std::filesystem::create_directory_symlink(
                 symlink_target_absolute, dest_file_path, error);

test/parallel/test-fs-cp-async-symlink-repeated-copy.mjs

@@ -0,0 +1,44 @@
+// This tests that repeatedly copying a directory containing a symlink
+// to an unrelated directory succeeds.
+// See https://github.com/nodejs/node/issues/65097.
+import { mustCall, mustNotMutateObjectDeep } from '../common/index.mjs';
+import { nextdir } from '../common/fs.js';
+import assert from 'node:assert';
+import { cp, mkdirSync, realpathSync, symlinkSync } from 'node:fs';
+import { join } from 'node:path';
+
+import tmpdir from '../common/tmpdir.js';
+tmpdir.refresh();
+
+const root = nextdir();
+const src = join(root, 'src');
+const dest = join(root, 'dest');
+const target = join(root, 'target');
+mkdirSync(src, mustNotMutateObjectDeep({ recursive: true }));
+mkdirSync(target);
+symlinkSync(target, join(src, 'link'));
+cp(src, dest, mustNotMutateObjectDeep({ recursive: true }), mustCall((err) => {
+  assert.ifError(err);
+  cp(src, dest, mustNotMutateObjectDeep({ recursive: true }), mustCall((err) => {
+    assert.ifError(err);
+    assert.strictEqual(realpathSync(join(dest, 'link')), realpathSync(target));
+  }));
+}));
+
+// A symlink with a relative target pointing to an unrelated directory.
+{
+  const root = nextdir();
+  const src = join(root, 'src');
+  const dest = join(root, 'dest');
+  const target = join(root, 'target');
+  mkdirSync(src, mustNotMutateObjectDeep({ recursive: true }));
+  mkdirSync(target);
+  symlinkSync('../target', join(src, 'link'));
+  cp(src, dest, mustNotMutateObjectDeep({ recursive: true }), mustCall((err) => {
+    assert.ifError(err);
+    cp(src, dest, mustNotMutateObjectDeep({ recursive: true }), mustCall((err) => {
+      assert.ifError(err);
+      assert.strictEqual(realpathSync(join(dest, 'link')), realpathSync(target));
+    }));
+  }));
+}

test/parallel/test-fs-cp-sync-symlink-repeated-copy.mjs

@@ -0,0 +1,48 @@
+// This tests that repeatedly copying a directory containing a symlink
+// to an unrelated directory succeeds.
+// See https://github.com/nodejs/node/issues/65097.
+import { mustNotMutateObjectDeep } from '../common/index.mjs';
+import { nextdir } from '../common/fs.js';
+import assert from 'node:assert';
+import { cpSync, mkdirSync, realpathSync, symlinkSync } from 'node:fs';
+import { join } from 'node:path';
+
+import tmpdir from '../common/tmpdir.js';
+tmpdir.refresh();
+
+const root = nextdir();
+const src = join(root, 'src');
+const dest = join(root, 'dest');
+const target = join(root, 'target');
+mkdirSync(src, mustNotMutateObjectDeep({ recursive: true }));
+mkdirSync(target);
+symlinkSync(target, join(src, 'link'));
+cpSync(src, dest, mustNotMutateObjectDeep({ recursive: true }));
+cpSync(src, dest, mustNotMutateObjectDeep({ recursive: true }));
+assert.strictEqual(realpathSync(join(dest, 'link')), realpathSync(target));
+
+// Also exercise the JavaScript (filter) path. The destination symlink
+// already exists at this point, so this covers the repeated-copy case on
+// the filtered code path as well.
+cpSync(src, dest, mustNotMutateObjectDeep({ recursive: true, filter: () => true }));
+cpSync(src, dest, mustNotMutateObjectDeep({ recursive: true, filter: () => true }));
+assert.strictEqual(realpathSync(join(dest, 'link')), realpathSync(target));
+
+// A symlink with a relative target pointing to an unrelated directory.
+{
+  const root = nextdir();
+  const src = join(root, 'src');
+  const dest = join(root, 'dest');
+  const target = join(root, 'target');
+  mkdirSync(src, mustNotMutateObjectDeep({ recursive: true }));
+  mkdirSync(target);
+  symlinkSync('../target', join(src, 'link'));
+  cpSync(src, dest, mustNotMutateObjectDeep({ recursive: true }));
+  cpSync(src, dest, mustNotMutateObjectDeep({ recursive: true }));
+  assert.strictEqual(realpathSync(join(dest, 'link')), realpathSync(target));
+
+  // Same as above, exercising the JavaScript (filter) path.
+  cpSync(src, dest, mustNotMutateObjectDeep({ recursive: true, filter: () => true }));
+  cpSync(src, dest, mustNotMutateObjectDeep({ recursive: true, filter: () => true }));
+  assert.strictEqual(realpathSync(join(dest, 'link')), realpathSync(target));
+}