Merge #1672

1672: Make `uname` always safe r=asomers a=koute

Currently `uname` doesn't check for errors and just blindly assumes that it always succeeds. According to the manpage this function can fail, even though no actual errors are defined:

```
RETURN VALUE
       Upon successful completion, a non-negative value shall be returned.  Otherwise, -1 shall be returned and errno set to indicate the error.

ERRORS
       No errors are defined.

       The following sections are informative.
```

Looking at [the glibc's sources](https://github.com/bminor/glibc/blob/b92a49359f33a461db080a33940d73f47c756126/posix/uname.c#L29) we can see that it indeed could fail if the internal `gethostname` call fails for some reason.

This code also assumes that every field of `utsname` is going to be initialized by the call to `uname`, which apparently is also not true. Even though the interface doesn't expose this field so it's not a problem in practice (although it might be UB since we do call `assume_init` on the whole struct) [the `utsname` does have a `domainname` field](https://docs.rs/libc/0.2.119/libc/struct.utsname.html) which glibc doesn't initialize.

The code also assumes that every field is a valid UTF-8 string, which is also technically not guaranteed.

The code also assumes that every field will be null terminated, which might not be true if any of the strings are too long (since glibc uses `strncpy` which will *not* null-terminate the string if it ends up running out of space).

This PR should fix all of these problems.

This is a breaking change.

Co-authored-by: Jan Bujak <jan@parity.io>

4 files changed, 50 insertions, 44 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index f52b5cdc..af84248e 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -80,6 +80,8 @@ This project adheres to [Semantic Versioning](https://semver.org/).
 - Deprecated `IpAddr`, `Ipv4Addr`, and `Ipv6Addr` in favor of their equivalents
   from the standard library.
   (#[1685](https://github.com/nix-rust/nix/pull/1685))
+- `uname` now returns a `Result` instead of blindly assuming the call never fails.
+- Getters on the `UtsName` struct now return a `&OsStr` instead of `&str`.
 
 ### Fixed
 
diff --git a/src/features.rs b/src/features.rs
index ed80fd71..61080986 100644
--- a/src/features.rs
+++ b/src/features.rs
@@ -3,7 +3,9 @@ pub use self::os::*;
 
 #[cfg(any(target_os = "linux", target_os = "android"))]
 mod os {
+    use std::os::unix::ffi::OsStrExt;
     use crate::sys::utsname::uname;
+    use crate::Result;
 
     // Features:
     // * atomic cloexec on socket: 2.6.27
@@ -22,15 +24,15 @@ mod os {
         *dst += (b - b'0') as usize;
     }
 
-    fn parse_kernel_version() -> usize {
-        let u = uname();
+    fn parse_kernel_version() -> Result<usize> {
+        let u = uname()?;
 
         let mut curr:  usize = 0;
         let mut major: usize = 0;
         let mut minor: usize = 0;
         let mut patch: usize = 0;
 
-        for b in u.release().bytes() {
+        for &b in u.release().as_bytes() {
             if curr >= 3 {
                 break;
             }
@@ -50,7 +52,7 @@ mod os {
             }
         }
 
-        if major >= 3 {
+        Ok(if major >= 3 {
             VERS_3
         } else if major >= 2 {
             if minor >= 7 {
@@ -68,29 +70,29 @@ mod os {
             }
         } else {
             VERS_UNKNOWN
-        }
+        })
     }
 
-    fn kernel_version() -> usize {
+    fn kernel_version() -> Result<usize> {
         static mut KERNEL_VERS: usize = 0;
 
         unsafe {
             if KERNEL_VERS == 0 {
-                KERNEL_VERS = parse_kernel_version();
+                KERNEL_VERS = parse_kernel_version()?;
             }
 
-            KERNEL_VERS
+            Ok(KERNEL_VERS)
         }
     }
 
     /// Check if the OS supports atomic close-on-exec for sockets
     pub fn socket_atomic_cloexec() -> bool {
-        kernel_version() >= VERS_2_6_27
+        kernel_version().map(|version| version >= VERS_2_6_27).unwrap_or(false)
     }
 
     #[test]
     pub fn test_parsing_kernel_version() {
-        assert!(kernel_version() > 0);
+        assert!(kernel_version().unwrap() > 0);
     }
 }
 
diff --git a/src/sys/utsname.rs b/src/sys/utsname.rs
index 98edee04..5bd3a539 100644
--- a/src/sys/utsname.rs
+++ b/src/sys/utsname.rs
@@ -1,8 +1,9 @@
 //! Get system identification
 use std::mem;
-use libc::{self, c_char};
-use std::ffi::CStr;
-use std::str::from_utf8_unchecked;
+use std::os::unix::ffi::OsStrExt;
+use std::ffi::OsStr;
+use libc::c_char;
+use crate::{Errno, Result};
 
 /// Describes the running system.  Return type of [`uname`].
 #[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)]
@@ -10,47 +11,48 @@ use std::str::from_utf8_unchecked;
 pub struct UtsName(libc::utsname);
 
 impl UtsName {
-    /// Name of the operating system implementation
-    pub fn sysname(&self) -> &str {
-        to_str(&(&self.0.sysname as *const c_char ) as *const *const c_char)
+    /// Name of the operating system implementation.
+    pub fn sysname(&self) -> &OsStr {
+        cast_and_trim(&self.0.sysname)
     }
 
     /// Network name of this machine.
-    pub fn nodename(&self) -> &str {
-        to_str(&(&self.0.nodename as *const c_char ) as *const *const c_char)
+    pub fn nodename(&self) -> &OsStr {
+        cast_and_trim(&self.0.nodename)
     }
 
     /// Release level of the operating system.
-    pub fn release(&self) -> &str {
-        to_str(&(&self.0.release as *const c_char ) as *const *const c_char)
+    pub fn release(&self) -> &OsStr {
+        cast_and_trim(&self.0.release)
     }
 
     /// Version level of the operating system.
-    pub fn version(&self) -> &str {
-        to_str(&(&self.0.version as *const c_char ) as *const *const c_char)
+    pub fn version(&self) -> &OsStr {
+        cast_and_trim(&self.0.version)
     }
 
     /// Machine hardware platform.
-    pub fn machine(&self) -> &str {
-        to_str(&(&self.0.machine as *const c_char ) as *const *const c_char)
+    pub fn machine(&self) -> &OsStr {
+        cast_and_trim(&self.0.machine)
     }
 }
 
 /// Get system identification
-pub fn uname() -> UtsName {
+pub fn uname() -> Result<UtsName> {
     unsafe {
-        let mut ret = mem::MaybeUninit::uninit();
-        libc::uname(ret.as_mut_ptr());
-        UtsName(ret.assume_init())
+        let mut ret = mem::MaybeUninit::zeroed();
+        Errno::result(libc::uname(ret.as_mut_ptr()))?;
+        Ok(UtsName(ret.assume_init()))
     }
 }
 
-#[inline]
-fn to_str<'a>(s: *const *const c_char) -> &'a str {
-    unsafe {
-        let res = CStr::from_ptr(*s).to_bytes();
-        from_utf8_unchecked(res)
-    }
+fn cast_and_trim(slice: &[c_char]) -> &OsStr {
+    let length = slice.iter().position(|&byte| byte == 0).unwrap_or(slice.len());
+    let bytes = unsafe {
+        std::slice::from_raw_parts(slice.as_ptr().cast(), length)
+    };
+
+    OsStr::from_bytes(bytes)
 }
 
 #[cfg(test)]
@@ -58,18 +60,18 @@ mod test {
     #[cfg(target_os = "linux")]
     #[test]
     pub fn test_uname_linux() {
-        assert_eq!(super::uname().sysname(), "Linux");
+        assert_eq!(super::uname().unwrap().sysname(), "Linux");
     }
 
     #[cfg(any(target_os = "macos", target_os = "ios"))]
     #[test]
     pub fn test_uname_darwin() {
-        assert_eq!(super::uname().sysname(), "Darwin");
+        assert_eq!(super::uname().unwrap().sysname(), "Darwin");
     }
 
     #[cfg(target_os = "freebsd")]
     #[test]
     pub fn test_uname_freebsd() {
-        assert_eq!(super::uname().sysname(), "FreeBSD");
+        assert_eq!(super::uname().unwrap().sysname(), "FreeBSD");
     }
 }
diff --git a/test/common/mod.rs b/test/common/mod.rs
index 84a0b4fa..544a1ae3 100644
--- a/test/common/mod.rs
+++ b/test/common/mod.rs
@@ -111,15 +111,15 @@ cfg_if! {
                 let version_requirement = VersionReq::parse($version_requirement)
                         .expect("Bad match_version provided");
 
-                let uname = nix::sys::utsname::uname();
-                println!("{}", uname.sysname());
-                println!("{}", uname.nodename());
-                println!("{}", uname.release());
-                println!("{}", uname.version());
-                println!("{}", uname.machine());
+                let uname = nix::sys::utsname::uname().unwrap();
+                println!("{}", uname.sysname().to_str().unwrap());
+                println!("{}", uname.nodename().to_str().unwrap());
+                println!("{}", uname.release().to_str().unwrap());
+                println!("{}", uname.version().to_str().unwrap());
+                println!("{}", uname.machine().to_str().unwrap());
 
                 // Fix stuff that the semver parser can't handle
-                let fixed_release = &uname.release().to_string()
+                let fixed_release = &uname.release().to_str().unwrap().to_string()
                     // Fedora 33 reports version as 4.18.el8_2.x86_64 or
                     // 5.18.200-fc33.x86_64.  Remove the underscore.
                     .replace("_", "-")