rust-lang / jobserver-rs

Apache License 2.0
69 stars 41 forks source link

Fix IO safety in `unix.rs` #101

Open beetrees opened 1 month ago

beetrees commented 1 month ago

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

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 commented 1 month ago

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

beetrees commented 1 month ago

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

NobodyXu commented 1 month ago
* 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