From b0758db8427587725cee4d0daa4beb6de8a12166 Mon Sep 17 00:00:00 2001 From: Stephen Marz Date: Fri, 2 Jan 2026 11:24:32 -0500 Subject: [PATCH 1/8] install: add error messages for failed-to-remove and cannot-stat. --- src/uu/install/locales/en-US.ftl | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/uu/install/locales/en-US.ftl b/src/uu/install/locales/en-US.ftl index 76265a2b1ff..4571909748c 100644 --- a/src/uu/install/locales/en-US.ftl +++ b/src/uu/install/locales/en-US.ftl @@ -48,7 +48,8 @@ install-error-mutually-exclusive-compare-preserve = Options --compare and --pres install-error-mutually-exclusive-compare-strip = Options --compare and --strip are mutually exclusive 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 } +install-error-failed-to-remove = failed to remove existing file { $path }: { $error } +install-error-cannot-stat = cannot stat { $path }: { $error } # Warning messages install-warning-compare-ignored = the --compare (-C) option is ignored when you specify a mode with non-permission bits From 6ebfcb6778dbb00fc4c84b7ec56f18464127fd88 Mon Sep 17 00:00:00 2001 From: Stephen Marz Date: Fri, 2 Jan 2026 11:24:40 -0500 Subject: [PATCH 2/8] install: add error messages for failed-to-remove and cannot-stat in French. --- src/uu/install/locales/fr-FR.ftl | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/uu/install/locales/fr-FR.ftl b/src/uu/install/locales/fr-FR.ftl index 330ceb7b477..1cd7fe5f66a 100644 --- a/src/uu/install/locales/fr-FR.ftl +++ b/src/uu/install/locales/fr-FR.ftl @@ -48,7 +48,8 @@ install-error-mutually-exclusive-compare-preserve = Les options --compare et --p install-error-mutually-exclusive-compare-strip = Les options --compare et --strip sont mutuellement exclusives 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 } +install-error-failed-to-remove = Échec de la suppression du fichier existant { $path }: { $error } +install-error-cannot-stat = impossible d'obtenir des informations sur le fichier. { $path }: { $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 From bcc5100549a1cda8c1b33a5d6d0038efd22b5fea Mon Sep 17 00:00:00 2001 From: Stephen Marz Date: Fri, 2 Jan 2026 11:26:10 -0500 Subject: [PATCH 3/8] install: add error messages to align with GNU for failed-to-remove and cannot-stat. --- src/uu/install/src/install.rs | 42 ++++++++++++++++++++++++++++++----- 1 file changed, 37 insertions(+), 5 deletions(-) diff --git a/src/uu/install/src/install.rs b/src/uu/install/src/install.rs index 84eefdcc870..b19685dd040 100644 --- a/src/uu/install/src/install.rs +++ b/src/uu/install/src/install.rs @@ -119,6 +119,12 @@ enum InstallError { #[error("{}", translate!("install-error-extra-operand", "operand" => .0.quote(), "usage" => .1.clone()))] ExtraOperand(OsString, String), + #[error("{}", translate!("install-error-failed-to-remove", "path" => .0.quote(), "error" => format!("{}", .1)))] + FailedToRemove(PathBuf, String), + + #[error("{}", translate!("install-error-cannot-stat", "path" => .0.quote(), "error" => format!("{}", .1)))] + CannotStat(PathBuf, String), + #[cfg(all(feature = "selinux", any(target_os = "linux", target_os = "android")))] #[error("{}", .0)] SelinuxContextFailed(String), @@ -678,7 +684,22 @@ fn standard(mut paths: Vec, b: &Behavior) -> UResult<()> { return copy_files_into_dir(sources, &target, b); } - if target.is_file() || is_new_file_path(&target) { + // target.is_file does not detect special files like character/block devices + // So, in a unix environment, we need to check the file type from metadata and + // not just trust is_file() + #[cfg(unix)] + let is_fl = { + let fl_type = std::fs::Metadata::from(std::fs::metadata(source)?).file_type(); + fl_type.is_file() + || fl_type.is_char_device() + || fl_type.is_block_device() + || fl_type.is_fifo() + }; + + #[cfg(not(unix))] + let is_fl = target.is_file(); + + if is_fl || is_new_file_path(&target) { copy(source, &target, b) } else { Err(InstallError::InvalidTarget(target).into()) @@ -820,6 +841,17 @@ fn copy_file(from: &Path, to: &Path) -> UResult<()> { } } + // If we don't include this check, then the `remove_file` below will fail + // and it will give an incorrect error message. However, if we check to + // see if the file exists, and it can't even be checked, this means we + // don't have permission to access the file, so we should return an error. + let to_stat = to.try_exists(); + if to_stat.is_err() { + return Err( + InstallError::CannotStat(to.to_path_buf(), to_stat.err().unwrap().to_string()).into(), + ); + } + if to.is_dir() && !from.is_dir() { return Err(InstallError::OverrideDirectoryFailed( to.to_path_buf().clone(), @@ -833,10 +865,10 @@ fn copy_file(from: &Path, to: &Path) -> UResult<()> { // appears at this path between the remove and create, it will fail safely if let Err(e) = fs::remove_file(to) { if e.kind() != std::io::ErrorKind::NotFound { - show_error!( - "{}", - translate!("install-error-failed-to-remove", "path" => to.quote(), "error" => format!("{e:?}")) - ); + // If we get here, then remove_file failed for some + // reason other than the file not existing. This means + // this should be a fatal error, not a warning. + return Err(InstallError::FailedToRemove(to.to_path_buf(), e.to_string()).into()); } } From 29ba3a9a1d67577b51a94b966bb4000d9fb797fc Mon Sep 17 00:00:00 2001 From: Stephen Marz Date: Sat, 3 Jan 2026 14:12:21 -0500 Subject: [PATCH 4/8] install: fix offending code that failed tests. Several cross-compatiblity checks added and fixed. --- src/uu/install/src/install.rs | 26 ++++++++++---------------- 1 file changed, 10 insertions(+), 16 deletions(-) diff --git a/src/uu/install/src/install.rs b/src/uu/install/src/install.rs index b19685dd040..869bf38cd5a 100644 --- a/src/uu/install/src/install.rs +++ b/src/uu/install/src/install.rs @@ -119,10 +119,10 @@ enum InstallError { #[error("{}", translate!("install-error-extra-operand", "operand" => .0.quote(), "usage" => .1.clone()))] ExtraOperand(OsString, String), - #[error("{}", translate!("install-error-failed-to-remove", "path" => .0.quote(), "error" => format!("{}", .1)))] + #[error("{}", translate!("install-error-failed-to-remove", "path" => .0.quote(), "error" => .1.clone()))] FailedToRemove(PathBuf, String), - #[error("{}", translate!("install-error-cannot-stat", "path" => .0.quote(), "error" => format!("{}", .1)))] + #[error("{}", translate!("install-error-cannot-stat", "path" => .0.quote(), "error" => .1.clone()))] CannotStat(PathBuf, String), #[cfg(all(feature = "selinux", any(target_os = "linux", target_os = "android")))] @@ -686,20 +686,17 @@ fn standard(mut paths: Vec, b: &Behavior) -> UResult<()> { // target.is_file does not detect special files like character/block devices // So, in a unix environment, we need to check the file type from metadata and - // not just trust is_file() + // not just trust is_file(). #[cfg(unix)] - let is_fl = { - let fl_type = std::fs::Metadata::from(std::fs::metadata(source)?).file_type(); - fl_type.is_file() - || fl_type.is_char_device() - || fl_type.is_block_device() - || fl_type.is_fifo() + let is_file = match metadata(&target) { + Ok(meta) => !meta.file_type().is_dir(), + Err(_) => false, }; #[cfg(not(unix))] - let is_fl = target.is_file(); + let is_file = target.is_file(); - if is_fl || is_new_file_path(&target) { + if is_file || is_new_file_path(&target) { copy(source, &target, b) } else { Err(InstallError::InvalidTarget(target).into()) @@ -845,11 +842,8 @@ fn copy_file(from: &Path, to: &Path) -> UResult<()> { // and it will give an incorrect error message. However, if we check to // see if the file exists, and it can't even be checked, this means we // don't have permission to access the file, so we should return an error. - let to_stat = to.try_exists(); - if to_stat.is_err() { - return Err( - InstallError::CannotStat(to.to_path_buf(), to_stat.err().unwrap().to_string()).into(), - ); + if let Err(to_stat) = to.try_exists() { + return Err(InstallError::CannotStat(to.to_path_buf(), to_stat.to_string()).into()); } if to.is_dir() && !from.is_dir() { From f176a6774c50ae6aa5c3ef16558b24477514d312 Mon Sep 17 00:00:00 2001 From: Stephen Marz Date: Wed, 7 Jan 2026 08:12:09 -0500 Subject: [PATCH 5/8] test_install: add two checks that should give Permission denied. One for failing to remove an existing file and one for trying to stat a file in /root. --- tests/by-util/test_install.rs | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/tests/by-util/test_install.rs b/tests/by-util/test_install.rs index 32cdbee8ca0..ac275ab9d3e 100644 --- a/tests/by-util/test_install.rs +++ b/tests/by-util/test_install.rs @@ -119,6 +119,24 @@ fn test_install_ancestors_mode_directories() { assert_eq!(0o40_200_u32, at.metadata(target_dir).permissions().mode()); } +#[test] +fn test_install_remove_impermissible_dst_file() { + let src_file = "/dev/null"; + let dst_file = "/dev/full"; + new_ucmd!().args(&[src_file, dst_file]).fails().stderr_only(format!( + "install: failed to remove existing file '{dst_file}': Permission denied\n" + )); +} + +#[test] +fn test_install_remove_inaccessible_dst_file() { + let src_file = "/dev/null"; + let dst_file = "/root/file"; + new_ucmd!().args(&[src_file, dst_file]).fails().stderr_only(format!( + "install: cannot stat '{dst_file}': Permission denied\n" + )); +} + #[test] fn test_install_ancestors_mode_directories_with_file() { let (at, mut ucmd) = at_and_ucmd!(); From 4153088b51c6c199a8ecf8e3c0f829ca468f238c Mon Sep 17 00:00:00 2001 From: Stephen Marz Date: Wed, 7 Jan 2026 08:12:59 -0500 Subject: [PATCH 6/8] install: update error messages and remove the additional Rust errorisms (e.g., (os error 13)) by using UIoError to print the error message. --- src/uu/install/src/install.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/uu/install/src/install.rs b/src/uu/install/src/install.rs index 869bf38cd5a..e967e87eec6 100644 --- a/src/uu/install/src/install.rs +++ b/src/uu/install/src/install.rs @@ -35,6 +35,7 @@ use uucore::selinux::{ }; use uucore::translate; use uucore::{format_usage, show, show_error, show_if_err}; +use uucore::error::UIoError; #[cfg(unix)] use std::os::unix::fs::MetadataExt; @@ -843,7 +844,8 @@ fn copy_file(from: &Path, to: &Path) -> UResult<()> { // see if the file exists, and it can't even be checked, this means we // don't have permission to access the file, so we should return an error. if let Err(to_stat) = to.try_exists() { - return Err(InstallError::CannotStat(to.to_path_buf(), to_stat.to_string()).into()); + let err = UIoError::from(to_stat); + return Err(InstallError::CannotStat(to.to_path_buf(), err.to_string()).into()); } if to.is_dir() && !from.is_dir() { @@ -862,7 +864,8 @@ fn copy_file(from: &Path, to: &Path) -> UResult<()> { // If we get here, then remove_file failed for some // reason other than the file not existing. This means // this should be a fatal error, not a warning. - return Err(InstallError::FailedToRemove(to.to_path_buf(), e.to_string()).into()); + let err = UIoError::from(e); + return Err(InstallError::FailedToRemove(to.to_path_buf(), err.to_string()).into()); } } From acda29c43e9b7ddac3478df5820f4800fae13108 Mon Sep 17 00:00:00 2001 From: Stephen Marz Date: Wed, 7 Jan 2026 08:22:42 -0500 Subject: [PATCH 7/8] install and test_install: run cargo 'fmt' --- src/uu/install/src/install.rs | 2 +- tests/by-util/test_install.rs | 18 ++++++++++++------ 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/src/uu/install/src/install.rs b/src/uu/install/src/install.rs index e967e87eec6..d287aadbd73 100644 --- a/src/uu/install/src/install.rs +++ b/src/uu/install/src/install.rs @@ -24,6 +24,7 @@ use uucore::backup_control::{self, BackupMode}; use uucore::buf_copy::copy_stream; use uucore::display::Quotable; use uucore::entries::{grp2gid, usr2uid}; +use uucore::error::UIoError; use uucore::error::{FromIo, UError, UResult, UUsageError}; use uucore::fs::dir_strip_dot_for_creation; use uucore::perms::{Verbosity, VerbosityLevel, wrap_chown}; @@ -35,7 +36,6 @@ use uucore::selinux::{ }; use uucore::translate; use uucore::{format_usage, show, show_error, show_if_err}; -use uucore::error::UIoError; #[cfg(unix)] use std::os::unix::fs::MetadataExt; diff --git a/tests/by-util/test_install.rs b/tests/by-util/test_install.rs index ac275ab9d3e..81cba4b639c 100644 --- a/tests/by-util/test_install.rs +++ b/tests/by-util/test_install.rs @@ -123,18 +123,24 @@ fn test_install_ancestors_mode_directories() { fn test_install_remove_impermissible_dst_file() { let src_file = "/dev/null"; let dst_file = "/dev/full"; - new_ucmd!().args(&[src_file, dst_file]).fails().stderr_only(format!( - "install: failed to remove existing file '{dst_file}': Permission denied\n" - )); + new_ucmd!() + .args(&[src_file, dst_file]) + .fails() + .stderr_only(format!( + "install: failed to remove existing file '{dst_file}': Permission denied\n" + )); } #[test] fn test_install_remove_inaccessible_dst_file() { let src_file = "/dev/null"; let dst_file = "/root/file"; - new_ucmd!().args(&[src_file, dst_file]).fails().stderr_only(format!( - "install: cannot stat '{dst_file}': Permission denied\n" - )); + new_ucmd!() + .args(&[src_file, dst_file]) + .fails() + .stderr_only(format!( + "install: cannot stat '{dst_file}': Permission denied\n" + )); } #[test] From 4668b77901f2f85421992ee8062a9bbc1fa27de3 Mon Sep 17 00:00:00 2001 From: Christopher Dryden Date: Sat, 14 Feb 2026 17:55:16 +0000 Subject: [PATCH 8/8] install: simplify error handling to use map_err_context and match GNU output --- src/uu/install/locales/en-US.ftl | 5 ++-- src/uu/install/locales/fr-FR.ftl | 5 ++-- src/uu/install/src/install.rs | 43 ++++++++------------------------ tests/by-util/test_install.rs | 28 ++++++++++----------- 4 files changed, 30 insertions(+), 51 deletions(-) diff --git a/src/uu/install/locales/en-US.ftl b/src/uu/install/locales/en-US.ftl index 4571909748c..05905a3e7df 100644 --- a/src/uu/install/locales/en-US.ftl +++ b/src/uu/install/locales/en-US.ftl @@ -48,8 +48,9 @@ install-error-mutually-exclusive-compare-preserve = Options --compare and --pres install-error-mutually-exclusive-compare-strip = Options --compare and --strip are mutually exclusive 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 } -install-error-cannot-stat = cannot stat { $path }: { $error } +install-error-failed-to-access = failed to access { $path } +install-error-failed-to-remove = cannot remove { $path } +install-error-cannot-create-file = cannot create regular file { $path } # Warning messages install-warning-compare-ignored = the --compare (-C) option is ignored when you specify a mode with non-permission bits diff --git a/src/uu/install/locales/fr-FR.ftl b/src/uu/install/locales/fr-FR.ftl index 1cd7fe5f66a..29077e0e984 100644 --- a/src/uu/install/locales/fr-FR.ftl +++ b/src/uu/install/locales/fr-FR.ftl @@ -48,8 +48,9 @@ install-error-mutually-exclusive-compare-preserve = Les options --compare et --p install-error-mutually-exclusive-compare-strip = Les options --compare et --strip sont mutuellement exclusives 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 }: { $error } -install-error-cannot-stat = impossible d'obtenir des informations sur le fichier. { $path }: { $error } +install-error-failed-to-access = impossible d'accéder à { $path } +install-error-failed-to-remove = impossible de supprimer { $path } +install-error-cannot-create-file = impossible de créer le fichier { $path } # 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 diff --git a/src/uu/install/src/install.rs b/src/uu/install/src/install.rs index d287aadbd73..93b634031c3 100644 --- a/src/uu/install/src/install.rs +++ b/src/uu/install/src/install.rs @@ -24,7 +24,6 @@ use uucore::backup_control::{self, BackupMode}; use uucore::buf_copy::copy_stream; use uucore::display::Quotable; use uucore::entries::{grp2gid, usr2uid}; -use uucore::error::UIoError; use uucore::error::{FromIo, UError, UResult, UUsageError}; use uucore::fs::dir_strip_dot_for_creation; use uucore::perms::{Verbosity, VerbosityLevel, wrap_chown}; @@ -120,12 +119,6 @@ enum InstallError { #[error("{}", translate!("install-error-extra-operand", "operand" => .0.quote(), "usage" => .1.clone()))] ExtraOperand(OsString, String), - #[error("{}", translate!("install-error-failed-to-remove", "path" => .0.quote(), "error" => .1.clone()))] - FailedToRemove(PathBuf, String), - - #[error("{}", translate!("install-error-cannot-stat", "path" => .0.quote(), "error" => .1.clone()))] - CannotStat(PathBuf, String), - #[cfg(all(feature = "selinux", any(target_os = "linux", target_os = "android")))] #[error("{}", .0)] SelinuxContextFailed(String), @@ -685,19 +678,7 @@ fn standard(mut paths: Vec, b: &Behavior) -> UResult<()> { return copy_files_into_dir(sources, &target, b); } - // target.is_file does not detect special files like character/block devices - // So, in a unix environment, we need to check the file type from metadata and - // not just trust is_file(). - #[cfg(unix)] - let is_file = match metadata(&target) { - Ok(meta) => !meta.file_type().is_dir(), - Err(_) => false, - }; - - #[cfg(not(unix))] - let is_file = target.is_file(); - - if is_file || is_new_file_path(&target) { + if (target.exists() && !target.is_dir()) || is_new_file_path(&target) { copy(source, &target, b) } else { Err(InstallError::InvalidTarget(target).into()) @@ -839,13 +820,10 @@ fn copy_file(from: &Path, to: &Path) -> UResult<()> { } } - // If we don't include this check, then the `remove_file` below will fail - // and it will give an incorrect error message. However, if we check to - // see if the file exists, and it can't even be checked, this means we - // don't have permission to access the file, so we should return an error. - if let Err(to_stat) = to.try_exists() { - let err = UIoError::from(to_stat); - return Err(InstallError::CannotStat(to.to_path_buf(), err.to_string()).into()); + if let Err(e) = to.try_exists() { + return Err(e.map_err_context( + || translate!("install-error-failed-to-access", "path" => to.quote()), + )); } if to.is_dir() && !from.is_dir() { @@ -861,11 +839,9 @@ fn copy_file(from: &Path, to: &Path) -> UResult<()> { // appears at this path between the remove and create, it will fail safely if let Err(e) = fs::remove_file(to) { if e.kind() != std::io::ErrorKind::NotFound { - // If we get here, then remove_file failed for some - // reason other than the file not existing. This means - // this should be a fatal error, not a warning. - let err = UIoError::from(e); - return Err(InstallError::FailedToRemove(to.to_path_buf(), err.to_string()).into()); + return Err(e.map_err_context( + || translate!("install-error-failed-to-remove", "path" => to.quote()), + )); } } @@ -875,7 +851,8 @@ fn copy_file(from: &Path, to: &Path) -> UResult<()> { .write(true) .create_new(true) .mode(0o600) - .open(to)?; + .open(to) + .map_err_context(|| translate!("install-error-cannot-create-file", "path" => to.quote()))?; copy_stream(&mut handle, &mut dest).map_err(|err| { InstallError::InstallFailed(from.to_path_buf(), to.to_path_buf(), err.to_string()) diff --git a/tests/by-util/test_install.rs b/tests/by-util/test_install.rs index 81cba4b639c..96501b76f11 100644 --- a/tests/by-util/test_install.rs +++ b/tests/by-util/test_install.rs @@ -120,27 +120,27 @@ fn test_install_ancestors_mode_directories() { } #[test] -fn test_install_remove_impermissible_dst_file() { - let src_file = "/dev/null"; - let dst_file = "/dev/full"; +#[cfg(target_os = "linux")] +fn test_install_cannot_remove_destination() { + if geteuid() == 0 { + return; + } new_ucmd!() - .args(&[src_file, dst_file]) + .args(&["/dev/null", "/dev/full"]) .fails() - .stderr_only(format!( - "install: failed to remove existing file '{dst_file}': Permission denied\n" - )); + .stderr_only("install: cannot remove '/dev/full': Permission denied\n"); } #[test] -fn test_install_remove_inaccessible_dst_file() { - let src_file = "/dev/null"; - let dst_file = "/root/file"; +#[cfg(target_os = "linux")] +fn test_install_cannot_create_destination() { + if geteuid() == 0 { + return; + } new_ucmd!() - .args(&[src_file, dst_file]) + .args(&["/dev/null", "/root/file"]) .fails() - .stderr_only(format!( - "install: cannot stat '{dst_file}': Permission denied\n" - )); + .stderr_only("install: cannot create regular file '/root/file': Permission denied\n"); } #[test]