From bdcd2ef00a3827e30c5b28a3ffcafa2c6f3ddf47 Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Tue, 24 Jun 2025 23:21:43 +0200 Subject: [PATCH 1/2] install: implement option -C Co-authored-by: Daniel Hofstetter --- src/uu/install/src/install.rs | 42 ++++++-- tests/by-util/test_install.rs | 187 ++++++++++++++++++++++++++++++++++ 2 files changed, 221 insertions(+), 8 deletions(-) diff --git a/src/uu/install/src/install.rs b/src/uu/install/src/install.rs index ee639de97..bc35da9d3 100644 --- a/src/uu/install/src/install.rs +++ b/src/uu/install/src/install.rs @@ -1000,6 +1000,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 +1054,13 @@ fn need_copy(from: &Path, to: &Path, b: &Behavior) -> UResult { 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 +1069,7 @@ fn need_copy(from: &Path, to: &Path, b: &Behavior) -> UResult { // 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 +1110,8 @@ fn need_copy(from: &Path, to: &Path, b: &Behavior) -> UResult { 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. diff --git a/tests/by-util/test_install.rs b/tests/by-util/test_install.rs index 451685a25..514654a13 100644 --- a/tests/by-util/test_install.rs +++ b/tests/by-util/test_install.rs @@ -1694,6 +1694,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() { From d9c72dcdc891283265b4e7a859ce8c5a61ffd1bd Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Sat, 28 Jun 2025 11:51:00 +0200 Subject: [PATCH 2/2] install -C: also the manage the ignore case --- src/uu/install/locales/en-US.ftl | 3 ++ src/uu/install/locales/fr-FR.ftl | 3 ++ src/uu/install/src/install.rs | 9 ++++ tests/by-util/test_install.rs | 92 ++++++++++++++++++++++++++++---- 4 files changed, 98 insertions(+), 9 deletions(-) diff --git a/src/uu/install/locales/en-US.ftl b/src/uu/install/locales/en-US.ftl index 0643cc557..32ab93820 100644 --- a/src/uu/install/locales/en-US.ftl +++ b/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 } diff --git a/src/uu/install/locales/fr-FR.ftl b/src/uu/install/locales/fr-FR.ftl index 10cc217b1..351bd2397 100644 --- a/src/uu/install/locales/fr-FR.ftl +++ b/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 } diff --git a/src/uu/install/src/install.rs b/src/uu/install/src/install.rs index bc35da9d3..7b58fd5dc 100644 --- a/src/uu/install/src/install.rs +++ b/src/uu/install/src/install.rs @@ -365,6 +365,15 @@ fn behavior(matches: &ArgMatches) -> UResult { 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::(OPT_OWNER) .map(|s| s.as_str()) diff --git a/tests/by-util/test_install.rs b/tests/by-util/test_install.rs index 514654a13..94a01eb6e 100644 --- a/tests/by-util/test_install.rs +++ b/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); @@ -2222,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(), @@ -2267,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 + ); + } +}