cp: Avoid following a destination symlink with -P

Previously, given 'cp -P a b', where 'a' and 'b' were both symlinks, cp
would end up replacing the target of 'b'.

Signed-off-by: Ryan Gonzalez <ryan.gonzalez@collabora.com>

src/uu/cp/src/cp.rs

@@ -1245,7 +1245,8 @@ fn handle_existing_dest(source: &Path, dest: &Path, options: &Options) -> CopyRe
 }
 
 /// Copy the a file from `source` to `dest`. `source` will be dereferenced if
-/// `options.dereference` is set to true. `dest` will always be dereferenced.
+/// `options.dereference` is set to true. `dest` will be dereferenced only if
+/// the source was not a symlink.
 ///
 /// Behavior when copying to existing files is contingent on the
 /// `options.overwrite` mode. If a file is skipped, the return type
@@ -1295,13 +1296,28 @@ fn copy_file(
     let context = context.as_str();
 
     // canonicalize dest and source so that later steps can work with the paths directly
-    let dest = canonicalize(dest, MissingHandling::Missing, ResolveMode::Physical).unwrap();
     let source = if options.dereference {
         canonicalize(source, MissingHandling::Missing, ResolveMode::Physical).unwrap()
     } else {
         source.to_owned()
     };
 
+    let source_is_symlink = fs::symlink_metadata(&source)
+        .context(context)?
+        .file_type()
+        .is_symlink();
+    let dest_already_exists_as_symlink = fs::symlink_metadata(&dest)
+        .map(|meta| meta.file_type().is_symlink())
+        .unwrap_or(false);
+
+    let dest = if !(source_is_symlink && dest_already_exists_as_symlink) {
+        canonicalize(dest, MissingHandling::Missing, ResolveMode::Physical).unwrap()
+    } else {
+        // Don't canonicalize a symlink copied over another symlink, because
+        // then we'll end up overwriting the destination's target.
+        dest.to_path_buf()
+    };
+
     let dest_permissions = if dest.exists() {
         dest.symlink_metadata().context(context)?.permissions()
     } else {
@@ -1339,7 +1355,14 @@ fn copy_file(
             fs::hard_link(&source, &dest).context(context)?;
         }
         CopyMode::Copy => {
-            copy_helper(&source, &dest, options, context, symlinked_files)?;
+            copy_helper(
+                &source,
+                &dest,
+                options,
+                context,
+                source_is_symlink,
+                symlinked_files,
+            )?;
         }
         CopyMode::SymLink => {
             symlink_file(&source, &dest, context, symlinked_files)?;
@@ -1355,10 +1378,24 @@ fn copy_file(
                 if src_time <= dest_time {
                     return Ok(());
                 } else {
-                    copy_helper(&source, &dest, options, context, symlinked_files)?;
+                    copy_helper(
+                        &source,
+                        &dest,
+                        options,
+                        context,
+                        source_is_symlink,
+                        symlinked_files,
+                    )?;
                 }
             } else {
-                copy_helper(&source, &dest, options, context, symlinked_files)?;
+                copy_helper(
+                    &source,
+                    &dest,
+                    options,
+                    context,
+                    source_is_symlink,
+                    symlinked_files,
+                )?;
             }
         }
         CopyMode::AttrOnly => {
@@ -1397,18 +1434,16 @@ fn copy_helper(
     dest: &Path,
     options: &Options,
     context: &str,
+    source_is_symlink: bool,
     symlinked_files: &mut HashSet<FileInformation>,
 ) -> CopyResult<()> {
     if options.parents {
         let parent = dest.parent().unwrap_or(dest);
         fs::create_dir_all(parent)?;
     }
 
-    let file_type = fs::symlink_metadata(&source)?.file_type();
-    let is_symlink = file_type.is_symlink();
-
     #[cfg(unix)]
-    let is_fifo = file_type.is_fifo();
+    let is_fifo = fs::symlink_metadata(&source)?.file_type().is_fifo();
     #[cfg(not(unix))]
     let is_fifo = false;
 
@@ -1420,7 +1455,7 @@ fn copy_helper(
     } else if is_fifo && options.recursive {
         #[cfg(unix)]
         copy_fifo(dest, options.overwrite)?;
-    } else if is_symlink {
+    } else if source_is_symlink {
         copy_link(source, dest, symlinked_files)?;
     } else if options.reflink_mode != ReflinkMode::Never {
         #[cfg(not(any(target_os = "linux", target_os = "macos")))]

tests/by-util/test_cp.rs

@@ -29,6 +29,7 @@ static TEST_EXISTING_FILE: &str = "existing_file.txt";
 static TEST_HELLO_WORLD_SOURCE: &str = "hello_world.txt";
 static TEST_HELLO_WORLD_SOURCE_SYMLINK: &str = "hello_world.txt.link";
 static TEST_HELLO_WORLD_DEST: &str = "copy_of_hello_world.txt";
+static TEST_HELLO_WORLD_DEST_SYMLINK: &str = "copy_of_hello_world.txt.link";
 static TEST_HOW_ARE_YOU_SOURCE: &str = "how_are_you.txt";
 static TEST_HOW_ARE_YOU_DEST: &str = "hello_dir/how_are_you.txt";
 static TEST_COPY_TO_FOLDER: &str = "hello_dir/";
@@ -688,6 +689,51 @@ fn test_cp_no_deref() {
     assert_eq!(at.read(path_to_check), "Hello, World!\n");
 }
 
+#[test]
+fn test_cp_no_deref_link_onto_link() {
+    let (at, mut ucmd) = at_and_ucmd!();
+
+    at.copy(TEST_HELLO_WORLD_SOURCE, TEST_HELLO_WORLD_DEST);
+
+    #[cfg(not(windows))]
+    let _r = fs::symlink(
+        TEST_HELLO_WORLD_SOURCE,
+        at.subdir.join(TEST_HELLO_WORLD_SOURCE_SYMLINK),
+    );
+    #[cfg(windows)]
+    let _r = symlink_file(
+        TEST_HELLO_WORLD_SOURCE,
+        at.subdir.join(TEST_HELLO_WORLD_SOURCE_SYMLINK),
+    );
+
+    #[cfg(not(windows))]
+    let _r = fs::symlink(
+        TEST_HELLO_WORLD_DEST,
+        at.subdir.join(TEST_HELLO_WORLD_DEST_SYMLINK),
+    );
+    #[cfg(windows)]
+    let _r = symlink_file(
+        TEST_HELLO_WORLD_DEST,
+        at.subdir.join(TEST_HELLO_WORLD_DEST_SYMLINK),
+    );
+
+    ucmd.arg("-P")
+        .arg(TEST_HELLO_WORLD_SOURCE_SYMLINK)
+        .arg(TEST_HELLO_WORLD_DEST_SYMLINK)
+        .succeeds();
+
+    // Ensure that the target of the destination was not modified.
+    assert!(!at
+        .symlink_metadata(TEST_HELLO_WORLD_DEST)
+        .file_type()
+        .is_symlink());
+    assert!(at
+        .symlink_metadata(TEST_HELLO_WORLD_DEST_SYMLINK)
+        .file_type()
+        .is_symlink());
+    assert_eq!(at.read(TEST_HELLO_WORLD_DEST_SYMLINK), "Hello, World!\n");
+}
+
 #[test]
 fn test_cp_strip_trailing_slashes() {
     let (at, mut ucmd) = at_and_ucmd!();