Merge #1121 #1130

1121: Fix #1093 r=asomers a=tathanhdinh

Hello,
This PR fixes the problem discovered in #1093.
Thanks for any comment.

1130: unistd: getgrouplist: Rework code to use `reserve_double_buffer_size` r=asomers a=otavio

The buffer resize logic can be simplified reusing the
`reserve_double_buffer_size` method.

Signed-off-by: Otavio Salvador <[email protected]>

Co-authored-by: Ta Thanh Dinh <[email protected]>
Co-authored-by: Otavio Salvador <[email protected]>

Author: 3 people

CHANGELOG.md

@@ -45,6 +45,9 @@ This project adheres to [Semantic Versioning](http://semver.org/).
   `self` by value.
   ([#1107](https://github.com/nix-rust/nix/pull/1107))
 
+- Type `&CString` for parameters of `exec(v|ve|vp|vpe|veat)` are changed to `&CStr`.
+  ([#1121](https://github.com/nix-rust/nix/pull/1121))
+
 ### Fixed
 - Fix length of abstract socket addresses
   ([#1120](https://github.com/nix-rust/nix/pull/1120))

src/unistd.rs

@@ -7,7 +7,7 @@ use fcntl::FcntlArg::F_SETFD;
 use libc::{self, c_char, c_void, c_int, c_long, c_uint, size_t, pid_t, off_t,
            uid_t, gid_t, mode_t, PATH_MAX};
 use std::{fmt, mem, ptr};
-use std::ffi::{CString, CStr, OsString, OsStr};
+use std::ffi::{CStr, OsString, OsStr};
 use std::os::unix::ffi::{OsStringExt, OsStrExt};
 use std::os::unix::io::RawFd;
 use std::path::PathBuf;
@@ -675,10 +675,9 @@ pub fn fchownat<P: ?Sized + NixPath>(
     Errno::result(res).map(drop)
 }
 
-fn to_exec_array(args: &[CString]) -> Vec<*const c_char> {
-    let mut args_p: Vec<*const c_char> = args.iter().map(|s| s.as_ptr()).collect();
-    args_p.push(ptr::null());
-    args_p
+fn to_exec_array(args: &[&CStr]) -> Vec<*const c_char> {
+    use std::iter::once;
+    args.iter().map(|s| s.as_ptr()).chain(once(ptr::null())).collect()
 }
 
 /// Replace the current process image with a new one (see
@@ -688,7 +687,7 @@ fn to_exec_array(args: &[CString]) -> Vec<*const c_char> {
 /// performs the same action but does not allow for customization of the
 /// environment for the new process.
 #[inline]
-pub fn execv(path: &CString, argv: &[CString]) -> Result<Void> {
+pub fn execv(path: &CStr, argv: &[&CStr]) -> Result<Void> {
     let args_p = to_exec_array(argv);
 
     unsafe {
@@ -712,7 +711,7 @@ pub fn execv(path: &CString, argv: &[CString]) -> Result<Void> {
 /// in the `args` list is an argument to the new process. Each element in the
 /// `env` list should be a string in the form "key=value".
 #[inline]
-pub fn execve(path: &CString, args: &[CString], env: &[CString]) -> Result<Void> {
+pub fn execve(path: &CStr, args: &[&CStr], env: &[&CStr]) -> Result<Void> {
     let args_p = to_exec_array(args);
     let env_p = to_exec_array(env);
 
@@ -733,7 +732,7 @@ pub fn execve(path: &CString, args: &[CString], env: &[CString]) -> Result<Void>
 /// would not work if "bash" was specified for the path argument, but `execvp`
 /// would assuming that a bash executable was on the system `PATH`.
 #[inline]
-pub fn execvp(filename: &CString, args: &[CString]) -> Result<Void> {
+pub fn execvp(filename: &CStr, args: &[&CStr]) -> Result<Void> {
     let args_p = to_exec_array(args);
 
     unsafe {
@@ -753,7 +752,7 @@ pub fn execvp(filename: &CString, args: &[CString]) -> Result<Void> {
 #[cfg(any(target_os = "haiku",
           target_os = "linux",
           target_os = "openbsd"))]
-pub fn execvpe(filename: &CString, args: &[CString], env: &[CString]) -> Result<Void> {
+pub fn execvpe(filename: &CStr, args: &[&CStr], env: &[&CStr]) -> Result<Void> {
     let args_p = to_exec_array(args);
     let env_p = to_exec_array(env);
 
@@ -781,7 +780,7 @@ pub fn execvpe(filename: &CString, args: &[CString], env: &[CString]) -> Result<
           target_os = "linux",
           target_os = "freebsd"))]
 #[inline]
-pub fn fexecve(fd: RawFd, args: &[CString], env: &[CString]) -> Result<Void> {
+pub fn fexecve(fd: RawFd, args: &[&CStr], env: &[&CStr]) -> Result<Void> {
     let args_p = to_exec_array(args);
     let env_p = to_exec_array(env);
 
@@ -804,8 +803,8 @@ pub fn fexecve(fd: RawFd, args: &[CString], env: &[CString]) -> Result<Void> {
 /// is referenced as a file descriptor to the base directory plus a path.
 #[cfg(any(target_os = "android", target_os = "linux"))]
 #[inline]
-pub fn execveat(dirfd: RawFd, pathname: &CString, args: &[CString],
-                env: &[CString], flags: super::fcntl::AtFlags) -> Result<Void> {
+pub fn execveat(dirfd: RawFd, pathname: &CStr, args: &[&CStr],
+                env: &[&CStr], flags: super::fcntl::AtFlags) -> Result<Void> {
     let args_p = to_exec_array(args);
     let env_p = to_exec_array(env);
 
@@ -1486,19 +1485,8 @@ pub fn getgrouplist(user: &CStr, group: Gid) -> Result<Vec<Gid>> {
             // BSD systems will still fill the groups buffer with as many
             // groups as possible, but Linux manpages do not mention this
             // behavior.
-
-            let cap = groups.capacity();
-            if cap >= ngroups_max as usize {
-                // We already have the largest capacity we can, give up
-                return Err(Error::invalid_argument());
-            }
-
-            // Reserve space for at least ngroups
-            groups.reserve(ngroups as usize);
-
-            // Even if the buffer gets resized to bigger than ngroups_max,
-            // don't ever ask for more than ngroups_max groups
-            ngroups = min(ngroups_max, groups.capacity() as c_int);
+            reserve_double_buffer_size(&mut groups, ngroups_max as usize)
+                .or_else(|_| Err(Error::invalid_argument()))?;
         }
     }
 }

test/test_unistd.rs

@@ -204,13 +204,13 @@ macro_rules! execve_test_factory(
                 dup2(writer, 1).unwrap();
                 let r = $syscall(
                     $exe,
-                    $(&CString::new($pathname).unwrap(), )*
-                    &[CString::new(b"".as_ref()).unwrap(),
-                      CString::new(b"-c".as_ref()).unwrap(),
+                    $(CString::new($pathname).unwrap().as_c_str(), )*
+                    &[CString::new(b"".as_ref()).unwrap().as_c_str(),
+                      CString::new(b"-c".as_ref()).unwrap().as_c_str(),
                       CString::new(b"echo nix!!! && echo foo=$foo && echo baz=$baz"
-                                   .as_ref()).unwrap()],
-                    &[CString::new(b"foo=bar".as_ref()).unwrap(),
-                      CString::new(b"baz=quux".as_ref()).unwrap()]
+                                   .as_ref()).unwrap().as_c_str()],
+                    &[CString::new(b"foo=bar".as_ref()).unwrap().as_c_str(),
+                      CString::new(b"baz=quux".as_ref()).unwrap().as_c_str()]
                     $(, $flags)*);
                 let _ = std::io::stderr()
                     .write_all(format!("{:?}", r).as_bytes());
@@ -238,18 +238,18 @@ macro_rules! execve_test_factory(
 
 cfg_if!{
     if #[cfg(target_os = "android")] {
-        execve_test_factory!(test_execve, execve, &CString::new("/system/bin/sh").unwrap());
+        execve_test_factory!(test_execve, execve, CString::new("/system/bin/sh").unwrap().as_c_str());
         execve_test_factory!(test_fexecve, fexecve, File::open("/system/bin/sh").unwrap().into_raw_fd());
     } else if #[cfg(any(target_os = "freebsd",
                         target_os = "linux"))] {
-        execve_test_factory!(test_execve, execve, &CString::new("/bin/sh").unwrap());
+        execve_test_factory!(test_execve, execve, CString::new("/bin/sh").unwrap().as_c_str());
         execve_test_factory!(test_fexecve, fexecve, File::open("/bin/sh").unwrap().into_raw_fd());
     } else if #[cfg(any(target_os = "dragonfly",
                         target_os = "ios",
                         target_os = "macos",
                         target_os = "netbsd",
                         target_os = "openbsd"))] {
-        execve_test_factory!(test_execve, execve, &CString::new("/bin/sh").unwrap());
+        execve_test_factory!(test_execve, execve, CString::new("/bin/sh").unwrap().as_c_str());
         // No fexecve() on DragonFly, ios, macos, NetBSD, OpenBSD.
         //
         // Note for NetBSD and OpenBSD: although rust-lang/libc includes it