From 1671edc3e7d3fea63fbf721071bd2ddbad8e9e67 Mon Sep 17 00:00:00 2001 From: vitalyd Date: Mon, 27 Sep 2021 09:55:59 -0400 Subject: [PATCH 1/3] Fix memory unsafety in unistd::getgrouplist Fixes #1541 --- CHANGELOG.md | 3 ++- src/unistd.rs | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f81268a0e5..ec3dc7c39f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -91,9 +91,10 @@ This project adheres to [Semantic Versioning](https://semver.org/). - 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 diff --git a/src/unistd.rs b/src/unistd.rs index 64759dc6bd..a9862d37a3 100644 --- a/src/unistd.rs +++ b/src/unistd.rs @@ -1540,8 +1540,7 @@ pub fn getgrouplist(user: &CStr, group: Gid) -> Result> { Ok(None) | Err(_) => ::max_value(), }; use std::cmp::min; - let mut ngroups = min(ngroups_max, 8); - let mut groups = Vec::::with_capacity(ngroups as usize); + let mut groups = Vec::::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> { } 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, From 8d6c2b3a70c650e4aed47d735c9f236ee45e9d26 Mon Sep 17 00:00:00 2001 From: Alan Somers Date: Tue, 28 Sep 2021 19:09:15 -0600 Subject: [PATCH 2/3] Revert "Expose SockAddr::from_raw_sockaddr" This reverts commit ed43d2c65e65dd68c9cf2dcf06f5ec45a44aaccd. 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. --- src/ifaddrs.rs | 8 ++++---- src/sys/socket/addr.rs | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/ifaddrs.rs b/src/ifaddrs.rs index 74f340505e..ed6328f3ef 100644 --- a/src/ifaddrs.rs +++ b/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 diff --git a/src/sys/socket/addr.rs b/src/sys/socket/addr.rs index 832a153f2a..b119642b3f 100644 --- a/src/sys/socket/addr.rs +++ b/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 { + pub(crate) unsafe fn from_libc_sockaddr(addr: *const libc::sockaddr) -> Option { 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()); From a60703430ee708f3899b411f7756e65ea44c8f2a Mon Sep 17 00:00:00 2001 From: Haruki Okada Date: Thu, 23 Sep 2021 20:31:17 +0900 Subject: [PATCH 3/3] Fix return value of posix_fadvise libc::posix_fadvise returns errnos directly rather than in the errno variable. --- CHANGELOG.md | 4 ++++ src/fcntl.rs | 9 +++++++-- test/test_fcntl.rs | 9 ++++----- 3 files changed, 15 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1139cf099d..af4fa9bc67 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -83,12 +83,16 @@ 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)) + ### Removed - Removed a couple of termios constants on redox that were never actually diff --git a/src/fcntl.rs b/src/fcntl.rs index aded27b444..dd8e59a6ec 100644 --- a/src/fcntl.rs +++ b/src/fcntl.rs @@ -667,9 +667,14 @@ mod posix_fadvise { offset: libc::off_t, len: libc::off_t, advice: PosixFadviseAdvice, - ) -> Result { + ) -> 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)) + } } } diff --git a/test/test_fcntl.rs b/test/test_fcntl.rs index 76252e6e06..db2acfbf52 100644 --- a/test/test_fcntl.rs +++ b/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)); } }