From 5876cd25814ed0a3d7f034854eeafdff50ad390b Mon Sep 17 00:00:00 2001 From: Kyle Manna Date: Tue, 14 Feb 2023 01:02:31 -0600 Subject: [PATCH 1/3] install: add missing directory chown * Re-factor the copy function chown into a function that can be re-used. * Fix bug where the group overwrote the user. * Add chown compatibility to follow GNU coreutils. * Reduce two chown calls to a single syscall only when needed. * Fixes #4361 --- src/uu/install/src/install.rs | 119 +++++++++++++++++----------------- 1 file changed, 59 insertions(+), 60 deletions(-) diff --git a/src/uu/install/src/install.rs b/src/uu/install/src/install.rs index 597eacc67..c3b64cdd8 100644 --- a/src/uu/install/src/install.rs +++ b/src/uu/install/src/install.rs @@ -466,6 +466,8 @@ fn directory(paths: &[String], b: &Behavior) -> UResult<()> { uucore::error::set_exit_code(1); continue; } + + chown_optional_user_group(path, b)?; } // If the exit code was set, or show! has been called at least once // (which sets the exit code as well), function execution will end after @@ -626,6 +628,62 @@ fn copy_files_into_dir(files: &[PathBuf], target_dir: &Path, b: &Behavior) -> UR Ok(()) } +/// Handle incomplete user/group parings for chown. +/// +/// Returns a Result type with the Err variant containing the error message. +/// +/// # Parameters +/// +/// _path_ must exist. +/// +/// # Errors +/// +/// If the owner or group are invalid or copy system call fails, we print a verbose error and +/// return an empty error value. +/// +fn chown_optional_user_group(path: &Path, b: &Behavior) -> UResult<()> { + let owner = if !b.owner.is_empty() { + match usr2uid(&b.owner) { + Ok(u) => Some(u), + _ => return Err(InstallError::NoSuchUser(b.owner.clone()).into()), + } + } else { + None + }; + + let group = if !b.group.is_empty() { + match grp2gid(&b.group) { + Ok(g) => Some(g), + _ => return Err(InstallError::NoSuchGroup(b.group.clone()).into()), + } + } else { + None + }; + + let meta = match fs::metadata(path) { + Ok(meta) => meta, + Err(e) => return Err(InstallError::MetadataFailed(e).into()), + }; + + let verbosity = Verbosity { + groups_only: owner.is_none(), + level: VerbosityLevel::Normal, + }; + + if owner.is_some() || group.is_some() { + match wrap_chown(path, &meta, owner, group, false, verbosity) { + Ok(n) => { + if !n.is_empty() { + show_error!("{}", n); + } + } + Err(e) => show_error!("{}", e), + } + } + + Ok(()) +} + /// Copy one file to a new location, changing metadata. /// /// Returns a Result type with the Err variant containing the error message. @@ -708,66 +766,7 @@ fn copy(from: &Path, to: &Path, b: &Behavior) -> UResult<()> { return Err(InstallError::ChmodFailed(to.to_path_buf()).into()); } - if !b.owner.is_empty() { - let meta = match fs::metadata(to) { - Ok(meta) => meta, - Err(e) => return Err(InstallError::MetadataFailed(e).into()), - }; - - let owner_id = match usr2uid(&b.owner) { - Ok(g) => g, - _ => return Err(InstallError::NoSuchUser(b.owner.clone()).into()), - }; - let gid = meta.gid(); - match wrap_chown( - to, - &meta, - Some(owner_id), - Some(gid), - false, - Verbosity { - groups_only: false, - level: VerbosityLevel::Normal, - }, - ) { - Ok(n) => { - if !n.is_empty() { - show_error!("{}", n); - } - } - Err(e) => show_error!("{}", e), - } - } - - if !b.group.is_empty() { - let meta = match fs::metadata(to) { - Ok(meta) => meta, - Err(e) => return Err(InstallError::MetadataFailed(e).into()), - }; - - let group_id = match grp2gid(&b.group) { - Ok(g) => g, - _ => return Err(InstallError::NoSuchGroup(b.group.clone()).into()), - }; - match wrap_chown( - to, - &meta, - Some(group_id), - None, - false, - Verbosity { - groups_only: true, - level: VerbosityLevel::Normal, - }, - ) { - Ok(n) => { - if !n.is_empty() { - show_error!("{}", n); - } - } - Err(e) => show_error!("{}", e), - } - } + chown_optional_user_group(to, b)?; if b.preserve_timestamps { let meta = match fs::metadata(from) { From 376f4d90ef7d8ee8b5d0283b6997796e10052719 Mon Sep 17 00:00:00 2001 From: Kyle Manna Date: Wed, 15 Feb 2023 21:28:40 -0600 Subject: [PATCH 2/3] install: add tests for invalid owner and group * Move the user and group resolution to the behavior decoding instead of re-running on every file/directory creation. Simplifies code. * Update error output to match GNU coreutils. * Add tests to verify invalid owner and group. --- src/uu/install/src/install.rs | 97 ++++++++++++++++++----------------- tests/by-util/test_install.rs | 94 +++++++++++++++++++++++++++++++++ 2 files changed, 143 insertions(+), 48 deletions(-) diff --git a/src/uu/install/src/install.rs b/src/uu/install/src/install.rs index c3b64cdd8..ec80709a9 100644 --- a/src/uu/install/src/install.rs +++ b/src/uu/install/src/install.rs @@ -41,8 +41,8 @@ pub struct Behavior { specified_mode: Option, backup_mode: BackupMode, suffix: String, - owner: String, - group: String, + owner_id: Option, + group_id: Option, verbose: bool, preserve_timestamps: bool, compare: bool, @@ -64,8 +64,8 @@ enum InstallError { InstallFailed(PathBuf, PathBuf, std::io::Error), StripProgramFailed(String), MetadataFailed(std::io::Error), - NoSuchUser(String), - NoSuchGroup(String), + InvalidUser(String), + InvalidGroup(String), OmittingDirectory(PathBuf), } @@ -117,8 +117,8 @@ impl Display for InstallError { ), Self::StripProgramFailed(msg) => write!(f, "strip program failed: {msg}"), Self::MetadataFailed(e) => Display::fmt(&uio_error!(e, ""), f), - Self::NoSuchUser(user) => write!(f, "no such user: {}", user.maybe_quote()), - Self::NoSuchGroup(group) => write!(f, "no such group: {}", group.maybe_quote()), + Self::InvalidUser(user) => write!(f, "invalid user: {}", user.quote()), + Self::InvalidGroup(group) => write!(f, "invalid group: {}", group.quote()), Self::OmittingDirectory(dir) => write!(f, "omitting directory {}", dir.quote()), } } @@ -391,21 +391,44 @@ fn behavior(matches: &ArgMatches) -> UResult { show_error!("Options --compare and --strip are mutually exclusive"); return Err(1.into()); } + + let owner = matches + .get_one::(OPT_OWNER) + .map(|s| s.as_str()) + .unwrap_or("") + .to_string(); + + let owner_id = if !owner.is_empty() { + match usr2uid(&owner) { + Ok(u) => Some(u), + _ => return Err(InstallError::InvalidUser(owner.clone()).into()), + } + } else { + None + }; + + let group = matches + .get_one::(OPT_GROUP) + .map(|s| s.as_str()) + .unwrap_or("") + .to_string(); + + let group_id = if !group.is_empty() { + match grp2gid(&group) { + Ok(g) => Some(g), + _ => return Err(InstallError::InvalidGroup(group.clone()).into()), + } + } else { + None + }; + Ok(Behavior { main_function, specified_mode, backup_mode, suffix: backup_control::determine_backup_suffix(matches), - owner: matches - .get_one::(OPT_OWNER) - .map(|s| s.as_str()) - .unwrap_or("") - .to_string(), - group: matches - .get_one::(OPT_GROUP) - .map(|s| s.as_str()) - .unwrap_or("") - .to_string(), + owner_id, + group_id, verbose: matches.get_flag(OPT_VERBOSE), preserve_timestamps, compare, @@ -467,7 +490,11 @@ fn directory(paths: &[String], b: &Behavior) -> UResult<()> { continue; } - chown_optional_user_group(path, b)?; + if chown_optional_user_group(path, b).is_err() { + // Error messages are printed by the chown_optional_user_group function! + uucore::error::set_exit_code(1); + continue; + } } // If the exit code was set, or show! has been called at least once // (which sets the exit code as well), function execution will end after @@ -642,36 +669,18 @@ fn copy_files_into_dir(files: &[PathBuf], target_dir: &Path, b: &Behavior) -> UR /// return an empty error value. /// fn chown_optional_user_group(path: &Path, b: &Behavior) -> UResult<()> { - let owner = if !b.owner.is_empty() { - match usr2uid(&b.owner) { - Ok(u) => Some(u), - _ => return Err(InstallError::NoSuchUser(b.owner.clone()).into()), - } - } else { - None - }; - - let group = if !b.group.is_empty() { - match grp2gid(&b.group) { - Ok(g) => Some(g), - _ => return Err(InstallError::NoSuchGroup(b.group.clone()).into()), - } - } else { - None - }; - let meta = match fs::metadata(path) { Ok(meta) => meta, Err(e) => return Err(InstallError::MetadataFailed(e).into()), }; let verbosity = Verbosity { - groups_only: owner.is_none(), + groups_only: b.owner_id.is_none(), level: VerbosityLevel::Normal, }; - if owner.is_some() || group.is_some() { - match wrap_chown(path, &meta, owner, group, false, verbosity) { + if b.owner_id.is_some() || b.group_id.is_some() { + match wrap_chown(path, &meta, b.owner_id, b.group_id, false, verbosity) { Ok(n) => { if !n.is_empty() { show_error!("{}", n); @@ -846,19 +855,11 @@ fn need_copy(from: &Path, to: &Path, b: &Behavior) -> UResult { // TODO: if -P (#1809) and from/to contexts mismatch, return true. - if !b.owner.is_empty() { - let owner_id = match usr2uid(&b.owner) { - Ok(id) => id, - _ => return Err(InstallError::NoSuchUser(b.owner.clone()).into()), - }; + if let Some(owner_id) = b.owner_id { if owner_id != to_meta.uid() { return Ok(true); } - } else if !b.group.is_empty() { - let group_id = match grp2gid(&b.group) { - Ok(id) => id, - _ => return Err(InstallError::NoSuchGroup(b.group.clone()).into()), - }; + } else if let Some(group_id) = b.group_id { if group_id != to_meta.gid() { return Ok(true); } diff --git a/tests/by-util/test_install.rs b/tests/by-util/test_install.rs index 1e6cd7606..342824fdb 100644 --- a/tests/by-util/test_install.rs +++ b/tests/by-util/test_install.rs @@ -1369,6 +1369,100 @@ fn test_install_dir_req_verbose() { .stdout_contains("install: creating directory 'sub5/a'\ninstall: creating directory 'sub5/a/b'\ninstall: creating directory 'sub5/a/b/c'\n'source_file1' -> 'sub5/a/b/c/file'"); } +#[test] +fn test_install_chown_file_invalid() { + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + + let file_1 = "source_file1"; + at.touch(file_1); + + scene + .ucmd() + .arg("-o") + .arg("test_invalid_user") + .arg(file_1) + .arg("target_file1") + .fails() + .stderr_contains("install: invalid user: 'test_invalid_user'"); + + scene + .ucmd() + .arg("-g") + .arg("test_invalid_group") + .arg(file_1) + .arg("target_file1") + .fails() + .stderr_contains("install: invalid group: 'test_invalid_group'"); + + scene + .ucmd() + .arg("-o") + .arg("test_invalid_user") + .arg("-g") + .arg("test_invalid_group") + .arg(file_1) + .arg("target_file1") + .fails() + .stderr_contains("install: invalid user: 'test_invalid_user'"); + + scene + .ucmd() + .arg("-g") + .arg("test_invalid_group") + .arg("-o") + .arg("test_invalid_user") + .arg(file_1) + .arg("target_file1") + .fails() + .stderr_contains("install: invalid user: 'test_invalid_user'"); +} + +#[test] +fn test_install_chown_directory_invalid() { + let scene = TestScenario::new(util_name!()); + + scene + .ucmd() + .arg("-o") + .arg("test_invalid_user") + .arg("-d") + .arg("dir1/dir2") + .fails() + .stderr_contains("install: invalid user: 'test_invalid_user'"); + + scene + .ucmd() + .arg("-g") + .arg("test_invalid_group") + .arg("-d") + .arg("dir1/dir2") + .fails() + .stderr_contains("install: invalid group: 'test_invalid_group'"); + + scene + .ucmd() + .arg("-o") + .arg("test_invalid_user") + .arg("-g") + .arg("test_invalid_group") + .arg("-d") + .arg("dir1/dir2") + .fails() + .stderr_contains("install: invalid user: 'test_invalid_user'"); + + scene + .ucmd() + .arg("-g") + .arg("test_invalid_group") + .arg("-o") + .arg("test_invalid_user") + .arg("-d") + .arg("dir1/dir2") + .fails() + .stderr_contains("install: invalid user: 'test_invalid_user'"); +} + #[test] fn test_install_compare_option() { let scene = TestScenario::new(util_name!()); From 05db5f744239d81bdfff00ef8f7c81981dc92a7f Mon Sep 17 00:00:00 2001 From: Kyle Manna Date: Sat, 18 Feb 2023 23:45:39 -0600 Subject: [PATCH 3/3] install: address merge request feedback * Explicitly handle Err() in match. * Move metdata functions deeper into `chown_optional_user_group()`. * Add `ChownFailed` to propagate errors. * Use `show_if_err!()` to wrap `chown_optional_user_group`. * Simplify chown verbose message handling. --- src/uu/install/src/install.rs | 58 ++++++++++++++++------------------- 1 file changed, 27 insertions(+), 31 deletions(-) diff --git a/src/uu/install/src/install.rs b/src/uu/install/src/install.rs index ec80709a9..5e6efd150 100644 --- a/src/uu/install/src/install.rs +++ b/src/uu/install/src/install.rs @@ -58,6 +58,7 @@ enum InstallError { DirNeedsArg(), CreateDirFailed(PathBuf, std::io::Error), ChmodFailed(PathBuf), + ChownFailed(PathBuf, String), InvalidTarget(PathBuf), TargetDirIsntDir(PathBuf), BackupFailed(PathBuf, PathBuf, std::io::Error), @@ -99,6 +100,7 @@ impl Display for InstallError { Display::fmt(&uio_error!(e, "failed to create {}", dir.quote()), f) } Self::ChmodFailed(file) => write!(f, "failed to chmod {}", file.quote()), + Self::ChownFailed(file, msg) => write!(f, "failed to chown {}: {}", file.quote(), msg), Self::InvalidTarget(target) => write!( f, "invalid target {}: No such file or directory", @@ -393,30 +395,30 @@ fn behavior(matches: &ArgMatches) -> UResult { } let owner = matches - .get_one::(OPT_OWNER) - .map(|s| s.as_str()) - .unwrap_or("") - .to_string(); + .get_one::(OPT_OWNER) + .map(|s| s.as_str()) + .unwrap_or("") + .to_string(); let owner_id = if !owner.is_empty() { match usr2uid(&owner) { Ok(u) => Some(u), - _ => return Err(InstallError::InvalidUser(owner.clone()).into()), + Err(_) => return Err(InstallError::InvalidUser(owner.clone()).into()), } } else { None }; let group = matches - .get_one::(OPT_GROUP) - .map(|s| s.as_str()) - .unwrap_or("") - .to_string(); + .get_one::(OPT_GROUP) + .map(|s| s.as_str()) + .unwrap_or("") + .to_string(); let group_id = if !group.is_empty() { match grp2gid(&group) { Ok(g) => Some(g), - _ => return Err(InstallError::InvalidGroup(group.clone()).into()), + Err(_) => return Err(InstallError::InvalidGroup(group.clone()).into()), } } else { None @@ -490,11 +492,7 @@ fn directory(paths: &[String], b: &Behavior) -> UResult<()> { continue; } - if chown_optional_user_group(path, b).is_err() { - // Error messages are printed by the chown_optional_user_group function! - uucore::error::set_exit_code(1); - continue; - } + show_if_err!(chown_optional_user_group(path, b)); } // If the exit code was set, or show! has been called at least once // (which sets the exit code as well), function execution will end after @@ -669,24 +667,22 @@ fn copy_files_into_dir(files: &[PathBuf], target_dir: &Path, b: &Behavior) -> UR /// return an empty error value. /// fn chown_optional_user_group(path: &Path, b: &Behavior) -> UResult<()> { - let meta = match fs::metadata(path) { - Ok(meta) => meta, - Err(e) => return Err(InstallError::MetadataFailed(e).into()), - }; - - let verbosity = Verbosity { - groups_only: b.owner_id.is_none(), - level: VerbosityLevel::Normal, - }; - if b.owner_id.is_some() || b.group_id.is_some() { + let meta = match fs::metadata(path) { + Ok(meta) => meta, + Err(e) => return Err(InstallError::MetadataFailed(e).into()), + }; + + // GNU coreutils doesn't print chown operations during install with verbose flag. + let verbosity = Verbosity { + groups_only: b.owner_id.is_none(), + level: VerbosityLevel::Normal, + }; + match wrap_chown(path, &meta, b.owner_id, b.group_id, false, verbosity) { - Ok(n) => { - if !n.is_empty() { - show_error!("{}", n); - } - } - Err(e) => show_error!("{}", e), + Ok(msg) if b.verbose && !msg.is_empty() => println!("chown: {msg}"), + Ok(_) => {} + Err(e) => return Err(InstallError::ChownFailed(path.to_path_buf(), e).into()), } }