Prevent broken pipes causing ICEs #49606

varkor · 2018-04-02T22:57:38Z

As the private std::io::print_to panics if there is an I/O error, which is used by println!, the compiler would ICE if one attempted to use a broken pipe (e.g. rustc --help | false). This introduces a new (private) macro try_println! which allows us to avoid this.

As a side note, it seems this macro might be useful publicly (and actually there seems to be a crate specifically for this purpose), though that can probably be left for a future discussion.

One slight alternative approach would be to simply early exit without an error (i.e. exit code 0), which this comment suggests is the usual approach. I've opted not to take that approach initially, because I think it's more helpful to know when there is a broken pipe.

Fixes #34376.

nikomatsakis · 2018-04-04T17:55:17Z

Maybe we should just use that crate? I guess this is minimal enough code it's probably not worth adding an external dependency.

nikomatsakis

I feel like I'd rather not entable libcore/libstd if we can avoid it. Thoughts?

nikomatsakis · 2018-04-04T17:57:15Z

src/libstd/io/stdio.rs

+#[doc(hidden)]
+pub fn _try_print(args: fmt::Arguments) -> io::Result<()> {
+    try_print_to(args, &LOCAL_STDOUT, stdout)
+}


seems fine, but I feel like we could easily use stable APIs to build this, no? Like, can't we just make librustc_driver use write! or something?

The primary reason I did it this way was to make sure try_println! had ensured identical behaviour as println! (apart from the panic behaviour). I'm not sure how important this is in practice, but felt that it was generally a good idea to keep things familiar, so as not to introduce some inconsistency that would lead to a different issue in the future. (Plus, if we ever want to make such functionality public, it's trivial.)

If you think this is overcomplicating things, though, I could write inside librustc_driver instead, yeah.

varkor · 2018-04-04T21:45:19Z

Maybe we should just use that crate? I guess this is minimal enough code it's probably not worth adding an external dependency.

Yes, it really is very minimal. Additionally, the behaviour of the crate isn't exactly the same as println! (#49606 (comment) again relevant).

nikomatsakis · 2018-04-06T14:18:48Z

What's an example of how println! and write! might differ?

varkor · 2018-04-06T19:12:54Z

My main concern was:

rust/src/libstd/io/stdio.rs

Line 701 in 7678d50

print_to(args, &LOCAL_STDOUT, stdout, "stdout");

write! wouldn't have this stdout fallback, though I'm not sure if this would ever actually be an issue from the context of librustc_driver. I'm not really familiar with the context.

nikomatsakis · 2018-04-06T20:26:13Z

OK, well, I don't really care that much either way. r=me for the compiler bits, but I'd like somebody from @rust-lang/libs to at least glance over the changes to println!.

r? @alexcrichton

alexcrichton · 2018-04-06T22:12:50Z

I'm personally a little concerned about the long-term maintainability of a solution like this, for example this doesn't change anything in src/librustc_errors, but doesn't that mean diagnostics will hit broken pipes? If we want to terminate on EPIPE I think we should just reenable the signal handler, right?

varkor · 2018-04-08T13:47:00Z

@alexcrichton: why/where was the signal handler originally disabled?

sfackler · 2018-04-08T15:37:26Z

@varkor

rust/src/libstd/sys/unix/mod.rs

Line 71 in f1ea23e

pub fn init() {

It's disabled by default because it only really makes sense for CLI programs that only touch stdin/stdout/stderr and don't do any e.g. networking.

varkor · 2018-04-10T21:59:56Z

@sfackler: Thanks! I didn't realise that EPIPE triggered SIGPIPE.

~~@alexcrichton: Are there any reasons to want to ignore SIGPIPE (i.e. is completely removing the signal handler okay)?~~

Also, are we happy with just exiting, or do we want to trigger an message?

varkor · 2018-04-10T22:57:19Z

Now an EPIPE will simply terminate rustc/rustdoc, by (re-)overriding the SIGPIPE signal handler.

alexcrichton · 2018-04-11T00:23:00Z

src/rustc/rustc.rs

+    unsafe {
+        // Set the SIGPIPE signal handler, so that an EPIPE
+        // will cause rustc to terminate, as expected.
+        assert!(signal(libc::SIGPIPE, libc::SIG_DFL) != libc::SIG_ERR);


I think this may not compile on Windows, and could the logic between rustc and rustdoc be shared?

pietroalbini · 2018-04-16T10:07:16Z

Ping from triage @alexcrichton! The author addressed your comments!

alexcrichton · 2018-04-16T14:13:25Z

@bors: r+

er sorry @varkor, I missed the update!

bors · 2018-04-16T14:13:26Z

📌 Commit 7ab31f6 has been approved by alexcrichton

Prevent broken pipes causing ICEs As the private `std::io::print_to` panics if there is an I/O error, which is used by `println!`, the compiler would ICE if one attempted to use a broken pipe (e.g. `rustc --help | false`). This introduces a new (private) macro `try_println!` which allows us to avoid this. As a side note, it seems this macro might be useful publicly (and actually there seems to be [a crate specifically for this purpose](https://crates.io/crates/try_print/)), though that can probably be left for a future discussion. One slight alternative approach would be to simply early exit without an error (i.e. exit code `0`), which [this comment](rust-lang#34376 (comment)) suggests is the usual approach. I've opted not to take that approach initially, because I think it's more helpful to know when there is a broken pipe. Fixes rust-lang#34376.

Rollup of 8 pull requests Successful merges: - #49555 (Inline most of the code paths for conversions with boxed slices) - #49606 (Prevent broken pipes causing ICEs) - #49646 (Use box syntax instead of Box::new in Mutex::remutex on Windows) - #49647 (Remove `underscore_lifetimes` and `match_default_bindings` from active feature list) - #49931 (Fix incorrect span in `&mut` suggestion) - #49959 (rustbuild: allow building tools with debuginfo) - #49965 (Remove warning about f64->f32 cast being potential UB) - #49994 (Remove unnecessary indentation in rustdoc book codeblock.) Failed merges:

…h726 rustc: Use `unix_sigpipe` instead of `rustc_driver::set_sigpipe_handler` This is the first (known) step towards starting to use `unix_sigpipe` in the wild. Eventually, `rustc_driver::set_sigpipe_handler` can be removed and all clients can use `unix_sigpipe` instead. For now we just start using `unix_sigpipe` in one place: `rustc` itself. It is easy to manually verify this change. If you remove `#[unix_sigpipe = "sig_dfl"]` and run `./x.py build` you will get an ICE when you do `./build/x86_64-unknown-linux-gnu/stage1/bin/rustc --help | false`. Add back `#[unix_sigpipe = "sig_dfl"]` and the ICE disappears again. PR that added `set_sigpipe_handler`: rust-lang#49606 Tracking issue for `unix_sigpipe`: rust-lang#97889 Not sure exactly how to label this PR. Going with T-libs for now since this is a T-libs feature. `@rustdoc` labels +T-libs

…triddle rustdoc: Use `unix_sigpipe` instead of `rustc_driver::set_sigpipe_handler` Do what was already done for `rustc` in rust-lang#102587, namely start using `unix_sigpipe` instead of `rustc_driver::set_sigpipe_handler`. After this has been merged, we can completely remove `rustc_driver::set_sigpipe_handler`. PR that added `set_sigpipe_handler`: rust-lang#49606 Tracking issue for `unix_sigpipe`: rust-lang#97889 Verification of this change --------------------------- 1. Remove `#[unix_sigpipe = "sig_dfl"]` 1. Run `./x.py build` 1. Run `./build/aarch64-apple-darwin/stage1/bin/rustdoc --help | false` 1. Observe ICE 1. Add back `#[unix_sigpipe = "sig_dfl"]` 1. Run `./x.py build` 1. Run `./build/aarch64-apple-darwin/stage1/bin/rustdoc --help | false` 1. Observe ICE fixed `@rustbot` labels +T-rustdoc

…triddle rustdoc: Use `unix_sigpipe` instead of `rustc_driver::set_sigpipe_handler` Do what was already done for `rustc` in rust-lang#102587, namely start using `unix_sigpipe` instead of `rustc_driver::set_sigpipe_handler`. After this has been merged, we can completely remove `rustc_driver::set_sigpipe_handler`. PR that added `set_sigpipe_handler`: rust-lang#49606 Tracking issue for `unix_sigpipe`: rust-lang#97889 Verification of this change --------------------------- 1. Remove `#[unix_sigpipe = "sig_dfl"]` 1. Run `./x.py build` 1. Run `./build/aarch64-apple-darwin/stage1/bin/rustdoc --help | false` 1. Observe ICE 1. Add back `#[unix_sigpipe = "sig_dfl"]` 1. Run `./x.py build` 1. Run `./build/aarch64-apple-darwin/stage1/bin/rustdoc --help | false` 1. Observe ICE fixed ``@rustbot`` labels +T-rustdoc

…h726 rustc: Use `unix_sigpipe` instead of `rustc_driver::set_sigpipe_handler` This is the first (known) step towards starting to use `unix_sigpipe` in the wild. Eventually, `rustc_driver::set_sigpipe_handler` can be removed and all clients can use `unix_sigpipe` instead. For now we just start using `unix_sigpipe` in one place: `rustc` itself. It is easy to manually verify this change. If you remove `#[unix_sigpipe = "sig_dfl"]` and run `./x.py build` you will get an ICE when you do `./build/x86_64-unknown-linux-gnu/stage1/bin/rustc --help | false`. Add back `#[unix_sigpipe = "sig_dfl"]` and the ICE disappears again. PR that added `set_sigpipe_handler`: rust-lang#49606 Tracking issue for `unix_sigpipe`: rust-lang#97889 Not sure exactly how to label this PR. Going with T-libs for now since this is a T-libs feature. ``@rustdoc`` labels +T-libs

…h726 rustc: Use `unix_sigpipe` instead of `rustc_driver::set_sigpipe_handler` This is the first (known) step towards starting to use `unix_sigpipe` in the wild. Eventually, `rustc_driver::set_sigpipe_handler` can be removed and all clients can use `unix_sigpipe` instead. For now we just start using `unix_sigpipe` in one place: `rustc` itself. It is easy to manually verify this change. If you remove `#[unix_sigpipe = "sig_dfl"]` and run `./x.py build` you will get an ICE when you do `./build/x86_64-unknown-linux-gnu/stage1/bin/rustc --help | false`. Add back `#[unix_sigpipe = "sig_dfl"]` and the ICE disappears again. PR that added `set_sigpipe_handler`: rust-lang#49606 Tracking issue for `unix_sigpipe`: rust-lang#97889 Not sure exactly how to label this PR. Going with T-libs for now since this is a T-libs feature. ```@rustdoc``` labels +T-libs

…h726 rustc: Use `unix_sigpipe` instead of `rustc_driver::set_sigpipe_handler` This is the first (known) step towards starting to use `unix_sigpipe` in the wild. Eventually, `rustc_driver::set_sigpipe_handler` can be removed and all clients can use `unix_sigpipe` instead. For now we just start using `unix_sigpipe` in one place: `rustc` itself. It is easy to manually verify this change. If you remove `#[unix_sigpipe = "sig_dfl"]` and run `./x.py build` you will get an ICE when you do `./build/x86_64-unknown-linux-gnu/stage1/bin/rustc --help | false`. Add back `#[unix_sigpipe = "sig_dfl"]` and the ICE disappears again. PR that added `set_sigpipe_handler`: rust-lang#49606 Tracking issue for `unix_sigpipe`: rust-lang#97889 Not sure exactly how to label this PR. Going with T-libs for now since this is a T-libs feature. ````@rustdoc```` labels +T-libs

rustc: Use `unix_sigpipe` instead of `rustc_driver::set_sigpipe_handler` This is the first (known) step towards starting to use `unix_sigpipe` in the wild. Eventually, `rustc_driver::set_sigpipe_handler` can be removed and all clients can use `unix_sigpipe` instead. For now we just start using `unix_sigpipe` in one place: `rustc` itself. It is easy to manually verify this change. If you remove `#[unix_sigpipe = "sig_dfl"]` and run `./x.py build` you will get an ICE when you do `./build/x86_64-unknown-linux-gnu/stage1/bin/rustc --help | false`. Add back `#[unix_sigpipe = "sig_dfl"]` and the ICE disappears again. PR that added `set_sigpipe_handler`: rust-lang/rust#49606 Tracking issue for `unix_sigpipe`: #97889 Not sure exactly how to label this PR. Going with T-libs for now since this is a T-libs feature. ````@rustdoc```` labels +T-libs

…, r=tmiasko Remove `rustc_driver::set_sigpipe_handler()` Its usage was removed in rust-lang#102587 and rust-lang#103495, so we do not need to keep it around any longer. According to [preliminary input](https://rust-lang.zulipchat.com/#narrow/stream/182449-t-compiler.2Fhelp/topic/Find.20.60rustc_driver.60.20dependent.20projects.3F/near/304490764), we do not need to worry about any deprecation cycle for this explicitly unstable API, and can just straight up remove it. PR that added `set_sigpipe_handler`: rust-lang#49606 Tracking issue for `unix_sigpipe`: rust-lang#97889 Migration instructions for any remaining clients --- Change from ```rust #![feature(rustc_private)] extern crate rustc_driver; fn main() { rustc_driver::set_sigpipe_handler(); // ... ``` to ```rust #![feature(unix_sigpipe)] #[unix_sigpipe = "sig_dfl"] fn main() { // ... ``` `@rustbot` labels +T-compiler

…, r=tmiasko Remove `rustc_driver::set_sigpipe_handler()` Its usage was removed in rust-lang#102587 and rust-lang#103495, so we do not need to keep it around any longer. According to [preliminary input](https://rust-lang.zulipchat.com/#narrow/stream/182449-t-compiler.2Fhelp/topic/Find.20.60rustc_driver.60.20dependent.20projects.3F/near/304490764), we do not need to worry about any deprecation cycle for this explicitly unstable API, and can just straight up remove it. PR that added `set_sigpipe_handler`: rust-lang#49606 Tracking issue for `unix_sigpipe`: rust-lang#97889 Migration instructions for any remaining clients --- Change from ```rust #![feature(rustc_private)] extern crate rustc_driver; fn main() { rustc_driver::set_sigpipe_handler(); // ... ``` to ```rust #![feature(unix_sigpipe)] #[unix_sigpipe = "sig_dfl"] fn main() { // ... ``` ``@rustbot`` labels +T-compiler

rust-highfive assigned nikomatsakis Apr 2, 2018

pietroalbini added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 2, 2018

nikomatsakis requested changes Apr 4, 2018

View reviewed changes

nikomatsakis added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 4, 2018

rust-highfive assigned alexcrichton and unassigned nikomatsakis Apr 6, 2018

varkor force-pushed the pipe-repair branch from 31e306a to 9e4ff95 Compare April 10, 2018 22:55

alexcrichton reviewed Apr 11, 2018

View reviewed changes

Prevent EPIPE causing ICEs in rustc and rustdoc

7ab31f6

varkor force-pushed the pipe-repair branch from 9e4ff95 to 7ab31f6 Compare April 11, 2018 10:05

pietroalbini added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 16, 2018

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 16, 2018

kennytm mentioned this pull request Apr 16, 2018

Rollup of 8 pull requests #50003

Merged

bors merged commit 7ab31f6 into rust-lang:master Apr 16, 2018

varkor deleted the pipe-repair branch April 16, 2018 23:23

Enselic mentioned this pull request Oct 3, 2022

rustc: Use unix_sigpipe instead of rustc_driver::set_sigpipe_handler #102587

Merged

Enselic mentioned this pull request Oct 24, 2022

rustdoc: Use unix_sigpipe instead of rustc_driver::set_sigpipe_handler #103495

Merged

Enselic mentioned this pull request Oct 25, 2022

Remove rustc_driver::set_sigpipe_handler() #103536

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent broken pipes causing ICEs #49606

Prevent broken pipes causing ICEs #49606

varkor commented Apr 2, 2018

nikomatsakis commented Apr 4, 2018

nikomatsakis left a comment

nikomatsakis Apr 4, 2018

varkor Apr 4, 2018

varkor commented Apr 4, 2018

nikomatsakis commented Apr 6, 2018

varkor commented Apr 6, 2018

nikomatsakis commented Apr 6, 2018

alexcrichton commented Apr 6, 2018

varkor commented Apr 8, 2018

sfackler commented Apr 8, 2018

varkor commented Apr 10, 2018 •

edited

Loading

varkor commented Apr 10, 2018

alexcrichton Apr 11, 2018

pietroalbini commented Apr 16, 2018

alexcrichton commented Apr 16, 2018

bors commented Apr 16, 2018

Prevent broken pipes causing ICEs #49606

Prevent broken pipes causing ICEs #49606

Conversation

varkor commented Apr 2, 2018

nikomatsakis commented Apr 4, 2018

nikomatsakis left a comment

Choose a reason for hiding this comment

nikomatsakis Apr 4, 2018

Choose a reason for hiding this comment

varkor Apr 4, 2018

Choose a reason for hiding this comment

varkor commented Apr 4, 2018

nikomatsakis commented Apr 6, 2018

varkor commented Apr 6, 2018

nikomatsakis commented Apr 6, 2018

alexcrichton commented Apr 6, 2018

varkor commented Apr 8, 2018

sfackler commented Apr 8, 2018

varkor commented Apr 10, 2018 • edited Loading

varkor commented Apr 10, 2018

alexcrichton Apr 11, 2018

Choose a reason for hiding this comment

pietroalbini commented Apr 16, 2018

alexcrichton commented Apr 16, 2018

bors commented Apr 16, 2018

varkor commented Apr 10, 2018 •

edited

Loading