Implement CommandExt::{exec, before_exec} #31409
Conversation
|
r? @aturon |
alexcrichton
force-pushed
the
command-exec
branch
2 times, most recently
from
a000226
to
0b6e8e6
Compare
|
☔ The latest upstream changes (presumably #30629) made this pull request unmergeable. Please resolve the merge conflicts. |
alexcrichton
force-pushed
the
command-exec
branch
from
0b6e8e6
to
60e12a9
Compare
|
Well, that was a joy to read. The refactoring pushing more of the logic into platform-specific sys modules is very good, and much of the code is cleaner as a result. I wasn't sure why you made the change to building up the argv/envp as you go along, rather than at the end. It's the one place where I feel like the code quality suffered, but maybe there's a good reason for it? (In any case, it could use slightly better documentation, as I mentioned in a couple places). Otherwise, LGTM! |
|
The initial change to argv/envp was initially motivated by "wow we seem to be allocating a lot more than we need to", but I think it'll end up enabling more flexible uses of Along those lines the only way to do that I think is to build up Does that make sense though? Perhaps it's just trying too hard? I just looked again and found an allocation so we're not necessarily protected from it... |
|
@alexcrichton Ok, the motivation of allocating in advance of fork makes sense. And the decrease in allocation is coming from the fact that That's good enough for me. r=me after a comment or two on the struct definition explaining how the fields relate/what the invariants are. |
|
Yeah I think when all's said and done we basically don't have to allocate a string for the |
alexcrichton
force-pushed
the
command-exec
branch
from
60e12a9
to
a3d5dd0
Compare
|
@bors: r=aturon a3d5dd077736fffdb993ea4923d108ef839ba157 |
alexcrichton
force-pushed
the
command-exec
branch
from
a3d5dd0
to
ad49b3c
Compare
|
@bors: r=aturon ad49b3c7c2b9c803372a9361d12f6a1d1370f169 |
|
⌛ Testing commit ad49b3c with merge a79c601... |
alexcrichton
force-pushed
the
command-exec
branch
from
ad49b3c
to
45d3436
Compare
|
@bors: r=aturon 45d3436 force |
|
⌛ Testing commit 45d3436 with merge 0f595d2... |
|
💔 Test failed - auto-linux-32-opt |
|
@bors: retry On Tue, Feb 9, 2016 at 5:05 PM, bors [email protected] wrote:
|
|
⌛ Testing commit 45d3436 with merge 04ff571... |
|
💔 Test failed - auto-linux-32-opt |
|
So... I have no idea what's going on here. This crash is reproducible or me during a 32-bit bootstrap, and I managed to reduce this segfault down to a relatively small program, and after some tweaks to libstd with running it through a custom script to get small IR, I got it to an even smaller IR. Unfortunately I can't really infer much from that IR other than it does indeed look like it double frees something. I'm not entirely sure where that comes from in the compiler, codegen, and/or optimization passes. Fortunately, however, this appears to be a stage0-only bug, as I can't reproduce on nightly. This is, however, a bug on stable, but not on beta. cc @rust-lang/compiler, does this look like a bug we've fixed recently? I'd just want to make sure we didn't accidentally paper over the bug or something like that. Otherwise it's super late and I'm probably rambling and/or not making sense. I think I know the pattern which triggers the bug, so I'll just change the patch tomorrow to avoid that pattern and try to re-land. |
Hmm, #30478? |
|
Aha, that would indeed do it @petrochenkov! Thanks! |
* Build up the argp/envp pointers while the `Command` is being constructed rather than only when `spawn` is called. This will allow better sharing of code between fork/exec paths. * Rename `child_after_fork` to `exec` and have it only perform the exec half of the spawning. This also means the return type has changed to `io::Error` rather than `!` to represent errors that happen.
This is a Unix-specific function which adds the ability to register a closure to run pre-exec to configure the child process as required (note that these closures are run post-fork). cc rust-lang#31398
On Unix we have to be careful to not call `waitpid` twice, but we don't have to be careful on Windows due to the way process handles work there. As a result the cached `Option<ExitStatus>` is only necessary on Unix, and it's also just an implementation detail of the Unix module. At the same time. also update some code in `kill` on Unix to avoid a wonky waitpid with WNOHANG. This was added in 0e190b9 to solve rust-lang#13124, but the `signal(0)` method is not supported any more so there's no need to for this workaround. I believe that this is no longer necessary as it's not really doing anything.
This better reflects what it's actually doing as we don't actually have an option for "leave this I/O slot as an empty hole".
The function `CreateProcess` is not itself unsafe to call from many threads, the article in question is pointing out that handles can be inherited by unintended child processes. This is basically the same race as the standard Unix open-then-set-cloexec race. Since the intention of the lock is to protect children from inheriting unintended handles, the lock is now lifted out to before the creation of the child I/O handles (which will all be inheritable). This will ensure that we only have one process in Rust at least creating inheritable handles at a time, preventing unintended inheritance to children.
Most of this is platform-specific anyway, and we generally have to jump through fewer hoops to do the equivalent operation on Windows. One benefit for Windows today is that this new structure avoids an extra `DuplicateHandle` when creating pipes. For Unix, however, the behavior should be the same. Note that this is just a pure refactoring, no functionality was added or removed.
This commit implements the `exec` function proposed in [RFC 1359][rfc] which is a function on the `CommandExt` trait to execute all parts of a `Command::spawn` without the `fork` on Unix. More details on the function itself can be found in the comments in the commit. [rfc]: rust-lang/rfcs#1359 cc rust-lang#31398
Helps cut down on #[cfg]!
Lost track of this during the std::process refactorings
alexcrichton
force-pushed
the
command-exec
branch
from
45d3436
to
d9c6a51
Compare
|
⌛ Testing commit d9c6a51 with merge 3308826... |
|
💔 Test failed - auto-mac-32-opt |
|
@bors: retry On Wed, Feb 10, 2016 at 12:55 PM, bors [email protected] wrote:
|
These commits are an implementation of rust-lang/rfcs#1359 which is tracked via #31398. The `before_exec` implementation fit easily with the current process spawning framework we have, but unfortunately the `exec` implementation required a bit of a larger refactoring. The stdio handles were all largely managed as implementation details of `std::process` and the `exec` function lived in `std::sys`, so the two didn't have access to one another. I took this as a sign that a deeper refactoring was necessary, and I personally feel that the end result is cleaner for both Windows and Unix. The commits should be separated nicely for reviewing (or all at once if you're feeling ambitious), but the changes made here were: * The process spawning on Unix was refactored in to a pre-exec and post-exec function. The post-exec function isn't allowed to do any allocations of any form, and management of transmitting errors back to the parent is managed by the pre-exec function (as it's the one that actually forks). * Some management of the exit status was pushed into platform-specific modules. On Unix we must cache the return value of `wait` as the pid is consumed after we wait on it, but on Windows we can just keep querying the system because the handle stays valid. * The `Stdio::None` variant was renamed to `Stdio::Null` to better reflect what it's doing. * The global lock on `CreateProcess` is now correctly positioned to avoid unintended inheritance of pipe handles that other threads are sending to their child processes. After a more careful reading of the article referenced the race is not in `CreateProcess` itself, but rather the property that handles are unintentionally shared. * All stdio management now happens in platform-specific modules. This provides a cleaner implementation/interpretation for `FromFraw{Fd,Handle}` for each platform as well as a cleaner transition from a configuration to what-to-do once we actually need to do the spawn. With these refactorings in place, implementing `before_exec` and `exec` ended up both being pretty trivial! (each in their own commit)
Update outdated comment in unix Command. The big comment in the `Command` struct has been incorrect for some time (at least since rust-lang#46789 which removed `envp`). Rather than try to remove the allocations, this PR just updates the comment to reflect reality. There is an explanation for the reasoning at rust-lang#31409 (comment), discussing the potential of being able to call `Command::exec` after `libc::fork`. That can still be done in the future, but I think for now it would be good to just correct the comment.
Update outdated comment in unix Command. The big comment in the `Command` struct has been incorrect for some time (at least since rust-lang#46789 which removed `envp`). Rather than try to remove the allocations, this PR just updates the comment to reflect reality. There is an explanation for the reasoning at rust-lang#31409 (comment), discussing the potential of being able to call `Command::exec` after `libc::fork`. That can still be done in the future, but I think for now it would be good to just correct the comment.
These commits are an implementation of rust-lang/rfcs#1359 which is tracked via #31398. The
before_execimplementation fit easily with the current process spawning framework we have, but unfortunately theexecimplementation required a bit of a larger refactoring. The stdio handles were all largely managed as implementation details ofstd::processand theexecfunction lived instd::sys, so the two didn't have access to one another.I took this as a sign that a deeper refactoring was necessary, and I personally feel that the end result is cleaner for both Windows and Unix. The commits should be separated nicely for reviewing (or all at once if you're feeling ambitious), but the changes made here were:
waitas the pid is consumed after we wait on it, but on Windows we can just keep querying the system because the handle stays valid.Stdio::Nonevariant was renamed toStdio::Nullto better reflect what it's doing.CreateProcessis now correctly positioned to avoid unintended inheritance of pipe handles that other threads are sending to their child processes. After a more careful reading of the article referenced the race is not inCreateProcessitself, but rather the property that handles are unintentionally shared.FromFraw{Fd,Handle}for each platform as well as a cleaner transition from a configuration to what-to-do once we actually need to do the spawn.With these refactorings in place, implementing
before_execandexecended up both being pretty trivial! (each in their own commit)