From 108d8a4f72e336b0e64235f6cd8615a8fb8a0227 Mon Sep 17 00:00:00 2001 From: Jiahao XU Date: Thu, 3 Aug 2023 23:44:13 +1000 Subject: [PATCH 1/2] 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 --- src/lib.rs | 26 ++++++++++++++------------ src/unix.rs | 15 ++++++++++++--- 2 files changed, 26 insertions(+), 15 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index fe000c3..d19b9c7 100644 --- a/src/lib.rs +++ b/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() @@ -293,6 +282,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 { Self::from_env_ext(false).client.ok() } diff --git a/src/unix.rs b/src/unix.rs index 12cbe05..b18bf6a 100644 --- a/src/unix.rs +++ b/src/unix.rs @@ -158,9 +158,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 { @@ -444,6 +445,14 @@ unsafe fn fd_check(fd: c_int, check_pipe: bool) -> Result<(), FromEnvErrorInner> } } +fn clone_fd_and_set_cloexec(fd: c_int) -> Result { + // 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))?; From 1c44c55be02338a09e5eb77931c12e5d4395cdc4 Mon Sep 17 00:00:00 2001 From: Jiahao XU Date: Sat, 2 Mar 2024 17:54:05 +1100 Subject: [PATCH 2/2] Use `BorrowedFd` instead of `ManuallyDrop` for cloning fd Signed-off-by: Jiahao XU --- src/unix.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/unix.rs b/src/unix.rs index b18bf6a..25efef1 100644 --- a/src/unix.rs +++ b/src/unix.rs @@ -446,10 +446,10 @@ unsafe fn fd_check(fd: c_int, check_pipe: bool) -> Result<(), FromEnvErrorInner> } fn clone_fd_and_set_cloexec(fd: c_int) -> Result { - // 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() + // Safety: fd is a valid fd dand it remains open until returns + unsafe { BorrowedFd::borrow_raw(fd) } + .try_clone_to_owned() + .map(File::from) .map_err(|err| FromEnvErrorInner::CannotOpenFd(fd, err)) }