search

rust-lang / jobserver-rs

Apache License 2.0
69 stars 39 forks source link

from_env causes io-unsafety in child processes #64

Open the8472 opened 8 months ago

the8472 commented 8 months ago

(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 commented 8 months ago

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 commented 8 months ago

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 commented 7 months ago

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 commented 6 months ago

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