Skip to content
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

Open
joshtriplett opened this issue Jul 10, 2019 · 39 comments
Open

Should Rust still ignore SIGPIPE by default? #62569

joshtriplett opened this issue Jul 10, 2019 · 39 comments
Labels
A-maybe-future-edition Something we may consider for a future edition. A-runtime Area: std's runtime and "pre-main" init for handling backtraces, unwinds, stack overflows I-lang-nominated The issue / PR has been nominated for discussion during a lang team meeting. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@joshtriplett
Copy link
Member

joshtriplett commented Jul 10, 2019

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?

@joshtriplett joshtriplett added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jul 10, 2019
@joshtriplett
Copy link
Member Author

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".

@Centril Centril added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Jul 10, 2019
@sfackler
Copy link
Member

If we stop ignoring SIGPIPE by default, we will break back compat for every socket server written in Rust, right?

@Centril
Copy link
Contributor

Centril commented Jul 10, 2019

Perhaps we should have a build-time option for this to make it easy to disable/enable in an application?

Why this and e.g. not an attribute on fn main() {? (As a general rule, I think having program semantics defined as much as possible by the source code is preferable to flags since it makes things more standalone.)

@joshtriplett
Copy link
Member Author

@sfackler Right now, we have inconsistent behavior, where your socket server will also break if you ever write main() in any language other than Rust, and turn your code into a library. That flies in the face of one of Rust's greatest strengths: "we don't have a runtime". I discovered this when looking over the pre-main startup code.

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.

@jonas-schievink jonas-schievink added the A-runtime Area: std's runtime and "pre-main" init for handling backtraces, unwinds, stack overflows label Jul 11, 2019
@BurntSushi
Copy link
Member

BurntSushi commented Jul 11, 2019

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 println! and have enough output to exceed stdout's default buffer size, then it's likely that running, for example, command | head -n1 will result in a panic. Secondly, if you're using writeln!(...)? instead, and are setting an error code when writeln!(...) fails, then it's likely that command | head -n1 will stop with an unexpected exit code.

For example, normally a process will exit with a 141 exit code upon receipt of a pipe error:

$ seq 1 10000 | head -n1
1
$ echo ${pipestatus[1]}
141

So to fix this, you firstly need to know enough to use writeln!(...), and then you either need to assume all errors produced by writeln!(...) are broken pipe errors (and emit the correct exit code), or explicitly check errors for io::ErrorKind::BrokenPipe and respond accordingly. The former is, strictly speaking, incorrect, even though it's likely the only error you'll encounter when writing to stdout is a broken pipe error (but the code using write! may not know that it is writing to stdout). The latter works, but it's tedious, and even I get it wrong:

$ rg . rust/ | head -n1
rust/bstr/rustfmt.toml:disable_all_formatting = true
$ echo ${pipestatus[1]}
0

This, presumably, should be 141. But at least it doesn't panic.

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 std, if it's possible, that un-ignores SIGPIPE and is intended to run at the beginning of main. If that's workable, then that might be preferrable since it adds fewer directives to the language itself. The other issue here is documentation and discoverability. i.e., How can we help folks writing CLI tools discover, in the first place, that they need to un-ignore SIGPIPE?

(Although, I guess adding a function to std and requiring users to call it doesn't fix @joshtriplett's case, where Rust is a library. Am I understanding that right?)

@alexcrichton
Copy link
Member

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).

@joshtriplett
Copy link
Member Author

@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.

@alexcrichton
Copy link
Member

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 Cargo.toml and a one-liner in src/main.rs to enable)

@joshtriplett
Copy link
Member Author

joshtriplett commented Jul 16, 2019

@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 #![no_ignore_sigpipe] would be easier. Because it should be possible to write Rust code that doesn't have any startup code at all, and with some care we could potentially eliminate all of the current startup code in the future.

@alexcrichton
Copy link
Member

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.

@OvermindDL1
Copy link

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 start to never register a sigpipe handler at all? I thought that was pretty baked in?

@Centril
Copy link
Contributor

Centril commented Jul 17, 2019

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.

@joshtriplett
Copy link
Member Author

@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.)

@OvermindDL1

It's possible to make a crate that causes start to never register a sigpipe handler at all? I thought that was pretty baked in?

Exactly. That's the main reason I'm asking after having built-in support.

@OvermindDL1
Copy link

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 main seems sufficient, or even a compiler flag (exposed via something in the Cargo.toml file perhaps as well?).

@Diggsey
Copy link
Contributor

Diggsey commented Jul 19, 2019

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.

@richfelker
Copy link

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 SIGPIPE to happen, without applications having to take their own measures to stop it, there is a right way that does not clobber any state. For each write or equivalent (note: send and sendto can just use sendmsg as their backend with MSG_NOSIGNAL flag), do the following:

  1. Block SIGPIPE.
  2. Perform the write.
  3. If it failed with EPIPE, call sigwaitinfoto consume theSIGPIPE` while it's still blocked.
  4. Restore the old mask.

This procedure is entirely portable and thread-safe, because signal masks are thread-local and SIGPIPE is generated synchronously for the thread, not for the process.

@hclarke
Copy link

hclarke commented Sep 9, 2019

an alternate solution:
println! calls std::process::exit(141) instead of panic!() if it fails on EPIPE

that way, both servers and cli tools work by default

@richfelker
Copy link

The issue of igoring SIGPIPE is related to, but separate from, the issue of what println! does on write failure. Changing signal dispositions behind the application's back in a way it has no way to fix up before exec'ing something else (AFAICT the original disposition is lost) is a much bigger problem than what println! does, since applications can just refrain from using println!.

With that said, I agree std::process::exit(141) would be less bad than panic!(). The latter should be for programming errors, not for completely valid runtime conditions.

@BurntSushi
Copy link
Member

This came up in a StackOverflow question, and my advice was basically, "either don't use println! in non-toy programs, or use this unsafe FFI binding via libc to re-enable the default behavior for handling PIPE signals."

@jyn514
Copy link
Member

jyn514 commented Jan 19, 2021

println! calls std::process::exit(141) instead of panic!() if it fails on EPIPE

If you have multiple threads, this will abort the whole process instead of only panicking the current thread.

@BurntSushi
Copy link
Member

@jyn514 yes. That's exactly what you want in most CLI programs.

@joshtriplett
Copy link
Member Author

@BurntSushi Programs using println! are not necessarily CLI programs, though. They may, for instance, be servers that use stdout/stderr for logging.

@wchargin
Copy link
Contributor

wchargin commented Feb 6, 2021

Another reason not to use std::process::exit(141) is that it is not
the same as being killed by a SIGPIPE. Standard shells will report
“termination by signal n” as return value n + 128, but this is a
property of the shell, not the system (e.g., man bash).

You can distinguish the two cases from the output of waitpid(2):

Short Rust program demonstrating signal behavior
use 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(())
}

This prints:

"exit 141" exited with code Some(141); signal: None
"echo sigpipe" exited with code None; signal: Some(13)
"kill -13 $$" exited with code None; signal: Some(13)

We can see that a bare exit terminates with a code and no signal,
whereas being killed terminates with a signal and no code.

Further reading: “Proper handling of SIGINT/SIGQUIT” describes how
this can be important. Choice quote: “You cannot "fake" the proper exit
status by an exit(3) with a special numeric value, even if you look up
the numeric value for your system
” (emphasis in original).

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Feb 27, 2024
…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 <martin.nordholts@codetale.se>
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 <martin.nordholts@codetale.se>
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 <martin.nordholts@codetale.se>
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 <martin.nordholts@codetale.se>
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
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Feb 27, 2024
…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 <martin.nordholts@codetale.se>
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 <martin.nordholts@codetale.se>
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 <martin.nordholts@codetale.se>
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 <martin.nordholts@codetale.se>
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
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Feb 28, 2024
…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 <martin.nordholts@codetale.se>
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 <martin.nordholts@codetale.se>
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 <martin.nordholts@codetale.se>
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 <martin.nordholts@codetale.se>
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
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Feb 28, 2024
…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 <martin.nordholts@codetale.se>
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 <martin.nordholts@codetale.se>
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 <martin.nordholts@codetale.se>
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 <martin.nordholts@codetale.se>
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
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Feb 28, 2024
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 <martin.nordholts@codetale.se>
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 <martin.nordholts@codetale.se>
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 <martin.nordholts@codetale.se>
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 <martin.nordholts@codetale.se>
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
@the8472
Copy link
Member

the8472 commented Feb 28, 2024

If you really really don't want SIGPIPE to happen, without applications having to take their own measures to stop it, there is a right way that does not clobber any state. For each write or equivalent (note: send and sendto can just use sendmsg as their backend with MSG_NOSIGNAL flag), do the following:

1. Block `SIGPIPE`.
2. Perform the `write`.
3. If it failed with `EPIPE, call `sigwaitinfo`to consume the`SIGPIPE` while it's still blocked.
4. Restore the old mask.

This procedure is entirely portable and thread-safe, because signal masks are thread-local and SIGPIPE is generated synchronously for the thread, not for the process.

This approach is interesting, but the issue with it is that you must now apply this to every single thing that calls write() and might be hitting a pipe, even File due to named fifos. And that wouldn't just be std code but also libraries that roll their own IO via libc calls. Which also means now everything that does IO pays increased syscall costs (which have gone up due to CPU vulnerability mitigations).
All that just to keep some global state intact that maybe ought be inherited by child processes.

An equivalent to MSG_NOSIGNAL for write() would be much more palatable but that does not appear to exist.

@jgoerzen
Copy link

jgoerzen commented Feb 29, 2024

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.

@bstrie bstrie added the A-maybe-future-edition Something we may consider for a future edition. label Mar 9, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Mar 10, 2024
…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
jhpratt added a commit to jhpratt/rust that referenced this issue Mar 11, 2024
…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
@Amanieu Amanieu removed the I-libs-nominated The issue / PR has been nominated for discussion during a libs team meeting. label Mar 13, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Mar 30, 2024
…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
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Mar 30, 2024
…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
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Mar 30, 2024
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
sunshowers added a commit to oxidecomputer/omicron that referenced this issue Apr 2, 2024
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
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this issue Apr 20, 2024
…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
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this issue Apr 27, 2024
…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
wcampbell0x2a added a commit to wcampbell0x2a/scuba that referenced this issue May 8, 2024
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
wcampbell0x2a added a commit to wcampbell0x2a/scuba that referenced this issue May 8, 2024
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
JonathonReinhart added a commit to JonathonReinhart/scuba that referenced this issue May 20, 2024
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
JonathonReinhart added a commit to JonathonReinhart/scuba that referenced this issue May 20, 2024
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
JonathonReinhart added a commit to JonathonReinhart/scuba that referenced this issue May 22, 2024
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
JonathonReinhart added a commit to JonathonReinhart/scuba that referenced this issue May 29, 2024
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-maybe-future-edition Something we may consider for a future edition. A-runtime Area: std's runtime and "pre-main" init for handling backtraces, unwinds, stack overflows I-lang-nominated The issue / PR has been nominated for discussion during a lang team meeting. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
Status: Idea
Development

No branches or pull requests