from_env causes io-unsafety in child processes

[the8472]

(copied from https://github.com/rust-lang/rust/pull/113730#issuecomment-1869013070)

These lines seem questionable: https://github.com/rust-lang/jobserver-rs/blob/b4bc5db730a93d01cf8d0d868a2b6f553f535d2d/src/unix.rs#L152-L153

They lead the fds not being available to child processes by default (unless reverted by Client::configure) but from_env_ext() does not remove the environment variables. Which means child processes are instructed to access file descriptor numbers for jobserver communication that aren't open anymore and may have been reopened to point to other files. This seems like a violation of IO-safety (https://github.com/rust-lang/rust/issues/116059#issuecomment-1733591651).

Either cloexec shouldn't be set or the environment variables should also be removed and only be added back via configure.

[NobodyXu]

I think removing the env arg makes sense?

Since if you create a new jobserver, its fd is cloexec and there's no jobserver env setup.

[petrochenkov]

I suspect that in a typical case you do want to pass the created or inherited jobserver to children processes.

In all cases caught by https://github.com/rust-lang/rust/pull/113730 the right solution was to pass the jobserver further (except one case when it didn't matter much).

[petrochenkov]

https://github.com/rust-lang/jobserver-rs/pull/65 is a draft PR that reverses the current behavior - jobserver is inherited by default, but the inheritance can be disabled by explicitly calling a method.

[NobodyXu]

I think the best way to fix this is to use named fifo instead of annoymous pipe.

Passing pipe by fd also shares its file description, meaning whenever one process sets O_NONBLOCK it affects all the other processes.

I've got bitten by this the hard way in cc