diff --git a/src/uu/chgrp/src/chgrp.rs b/src/uu/chgrp/src/chgrp.rs index 065b52ccae6..f79d4a18436 100644 --- a/src/uu/chgrp/src/chgrp.rs +++ b/src/uu/chgrp/src/chgrp.rs @@ -8,7 +8,7 @@ #![cfg(unix)] use uucore::display::Quotable; -use uucore::entries; +use uucore::entries::{self, Group, Locate}; use uucore::error::{FromIo, UError, UResult, USimpleError}; use uucore::format_usage; use uucore::perms::{GidUidOwnerFilter, IfFrom, chown_base, options}; @@ -16,6 +16,7 @@ use uucore::translate; use clap::{Arg, ArgAction, ArgMatches, Command}; +use std::ffi::{OsStr, OsString}; use std::fs; use std::os::unix::fs::MetadataExt; @@ -27,27 +28,26 @@ enum ChgrpError { impl UError for ChgrpError {} -fn parse_gid_from_str(group: &str) -> Result { - if let Some(gid_str) = group.strip_prefix(':') { - // Handle :gid format +fn parse_gid_from_str(group: &OsStr) -> Result { + if let Some(gid_str) = group.to_str().and_then(|s| s.strip_prefix(':')) { gid_str .parse::() .map_err(|_| translate!("chgrp-error-invalid-group-id", "gid_str" => gid_str)) } else { - // Try as group name first - match entries::grp2gid(group) { - Ok(g) => Ok(g), - // If group name lookup fails, try parsing as raw number - Err(_) => group - .parse::() - .map_err(|_| translate!("chgrp-error-invalid-group", "group" => group)), - } + Group::locate(group).map(|g| g.gid).or_else(|_| { + group + .to_str() + .and_then(|s| s.parse::().ok()) + .ok_or_else( + || translate!("chgrp-error-invalid-group", "group" => group.to_string_lossy()), + ) + }) } } fn get_dest_gid(matches: &ArgMatches) -> UResult<(Option, String)> { let mut raw_group = String::new(); - let dest_gid = if let Some(file) = matches.get_one::(options::REFERENCE) { + let dest_gid = if let Some(file) = matches.get_one::(options::REFERENCE) { let path = std::path::Path::new(file); fs::metadata(path) .map(|meta| { @@ -60,10 +60,10 @@ fn get_dest_gid(matches: &ArgMatches) -> UResult<(Option, String)> { )? } else { let group = matches - .get_one::(options::ARG_GROUP) - .map(String::as_str) + .get_one::(options::ARG_GROUP) + .map(OsString::as_os_str) .unwrap_or_default(); - raw_group = group.to_string(); + raw_group = group.to_string_lossy().into_owned(); if group.is_empty() { None } else { @@ -78,9 +78,9 @@ fn get_dest_gid(matches: &ArgMatches) -> UResult<(Option, String)> { fn parse_gid_and_uid(matches: &ArgMatches) -> UResult { // Handle --from option - let filter = if let Some(from_group) = matches.get_one::(options::FROM) { + let filter = if let Some(from_group) = matches.get_one::(options::FROM) { let Ok(gid) = parse_gid_from_str(from_group) else { - return Err(ChgrpError::InvalidUser(from_group.clone()).into()); + return Err(ChgrpError::InvalidUser(from_group.to_string_lossy().into_owned()).into()); }; IfFrom::Group(gid) } else { @@ -159,12 +159,13 @@ pub fn uu_app() -> Command { .long(options::REFERENCE) .value_name("RFILE") .value_hint(clap::ValueHint::FilePath) - .value_parser(clap::value_parser!(std::ffi::OsString)) + .value_parser(clap::value_parser!(OsString)) .help(translate!("chgrp-help-reference")), ) .arg( Arg::new(options::FROM) .long(options::FROM) + .value_parser(clap::value_parser!(OsString)) .value_name("GROUP") .help(translate!("chgrp-help-from")), ) diff --git a/src/uu/chown/src/chown.rs b/src/uu/chown/src/chown.rs index 3e10dfcc05d..6a08b5fb270 100644 --- a/src/uu/chown/src/chown.rs +++ b/src/uu/chown/src/chown.rs @@ -13,12 +13,13 @@ use uucore::{format_usage, show_warning, translate}; use clap::{Arg, ArgAction, ArgMatches, Command}; -use std::ffi::OsString; +use std::ffi::{CString, OsStr, OsString}; use std::fs; +use std::os::unix::ffi::OsStrExt; use std::os::unix::fs::MetadataExt; fn parse_gid_uid_and_filter(matches: &ArgMatches) -> UResult { - let filter = if let Some(spec) = matches.get_one::(options::FROM) { + let filter = if let Some(spec) = matches.get_one::(options::FROM) { match parse_spec(spec, ':')? { (Some(uid), None) => IfFrom::User(uid), (None, Some(gid)) => IfFrom::Group(gid), @@ -46,11 +47,9 @@ fn parse_gid_uid_and_filter(matches: &ArgMatches) -> UResult entries::gid2grp(gid).unwrap_or_else(|_| gid.to_string()) ); } else { - raw_owner = matches - .get_one::(options::ARG_OWNER) - .unwrap() - .into(); - let (u, g) = parse_spec(&raw_owner, ':')?; + let spec = matches.get_one::(options::ARG_OWNER).unwrap(); + let (u, g) = parse_spec(spec, ':')?; + raw_owner = spec.to_string_lossy().into_owned(); dest_uid = u; dest_gid = g; } @@ -98,7 +97,8 @@ pub fn uu_app() -> Command { Arg::new(options::FROM) .long(options::FROM) .help(translate!("chown-help-from")) - .value_name("CURRENT_OWNER:CURRENT_GROUP"), + .value_name("CURRENT_OWNER:CURRENT_GROUP") + .value_parser(clap::value_parser!(OsString)), ) .arg( Arg::new(options::preserve_root::PRESERVE) @@ -151,7 +151,7 @@ pub fn uu_app() -> Command { } /// Parses the user string to extract the UID. -fn parse_uid(user: &str, spec: &str) -> UResult> { +fn parse_uid(user: &OsStr, spec: &OsStr) -> UResult> { if user.is_empty() { return Ok(None); } @@ -161,24 +161,27 @@ fn parse_uid(user: &str, spec: &str) -> UResult> { } // Fallback: `user` string contains a numeric user ID - user.parse().map(Some).map_err(|_| { - USimpleError::new( - 1, - translate!("chown-error-invalid-user", "user" => spec.quote()), - ) - }) + user.to_str() + .and_then(|s| s.parse().ok()) + .map(Some) + .ok_or_else(|| { + USimpleError::new( + 1, + translate!("chown-error-invalid-user", "user" => spec.quote()), + ) + }) } /// Parses the group string to extract the GID. -fn parse_gid(group: &str, spec: &str) -> UResult> { +fn parse_gid(group: &OsStr, spec: &OsStr) -> UResult> { if group.is_empty() { return Ok(None); } match Group::locate(group) { Ok(g) => Ok(Some(g.gid)), - Err(_) => match group.parse() { - Ok(gid) => Ok(Some(gid)), - Err(_) => Err(USimpleError::new( + Err(_) => match group.to_str().and_then(|s| s.parse().ok()) { + Some(gid) => Ok(Some(gid)), + None => Err(USimpleError::new( 1, translate!("chown-error-invalid-group", "group" => spec.quote()), )), @@ -200,14 +203,15 @@ fn parse_gid(group: &str, spec: &str) -> UResult> { /// separator between the owner and group; calling code should set /// this to `':'`. If a separator is given but the group is omitted /// (as in `"owner:"`), the group is set to the owner's login group. -fn parse_spec(spec: &str, sep: char) -> UResult<(Option, Option)> { +fn parse_spec(spec: &OsStr, sep: char) -> UResult<(Option, Option)> { assert!(['.', ':'].contains(&sep)); - let mut args = spec.splitn(2, sep); - let user = args.next().unwrap_or(""); - let group = args.next().unwrap_or(""); + let bytes = spec.as_bytes(); + let mut args = bytes.splitn(2, |&c| c == sep as u8); + let user = OsStr::from_bytes(args.next().unwrap_or_default()); + let group = OsStr::from_bytes(args.next().unwrap_or_default()); // dot separator: try as username first, fall back to owner.group (like GNU) - if sep == ':' && !spec.contains(':') && spec.contains('.') { + if sep == ':' && !bytes.contains(&b':') && bytes.contains(&b'.') { if let Ok(uid) = parse_uid(user, spec) { let gid = parse_gid(group, spec)?; return Ok((uid, gid)); @@ -222,9 +226,12 @@ fn parse_spec(spec: &str, sep: char) -> UResult<(Option, Option)> { if !user.is_empty() && group.is_empty() && spec != user { // A separator was given but the group was not ("owner:"), so the // group is set to the owner's login group. - return match Passwd::locate(user) { - // Lookup must occur through name, not ID, to be valid here. - Ok(passwd) if passwd.name == user => Ok((Some(passwd.uid), Some(passwd.gid))), + // Resolve by name only: a numeric fallback must not supply a login group. + let passwd = CString::new(user.as_bytes()) + .ok() + .and_then(|name| Passwd::locate(name.as_c_str()).ok()); + return match passwd { + Some(passwd) => Ok((Some(passwd.uid), Some(passwd.gid))), _ => Err(USimpleError::new( 1, translate!("chown-error-invalid-spec", "spec" => spec.quote()), @@ -247,18 +254,24 @@ mod test { env::set_var("LANG", "C"); } let _ = locale::setup_localization("chown"); - assert!(matches!(parse_spec(":", ':'), Ok((None, None)))); - assert!(matches!(parse_spec(".", ':'), Ok((None, None)))); - assert!(matches!(parse_spec(".", '.'), Ok((None, None)))); - assert!(format!("{}", parse_spec("::", ':').err().unwrap()).starts_with("invalid group: ")); - assert!(format!("{}", parse_spec("..", ':').err().unwrap()).starts_with("invalid group: ")); + assert!(matches!(parse_spec(OsStr::new(":"), ':'), Ok((None, None)))); + assert!(matches!(parse_spec(OsStr::new("."), ':'), Ok((None, None)))); + assert!(matches!(parse_spec(OsStr::new("."), '.'), Ok((None, None)))); + assert!( + format!("{}", parse_spec(OsStr::new("::"), ':').err().unwrap()) + .starts_with("invalid group: ") + ); + assert!( + format!("{}", parse_spec(OsStr::new(".."), ':').err().unwrap()) + .starts_with("invalid group: ") + ); } /// Test that "USER:" sets the group to the user's login group. #[test] fn test_parse_spec_login_group() { if let Ok(passwd) = Passwd::locate("root") { - let (uid, gid) = parse_spec("root:", ':').unwrap(); + let (uid, gid) = parse_spec(OsStr::new("root:"), ':').unwrap(); assert_eq!(uid, Some(passwd.uid)); assert_eq!(gid, Some(passwd.gid)); } @@ -268,11 +281,17 @@ mod test { #[test] fn test_parse_spec_nameless_ids() { // This assumes that there is no named user with ID 12345. - assert!(matches!(parse_spec("12345", ':'), Ok((Some(12345), None)))); + assert!(matches!( + parse_spec(OsStr::new("12345"), ':'), + Ok((Some(12345), None)) + )); // This assumes that there is no named group with ID 54321. - assert!(matches!(parse_spec(":54321", ':'), Ok((None, Some(54321))))); assert!(matches!( - parse_spec("12345:54321", ':'), + parse_spec(OsStr::new(":54321"), ':'), + Ok((None, Some(54321))) + )); + assert!(matches!( + parse_spec(OsStr::new("12345:54321"), ':'), Ok((Some(12345), Some(54321))) )); } diff --git a/src/uu/pinky/src/platform/unix.rs b/src/uu/pinky/src/platform/unix.rs index b8adb543a43..725f8f87bd0 100644 --- a/src/uu/pinky/src/platform/unix.rs +++ b/src/uu/pinky/src/platform/unix.rs @@ -228,7 +228,7 @@ impl Pinky { column!(row, "{1:<8.0$}", utmpx::UT_NAMESIZE, ut.user()); if self.layout.real_name { - let name = Passwd::locate(ut.user().as_ref()) + let name = Passwd::locate(ut.user().as_str()) .ok() .and_then(|pw| real_name(&pw.name, pw.user_info.as_deref())); match name { diff --git a/src/uucore/src/lib/features/entries.rs b/src/uucore/src/lib/features/entries.rs index bcf6779130f..5a2c634589f 100644 --- a/src/uucore/src/lib/features/entries.rs +++ b/src/uucore/src/lib/features/entries.rs @@ -39,10 +39,11 @@ use libc::{getgrgid, getgrnam}; use libc::{getpwnam, getpwuid, group, passwd}; use libc::{gid_t, uid_t}; -use std::ffi::CString; +use std::ffi::{CString, OsStr}; use std::io::Error as IOError; use std::io::ErrorKind; use std::io::Result as IOResult; +use std::os::unix::ffi::OsStrExt; use std::ptr; use std::sync::Mutex; @@ -277,35 +278,41 @@ macro_rules! f { } } - impl<'a> Locate<&'a str> for $st { - fn locate(k: &'a str) -> IOResult { + impl Locate<&CStr> for $st { + fn locate(k: &CStr) -> IOResult { let _guard = PW_LOCK.lock(); - // SAFETY: We're holding PW_LOCK. + // SAFETY: We're holding PW_LOCK and k is NUL-terminated. unsafe { - let cstring = CString::new(k)?; - // try to get user or group with name matching the input with these tow lines: - // f!(getpwnam, getpwuid, uid_t, Passwd); - // f!(getgrnam, getgrgid, gid_t, Group); - let data = $fnam(cstring.as_ptr()); + let data = $fnam(k.as_ptr()); if !data.is_null() { - return Ok($st::from_raw(ptr::read(data.cast_const()))); - } - if let Ok(id) = k.parse::<$t>() { - let data = $fid(id); - if !data.is_null() { - Ok($st::from_raw(ptr::read(data.cast_const()))) - } else { - Err(IOError::new( - ErrorKind::NotFound, - format!("No such id: {id}"), - )) - } + Ok($st::from_raw(ptr::read(data.cast_const()))) } else { - Err(IOError::new(ErrorKind::NotFound, format!("Not found: {k}"))) + Err(IOError::new( + ErrorKind::NotFound, + format!("Not found: {}", k.to_string_lossy()), + )) } } } } + + impl Locate<&OsStr> for $st { + fn locate(k: &OsStr) -> IOResult { + let name = CString::new(k.as_bytes())?; + Self::locate(name.as_c_str()).or_else(|error| { + match k.to_str().and_then(|s| s.parse::<$t>().ok()) { + Some(id) => Self::locate(id), + None => Err(error), + } + }) + } + } + + impl Locate<&str> for $st { + fn locate(k: &str) -> IOResult { + Self::locate(OsStr::new(k)) + } + } }; } diff --git a/src/uucore/src/lib/features/perms.rs b/src/uucore/src/lib/features/perms.rs index 3eb3dca9fae..0c5e2b86554 100644 --- a/src/uucore/src/lib/features/perms.rs +++ b/src/uucore/src/lib/features/perms.rs @@ -986,11 +986,11 @@ pub fn chown_base( if help || !reference { // add both positional arguments // arg_group is only required if - command = command.arg( - Arg::new(add_arg_if_not_reference) - .value_name(add_arg_if_not_reference) - .required(true), - ); + let owner_arg = Arg::new(add_arg_if_not_reference) + .value_name(add_arg_if_not_reference) + .required(true) + .value_parser(clap::value_parser!(OsString)); + command = command.arg(owner_arg); } command = command.arg( Arg::new(options::ARG_FILES) diff --git a/tests/by-util/test_chgrp.rs b/tests/by-util/test_chgrp.rs index 3f29e130a69..d8f924fade5 100644 --- a/tests/by-util/test_chgrp.rs +++ b/tests/by-util/test_chgrp.rs @@ -693,3 +693,23 @@ fn verbose_missing_file_write_error_is_reported_not_panic() { .fails_with_code(1) .stderr_contains("chgrp: write error: No space left on device"); } + +#[test] +fn test_unknown_non_utf8_group_reports_lookup_error() { + use std::ffi::OsStr; + use std::os::unix::ffi::OsStrExt; + + new_ucmd!() + .args(&[OsStr::from_bytes(b"unknown_\xff"), OsStr::new("file")]) + .fails_with_code(1) + .stderr_contains("invalid group:"); + new_ucmd!() + .args(&[ + OsStr::new("--from"), + OsStr::from_bytes(b"unknown_\xff"), + OsStr::new(&getegid().to_string()), + OsStr::new("file"), + ]) + .fails_with_code(1) + .stderr_contains("invalid user:"); +} diff --git a/tests/by-util/test_chown.rs b/tests/by-util/test_chown.rs index 4d77788792a..808d8f8c906 100644 --- a/tests/by-util/test_chown.rs +++ b/tests/by-util/test_chown.rs @@ -1141,3 +1141,25 @@ fn verbose_missing_file_write_error_is_reported_not_panic() { .fails_with_code(1) .stderr_contains("chown: write error: No space left on device"); } + +#[test] +fn test_unknown_non_utf8_account_reports_lookup_error() { + use std::ffi::OsStr; + use std::os::unix::ffi::OsStrExt; + for (spec, message) in [ + (b"unknown_\xff".as_slice(), "invalid user:"), + (b":unknown_\xff".as_slice(), "invalid group:"), + ] { + new_ucmd!() + .arg(OsStr::from_bytes(spec)) + .arg("file") + .fails_with_code(1) + .stderr_contains(message); + new_ucmd!() + .arg("--from") + .arg(OsStr::from_bytes(spec)) + .args(&["0", "file"]) + .fails_with_code(1) + .stderr_contains(message); + } +}