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

scubainit: Use nightly flag -Zon-broken-pipe=inherit to fix SIGPIPE #253

Conversation

wcampbell0x2a
Copy link

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 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
@haboustak
Copy link
Contributor

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

The documentation for the compiler flag seems to incorrectly describe the historical behavior when the flag isn't set.

If -Zon-broken-pipe is not used, libstd will behave in the manner it has since 2014, before Rust 1.0. SIGPIPE will be set to SIG_IGN before fn main() and result in EPIPE errors which are converted to std::io::ErrorKind::BrokenPipe.

When spawning child processes, SIGPIPE will be set to SIG_DFL before doing the underlying exec() syscall.

I think that scuba would have been fine if SIGPIPE had been set to SIG_DFL by Rust before exec() since 2014. It has no need to terminate on or handle SIGPIPE, it just needs to be a transparent executor. Resetting signal actions before exec() is often correct/required, but also seems to be a breaking change.

Since there's a lot of churn related to this feature, I might be inclined to suggest the simple route, and reset the signal action at the top of main() or run_scubainit().

// Restore the default SIGPIPE action
fn restore_sigpipe() {
    unsafe {
        libc::signal(libc::SIGPIPE, libc::SIG_DFL);
    }
}

@wcampbell0x2a
Copy link
Author

Since there's a lot of churn related to this feature, I might be inclined to suggest the simple route, and reset the signal action at the top of main() or run_scubainit().

The cost of 2 syscalls vs using an unstable option seems in favor of the syscalls, I agree.

@JonathonReinhart
Copy link
Owner

Thanks folks for clearly presenting both options here. I went ahead and opened #255 with Mike's suggestion of restoring the default signal disposition ourselves.

Please take a look!

@JonathonReinhart
Copy link
Owner

Thank you very much for your contribution!

I believe this PR has been obsoleted by #255, which I just released in v2.13.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants