Fix IO safety in `unix.rs` #101

beetrees · 2024-07-21T11:58:40Z

As well as using BorrowedFd as opposed to c_int in more places, this PR fixes two related bugs:

Unix Client::configure() wouldn't ensure the file descriptors passed to Command::configure remained valid until the command was actually spawned. This meant that if the Client was dropped before the Command was spawned, the set_cloexec call could end up hitting an unrelated file descriptor (which the spawned process would then treat as a jobserver).
Unix string_arg uses the FDs from ClientCreationArg::Fds to put in the environment variables, but before this PR Client::configure would use the FDs from self.read/self.write instead. This would result in the wrong FDs getting passed to set_cloexec. This was basically harmless (apart from having the use-after-drop problem) as the only time the two sets of FDs differ is when the jobserver has been inherited from the environment, in which case the FDs are going to be not-CLOEXEC anyway.

I've also removed the unsafe from the definitions of Client::mk and Client::from_fds as they don't appear to have any safety requirements.

beetrees · 2024-07-21T12:15:07Z

So the behaviour of spawning a configured command after the (not-inherited) jobserver Client is dropped differs:

On Windows: The command spawns successfully but attempting to inherit the jobserver will error.
On Unix before this PR: The command would either fail to spawn (if no FDs had been opened in place of the FDs which were closed) or would spawn successfully with the child process being passed whatever FDs happened to have the same number as the former pipe ends from the parent process.
On Unix after this PR: The command spawns successfully and the jobserver is inheritable and usable.

NobodyXu · 2024-07-21T14:16:42Z

src/unix.rs

+                        -1 => {
+                            let err = io::Error::last_os_error();
+                            if err.raw_os_error() == Some(libc::ENOSYS) {
+                                PIPE2_AVAILABLE.store(false, Ordering::SeqCst);


I think Relaxed here is definitely enough for store and load

Suggested change

PIPE2_AVAILABLE.store(false, Ordering::SeqCst);

PIPE2_AVAILABLE.store(false, Ordering::Relaxed);

NobodyXu · 2024-07-21T14:17:22Z

src/unix.rs

-                            return Err(err);
+                unsafe {
+                    let mut pipes = [0; 2];
+                    match libc::syscall(libc::SYS_pipe2, pipes.as_mut_ptr(), libc::O_CLOEXEC) {


Could use cvt here and match on the error:

Suggested change

match libc::syscall(libc::SYS_pipe2, pipes.as_mut_ptr(), libc::O_CLOEXEC) {

match cvt(libc::syscall(libc::SYS_pipe2, pipes.as_mut_ptr(), libc::O_CLOEXEC)) {

Unfortunately libc::syscall returns a c_long whereas cvt takes a c_int.

NobodyXu · 2024-07-21T14:18:24Z

src/unix.rs

+        drop(set_cloexec(read.as_fd(), true));
+        drop(set_cloexec(write.as_fd(), true));


I think the error should be propagated instead of ignored:

Suggested change

drop(set_cloexec(read.as_fd(), true));

drop(set_cloexec(write.as_fd(), true));

set_cloexec(read.as_fd(), true)?;

set_cloexec(write.as_fd(), true)?;

NobodyXu · 2024-07-21T14:18:45Z

src/unix.rs

-                            PIPE2_AVAILABLE.store(false, Ordering::SeqCst);
-                        } else {
-                            return Err(err);
+                unsafe {


My two cent is that smaller unsafe block is better, as it makes it easier to read

I've shrunk it further.

NobodyXu · 2024-07-21T14:21:26Z

tests/spawn-after-drop.rs

+
+use jobserver::{Client, FromEnvErrorKind};
+
+macro_rules! t {


I don't think we need t! anymore, it's probably used in times where .unwrap() isn't available

Replaced with .unwrap().

NobodyXu · 2024-07-21T14:26:12Z

src/unix.rs

+                    // Keep a reference to the jobserver alive in the closure so that
+                    // the pipe FDs aren't closed, otherwise `set_cloexec` might end up
+                    // targetting a completely unrelated file descriptor.
+                    let arc = self.clone();


This is awesome, now it would always work once Client::configure is called

NobodyXu · 2024-07-21T14:27:45Z

tests/spawn-after-drop.rs

+            let client = unsafe {
+                t!(match Client::from_env_ext(true).client {
+                    // Its ok for a dropped jobservers path to no longer exist (e.g. on Windows).
+                    Err(e) if matches!(e.kind(), FromEnvErrorKind::CannotOpenPath) => return,


IMHO it's kind of a bad behavior that it has different behavior on different platform.

Unfortunately having the same behaviour would be difficult:

On Windows, the child process doesn't have any OS-level reference to the jobserver until it opens the semaphore, so if the parent process Client is dropped before Client::from_env_ext is called then Client::from_env_ext will fail as semaphores are automatically deleted when there are no handles referencing them.

On Linux (when using pipes, which is what jobserver-rs currently does when creating it's own jobserver) the opposite is true: as long as the FDs are alive when the process is spawned (which this PR ensures) then Client::from_env_ext will succeed in the child process even if the parent process Client is dropped before the client process calls it.

The only possibility that comes to mind would be if on Windows the parent process lets the child process inherit a copy of the semaphore handle; this would mean that the semaphore would stick around for as long as that process was running. The difficult bit is ensuring only the desired processes inherit the handle and making sure the handle isn't closed before the command is spawned. This would require stdlib support, possibly involving PROC_THREAD_ATTRIBUTE_HANDLE_LIST and rust-lang/rust#114854.

@ChrisDenton, one more case to start supporting this.

I am happy to collaborate would @beetrees on this to stop processes from inheriting handles.

Check out rust-lang/rust#115501.

beetrees · 2024-07-21T17:05:19Z

(I've also fixed an unused import warning in the unix.rs tests on Linux)

NobodyXu · 2024-07-23T14:16:40Z

* On Windows: The command spawns successfully but attempting to inherit the jobserver will error.

That's unfortunate since windows don't have pre_exec.

I have a fork of jobserver, which I workaround this issue by forcing user to pass in a closure and spawn the process there Client::configure_and_run:

pub fn configure_and_run<Cmd, F, R>(&self, cmd: Cmd, f: F) -> Result<R>
where
    Cmd: Command,
    F: FnOnce(&mut Cmd) -> Result<R>;

* On Unix after this PR: The command spawns successfully and the jobserver is inheritable and usable.

I was planning to open a PR to optimize away the pre_exec, if the Client is created from_env, since the original fds inherited from the environment should be valid.

Though that'd mean the jobserver would be inherited without Client::configure

beetrees force-pushed the fix-io-safety branch 2 times, most recently from 49877d1 to 40cffa2 Compare July 21, 2024 12:10

NobodyXu suggested changes Jul 21, 2024

View reviewed changes

beetrees force-pushed the fix-io-safety branch from 40cffa2 to 49b43ed Compare July 21, 2024 16:17

Fix IO safety in unix.rs

05db131

beetrees force-pushed the fix-io-safety branch from 49b43ed to 05db131 Compare July 21, 2024 16:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix IO safety in `unix.rs` #101

Fix IO safety in `unix.rs` #101

beetrees commented Jul 21, 2024

beetrees commented Jul 21, 2024

NobodyXu Jul 21, 2024

beetrees Jul 21, 2024

NobodyXu Jul 21, 2024

beetrees Jul 21, 2024

NobodyXu Jul 21, 2024

beetrees Jul 21, 2024

NobodyXu Jul 21, 2024

beetrees Jul 21, 2024

NobodyXu Jul 21, 2024

beetrees Jul 21, 2024

NobodyXu Jul 21, 2024

NobodyXu Jul 21, 2024

beetrees Jul 21, 2024

michaelvanstraten Aug 28, 2024

michaelvanstraten Aug 28, 2024

michaelvanstraten Aug 28, 2024

beetrees commented Jul 21, 2024

NobodyXu commented Jul 23, 2024

	PIPE2_AVAILABLE.store(false, Ordering::SeqCst);
	PIPE2_AVAILABLE.store(false, Ordering::Relaxed);

	match libc::syscall(libc::SYS_pipe2, pipes.as_mut_ptr(), libc::O_CLOEXEC) {
	match cvt(libc::syscall(libc::SYS_pipe2, pipes.as_mut_ptr(), libc::O_CLOEXEC)) {

		drop(set_cloexec(read.as_fd(), true));
		drop(set_cloexec(write.as_fd(), true));

Fix IO safety in unix.rs #101

Are you sure you want to change the base?

Fix IO safety in unix.rs #101

Conversation

beetrees commented Jul 21, 2024

beetrees commented Jul 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beetrees commented Jul 21, 2024

NobodyXu commented Jul 23, 2024

Fix IO safety in `unix.rs` #101

Fix IO safety in `unix.rs` #101