-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Should Rust still ignore SIGPIPE by default? #62569
Comments
I'm not sure if the @rust-lang/libs team is the right team for this; suggestions welcome if another team would be the right owner for "behavior of Rust at application start-up". |
If we stop ignoring SIGPIPE by default, we will break back compat for every socket server written in Rust, right? |
Why this and e.g. not an attribute on |
@sfackler Right now, we have inconsistent behavior, where your socket server will also break if you ever write At the very least, we need to document that we ignore SIGPIPE, and we need to provide an easy way to disable this behavior. I'd also fully expect that any change to the default itself would require a sizable transition period (or even an edition). @Centril Good point! An attribute within the source code (on main or the top-level module) does indeed seem preferable, rather than a build-time option. |
I agree that we should do something about this. I don't think we can change the current default behavior because of backcompat concerns, as @sfackler mentioned, but the current status quo is that nominal command line applications start out of the gate as broken, and it's a fairly subtle issue to fix. Firstly, if you're using For example, normally a process will exit with a
So to fix this, you firstly need to know enough to use
This, presumably, should be So yeah, on the one hand, ignoring SIGPIPE by default means pure Rust networked applications probably have the right behavior by default, but not ignoring SIGPIPE by default means command line applications probably have the wrong behavior by default. Both of these groups seem fairly sizable to me. I don't mind the idea of some incantation one can put in the source code to fix this. An alternative might be to just provide a function in (Although, I guess adding a function to |
I personally agree with @sfackler that I don't think we can really change any defaults due to backwards-compatibility reasons. AFAIK this really only affects one thing which is unix-y CLI applications. Any portable CLI application to Windows cannot rely on the existence of SIGPIPE and almost all non-CLI applications I imagine want to receive errors instead of SIGPIPE to handle. That feels, to me, niche enough that we made the defaults the right way and there should be documentation/crates for authors writing unix-y CLI applications (aka ripgrep and such). |
@alexcrichton I agree that we have to handle backwards-compatibility, but that wouldn't stop us from having a top-level attribute to opt-out of it. |
If the proposal ends up boiling down to adding a new top-level attribute to opt-out, then I think a blocker in my mind for the proposal would be to justify why a change to the compiler/standard library is necessary when otherwise a crate on crates.io would only require two lines to add to a project (e.g. a dependency directive in |
@alexcrichton Because there's a difference between "do some setup to ignore SIGPIPE, then un-ignore it" and "never ignore SIGPIPE in the first place". Because people want to build smaller, faster executables. Because libraries and applications shouldn't have subtle differences in behavior, especially undocumented differences. Because you can do this in C and Rust makes it difficult. Because not every project should need to have a dependency to do this. Because it's a pain to add a platform-specific dependency and a platform-specific one-liner for an application that could otherwise be portable, and |
Er sorry that sort of feels like a particularly antagonistic reponse to me? Many of your "Because ..." can be pretty trivially fixed (the crate doesn't have to be platform specific, difficulty seems like it's a function of the crate, etc) and seem like they're maybe blowing the problem out of proportion (we're not slowing down executables or bloating them by adding an extra syscall at the beginning)? I think one of the main points you're bringing up is the difference in library/binary behavior today, but from what @sfackler mentioned above and I agree we unfortunately can't change the binary behavior and library authors largely just need to be aware of this if they run into it (which thankfully isn't all the time). We can of course also have crates to smooth this over one way or another. My point (which I don't think you really addressed?) was that I don't understand why a compiler solution is needed. It seems that a suitable crate can be added to crates.io to solve almost all of these issues. |
It's possible to make a crate that causes |
If this can be reasonably supported as a library function then that seems like the best solution since it is low cost. At any rate if people absolutely don't want to take on that dependency then libstd is still lower cost than a built in attribute. All in all I think @alexcrichton makes fine points. |
@alexcrichton My apologies, I certainly didn't intend to come across as antagonistic. My most specific point is that a library crate can only undo what std does, it can't stop std from doing it in the first place. I'd like a solution that provides more control over what Rust's startup code is doing in the first place. (Also, as one more reason, every unnecessary syscall also represents an additional syscall needed in seccomp filters to lock down a program.)
Exactly. That's the main reason I'm asking after having built-in support. |
I thought as much, it really should be in the compiler then. An attribute on |
This seems like a problem that could be fixed with the next edition? That would justify the use of an attribute to disable/enable this feature, in that upgrading binary crates to the new edition would need to add the attribute to re-enable this feature. |
Changing any signal disposition by default in the startup code is inherently broken, because dispositions are inherited across fork/exec. The same goes for signal masking. IMO this behavior should just be removed. If you really really don't want
This procedure is entirely portable and thread-safe, because signal masks are thread-local and |
an alternate solution: that way, both servers and cli tools work by default |
The issue of igoring With that said, I agree |
This came up in a StackOverflow question, and my advice was basically, "either don't use |
If you have multiple threads, this will abort the whole process instead of only panicking the current thread. |
@jyn514 yes. That's exactly what you want in most CLI programs. |
@BurntSushi Programs using |
Another reason not to use You can distinguish the two cases from the output of Short Rust program demonstrating signal behavioruse std::os::unix::process::ExitStatusExt;
use std::process::{Command, Stdio};
/// Runs POSIX shell command `shell` in a context with its stdout closed (probably; technically
/// racy), and reports on the exit status's code or signal.
fn check(shell: &str) -> std::io::Result<()> {
let mut child = Command::new("sh")
.args(&["-c", "sleep 0.1; exec sh -c \"$@\"", ":", shell])
.stdout(Stdio::piped())
.spawn()?;
drop(child.stdout.take().unwrap());
let status = child.wait()?;
println!(
"{:?} exited with code {:?}; signal: {:?}",
shell,
status.code(),
status.signal(),
);
Ok(())
}
fn main() -> std::io::Result<()> {
check("exit 141")?;
check("echo sigpipe")?;
check("kill -13 $$")?;
Ok(())
}
We can see that a bare Further reading: “Proper handling of SIGINT/SIGQUIT” describes how |
…r=davidtwco unix_sigpipe: Simple fixes and improvements in tests In rust-lang#120832 I included 5 preparatory commits. It will take a while before discussions there and in rust-lang#62569 is settled, so here is a PR that splits out 4 of the commits that are easy to review, to get them out of the way. r? ``@davidtwco`` who already approved these commits in rust-lang#120832 (but I have tweaked them a bit and rebased them since then). For the convenience of my reviewer, here are the full commit messages of the commits: <details> <summary>Click to expand</summary> ``` commit 948b1d6 (HEAD -> unix_sigpipe-tests-fixes, origin/unix_sigpipe-tests-fixes) Author: Martin Nordholts <[email protected]> Date: Fri Feb 9 07:57:27 2024 0100 tests: Add unix_sigpipe-different-duplicates.rs test variant To make sure that #[unix_sigpipe = "x"] #[unix_sigpipe = "y"] behaves like #[unix_sigpipe = "x"] #[unix_sigpipe = "x"] commit d14f158 Author: Martin Nordholts <[email protected]> Date: Fri Feb 9 08:47:47 2024 0100 tests: Combine unix_sigpipe-not-used.rs and unix_sigpipe-only-feature.rs The only difference between the files is the presence/absence of #![feature(unix_sigpipe)] attribute. Avoid duplication by using revisions instead. commit a1cb3db Author: Martin Nordholts <[email protected]> Date: Fri Feb 9 06:44:56 2024 0100 tests: Rename unix_sigpipe.rs to unix_sigpipe-bare.rs for clarity The test is for the "bare" variant of the attribute that looks like this: #[unix_sigpipe] which is not allowed, because it must look like this: #[unix_sigpipe = "sig_ign"] commit e060274 Author: Martin Nordholts <[email protected]> Date: Fri Feb 9 05:48:24 2024 0100 tests: Fix typo unix_sigpipe-error.rs -> unix_sigpipe-sig_ign.rs There is no error expected. It's simply the "regular" test for sig_ign. So rename it. ``` </details> Tracking issue: rust-lang#97889
Rollup merge of rust-lang#121527 - Enselic:unix_sigpipe-tests-fixes, r=davidtwco unix_sigpipe: Simple fixes and improvements in tests In rust-lang#120832 I included 5 preparatory commits. It will take a while before discussions there and in rust-lang#62569 is settled, so here is a PR that splits out 4 of the commits that are easy to review, to get them out of the way. r? ``@davidtwco`` who already approved these commits in rust-lang#120832 (but I have tweaked them a bit and rebased them since then). For the convenience of my reviewer, here are the full commit messages of the commits: <details> <summary>Click to expand</summary> ``` commit 948b1d6 (HEAD -> unix_sigpipe-tests-fixes, origin/unix_sigpipe-tests-fixes) Author: Martin Nordholts <[email protected]> Date: Fri Feb 9 07:57:27 2024 0100 tests: Add unix_sigpipe-different-duplicates.rs test variant To make sure that #[unix_sigpipe = "x"] #[unix_sigpipe = "y"] behaves like #[unix_sigpipe = "x"] #[unix_sigpipe = "x"] commit d14f158 Author: Martin Nordholts <[email protected]> Date: Fri Feb 9 08:47:47 2024 0100 tests: Combine unix_sigpipe-not-used.rs and unix_sigpipe-only-feature.rs The only difference between the files is the presence/absence of #![feature(unix_sigpipe)] attribute. Avoid duplication by using revisions instead. commit a1cb3db Author: Martin Nordholts <[email protected]> Date: Fri Feb 9 06:44:56 2024 0100 tests: Rename unix_sigpipe.rs to unix_sigpipe-bare.rs for clarity The test is for the "bare" variant of the attribute that looks like this: #[unix_sigpipe] which is not allowed, because it must look like this: #[unix_sigpipe = "sig_ign"] commit e060274 Author: Martin Nordholts <[email protected]> Date: Fri Feb 9 05:48:24 2024 0100 tests: Fix typo unix_sigpipe-error.rs -> unix_sigpipe-sig_ign.rs There is no error expected. It's simply the "regular" test for sig_ign. So rename it. ``` </details> Tracking issue: rust-lang#97889
This approach is interesting, but the issue with it is that you must now apply this to every single thing that calls An equivalent to |
Another problem with this is it is expensive; introducing multiple system calls for every call to write() -- which often occur in a tight loop -- is quite undesirable. |
…ark-Simulacrum unix_sigpipe: Add test for SIGPIPE disposition in child processes To make it clearer what the impact would be to stop using `SIG_IGN` and instead use a noop handler, like suggested [here](rust-lang#62569 (comment)) and implemented [here](rust-lang#121578). Part of rust-lang#97889
…ark-Simulacrum unix_sigpipe: Add test for SIGPIPE disposition in child processes To make it clearer what the impact would be to stop using `SIG_IGN` and instead use a noop handler, like suggested [here](rust-lang#62569 (comment)) and implemented [here](rust-lang#121578). Part of rust-lang#97889
…ark-Simulacrum unix_sigpipe: Add test for SIGPIPE disposition in child processes To make it clearer what the impact would be to stop using `SIG_IGN` and instead use a noop handler, like suggested [here](rust-lang#62569 (comment)) and implemented [here](rust-lang#121578). Part of rust-lang#97889
…ark-Simulacrum unix_sigpipe: Add test for SIGPIPE disposition in child processes To make it clearer what the impact would be to stop using `SIG_IGN` and instead use a noop handler, like suggested [here](rust-lang#62569 (comment)) and implemented [here](rust-lang#121578). Part of rust-lang#97889
Rollup merge of rust-lang#121573 - Enselic:sigpipe-child-process, r=Mark-Simulacrum unix_sigpipe: Add test for SIGPIPE disposition in child processes To make it clearer what the impact would be to stop using `SIG_IGN` and instead use a noop handler, like suggested [here](rust-lang#62569 (comment)) and implemented [here](rust-lang#121578). Part of rust-lang#97889
We have a number of CLI tools that panic when piped to things like `head` instead of quietly exiting. There's a long history about this within the Rust community (see rust-lang/rust#62569), but the long and short of it is that SIGPIPE really should be set to its default handler (`SIG_DFL`, terminate the process) for CLI tools. Because oxlog doesn't make any network requests, reset the SIGPIPE handler to `SIG_DFL`. I looked at also potentially doing this for some of our other CLI tools that wait on network services. This should be fine to do if and only if whenever we send data over a socket, the `MSG_NOSIGNAL` flag is set. (This causes an `EPIPE` error to be returned, but no `SIGPIPE` signal to be generated.) Rust does set this flag [here]. **However, as of Rust 1.77 this flag is not set on illumos.** That's a bug and I'll fix it in Rust upstream. [here]: https://github.com/rust-lang/rust/blob/877d36b1928b5a4f7d193517b48290ecbe404d71/library/std/src/sys_common/net.rs#L32
…riplett Support `#[unix_sigpipe = "inherit|sig_dfl"]` on `fn main()` to prevent ignoring `SIGPIPE` When enabled, programs don't have to explicitly handle `ErrorKind::BrokenPipe` any longer. Currently, the program ```rust fn main() { loop { println!("hello world"); } } ``` will print an error if used with a short-lived pipe, e.g. % ./main | head -n 1 hello world thread 'main' panicked at 'failed printing to stdout: Broken pipe (os error 32)', library/std/src/io/stdio.rs:1016:9 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace by enabling `#[unix_sigpipe = "sig_dfl"]` like this ```rust #![feature(unix_sigpipe)] #[unix_sigpipe = "sig_dfl"] fn main() { loop { println!("hello world"); } } ``` there is no error, because `SIGPIPE` will not be ignored and thus the program will be killed appropriately: % ./main | head -n 1 hello world The current libstd behaviour of ignoring `SIGPIPE` before `fn main()` can be explicitly requested by using `#[unix_sigpipe = "sig_ign"]`. With `#[unix_sigpipe = "inherit"]`, no change at all is made to `SIGPIPE`, which typically means the behaviour will be the same as `#[unix_sigpipe = "sig_dfl"]`. See rust-lang/rust#62569 and referenced issues for discussions regarding the `SIGPIPE` problem itself See the [this](https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/Proposal.3A.20First.20step.20towards.20solving.20the.20SIGPIPE.20problem) Zulip topic for more discussions, including about this PR. Tracking issue: rust-lang/rust#97889
…riplett Support `#[unix_sigpipe = "inherit|sig_dfl"]` on `fn main()` to prevent ignoring `SIGPIPE` When enabled, programs don't have to explicitly handle `ErrorKind::BrokenPipe` any longer. Currently, the program ```rust fn main() { loop { println!("hello world"); } } ``` will print an error if used with a short-lived pipe, e.g. % ./main | head -n 1 hello world thread 'main' panicked at 'failed printing to stdout: Broken pipe (os error 32)', library/std/src/io/stdio.rs:1016:9 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace by enabling `#[unix_sigpipe = "sig_dfl"]` like this ```rust #![feature(unix_sigpipe)] #[unix_sigpipe = "sig_dfl"] fn main() { loop { println!("hello world"); } } ``` there is no error, because `SIGPIPE` will not be ignored and thus the program will be killed appropriately: % ./main | head -n 1 hello world The current libstd behaviour of ignoring `SIGPIPE` before `fn main()` can be explicitly requested by using `#[unix_sigpipe = "sig_ign"]`. With `#[unix_sigpipe = "inherit"]`, no change at all is made to `SIGPIPE`, which typically means the behaviour will be the same as `#[unix_sigpipe = "sig_dfl"]`. See rust-lang/rust#62569 and referenced issues for discussions regarding the `SIGPIPE` problem itself See the [this](https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/Proposal.3A.20First.20step.20towards.20solving.20the.20SIGPIPE.20problem) Zulip topic for more discussions, including about this PR. Tracking issue: rust-lang/rust#97889
Rust ignores SIGPIPE by default, this patch overrides that behaviour to fix this by *not* panic'ing on Broken pipes and restore old scubainit behavior. In short, the following fails: > image: debian:latest > > aliases: > test: yes '' | echo "test" $ scuba test: > test > yes: standard output: Broken pipe * Use nightly on-broken-pipe="inherit" to inherit the behavior from the parent process, Instead of killing our process. See docs here: https://github.com/rust-lang/rust/blob/master/src/doc/unstable-book/src/compiler-flags/on-broken-pipe.md This nightly fix is definitely unstable(with very recent API changes), but hopefully they keep this interface as a compiler option the same until stabilization. * Add test to verify this doesn't break in the future * Add rust-toolchain.toml to define the locked rust version since this requires a nightly option. I specifically didn't think nightly was an issue, since you were looking into using -Zbuild-std for scubainit size minimization (which has a long path to stabilization) This has been a longstanding issue for the Rust language: - rust-lang/rust#62569 - rust-lang/rust#97889
Rust ignores SIGPIPE by default, this patch overrides that behaviour to fix this by *not* panic'ing on Broken pipes and restore old scubainit behavior. In short, the following fails: > image: debian:latest > > aliases: > test: yes '' | echo "test" $ scuba test: > test > yes: standard output: Broken pipe * Use nightly on-broken-pipe="inherit" to inherit the behavior from the parent process, Instead of killing our process. See docs here: https://github.com/rust-lang/rust/blob/master/src/doc/unstable-book/src/compiler-flags/on-broken-pipe.md This nightly fix is definitely unstable(with very recent API changes), but hopefully they keep this interface as a compiler option the same until stabilization. * Add test to verify this doesn't break in the future * Add rust-toolchain.toml to define the locked rust version since this requires a nightly option. I specifically didn't think nightly was an issue, since you were looking into using -Zbuild-std for scubainit size minimization (which has a long path to stabilization) This has been a longstanding issue for the Rust language: - rust-lang/rust#62569 - rust-lang/rust#97889
Rust pre-main code may change the SIGPIPE disposition to ignore: * rust-lang/rust#62569 * rust-lang/rust#97889 We could use the nightly compiler flag -Zon-broken-pipe=inherit to disable this behavior. Instead, we take the simpler route and restore the default disposition ourselves. Fixes #254
Rust pre-main code may change the SIGPIPE disposition to ignore: * rust-lang/rust#62569 * rust-lang/rust#97889 We could use the nightly compiler flag -Zon-broken-pipe=inherit to disable this behavior. Instead, we take the simpler route and restore the default disposition ourselves. Fixes #254
Rust pre-main code may change the SIGPIPE disposition to ignore: * rust-lang/rust#62569 * rust-lang/rust#97889 We could use the nightly compiler flag -Zon-broken-pipe=inherit to disable this behavior. Instead, we take the simpler route and restore the default disposition ourselves. Fixes #254
Rust pre-main code may change the SIGPIPE disposition to ignore: * rust-lang/rust#62569 * rust-lang/rust#97889 We could use the nightly compiler flag -Zon-broken-pipe=inherit to disable this behavior. Instead, we take the simpler route and restore the default disposition ourselves. Fixes #254
Back in 2014, the Rust startup code (which runs before main) started ignoring SIGPIPE by default: #13158
This dates back to when Rust had a runtime and a green-threads implementation, and more magic handling of I/O. The pull request mentions that it "brings the behavior inline with libgreen".
This doesn't seem to be documented anywhere, as far as I can tell; at a minimum, this needs documentation.
But I'd like to question whether we should still do this by default. See #46016 for some recent discussion of this. This changes the default behavior of pipes from what people might expect when writing UNIX applications. Rust currently resets the signal mask in a child process, but that's only if you use Rust to launch the child process rather than calling a library to do so. And in addition to that, signal handlers are process-wide, and people might not expect Rust to change process-wide state like this. This also means that Rust libraries will not have the same behavior as Rust applications.
Either way, this is something we need to document far better. Some people will want one behavior, and some people will want the other, so whichever we use as the default needs careful documentation and a documented procedure to change it.
EDIT (based on a suggestion from @Centril): Perhaps we could control this with an attribute on
main
or on the top-level module?The text was updated successfully, but these errors were encountered: