Merge #1538 #1545 #1546

1538: posix_fadvise doesn't return -1 as sentinel value r=asomers a=ocadaruma

## Summary
- `posix_fadvise(2)` does return error number directly (i.e. not through `errno`)
  * refs: https://man7.org/linux/man-pages/man2/posix_fadvise.2.html , https://man7.org/linux/man-pages/man2/posix_fadvise.2.html
- However `posix_fadvise`-binding uses `Errno::result` to translate the error now, which is mis-use.

1545: Fix memory unsafety in unistd::getgrouplist r=asomers a=asomers

Fixes #1541

1546: Revert "Expose SockAddr::from_raw_sockaddr" r=asomers a=asomers

This reverts commit ed43d2c.

As discussed in #1544 the API of this function needs to change.  For
now, revert the PR that made it public, because it has not yet been
included in any release.

Co-authored-by: Haruki Okada <ocadaruma@gmail.com>
Co-authored-by: vitalyd <vitalyd@gmail.com>
Co-authored-by: Alan Somers <asomers@gmail.com>

Author: 4 people

CHANGELOG.md

@@ -83,11 +83,17 @@ This project adheres to [Semantic Versioning](https://semver.org/).
 
 ### Fixed
 
+- `posix_fadvise` now returns errors in the conventional way, rather than as a
+  non-zero value in `Ok()`.
+  (#[1538](https://github.com/nix-rust/nix/pull/1538))
 - Added more errno definitions for better backwards compatibility with
   Nix 0.21.0.
   (#[1467](https://github.com/nix-rust/nix/pull/1467))
 - Fixed potential undefined behavior in `Signal::try_from` on some platforms.
   (#[1484](https://github.com/nix-rust/nix/pull/1484))
+- Fixed buffer overflow in `unistd::getgrouplist`.
+  (#[1545](https://github.com/nix-rust/nix/pull/1545))
+
 
 ### Removed
 

src/fcntl.rs

@@ -667,9 +667,14 @@ mod posix_fadvise {
         offset: libc::off_t,
         len: libc::off_t,
         advice: PosixFadviseAdvice,
-    ) -> Result<libc::c_int> {
+    ) -> Result<()> {
         let res = unsafe { libc::posix_fadvise(fd, offset, len, advice as libc::c_int) };
-        Errno::result(res)
+
+        if res == 0 {
+            Ok(())
+        } else {
+            Err(Errno::from_i32(res))
+        }
     }
 }
 

src/ifaddrs.rs

@@ -46,8 +46,8 @@ impl InterfaceAddress {
     /// Create an `InterfaceAddress` from the libc struct.
     fn from_libc_ifaddrs(info: &libc::ifaddrs) -> InterfaceAddress {
         let ifname = unsafe { ffi::CStr::from_ptr(info.ifa_name) };
-        let address = unsafe { SockAddr::from_raw_sockaddr(info.ifa_addr) };
-        let netmask = unsafe { SockAddr::from_raw_sockaddr(info.ifa_netmask) };
+        let address = unsafe { SockAddr::from_libc_sockaddr(info.ifa_addr) };
+        let netmask = unsafe { SockAddr::from_libc_sockaddr(info.ifa_netmask) };
         let mut addr = InterfaceAddress {
             interface_name: ifname.to_string_lossy().to_string(),
             flags: InterfaceFlags::from_bits_truncate(info.ifa_flags as i32),
@@ -59,9 +59,9 @@ impl InterfaceAddress {
 
         let ifu = get_ifu_from_sockaddr(info);
         if addr.flags.contains(InterfaceFlags::IFF_POINTOPOINT) {
-            addr.destination = unsafe { SockAddr::from_raw_sockaddr(ifu) };
+            addr.destination = unsafe { SockAddr::from_libc_sockaddr(ifu) };
         } else if addr.flags.contains(InterfaceFlags::IFF_BROADCAST) {
-            addr.broadcast = unsafe { SockAddr::from_raw_sockaddr(ifu) };
+            addr.broadcast = unsafe { SockAddr::from_libc_sockaddr(ifu) };
         }
 
         addr

src/sys/socket/addr.rs

@@ -802,7 +802,7 @@ impl SockAddr {
     /// unsafe because it takes a raw pointer as argument.  The caller must
     /// ensure that the pointer is valid.
     #[cfg(not(target_os = "fuchsia"))]
-    pub unsafe fn from_raw_sockaddr(addr: *const libc::sockaddr) -> Option<SockAddr> {
+    pub(crate) unsafe fn from_libc_sockaddr(addr: *const libc::sockaddr) -> Option<SockAddr> {
         if addr.is_null() {
             None
         } else {
@@ -1381,7 +1381,7 @@ mod tests {
     fn test_macos_loopback_datalink_addr() {
         let bytes = [20i8, 18, 1, 0, 24, 3, 0, 0, 108, 111, 48, 0, 0, 0, 0, 0];
         let sa = bytes.as_ptr() as *const libc::sockaddr;
-        let _sock_addr = unsafe { SockAddr::from_raw_sockaddr(sa) };
+        let _sock_addr = unsafe { SockAddr::from_libc_sockaddr(sa) };
         assert!(_sock_addr.is_none());
     }
 
@@ -1396,7 +1396,7 @@ mod tests {
         let bytes = [20i8, 18, 7, 0, 6, 3, 6, 0, 101, 110, 48, 24, 101, -112, -35, 76, -80];
         let ptr = bytes.as_ptr();
         let sa = ptr as *const libc::sockaddr;
-        let _sock_addr = unsafe { SockAddr::from_raw_sockaddr(sa) };
+        let _sock_addr = unsafe { SockAddr::from_libc_sockaddr(sa) };
 
         assert!(_sock_addr.is_some());
 
@@ -1418,7 +1418,7 @@ mod tests {
         let bytes = [25u8, 0, 0, 0, 6, 0, 6, 0, 24, 101, 144, 221, 76, 176];
         let ptr = bytes.as_ptr();
         let sa = ptr as *const libc::sockaddr;
-        let _sock_addr = unsafe { SockAddr::from_raw_sockaddr(sa) };
+        let _sock_addr = unsafe { SockAddr::from_libc_sockaddr(sa) };
 
         assert!(_sock_addr.is_some());
 

src/unistd.rs

@@ -1540,8 +1540,7 @@ pub fn getgrouplist(user: &CStr, group: Gid) -> Result<Vec<Gid>> {
         Ok(None) | Err(_) => <c_int>::max_value(),
     };
     use std::cmp::min;
-    let mut ngroups = min(ngroups_max, 8);
-    let mut groups = Vec::<Gid>::with_capacity(ngroups as usize);
+    let mut groups = Vec::<Gid>::with_capacity(min(ngroups_max, 8) as usize);
     cfg_if! {
         if #[cfg(any(target_os = "ios", target_os = "macos"))] {
             type getgrouplist_group_t = c_int;
@@ -1551,6 +1550,7 @@ pub fn getgrouplist(user: &CStr, group: Gid) -> Result<Vec<Gid>> {
     }
     let gid: gid_t = group.into();
     loop {
+        let mut ngroups = groups.capacity() as i32;
         let ret = unsafe {
             libc::getgrouplist(user.as_ptr(),
                                gid as getgrouplist_group_t,

test/test_fcntl.rs

@@ -473,17 +473,16 @@ mod test_posix_fadvise {
     fn test_success() {
         let tmp = NamedTempFile::new().unwrap();
         let fd = tmp.as_raw_fd();
-        let res = posix_fadvise(fd, 0, 100, PosixFadviseAdvice::POSIX_FADV_WILLNEED).unwrap();
+        let res = posix_fadvise(fd, 0, 100, PosixFadviseAdvice::POSIX_FADV_WILLNEED);
 
-        assert_eq!(res, 0);
+        assert!(res.is_ok());
     }
 
     #[test]
     fn test_errno() {
         let (rd, _wr) = pipe().unwrap();
-        let errno = posix_fadvise(rd as RawFd, 0, 100, PosixFadviseAdvice::POSIX_FADV_WILLNEED)
-                                 .unwrap();
-        assert_eq!(errno, Errno::ESPIPE as i32);
+        let res = posix_fadvise(rd as RawFd, 0, 100, PosixFadviseAdvice::POSIX_FADV_WILLNEED);
+        assert_eq!(res, Err(Errno::ESPIPE));
     }
 }