Merge pull request #2653 from blyxxyz/entries-safety

uucore::entries: Fix safety issues

.vscode/cspell.dictionaries/workspace.wordlist.txt

@@ -182,6 +182,7 @@ getgrgid
 getgrnam
 getgrouplist
 getgroups
+getpwent
 getpwnam
 getpwuid
 getuid

src/uu/chown/src/chown.rs

@@ -183,7 +183,7 @@ fn parse_spec(spec: &str, sep: char) -> UResult<(Option<u32>, Option<u32>)> {
 
     let uid = if !user.is_empty() {
         Some(match Passwd::locate(user) {
-            Ok(u) => u.uid(), // We have been able to get the uid
+            Ok(u) => u.uid, // We have been able to get the uid
             Err(_) =>
             // we have NOT been able to find the uid
             // but we could be in the case where we have user.group
@@ -208,7 +208,7 @@ fn parse_spec(spec: &str, sep: char) -> UResult<(Option<u32>, Option<u32>)> {
         Some(
             Group::locate(group)
                 .map_err(|_| USimpleError::new(1, format!("invalid group: {}", spec.quote())))?
-                .gid(),
+                .gid,
         )
     } else {
         None

src/uu/id/src/id.rs

@@ -245,7 +245,7 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
         // GNU's `id` does not support the flags: -p/-P/-A.
         if matches.is_present(options::OPT_PASSWORD) {
             // BSD's `id` ignores all but the first specified user
-            pline(possible_pw.map(|v| v.uid()));
+            pline(possible_pw.as_ref().map(|v| v.uid));
             return Ok(());
         };
         if matches.is_present(options::OPT_HUMAN_READABLE) {
@@ -259,7 +259,7 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
             return Ok(());
         }
 
-        let (uid, gid) = possible_pw.map(|p| (p.uid(), p.gid())).unwrap_or((
+        let (uid, gid) = possible_pw.as_ref().map(|p| (p.uid, p.gid)).unwrap_or((
             if state.rflag { getuid() } else { geteuid() },
             if state.rflag { getgid() } else { getegid() },
         ));
@@ -302,7 +302,7 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
 
         let groups = entries::get_groups_gnu(Some(gid)).unwrap();
         let groups = if state.user_specified {
-            possible_pw.map(|p| p.belongs_to()).unwrap()
+            possible_pw.as_ref().map(|p| p.belongs_to()).unwrap()
         } else {
             groups.clone()
         };
@@ -453,7 +453,7 @@ pub fn uu_app() -> App<'static, 'static> {
 
 fn pretty(possible_pw: Option<Passwd>) {
     if let Some(p) = possible_pw {
-        print!("uid\t{}\ngroups\t", p.name());
+        print!("uid\t{}\ngroups\t", p.name);
         println!(
             "{}",
             p.belongs_to()
@@ -466,10 +466,10 @@ fn pretty(possible_pw: Option<Passwd>) {
         let login = cstr2cow!(getlogin() as *const _);
         let rid = getuid();
         if let Ok(p) = Passwd::locate(rid) {
-            if login == p.name() {
+            if login == p.name {
                 println!("login\t{}", login);
             }
-            println!("uid\t{}", p.name());
+            println!("uid\t{}", p.name);
         } else {
             println!("uid\t{}", rid);
         }
@@ -477,7 +477,7 @@ fn pretty(possible_pw: Option<Passwd>) {
         let eid = getegid();
         if eid == rid {
             if let Ok(p) = Passwd::locate(eid) {
-                println!("euid\t{}", p.name());
+                println!("euid\t{}", p.name);
             } else {
                 println!("euid\t{}", eid);
             }
@@ -486,7 +486,7 @@ fn pretty(possible_pw: Option<Passwd>) {
         let rid = getgid();
         if rid != eid {
             if let Ok(g) = Group::locate(rid) {
-                println!("euid\t{}", g.name());
+                println!("euid\t{}", g.name);
             } else {
                 println!("euid\t{}", rid);
             }
@@ -511,16 +511,16 @@ fn pline(possible_uid: Option<uid_t>) {
 
     println!(
         "{}:{}:{}:{}:{}:{}:{}:{}:{}:{}",
-        pw.name(),
+        pw.name,
-        pw.user_passwd(),
+        pw.user_passwd,
-        pw.uid(),
+        pw.uid,
-        pw.gid(),
+        pw.gid,
-        pw.user_access_class(),
+        pw.user_access_class,
-        pw.passwd_change_time(),
+        pw.passwd_change_time,
-        pw.expiration(),
+        pw.expiration,
-        pw.user_info(),
+        pw.user_info,
-        pw.user_dir(),
+        pw.user_dir,
-        pw.user_shell()
+        pw.user_shell
     );
 }
 
@@ -531,13 +531,7 @@ fn pline(possible_uid: Option<uid_t>) {
 
     println!(
         "{}:{}:{}:{}:{}:{}:{}",
-        pw.name(),
+        pw.name, pw.user_passwd, pw.uid, pw.gid, pw.user_info, pw.user_dir, pw.user_shell
-        pw.user_passwd(),
-        pw.uid(),
-        pw.gid(),
-        pw.user_info(),
-        pw.user_dir(),
-        pw.user_shell()
     );
 }
 

src/uu/pinky/src/pinky.rs

@@ -267,11 +267,11 @@ impl Pinky {
 
         if self.include_fullname {
             if let Ok(pw) = Passwd::locate(ut.user().as_ref()) {
-                let mut gecos = pw.user_info().into_owned();
+                let mut gecos = pw.user_info;
                 if let Some(n) = gecos.find(',') {
                     gecos.truncate(n + 1);
                 }
-                print!(" {:<19.19}", gecos.replace("&", &pw.name().capitalize()));
+                print!(" {:<19.19}", gecos.replace("&", &pw.name.capitalize()));
             } else {
                 print!(" {:19}", "        ???");
             }
@@ -333,13 +333,13 @@ impl Pinky {
         for u in &self.names {
             print!("Login name: {:<28}In real life: ", u);
             if let Ok(pw) = Passwd::locate(u.as_str()) {
-                println!(" {}", pw.user_info().replace("&", &pw.name().capitalize()));
+                println!(" {}", pw.user_info.replace("&", &pw.name.capitalize()));
                 if self.include_home_and_shell {
-                    print!("Directory: {:<29}", pw.user_dir());
+                    print!("Directory: {:<29}", pw.user_dir);
-                    println!("Shell:  {}", pw.user_shell());
+                    println!("Shell:  {}", pw.user_shell);
                 }
                 if self.include_project {
-                    let mut p = PathBuf::from(pw.user_dir().as_ref());
+                    let mut p = PathBuf::from(&pw.user_dir);
                     p.push(".project");
                     if let Ok(f) = File::open(p) {
                         print!("Project: ");
@@ -347,7 +347,7 @@ impl Pinky {
                     }
                 }
                 if self.include_plan {
-                    let mut p = PathBuf::from(pw.user_dir().as_ref());
+                    let mut p = PathBuf::from(&pw.user_dir);
                     p.push(".plan");
                     if let Ok(f) = File::open(p) {
                         println!("Plan:");

src/uucore/src/lib/features/entries.rs

@@ -41,12 +41,15 @@ use libc::{c_char, c_int, gid_t, uid_t};
 use libc::{getgrgid, getgrnam, getgroups};
 use libc::{getpwnam, getpwuid, group, passwd};
 
-use std::borrow::Cow;
+use std::convert::TryInto;
 use std::ffi::{CStr, CString};
 use std::io::Error as IOError;
 use std::io::ErrorKind;
 use std::io::Result as IOResult;
 use std::ptr;
+use std::sync::Mutex;
+
+use once_cell::sync::Lazy;
 
 extern "C" {
     /// From: https://man7.org/linux/man-pages/man3/getgrouplist.3.html
@@ -69,19 +72,31 @@ extern "C" {
 /// > to be used in a further call to getgroups().
 #[cfg(not(target_os = "redox"))]
 pub fn get_groups() -> IOResult<Vec<gid_t>> {
-    let ngroups = unsafe { getgroups(0, ptr::null_mut()) };
+    let mut groups = Vec::new();
-    if ngroups == -1 {
+    loop {
-        return Err(IOError::last_os_error());
+        let ngroups = match unsafe { getgroups(0, ptr::null_mut()) } {
-    }
+            -1 => return Err(IOError::last_os_error()),
-    let mut groups = Vec::with_capacity(ngroups as usize);
+            // Not just optimization; 0 would mess up the next call
-    let ngroups = unsafe { getgroups(ngroups, groups.as_mut_ptr()) };
+            0 => return Ok(Vec::new()),
-    if ngroups == -1 {
+            n => n,
-        Err(IOError::last_os_error())
+        };
-    } else {
+
-        unsafe {
+        // This is a small buffer, so we can afford to zero-initialize it and
-            groups.set_len(ngroups as usize);
+        // use safe Vec operations
+        groups.resize(ngroups.try_into().unwrap(), 0);
+        let res = unsafe { getgroups(ngroups, groups.as_mut_ptr()) };
+        if res == -1 {
+            let err = IOError::last_os_error();
+            if err.raw_os_error() == Some(libc::EINVAL) {
+                // Number of groups changed, retry
+                continue;
+            } else {
+                return Err(err);
+            }
+        } else {
+            groups.truncate(ngroups.try_into().unwrap());
+            return Ok(groups);
         }
-        Ok(groups)
     }
 }
 
@@ -124,77 +139,57 @@ fn sort_groups(mut groups: Vec<gid_t>, egid: gid_t) -> Vec<gid_t> {
     groups
 }
 
-#[derive(Copy, Clone)]
+#[derive(Clone, Debug)]
 pub struct Passwd {
-    inner: passwd,
+    /// AKA passwd.pw_name
+    pub name: String,
+    /// AKA passwd.pw_uid
+    pub uid: uid_t,
+    /// AKA passwd.pw_gid
+    pub gid: gid_t,
+    /// AKA passwd.pw_gecos
+    pub user_info: String,
+    /// AKA passwd.pw_shell
+    pub user_shell: String,
+    /// AKA passwd.pw_dir
+    pub user_dir: String,
+    /// AKA passwd.pw_passwd
+    pub user_passwd: String,
+    /// AKA passwd.pw_class
+    #[cfg(any(target_os = "freebsd", target_vendor = "apple"))]
+    pub user_access_class: String,
+    /// AKA passwd.pw_change
+    #[cfg(any(target_os = "freebsd", target_vendor = "apple"))]
+    pub passwd_change_time: time_t,
+    /// AKA passwd.pw_expire
+    #[cfg(any(target_os = "freebsd", target_vendor = "apple"))]
+    pub expiration: time_t,
 }
 
-macro_rules! cstr2cow {
+/// SAFETY: ptr must point to a valid C string.
-    ($v:expr) => {
+unsafe fn cstr2string(ptr: *const c_char) -> String {
-        unsafe { CStr::from_ptr($v).to_string_lossy() }
+    CStr::from_ptr(ptr).to_string_lossy().into_owned()
-    };
 }
 
 impl Passwd {
-    /// AKA passwd.pw_name
+    /// SAFETY: All the pointed-to strings must be valid and not change while
-    pub fn name(&self) -> Cow<str> {
+    /// the function runs. That means PW_LOCK must be held.
-        cstr2cow!(self.inner.pw_name)
+    unsafe fn from_raw(raw: passwd) -> Self {
-    }
+        Passwd {
-
+            name: cstr2string(raw.pw_name),
-    /// AKA passwd.pw_uid
+            uid: raw.pw_uid,
-    pub fn uid(&self) -> uid_t {
+            gid: raw.pw_gid,
-        self.inner.pw_uid
+            user_info: cstr2string(raw.pw_gecos),
-    }
+            user_shell: cstr2string(raw.pw_shell),
-
+            user_dir: cstr2string(raw.pw_dir),
-    /// AKA passwd.pw_gid
+            user_passwd: cstr2string(raw.pw_passwd),
-    pub fn gid(&self) -> gid_t {
+            #[cfg(any(target_os = "freebsd", target_vendor = "apple"))]
-        self.inner.pw_gid
+            user_access_class: cstr2string(raw.pw_class),
-    }
+            #[cfg(any(target_os = "freebsd", target_vendor = "apple"))]
-
+            passwd_change_time: raw.pw_change,
-    /// AKA passwd.pw_gecos
+            #[cfg(any(target_os = "freebsd", target_vendor = "apple"))]
-    pub fn user_info(&self) -> Cow<str> {
+            expiration: raw.pw_expire,
-        cstr2cow!(self.inner.pw_gecos)
+        }
-    }
-
-    /// AKA passwd.pw_shell
-    pub fn user_shell(&self) -> Cow<str> {
-        cstr2cow!(self.inner.pw_shell)
-    }
-
-    /// AKA passwd.pw_dir
-    pub fn user_dir(&self) -> Cow<str> {
-        cstr2cow!(self.inner.pw_dir)
-    }
-
-    /// AKA passwd.pw_passwd
-    pub fn user_passwd(&self) -> Cow<str> {
-        cstr2cow!(self.inner.pw_passwd)
-    }
-
-    /// AKA passwd.pw_class
-    #[cfg(any(target_os = "freebsd", target_vendor = "apple"))]
-    pub fn user_access_class(&self) -> Cow<str> {
-        cstr2cow!(self.inner.pw_class)
-    }
-
-    /// AKA passwd.pw_change
-    #[cfg(any(target_os = "freebsd", target_vendor = "apple"))]
-    pub fn passwd_change_time(&self) -> time_t {
-        self.inner.pw_change
-    }
-
-    /// AKA passwd.pw_expire
-    #[cfg(any(target_os = "freebsd", target_vendor = "apple"))]
-    pub fn expiration(&self) -> time_t {
-        self.inner.pw_expire
-    }
-
-    pub fn as_inner(&self) -> &passwd {
-        &self.inner
-    }
-
-    pub fn into_inner(self) -> passwd {
-        self.inner
     }
 
     /// This is a wrapper function for `libc::getgrouplist`.
@@ -214,49 +209,44 @@ impl Passwd {
     pub fn belongs_to(&self) -> Vec<gid_t> {
         let mut ngroups: c_int = 8;
         let mut ngroups_old: c_int;
-        let mut groups = Vec::with_capacity(ngroups as usize);
+        let mut groups = vec![0; ngroups.try_into().unwrap()];
-        let gid = self.inner.pw_gid;
+        let name = CString::new(self.name.clone()).unwrap();
-        let name = self.inner.pw_name;
         loop {
             ngroups_old = ngroups;
-            if unsafe { getgrouplist(name, gid, groups.as_mut_ptr(), &mut ngroups) } == -1 {
+            if unsafe { getgrouplist(name.as_ptr(), self.gid, groups.as_mut_ptr(), &mut ngroups) }
+                == -1
+            {
                 if ngroups == ngroups_old {
                     ngroups *= 2;
                 }
-                groups.resize(ngroups as usize, 0);
+                groups.resize(ngroups.try_into().unwrap(), 0);
             } else {
                 break;
             }
         }
-        unsafe {
+        let ngroups = ngroups.try_into().unwrap();
-            groups.set_len(ngroups as usize);
+        assert!(ngroups <= groups.len());
-        }
+        groups.truncate(ngroups);
-        groups.truncate(ngroups as usize);
         groups
     }
 }
 
+#[derive(Clone, Debug)]
 pub struct Group {
-    inner: group,
+    /// AKA group.gr_name
+    pub name: String,
+    /// AKA group.gr_gid
+    pub gid: gid_t,
 }
 
 impl Group {
-    /// AKA group.gr_name
+    /// SAFETY: gr_name must be valid and not change while
-    pub fn name(&self) -> Cow<str> {
+    /// the function runs. That means PW_LOCK must be held.
-        cstr2cow!(self.inner.gr_name)
+    unsafe fn from_raw(raw: group) -> Self {
-    }
+        Group {
-
+            name: cstr2string(raw.gr_name),
-    /// AKA group.gr_gid
+            gid: raw.gr_gid,
-    pub fn gid(&self) -> gid_t {
+        }
-        self.inner.gr_gid
-    }
-
-    pub fn as_inner(&self) -> &group {
-        &self.inner
-    }
-
-    pub fn into_inner(self) -> group {
-        self.inner
     }
 }
 
@@ -267,17 +257,32 @@ pub trait Locate<K> {
         Self: ::std::marker::Sized;
 }
 
+// These functions are not thread-safe:
+// > The return value may point to a static area, and may be
+// > overwritten by subsequent calls to getpwent(3), getpwnam(),
+// > or getpwuid().
+// This applies not just to the struct but also the strings it points
+// to, so we must copy all the data we want before releasing the lock.
+// (Technically we must also ensure that the raw functions aren't being called
+// anywhere else in the program.)
+static PW_LOCK: Lazy<Mutex<()>> = Lazy::new(|| Mutex::new(()));
+
 macro_rules! f {
     ($fnam:ident, $fid:ident, $t:ident, $st:ident) => {
         impl Locate<$t> for $st {
             fn locate(k: $t) -> IOResult<Self> {
+                let _guard = PW_LOCK.lock();
+                // SAFETY: We're holding PW_LOCK.
                 unsafe {
                     let data = $fid(k);
                     if !data.is_null() {
-                        Ok($st {
+                        Ok($st::from_raw(ptr::read(data as *const _)))
-                            inner: ptr::read(data as *const _),
-                        })
                     } else {
+                        // FIXME: Resource limits, signals and I/O failure may
+                        // cause this too. See getpwnam(3).
+                        // errno must be set to zero before the call. We can
+                        // use libc::__errno_location() on some platforms.
+                        // The same applies for the two cases below.
                         Err(IOError::new(
                             ErrorKind::NotFound,
                             format!("No such id: {}", k),
@@ -289,25 +294,26 @@ macro_rules! f {
 
         impl<'a> Locate<&'a str> for $st {
             fn locate(k: &'a str) -> IOResult<Self> {
+                let _guard = PW_LOCK.lock();
                 if let Ok(id) = k.parse::<$t>() {
-                    let data = unsafe { $fid(id) };
+                    // SAFETY: We're holding PW_LOCK.
-                    if !data.is_null() {
+                    unsafe {
-                        Ok($st {
+                        let data = $fid(id);
-                            inner: unsafe { ptr::read(data as *const _) },
+                        if !data.is_null() {
-                        })
+                            Ok($st::from_raw(ptr::read(data as *const _)))
-                    } else {
+                        } else {
-                        Err(IOError::new(
+                            Err(IOError::new(
-                            ErrorKind::NotFound,
+                                ErrorKind::NotFound,
-                            format!("No such id: {}", id),
+                                format!("No such id: {}", id),
-                        ))
+                            ))
+                        }
                     }
                 } else {
+                    // SAFETY: We're holding PW_LOCK.
                     unsafe {
                         let data = $fnam(CString::new(k).unwrap().as_ptr());
                         if !data.is_null() {
-                            Ok($st {
+                            Ok($st::from_raw(ptr::read(data as *const _)))
-                                inner: ptr::read(data as *const _),
-                            })
                         } else {
                             Err(IOError::new(
                                 ErrorKind::NotFound,
@@ -327,24 +333,24 @@ f!(getgrnam, getgrgid, gid_t, Group);
 
 #[inline]
 pub fn uid2usr(id: uid_t) -> IOResult<String> {
-    Passwd::locate(id).map(|p| p.name().into_owned())
+    Passwd::locate(id).map(|p| p.name)
 }
 
 #[cfg(not(target_os = "redox"))]
 #[inline]
 pub fn gid2grp(id: gid_t) -> IOResult<String> {
-    Group::locate(id).map(|p| p.name().into_owned())
+    Group::locate(id).map(|p| p.name)
 }
 
 #[inline]
 pub fn usr2uid(name: &str) -> IOResult<uid_t> {
-    Passwd::locate(name).map(|p| p.uid())
+    Passwd::locate(name).map(|p| p.uid)
 }
 
 #[cfg(not(target_os = "redox"))]
 #[inline]
 pub fn grp2gid(name: &str) -> IOResult<gid_t> {
-    Group::locate(name).map(|p| p.gid())
+    Group::locate(name).map(|p| p.gid)
 }
 
 #[cfg(test)]

tests/by-util/test_pinky.rs

@@ -24,14 +24,11 @@ fn test_capitalize() {
 fn test_long_format() {
     let login = "root";
     let pw: Passwd = Passwd::locate(login).unwrap();
-    let real_name = pw.user_info().replace("&", &pw.name().capitalize());
+    let real_name = pw.user_info.replace("&", &pw.name.capitalize());
     let ts = TestScenario::new(util_name!());
     ts.ucmd().arg("-l").arg(login).succeeds().stdout_is(format!(
         "Login name: {:<28}In real life:  {}\nDirectory: {:<29}Shell:  {}\n\n",
-        login,
+        login, real_name, pw.user_dir, pw.user_shell
-        real_name,
-        pw.user_dir(),
-        pw.user_shell()
     ));
 
     ts.ucmd()