feat: Make Client::from_env safe to call for any number of time

So that it can be used in different crates without causing UB.

Signed-off-by: Jiahao XU <[email protected]>

Author: NobodyXu

src/lib.rs

@@ -231,13 +231,6 @@ impl Client {
     /// result with the connected client will be returned. In other cases
     /// result will contain `Err(FromEnvErr)`.
     ///
-    /// Note that on Unix the `Client` returned **takes ownership of the file
-    /// descriptors specified in the environment**. Jobservers on Unix are
-    /// implemented with `pipe` file descriptors, and they're inherited from
-    /// parent processes. This `Client` returned takes ownership of the file
-    /// descriptors for this process and will close the file descriptors after
-    /// this value is dropped.
-    ///
     /// Additionally on Unix this function will configure the file descriptors
     /// with `CLOEXEC` so they're not automatically inherited by spawned
     /// children.
@@ -256,11 +249,7 @@ impl Client {
     /// make sure to take ownership properly of the file descriptors passed
     /// down, if any.
     ///
-    /// It's generally unsafe to call this function twice in a program if the
-    /// previous invocation returned `Some`.
-    ///
-    /// Note, though, that on Windows it should be safe to call this function
-    /// any number of times.
+    /// It is ok to call this function any number of times.
     pub unsafe fn from_env_ext(check_pipe: bool) -> FromEnv {
         let (env, var_os) = match ["CARGO_MAKEFLAGS", "MAKEFLAGS", "MFLAGS"]
             .iter()
@@ -299,6 +288,19 @@ impl Client {
     /// environment.
     ///
     /// Wraps `from_env_ext` and discards error details.
+    ///
+    /// # Safety
+    ///
+    /// This function is `unsafe` to call on Unix specifically as it
+    /// transitively requires usage of the `from_raw_fd` function, which is
+    /// itself unsafe in some circumstances.
+    ///
+    /// It's recommended to call this function very early in the lifetime of a
+    /// program before any other file descriptors are opened. That way you can
+    /// make sure to take ownership properly of the file descriptors passed
+    /// down, if any.
+    ///
+    /// It is ok to call this function any number of times.
     pub unsafe fn from_env() -> Option<Client> {
         Self::from_env_ext(false).client.ok()
     }

src/unix.rs

@@ -149,9 +149,10 @@ impl Client {
             }
         }
 
-        drop(set_cloexec(read, true));
-        drop(set_cloexec(write, true));
-        Ok(Some(Client::from_fds(read, write)))
+        Ok(Some(Client::Pipe {
+            read: clone_fd_and_set_cloexec(read)?,
+            write: clone_fd_and_set_cloexec(write)?,
+        }))
     }
 
     unsafe fn from_fds(read: c_int, write: c_int) -> Client {
@@ -435,6 +436,14 @@ unsafe fn fd_check(fd: c_int, check_pipe: bool) -> Result<(), FromEnvErrorInner>
     }
 }
 
+fn clone_fd_and_set_cloexec(fd: c_int) -> Result<File, FromEnvErrorInner> {
+    // Safety: File is wrapped in `ManuallyDrop` to prevent closing on drop
+    // since we don't own the fd.
+    mem::ManuallyDrop::new(unsafe { File::from_raw_fd(fd) })
+        .try_clone()
+        .map_err(|err| FromEnvErrorInner::CannotOpenFd(fd, err))
+}
+
 fn set_cloexec(fd: c_int, set: bool) -> io::Result<()> {
     unsafe {
         let previous = cvt(libc::fcntl(fd, libc::F_GETFD))?;