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() {