chown: preserve non-UTF-8 account names

src/uu/chgrp/src/chgrp.rs

@@ -8,14 +8,15 @@
 #![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};
 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<u32, String> {
-    if let Some(gid_str) = group.strip_prefix(':') {
-        // Handle :gid format
+fn parse_gid_from_str(group: &OsStr) -> Result<u32, String> {
+    if let Some(gid_str) = group.to_str().and_then(|s| s.strip_prefix(':')) {
         gid_str
             .parse::<u32>()
             .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::<u32>()
-                .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::<u32>().ok())
+                .ok_or_else(
+                    || translate!("chgrp-error-invalid-group", "group" => group.to_string_lossy()),
+                )
+        })
     }
 }
 
 fn get_dest_gid(matches: &ArgMatches) -> UResult<(Option<u32>, String)> {
     let mut raw_group = String::new();
-    let dest_gid = if let Some(file) = matches.get_one::<std::ffi::OsString>(options::REFERENCE) {
+    let dest_gid = if let Some(file) = matches.get_one::<OsString>(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<u32>, String)> {
             )?
     } else {
         let group = matches
-            .get_one::<String>(options::ARG_GROUP)
-            .map(String::as_str)
+            .get_one::<OsString>(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<u32>, String)> {
 
 fn parse_gid_and_uid(matches: &ArgMatches) -> UResult<GidUidOwnerFilter> {
     // Handle --from option
-    let filter = if let Some(from_group) = matches.get_one::<String>(options::FROM) {
+    let filter = if let Some(from_group) = matches.get_one::<OsString>(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")),
         )

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<GidUidOwnerFilter> {
-    let filter = if let Some(spec) = matches.get_one::<String>(options::FROM) {
+    let filter = if let Some(spec) = matches.get_one::<OsString>(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<GidUidOwnerFilter>
             entries::gid2grp(gid).unwrap_or_else(|_| gid.to_string())
         );
     } else {
-        raw_owner = matches
-            .get_one::<String>(options::ARG_OWNER)
-            .unwrap()
-            .into();
-        let (u, g) = parse_spec(&raw_owner, ':')?;
+        let spec = matches.get_one::<OsString>(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<Option<u32>> {
+fn parse_uid(user: &OsStr, spec: &OsStr) -> UResult<Option<u32>> {
     if user.is_empty() {
         return Ok(None);
     }
@@ -161,24 +161,27 @@ fn parse_uid(user: &str, spec: &str) -> UResult<Option<u32>> {
     }
 
     // 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<Option<u32>> {
+fn parse_gid(group: &OsStr, spec: &OsStr) -> UResult<Option<u32>> {
     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<Option<u32>> {
 /// 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<u32>, Option<u32>)> {
+fn parse_spec(spec: &OsStr, sep: char) -> UResult<(Option<u32>, Option<u32>)> {
     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<u32>, Option<u32>)> {
     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)))
         ));
     }

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 {

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<Self> {
+        impl Locate<&CStr> for $st {
+            fn locate(k: &CStr) -> IOResult<Self> {
                 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<Self> {
+                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> {
+                Self::locate(OsStr::new(k))
+            }
+        }
     };
 }
 

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)

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:");
+}