Merge pull request #4820 from sylvestre/thru-dangling-2

cp: fix cp -f f loop when loop is a symlink loop

src/uu/cp/src/cp.rs

@@ -38,7 +38,8 @@ use uucore::backup_control::{self, BackupMode};
 use uucore::display::Quotable;
 use uucore::error::{set_exit_code, UClapError, UError, UResult, UUsageError};
 use uucore::fs::{
-    canonicalize, paths_refer_to_same_file, FileInformation, MissingHandling, ResolveMode,
+    canonicalize, is_symlink_loop, paths_refer_to_same_file, FileInformation, MissingHandling,
+    ResolveMode,
 };
 use uucore::update_control::{self, UpdateMode};
 use uucore::{
@@ -1389,11 +1390,10 @@ fn handle_existing_dest(
             backup_dest(dest, &backup_path)?;
         }
     }
-
     match options.overwrite {
         // FIXME: print that the file was removed if --verbose is enabled
         OverwriteMode::Clobber(ClobberMode::Force) => {
-            if fs::metadata(dest)?.permissions().readonly() {
+            if is_symlink_loop(dest) || fs::metadata(dest)?.permissions().readonly() {
                 fs::remove_file(dest)?;
             }
         }
@@ -1501,6 +1501,7 @@ fn copy_file(
                 options.overwrite,
                 OverwriteMode::Clobber(ClobberMode::RemoveDestination)
             )
+            && !is_symlink_loop(dest)
             && std::env::var_os("POSIXLY_CORRECT").is_none()
         {
             return Err(Error::Error(format!(

src/uucore/Cargo.toml

@@ -54,6 +54,7 @@ nix = { workspace=true, features = ["fs", "uio", "zerocopy", "signal"] }
 [dev-dependencies]
 clap = { workspace=true }
 once_cell = { workspace=true }
+tempfile = { workspace=true }
 
 [target.'cfg(target_os = "windows")'.dependencies]
 winapi-util = { version= "0.1.5", optional=true }

src/uucore/src/lib/features/fs.rs

@@ -584,10 +584,45 @@ pub fn make_path_relative_to<P1: AsRef<Path>, P2: AsRef<Path>>(path: P1, to: P2)
     components.iter().collect()
 }
 
+/// Checks if there is a symlink loop in the given path.
+///
+/// A symlink loop is a chain of symlinks where the last symlink points back to one of the previous symlinks in the chain.
+///
+/// # Arguments
+///
+/// * `path` - A reference to a `Path` representing the starting path to check for symlink loops.
+///
+/// # Returns
+///
+/// * `bool` - Returns `true` if a symlink loop is detected, `false` otherwise.
+pub fn is_symlink_loop(path: &Path) -> bool {
+    let mut visited_symlinks = HashSet::new();
+    let mut current_path = path.to_path_buf();
+
+    while let (Ok(metadata), Ok(link)) = (
+        current_path.symlink_metadata(),
+        fs::read_link(&current_path),
+    ) {
+        if !metadata.file_type().is_symlink() {
+            return false;
+        }
+        if !visited_symlinks.insert(current_path.clone()) {
+            return true;
+        }
+        current_path = link;
+    }
+
+    false
+}
+
 #[cfg(test)]
 mod tests {
     // Note this useful idiom: importing names from outer (for mod tests) scope.
     use super::*;
+    #[cfg(unix)]
+    use std::os::unix;
+    #[cfg(unix)]
+    use tempfile::tempdir;
 
     struct NormalizePathTestCase<'a> {
         path: &'a str,
@@ -695,4 +730,41 @@ mod tests {
             display_permissions_unix(S_IFCHR | S_ISVTX as mode_t | 0o054, true)
         );
     }
+
+    #[cfg(unix)]
+    #[test]
+    fn test_is_symlink_loop_no_loop() {
+        let temp_dir = tempdir().unwrap();
+        let file_path = temp_dir.path().join("file.txt");
+        let symlink_path = temp_dir.path().join("symlink");
+
+        fs::write(&file_path, "test content").unwrap();
+        unix::fs::symlink(&file_path, &symlink_path).unwrap();
+
+        assert!(!is_symlink_loop(&symlink_path));
+    }
+
+    #[cfg(unix)]
+    #[test]
+    fn test_is_symlink_loop_direct_loop() {
+        let temp_dir = tempdir().unwrap();
+        let symlink_path = temp_dir.path().join("loop");
+
+        unix::fs::symlink(&symlink_path, &symlink_path).unwrap();
+
+        assert!(is_symlink_loop(&symlink_path));
+    }
+
+    #[cfg(unix)]
+    #[test]
+    fn test_is_symlink_loop_indirect_loop() {
+        let temp_dir = tempdir().unwrap();
+        let symlink1_path = temp_dir.path().join("symlink1");
+        let symlink2_path = temp_dir.path().join("symlink2");
+
+        unix::fs::symlink(&symlink1_path, &symlink2_path).unwrap();
+        unix::fs::symlink(&symlink2_path, &symlink1_path).unwrap();
+
+        assert!(is_symlink_loop(&symlink1_path));
+    }
 }

src/uucore/src/lib/features/mode.rs

@@ -209,7 +209,7 @@ mod test {
         assert_eq!(super::parse_mode("u+x").unwrap(), 0o766);
         assert_eq!(
             super::parse_mode("+x").unwrap(),
-            if !crate::os::is_wsl_1() { 0o777 } else { 0o776 }
+            if crate::os::is_wsl_1() { 0o776 } else { 0o777 }
         );
         assert_eq!(super::parse_mode("a-w").unwrap(), 0o444);
         assert_eq!(super::parse_mode("g-r").unwrap(), 0o626);

tests/by-util/test_cp.rs

@@ -2495,6 +2495,20 @@ fn test_remove_destination_symbolic_link_loop() {
     assert!(at.file_exists("loop"));
 }
 
+#[test]
+#[cfg(not(windows))]
+fn test_cp_symbolic_link_loop() {
+    let (at, mut ucmd) = at_and_ucmd!();
+    at.symlink_file("loop", "loop");
+    at.plus("loop");
+    at.touch("f");
+    ucmd.args(&["-f", "f", "loop"])
+        .succeeds()
+        .no_stdout()
+        .no_stderr();
+    assert!(at.file_exists("loop"));
+}
+
 /// Test that copying a directory to itself is disallowed.
 #[test]
 fn test_copy_directory_to_itself_disallowed() {

tests/by-util/test_mv.rs

@@ -72,7 +72,7 @@ fn test_mv_move_file_into_dir_with_target_arg() {
         .succeeds()
         .no_stderr();
 
-    assert!(at.file_exists(format!("{dir}/{file}")))
+    assert!(at.file_exists(format!("{dir}/{file}")));
 }
 
 #[test]
@@ -90,7 +90,7 @@ fn test_mv_move_file_into_file_with_target_arg() {
         .fails()
         .stderr_is(format!("mv: target directory '{file1}': Not a directory\n"));
 
-    assert!(at.file_exists(file1))
+    assert!(at.file_exists(file1));
 }
 
 #[test]