Merge pull request #8265 from sylvestre/install-C

install: implement option -C

src/uu/install/locales/en-US.ftl

@@ -48,6 +48,9 @@ install-error-missing-file-operand = missing file operand
 install-error-missing-destination-operand = missing destination file operand after '{ $path }'
 install-error-failed-to-remove = Failed to remove existing file { $path }. Error: { $error }
 
+# Warning messages
+install-warning-compare-ignored = the --compare (-C) option is ignored when you specify a mode with non-permission bits
+
 # Verbose output
 install-verbose-creating-directory = creating directory { $path }
 install-verbose-creating-directory-step = install: creating directory { $path }

src/uu/install/locales/fr-FR.ftl

@@ -48,6 +48,9 @@ install-error-missing-file-operand = opérande de fichier manquant
 install-error-missing-destination-operand = opérande de fichier de destination manquant après '{ $path }'
 install-error-failed-to-remove = Échec de la suppression du fichier existant { $path }. Erreur : { $error }
 
+# Messages d'avertissement
+install-warning-compare-ignored = l'option --compare (-C) est ignorée quand un mode est indiqué avec des bits non liés à des droits
+
 # Sortie détaillée
 install-verbose-creating-directory = création du répertoire { $path }
 install-verbose-creating-directory-step = install : création du répertoire { $path }

src/uu/install/src/install.rs

@@ -365,6 +365,15 @@ fn behavior(matches: &ArgMatches) -> UResult<Behavior> {
         return Err(1.into());
     }
 
+    // Check if compare is used with non-permission mode bits
+    if compare && specified_mode.is_some() {
+        let mode = specified_mode.unwrap();
+        let non_permission_bits = 0o7000; // setuid, setgid, sticky bits
+        if mode & non_permission_bits != 0 {
+            show_error!("{}", get_message("install-warning-compare-ignored"));
+        }
+    }
+
     let owner = matches
         .get_one::<String>(OPT_OWNER)
         .map(|s| s.as_str())
@@ -1000,6 +1009,30 @@ fn copy(from: &Path, to: &Path, b: &Behavior) -> UResult<()> {
     Ok(())
 }
 
+/// Check if a file needs to be copied due to ownership differences when no explicit group is specified.
+/// Returns true if the destination file's ownership would differ from what it should be after installation.
+fn needs_copy_for_ownership(to: &Path, to_meta: &fs::Metadata) -> bool {
+    use std::os::unix::fs::MetadataExt;
+
+    // Check if the destination file's owner differs from the effective user ID
+    if to_meta.uid() != geteuid() {
+        return true;
+    }
+
+    // For group, we need to determine what the group would be after installation
+    // If no group is specified, the behavior depends on the directory:
+    // - If the directory has setgid bit, the file inherits the directory's group
+    // - Otherwise, the file gets the user's effective group
+    let expected_gid = to
+        .parent()
+        .and_then(|parent| metadata(parent).ok())
+        .filter(|parent_meta| parent_meta.mode() & 0o2000 != 0)
+        .map(|parent_meta| parent_meta.gid())
+        .unwrap_or(getegid());
+
+    to_meta.gid() != expected_gid
+}
+
 /// Return true if a file is necessary to copy. This is the case when:
 ///
 /// - _from_ or _to_ is nonexistent;
@@ -1030,6 +1063,13 @@ fn need_copy(from: &Path, to: &Path, b: &Behavior) -> UResult<bool> {
         return Ok(true);
     };
 
+    // Check if the destination is a symlink (should always be replaced)
+    if let Ok(to_symlink_meta) = fs::symlink_metadata(to) {
+        if to_symlink_meta.file_type().is_symlink() {
+            return Ok(true);
+        }
+    }
+
     // Define special file mode bits (setuid, setgid, sticky).
     let extra_mode: u32 = 0o7000;
     // Define all file mode bits (including permissions).
@@ -1038,7 +1078,7 @@ fn need_copy(from: &Path, to: &Path, b: &Behavior) -> UResult<bool> {
 
     // Check if any special mode bits are set in the specified mode,
     // source file mode, or destination file mode.
-    if b.specified_mode.unwrap_or(0) & extra_mode != 0
+    if b.mode() & extra_mode != 0
         || from_meta.mode() & extra_mode != 0
         || to_meta.mode() & extra_mode != 0
     {
@@ -1079,13 +1119,8 @@ fn need_copy(from: &Path, to: &Path, b: &Behavior) -> UResult<bool> {
         if group_id != to_meta.gid() {
             return Ok(true);
         }
-    } else {
-        #[cfg(not(target_os = "windows"))]
-        // Check if the destination file's owner or group
-        // differs from the effective user/group ID of the process.
-        if to_meta.uid() != geteuid() || to_meta.gid() != getegid() {
-            return Ok(true);
-        }
+    } else if needs_copy_for_ownership(to, &to_meta) {
+        return Ok(true);
     }
 
     // Check if the contents of the source and destination files differ.

tests/by-util/test_install.rs

@@ -643,7 +643,9 @@ fn test_install_copy_then_compare_file_with_extra_mode() {
         .arg("-m")
         .arg("1644")
         .succeeds()
-        .no_stderr();
+        .stderr_contains(
+            "the --compare (-C) option is ignored when you specify a mode with non-permission bits",
+        );
 
     file2_meta = at.metadata(file2);
     let after_install_sticky = FileTime::from_last_modification_time(&file2_meta);
@@ -1694,6 +1696,193 @@ fn test_install_compare_option() {
         .stderr_contains("Options --compare and --strip are mutually exclusive");
 }
 
+#[test]
+#[cfg(not(target_os = "openbsd"))]
+fn test_install_compare_basic() {
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    let source = "source_file";
+    let dest = "dest_file";
+
+    at.write(source, "test content");
+
+    // First install should copy
+    scene
+        .ucmd()
+        .args(&["-Cv", "-m644", source, dest])
+        .succeeds()
+        .stdout_contains(format!("'{source}' -> '{dest}'"));
+
+    // Second install with same mode should be no-op (compare works)
+    scene
+        .ucmd()
+        .args(&["-Cv", "-m644", source, dest])
+        .succeeds()
+        .no_stdout();
+
+    // Test that compare works correctly when content actually differs
+    let source2 = "source2";
+    at.write(source2, "different content");
+
+    scene
+        .ucmd()
+        .args(&["-Cv", "-m644", source2, dest])
+        .succeeds()
+        .stdout_contains("removed")
+        .stdout_contains(format!("'{source2}' -> '{dest}'"));
+
+    // Second install should be no-op since content is now identical
+    scene
+        .ucmd()
+        .args(&["-Cv", "-m644", source2, dest])
+        .succeeds()
+        .no_stdout();
+}
+
+#[test]
+#[cfg(not(any(target_os = "openbsd", target_os = "freebsd")))]
+fn test_install_compare_special_mode_bits() {
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    let source = "source_file";
+    let dest = "dest_file";
+
+    at.write(source, "test content");
+
+    // Special mode bits - setgid (tests the core bug fix)
+    // When setgid bit is set, -C should be ignored (always copy)
+    // This tests the bug where b.specified_mode.unwrap_or(0) was used instead of b.mode()
+    scene
+        .ucmd()
+        .args(&["-Cv", "-m2755", source, dest])
+        .succeeds()
+        .stdout_contains(format!("'{source}' -> '{dest}'"));
+
+    // Second install with same setgid mode should ALSO copy (not skip)
+    // because -C option should be ignored when special mode bits are present
+    scene
+        .ucmd()
+        .args(&["-Cv", "-m2755", source, dest])
+        .succeeds()
+        .stdout_contains("removed")
+        .stdout_contains(format!("'{source}' -> '{dest}'"));
+
+    // Special mode bits - setuid
+    scene
+        .ucmd()
+        .args(&["-Cv", "-m4755", source, dest])
+        .succeeds()
+        .stdout_contains("removed")
+        .stdout_contains(format!("'{source}' -> '{dest}'"));
+
+    // Second install with setuid should also copy
+    scene
+        .ucmd()
+        .args(&["-Cv", "-m4755", source, dest])
+        .succeeds()
+        .stdout_contains("removed")
+        .stdout_contains(format!("'{source}' -> '{dest}'"));
+
+    // Special mode bits - sticky bit
+    scene
+        .ucmd()
+        .args(&["-Cv", "-m1755", source, dest])
+        .succeeds()
+        .stdout_contains("removed")
+        .stdout_contains(format!("'{source}' -> '{dest}'"));
+
+    // Second install with sticky bit should also copy
+    scene
+        .ucmd()
+        .args(&["-Cv", "-m1755", source, dest])
+        .succeeds()
+        .stdout_contains("removed")
+        .stdout_contains(format!("'{source}' -> '{dest}'"));
+
+    // Back to normal mode - compare should work again
+    scene
+        .ucmd()
+        .args(&["-Cv", "-m644", source, dest])
+        .succeeds()
+        .stdout_contains("removed")
+        .stdout_contains(format!("'{source}' -> '{dest}'"));
+
+    // Second install with normal mode should be no-op
+    scene
+        .ucmd()
+        .args(&["-Cv", "-m644", source, dest])
+        .succeeds()
+        .no_stdout();
+}
+
+#[test]
+#[cfg(not(target_os = "openbsd"))]
+fn test_install_compare_group_ownership() {
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    let source = "source_file";
+    let dest = "dest_file";
+
+    at.write(source, "test content");
+
+    let user_group = std::process::Command::new("id")
+        .arg("-nrg")
+        .output()
+        .map_or_else(
+            |_| "users".to_string(),
+            |output| String::from_utf8_lossy(&output.stdout).trim().to_string(),
+        ); // fallback group name
+
+    // Install with explicit group
+    scene
+        .ucmd()
+        .args(&["-Cv", "-m664", "-g", &user_group, source, dest])
+        .succeeds()
+        .stdout_contains(format!("'{source}' -> '{dest}'"));
+
+    // Install without group - this should detect that no copy is needed
+    // because the file already has the correct group (user's group)
+    scene
+        .ucmd()
+        .args(&["-Cv", "-m664", source, dest])
+        .succeeds()
+        .no_stdout(); // Should be no-op if group ownership logic is correct
+}
+
+#[test]
+#[cfg(not(target_os = "openbsd"))]
+fn test_install_compare_symlink_handling() {
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    let source = "source_file";
+    let symlink_dest = "symlink_dest";
+    let target_file = "target_file";
+
+    at.write(source, "test content");
+    at.write(target_file, "test content"); // Same content to test that symlinks are always replaced
+    at.symlink_file(target_file, symlink_dest);
+
+    // Create a symlink as destination pointing to a different file - should always be replaced
+    scene
+        .ucmd()
+        .args(&["-Cv", "-m644", source, symlink_dest])
+        .succeeds()
+        .stdout_contains("removed")
+        .stdout_contains(format!("'{source}' -> '{symlink_dest}'"));
+
+    // Even if content would be the same, symlink destination should be replaced
+    // Now symlink_dest is a regular file, so compare should work normally
+    scene
+        .ucmd()
+        .args(&["-Cv", "-m644", source, symlink_dest])
+        .succeeds()
+        .no_stdout(); // Now it's a regular file, so compare should work
+}
+
 #[test]
 // Matches part of tests/install/basic-1
 fn test_t_exist_dir() {
@@ -2035,20 +2224,37 @@ fn test_selinux() {
 
     let args = ["-Z", "--context=unconfined_u:object_r:user_tmp_t:s0"];
     for arg in args {
-        new_ucmd!()
+        let result = new_ucmd!()
             .arg(arg)
             .arg("-v")
             .arg(at.plus_as_string(src))
             .arg(at.plus_as_string(dest))
-            .succeeds()
-            .stdout_contains("orig' -> '");
+            .run();
+
+        // Skip test if SELinux is not enabled
+        if result
+            .stderr_str()
+            .contains("SELinux is not enabled on this system")
+        {
+            println!("Skipping SELinux test: SELinux is not enabled");
+            at.remove(&at.plus_as_string(dest));
+            continue;
+        }
+
+        result.success().stdout_contains("orig' -> '");
 
         let getfattr_output = Command::new("getfattr")
             .arg(at.plus_as_string(dest))
             .arg("-n")
             .arg("security.selinux")
-            .output()
-            .expect("Failed to run `getfattr` on the destination file");
+            .output();
+
+        // Skip test if getfattr is not available
+        let Ok(getfattr_output) = getfattr_output else {
+            println!("Skipping SELinux test: getfattr not available");
+            at.remove(&at.plus_as_string(dest));
+            continue;
+        };
         println!("{:?}", getfattr_output);
         assert!(
             getfattr_output.status.success(),
@@ -2080,14 +2286,69 @@ fn test_selinux_invalid_args() {
         "--context=nconfined_u:object_r:user_tmp_t:s0",
     ];
     for arg in args {
-        new_ucmd!()
+        let result = new_ucmd!()
             .arg(arg)
             .arg("-v")
             .arg(at.plus_as_string(src))
             .arg(at.plus_as_string(dest))
-            .fails()
-            .stderr_contains("failed to set default file creation");
+            .fails();
+
+        let stderr = result.stderr_str();
+        assert!(
+            stderr.contains("failed to set default file creation")
+                || stderr.contains("SELinux is not enabled on this system"),
+            "Expected stderr to contain either 'failed to set default file creation' or 'SELinux is not enabled on this system', but got: '{}'",
+            stderr
+        );
 
         at.remove(&at.plus_as_string(dest));
     }
 }
+
+#[test]
+#[cfg(not(any(target_os = "openbsd", target_os = "freebsd")))]
+fn test_install_compare_with_mode_bits() {
+    let test_cases = [
+        ("4755", "setuid bit", true),
+        ("2755", "setgid bit", true),
+        ("1755", "sticky bit", true),
+        ("7755", "setuid + setgid + sticky bits", true),
+        ("755", "permission-only mode", false),
+    ];
+
+    for (mode, description, should_warn) in test_cases {
+        let scene = TestScenario::new(util_name!());
+        let at = &scene.fixtures;
+        let source = format!("source_file_{}", mode);
+        let dest = format!("dest_file_{}", mode);
+
+        at.write(&source, "test content");
+
+        let mode_arg = format!("--mode={}", mode);
+
+        if should_warn {
+            scene.ucmd().args(&["-C", &mode_arg, &source, &dest])
+                .succeeds()
+                .stderr_contains("the --compare (-C) option is ignored when you specify a mode with non-permission bits");
+        } else {
+            scene
+                .ucmd()
+                .args(&["-C", &mode_arg, &source, &dest])
+                .succeeds()
+                .no_stderr();
+
+            // Test second install should be no-op due to -C
+            scene
+                .ucmd()
+                .args(&["-C", &mode_arg, &source, &dest])
+                .succeeds()
+                .no_stderr();
+        }
+
+        assert!(
+            at.file_exists(&dest),
+            "Failed to create dest file for {}",
+            description
+        );
+    }
+}