-
Notifications
You must be signed in to change notification settings - Fork 12.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Reset signal behavior before starting children with std::process #25784
Conversation
3617b67
to
15d62f6
Compare
@@ -143,138 +143,6 @@ mod imp { | |||
pub unsafe fn drop_handler(handler: &mut Handler) { | |||
munmap(handler._data, SIGSTKSZ); | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh dear, thanks for the eagle eyes here! Glad to see this duplication reduced.
This is some fascinating investigation @geofft, very nice work! |
libuv appears to be processing signals via signal handlers. I can look in more detail if you think it's interesting, but if they're doing handlers instead of a thread and asking library users to ignore
Seems to get this right:
Python 3 (at least 3.4) appears to have fixed this relative to Python 2.7:
Yeah, I do actually mostly agree: there is definitely a use case for intentionally starting a child with ignored signals, e.g., writing If you're worried about the regression possibility with signal masks (although the docs don't promise anything either way, and I can't imagine any users will ever care, let alone have cared), we could set things up to reset |
Yeah I think the change here to reset Perhaps a test could be added at least which requires procmask to be reset on spawn? I think it's fine to do, but it'd be nice to have a regression test to ensure it's not accidentally removed as well! |
8c0641c
to
8e2e0ee
Compare
OK, I've added a Is this ready for merge? Are you happy with the changes to the bindings? |
Looks good to me, thanks @geofft! Can you also tag the test with |
Doesn't its presence in libstd/sys/unix/ make that implicit, since that path is only brought in by libstd/lib.rs's |
UNIX specifies that signal dispositions and masks get inherited to child processes, but in general, programs are not very robust to being started with non-default signal dispositions or to signals being blocked. For example, libstd sets `SIGPIPE` to be ignored, on the grounds that Rust code using libstd will get the `EPIPE` errno and handle it correctly. But shell pipelines are built around the assumption that `SIGPIPE` will have its default behavior of killing the process, so that things like `head` work: ``` geofft@titan:/tmp$ for i in `seq 1 20`; do echo "$i"; done | head -1 1 geofft@titan:/tmp$ cat bash.rs fn main() { std::process::Command::new("bash").status(); } geofft@titan:/tmp$ ./bash geofft@titan:/tmp$ for i in `seq 1 20`; do echo "$i"; done | head -1 1 bash: echo: write error: Broken pipe bash: echo: write error: Broken pipe bash: echo: write error: Broken pipe bash: echo: write error: Broken pipe bash: echo: write error: Broken pipe [...] ``` Here, `head` is supposed to terminate the input process quietly, but the bash subshell has inherited the ignored disposition of `SIGPIPE` from its Rust grandparent process. So it gets a bunch of `EPIPE`s that it doesn't know what to do with, and treats it as a generic, transient error. You can see similar behavior with `find / | head`, `yes | head`, etc. This PR resets Rust's `SIGPIPE` handler, as well as any signal mask that may have been set, before spawning a child. Setting a signal mask, and then using a dedicated thread or something like `signalfd` to dequeue signals, is one of two reasonable ways for a library to process signals. See tokio-rs/mio#16 for more discussion about this approach to signal handling and why it needs a change to `std::process`. The other approach is for the library to set a signal-handling function (`signal()` / `sigaction()`): in that case, dispositions are reset to the default behavior on exec (since the function pointer isn't valid across exec), so we don't have to care about that here. As part of this PR, I noticed that we had two somewhat-overlapping sets of bindings to signal functionality in `libstd`. One dated to old-IO and probably the old runtime, and was mostly unused. The other is currently used by `stack_overflow.rs`. I consolidated the two bindings into one set, and double-checked them by hand against all supported platforms' headers. This probably means it's safe to enable `stack_overflow.rs` on more targets, but I'm not including such a change in this PR. r? @alexcrichton cc @Zoxc for changes to `stack_overflow.rs`
⌛ Testing commit 8e2e0ee with merge 52f12c9... |
💔 Test failed - auto-linux-64-x-android-t |
Oops, fixed. Can I test-build Android without having a local Android dev environment? |
@bors: r+ 824a928 You'll need to pass |
⌛ Testing commit 824a928 with merge 2b457ad... |
UNIX specifies that signal dispositions and masks get inherited to child processes, but in general, programs are not very robust to being started with non-default signal dispositions or to signals being blocked. For example, libstd sets `SIGPIPE` to be ignored, on the grounds that Rust code using libstd will get the `EPIPE` errno and handle it correctly. But shell pipelines are built around the assumption that `SIGPIPE` will have its default behavior of killing the process, so that things like `head` work: ``` geofft@titan:/tmp$ for i in `seq 1 20`; do echo "$i"; done | head -1 1 geofft@titan:/tmp$ cat bash.rs fn main() { std::process::Command::new("bash").status(); } geofft@titan:/tmp$ ./bash geofft@titan:/tmp$ for i in `seq 1 20`; do echo "$i"; done | head -1 1 bash: echo: write error: Broken pipe bash: echo: write error: Broken pipe bash: echo: write error: Broken pipe bash: echo: write error: Broken pipe bash: echo: write error: Broken pipe [...] ``` Here, `head` is supposed to terminate the input process quietly, but the bash subshell has inherited the ignored disposition of `SIGPIPE` from its Rust grandparent process. So it gets a bunch of `EPIPE`s that it doesn't know what to do with, and treats it as a generic, transient error. You can see similar behavior with `find / | head`, `yes | head`, etc. This PR resets Rust's `SIGPIPE` handler, as well as any signal mask that may have been set, before spawning a child. Setting a signal mask, and then using a dedicated thread or something like `signalfd` to dequeue signals, is one of two reasonable ways for a library to process signals. See tokio-rs/mio#16 for more discussion about this approach to signal handling and why it needs a change to `std::process`. The other approach is for the library to set a signal-handling function (`signal()` / `sigaction()`): in that case, dispositions are reset to the default behavior on exec (since the function pointer isn't valid across exec), so we don't have to care about that here. As part of this PR, I noticed that we had two somewhat-overlapping sets of bindings to signal functionality in `libstd`. One dated to old-IO and probably the old runtime, and was mostly unused. The other is currently used by `stack_overflow.rs`. I consolidated the two bindings into one set, and double-checked them by hand against all supported platforms' headers. This probably means it's safe to enable `stack_overflow.rs` on more targets, but I'm not including such a change in this PR. r? @alexcrichton cc @Zoxc for changes to `stack_overflow.rs`
💔 Test failed - auto-linux-64-x-android-t |
Bah, I think I should go and set up build toolchains for all platforms. A |
Ah feel free to throw up just a blanket |
Can you file a bug for this so we remember to look at it later? |
UNIX specifies that signal dispositions and masks get inherited to child processes, but in general, programs are not very robust to being started with non-default signal dispositions or to signals being blocked. For example, libstd sets `SIGPIPE` to be ignored, on the grounds that Rust code using libstd will get the `EPIPE` errno and handle it correctly. But shell pipelines are built around the assumption that `SIGPIPE` will have its default behavior of killing the process, so that things like `head` work: ``` geofft@titan:/tmp$ for i in `seq 1 20`; do echo "$i"; done | head -1 1 geofft@titan:/tmp$ cat bash.rs fn main() { std::process::Command::new("bash").status(); } geofft@titan:/tmp$ ./bash geofft@titan:/tmp$ for i in `seq 1 20`; do echo "$i"; done | head -1 1 bash: echo: write error: Broken pipe bash: echo: write error: Broken pipe bash: echo: write error: Broken pipe bash: echo: write error: Broken pipe bash: echo: write error: Broken pipe [...] ``` Here, `head` is supposed to terminate the input process quietly, but the bash subshell has inherited the ignored disposition of `SIGPIPE` from its Rust grandparent process. So it gets a bunch of `EPIPE`s that it doesn't know what to do with, and treats it as a generic, transient error. You can see similar behavior with `find / | head`, `yes | head`, etc. This PR resets Rust's `SIGPIPE` handler, as well as any signal mask that may have been set, before spawning a child. Setting a signal mask, and then using a dedicated thread or something like `signalfd` to dequeue signals, is one of two reasonable ways for a library to process signals. See tokio-rs/mio#16 for more discussion about this approach to signal handling and why it needs a change to `std::process`. The other approach is for the library to set a signal-handling function (`signal()` / `sigaction()`): in that case, dispositions are reset to the default behavior on exec (since the function pointer isn't valid across exec), so we don't have to care about that here. As part of this PR, I noticed that we had two somewhat-overlapping sets of bindings to signal functionality in `libstd`. One dated to old-IO and probably the old runtime, and was mostly unused. The other is currently used by `stack_overflow.rs`. I consolidated the two bindings into one set, and double-checked them by hand against all supported platforms' headers. This probably means it's safe to enable `stack_overflow.rs` on more targets, but I'm not including such a change in this PR. r? @alexcrichton cc @Zoxc for changes to `stack_overflow.rs`
💔 Test failed - auto-linux-64-x-android-t |
🔒 Merge conflict |
☔ The latest upstream changes (presumably #25641) made this pull request unmergeable. Please resolve the merge conflicts. |
2a93dca
to
3a3d864
Compare
@bors r=alexcrichton |
📌 Commit 3a3d864 has been approved by |
⌛ Testing commit 3a3d864 with merge 00b862b... |
💔 Test failed - auto-linux-64-x-android-t |
Hrrm. I don't think this test failure is related to my code, despite being on Android again.
Seems related to #26192, but the Android tests passed there, so I'm confused. |
c::pthread_sigmask(c::SIG_SETMASK, &set, ptr::null_mut()) != 0 || | ||
libc::funcs::posix01::signal::signal( | ||
libc::SIGPIPE, mem::transmute(c::SIG_DFL) | ||
) == mem::transmute(c::SIG_ERR) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this use as foo
instead of mem::transmute
? The transmute may be a bit of a heavy hammer for this operation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Transmute checks size, and SIG_ERR
is ~0. Really the problem is that liblibc and c.rs have different sighandler_t
types. c.rs uses *mut c_void
since that's what we need for struct sigaction
(well, really a function pointer); liblibc calls it a size_t
, which is probably the same but I didn't want to assume that. Ideally we should synchronize these.
(Actually what I really want is an extended nullable-pointer optimization where I can say that 0, 1, and ~0 are invalid and force an enum {SIG_DFL = 0, SIG_IGN = 1, SIG_ERR = ~0, handler(extern fn(c_int))
to be represented as a single pointer, but I assume that's super hard.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow that definitely sounds like a mess, this already landed so it's fine for now, but I may do a drive-by fix at some point to just cast these both to a usize
and compare that (not critical at all though)
Ah unfortunately that constant is unstable currently, and it just looks like you deleted the usage of it, so you may be able to delete the associated |
The test that used it was removed in 700e627.
It looks like a lot of this dated to previous incarnations of the io module, etc., and went unused in the reworking leading up to 1.0. Remove everything we're not actively using (except for signal handling, which will be reworked in the next commit).
Both c.rs and stack_overflow.rs had bindings of libc's signal-handling routines. It looks like the split dated from rust-lang#16388, when (what is now) c.rs was in libnative but not libgreen. Nobody is currently using the c.rs bindings, but they're a bit more accurate in some places. Move everything to c.rs (since I'll need signal handling in process.rs, and we should avoid duplication), clean up the bindings, and manually double-check everything against the relevant system headers (fixing a few things in the process).
Make sure that child processes don't get affected by libstd's desire to ignore SIGPIPE, nor a third-party library's signal mask (which is needed to use either a signal-handling thread correctly or to use signalfd / kqueue correctly).
signal(), sigemptyset(), and sigaddset() are only available as inline functions until Android API 21. liblibc already handles signal() appropriately, so drop it from c.rs; translate sigemptyset() and sigaddset() (which is only used in a test) by hand from the C inlines. We probably want to revert this commit when we bump Android API level.
3a3d864
to
a8dbb92
Compare
@bors r=alexcrichton |
📌 Commit a8dbb92 has been approved by |
⌛ Testing commit a8dbb92 with merge 4e2a898... |
UNIX specifies that signal dispositions and masks get inherited to child processes, but in general, programs are not very robust to being started with non-default signal dispositions or to signals being blocked. For example, libstd sets `SIGPIPE` to be ignored, on the grounds that Rust code using libstd will get the `EPIPE` errno and handle it correctly. But shell pipelines are built around the assumption that `SIGPIPE` will have its default behavior of killing the process, so that things like `head` work: ``` geofft@titan:/tmp$ for i in `seq 1 20`; do echo "$i"; done | head -1 1 geofft@titan:/tmp$ cat bash.rs fn main() { std::process::Command::new("bash").status(); } geofft@titan:/tmp$ ./bash geofft@titan:/tmp$ for i in `seq 1 20`; do echo "$i"; done | head -1 1 bash: echo: write error: Broken pipe bash: echo: write error: Broken pipe bash: echo: write error: Broken pipe bash: echo: write error: Broken pipe bash: echo: write error: Broken pipe [...] ``` Here, `head` is supposed to terminate the input process quietly, but the bash subshell has inherited the ignored disposition of `SIGPIPE` from its Rust grandparent process. So it gets a bunch of `EPIPE`s that it doesn't know what to do with, and treats it as a generic, transient error. You can see similar behavior with `find / | head`, `yes | head`, etc. This PR resets Rust's `SIGPIPE` handler, as well as any signal mask that may have been set, before spawning a child. Setting a signal mask, and then using a dedicated thread or something like `signalfd` to dequeue signals, is one of two reasonable ways for a library to process signals. See tokio-rs/mio#16 for more discussion about this approach to signal handling and why it needs a change to `std::process`. The other approach is for the library to set a signal-handling function (`signal()` / `sigaction()`): in that case, dispositions are reset to the default behavior on exec (since the function pointer isn't valid across exec), so we don't have to care about that here. As part of this PR, I noticed that we had two somewhat-overlapping sets of bindings to signal functionality in `libstd`. One dated to old-IO and probably the old runtime, and was mostly unused. The other is currently used by `stack_overflow.rs`. I consolidated the two bindings into one set, and double-checked them by hand against all supported platforms' headers. This probably means it's safe to enable `stack_overflow.rs` on more targets, but I'm not including such a change in this PR. r? @alexcrichton cc @Zoxc for changes to `stack_overflow.rs`
UNIX specifies that signal dispositions and masks get inherited to child processes, but in general, programs are not very robust to being started with non-default signal dispositions or to signals being blocked. For example, libstd sets
SIGPIPE
to be ignored, on the grounds that Rust code using libstd will get theEPIPE
errno and handle it correctly. But shell pipelines are built around the assumption thatSIGPIPE
will have its default behavior of killing the process, so that things likehead
work:Here,
head
is supposed to terminate the input process quietly, but the bash subshell has inherited the ignored disposition ofSIGPIPE
from its Rust grandparent process. So it gets a bunch ofEPIPE
s that it doesn't know what to do with, and treats it as a generic, transient error. You can see similar behavior withfind / | head
,yes | head
, etc.This PR resets Rust's
SIGPIPE
handler, as well as any signal mask that may have been set, before spawning a child. Setting a signal mask, and then using a dedicated thread or something likesignalfd
to dequeue signals, is one of two reasonable ways for a library to process signals. See tokio-rs/mio#16 for more discussion about this approach to signal handling and why it needs a change tostd::process
. The other approach is for the library to set a signal-handling function (signal()
/sigaction()
): in that case, dispositions are reset to the default behavior on exec (since the function pointer isn't valid across exec), so we don't have to care about that here.As part of this PR, I noticed that we had two somewhat-overlapping sets of bindings to signal functionality in
libstd
. One dated to old-IO and probably the old runtime, and was mostly unused. The other is currently used bystack_overflow.rs
. I consolidated the two bindings into one set, and double-checked them by hand against all supported platforms' headers. This probably means it's safe to enablestack_overflow.rs
on more targets, but I'm not including such a change in this PR.r? @alexcrichton
cc @Zoxc for changes to
stack_overflow.rs