From accf3d251168e10ecbb0b4ae200c3196b950dac9 Mon Sep 17 00:00:00 2001
From: Kevin K <kbknapp@gmail.com>
Date: Mon, 29 Nov 2021 14:19:21 -0500
Subject: [PATCH] fix(panic): swallows broken pipe errors

We were unwrapping an `Err` from the `write!` calls, of which things
like `BrokenPipe` can happen when the write end of a pipe is closed.
Because we were exit'ing the process anyways at the time the `panic` was
occurring, we are now just swallowing the error.

Changing this API to bubble up the result would be a
breaking change.

Another approach would be to try and write to stdout, if that fails due
to a broken pipe then use stderr. However, that would change the
semantics in what could be argued is a breaking change. Simply dropping
the error, can always be changed to this "use stderr if stdout is
closed" approach later if desired.

For a great explanation of the types of errors see the README in
`calm_io`:

https://github.com/myrrlyn/calm_io/blob/a42845575a04cd8b65e92c19d104627f5fcad3d7/README.md
---
 Cargo.toml    |  2 +-
 src/errors.rs | 23 +++++++++++++++++++----
 src/macros.rs |  4 ++++
 3 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/Cargo.toml b/Cargo.toml
index 4c450e78fd6..543d048ee2e 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name = "clap"
-version = "2.33.3"
+version = "2.33.4"
 authors = ["Kevin K. <kbknapp@gmail.com>"]
 exclude = ["examples/*", "clap-test/*", "tests/*", "benches/*", "*.png", "clap-perf/*", "*.dot"]
 repository = "https://github.com/clap-rs/clap"
diff --git a/src/errors.rs b/src/errors.rs
index 1f86720fd5b..74c20b81485 100644
--- a/src/errors.rs
+++ b/src/errors.rs
@@ -391,14 +391,29 @@ impl Error {
         }
     }
 
-    /// Prints the error to `stderr` and exits with a status of `1`
+    /// Prints the error message and exits. If `Error::use_stderr` evaluates to `true`, the message
+    /// will be written to `stderr` and exits with a status of `1`. Otherwise, `stdout` is used
+    /// with a status of `0`.
     pub fn exit(&self) -> ! {
         if self.use_stderr() {
-            wlnerr!("{}", self.message);
+            wlnerr!(@nopanic "{}", self.message);
             process::exit(1);
         }
-        let out = io::stdout();
-        writeln!(&mut out.lock(), "{}", self.message).expect("Error writing Error to stdout");
+        // We are deliberately dropping errors here. We could match on the error kind, and only
+        // drop things such as `std::io::ErrorKind::BrokenPipe`, however nothing is being bubbled
+        // up or reported back to the caller and we will be exit'ing the process anyways.
+        // Additionally, changing this API to bubble up the result would be a breaking change.
+        //
+        // Another approach could be to try and write to stdout, if that fails due to a broken pipe
+        // then use stderr. However, that would change the semantics in what could be argued is a
+        // breaking change. Simply dropping the error, can always be changed to this "use stderr if
+        // stdout is closed" approach later if desired.
+        //
+        // A good explanation of the types of errors are SIGPIPE where the read side of the pipe
+        // closes before the write side. See the README in `calm_io` for a good explanation:
+        //
+        // https://github.com/myrrlyn/calm_io/blob/a42845575a04cd8b65e92c19d104627f5fcad3d7/README.md
+        let _ = writeln!(&mut io::stdout().lock(), "{}", self.message);
         process::exit(0);
     }
 
diff --git a/src/macros.rs b/src/macros.rs
index 3b5a36b99b2..01cbca8095d 100644
--- a/src/macros.rs
+++ b/src/macros.rs
@@ -839,6 +839,10 @@ macro_rules! impl_settings {
 
 // Convenience for writing to stderr thanks to https://github.com/BurntSushi
 macro_rules! wlnerr(
+    (@nopanic $($arg:tt)*) => ({
+        use std::io::{Write, stderr};
+        let _ = writeln!(&mut stderr().lock(), $($arg)*);
+    });
     ($($arg:tt)*) => ({
         use std::io::{Write, stderr};
         writeln!(&mut stderr(), $($arg)*).ok();