mab122
/
radicle-heartwood-lfs
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

cli: Handle broken pipe (SIGPIPE) gracefully 

Prevent `rad` from panicking when its stdout is a closed pipe, e.g.
when running `rad config | head`.

The broken pipes are handled at the output boundaries using three
layers of defence:

1. Inner: Route all stdout output through `term::print()` and
   `term::print_inline()`, which use explicit `writeln!`/`write!` to
   a locked stdout and silently ignore write errors. All `println!`
   calls in `radicle-cli` and `radicle-term` are converted.
2. Middle: Catch `BrokenPipe` errors that propagate up via `anyhow`
   from subcommands in `main::run()`, and exit cleanly with code 0.
3. Outer: Install a panic hook that intercepts `println!` panics
   containing "Broken pipe" (from dependencies like clap) and exits
   cleanly, chained in front of human_panic's hook.

To prevent regressions, `#![deny(clippy::print_stdout)]` is added to
`radicle-cli`, requiring all future stdout output to go through the
safe `term::` functions.

Note: `eprintln!` calls (stderr) are not handled, as broken stderr
pipes are extremely rare in practice.
This commit is contained in:
Fintan Halpenny 2026-04-02 16:57:28 +00:00 committed by Lorenz Leutgeb
parent e1f16bee26
commit 1a31a9f541
16 changed files with 145 additions and 36 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
Cargo.lock generated
View File

@ -3022,6 +3022,7 @@ name = "radicle-cli"
version = "0.20.0"
dependencies = [
"anyhow",
"backtrace",
"chrono",
"clap",
"clap_complete",

1
Cargo.toml
View File

@ -20,6 +20,7 @@ rust-version = "1.85.0"
[workspace.dependencies]
amplify = { version = "4.0.0", default-features = false }
backtrace = "0.3.75"
bstr = "1.3"
bytes = "1.11.1"
chrono = { version = "0.4.26", default-features = false }

3
crates/radicle-cli/Cargo.toml
View File

@ -14,7 +14,7 @@ name = "rad"
path = "src/main.rs"
[features]
default = ["i2p", "tor"]
default = ["backtrace", "i2p","tor"]
i2p = ["radicle/i2p"]
tor = ["radicle/tor"]
@ -57,6 +57,7 @@ tree-sitter-toml-ng = "0.6.0"
tree-sitter-typescript = "0.23.2"
[target.'cfg(unix)'.dependencies]
backtrace = { workspace = true, optional = true }
shlex = { workspace = true }
[target.'cfg(windows)'.dependencies]

6
crates/radicle-cli/src/commands/cob.rs
View File

@ -80,7 +80,7 @@ pub fn run(args: Args, ctx: impl term::Context) -> anyhow::Result<()> {
oid
}
};
println!("{oid}");
term::print(oid);
}
Migrate => {
let mut db = profile.cobs_db_mut()?;
@ -100,7 +100,7 @@ pub fn run(args: Args, ctx: impl term::Context) -> anyhow::Result<()> {
FilteredTypeName::from(type_name).as_ref(),
)?;
for cob in cobs {
println!("{}", cob.id);
term::print(cob.id);
}
}
Log {
@ -215,7 +215,7 @@ pub fn run(args: Args, ctx: impl term::Context) -> anyhow::Result<()> {
}
};
println!("{oid}");
term::print(oid);
}
}
Ok(())

1
crates/radicle-cli/src/commands/cob/args.rs
View File

@ -400,7 +400,6 @@ mod test {
.chain(ARGS.iter())
.collect::<Vec<_>>(),
);
println!("{args:?}");
assert!(args.is_ok())
}

2
crates/radicle-cli/src/commands/debug.rs
View File

@ -60,7 +60,7 @@ fn debug(profile: Option<&Profile>) -> anyhow::Result<()> {
warnings: collect_warnings(profile),
};
println!("{}", serde_json::to_string_pretty(&debug).unwrap());
term::print(serde_json::to_string_pretty(&debug).unwrap());
Ok(())
}

4
crates/radicle-cli/src/commands/id.rs
View File

@ -373,7 +373,7 @@ fn print(
profile: &Profile,
) -> anyhow::Result<()> {
print_meta(revision, previous, profile)?;
println!();
term::blank();
print_diff(revision.parent.as_ref(), &revision.id, repo)?;
Ok(())
@ -514,7 +514,7 @@ fn print_diff(
if let Some(modified) = diff.modified().next() {
let diff = modified.diff.to_unified_string()?;
print!("{diff}");
term::print_inline(diff);
} else {
term::print(term::format::italic("No changes."));
}

40
crates/radicle-cli/src/commands/inspect.rs
View File

@ -68,7 +68,7 @@ pub fn run(args: Args, ctx: impl term::Context) -> anyhow::Result<()> {
let refs = RefsAt::new(&repo, remote)?;
let sigrefs = SignedRefs::load_at(refs.at, remote, &repo);
println!(
term::print(format_args!(
"{:<48} {} {}",
term::format::tertiary(remote.to_human()),
term::format::secondary(refs.at),
@ -103,7 +103,7 @@ pub fn run(args: Args, ctx: impl term::Context) -> anyhow::Result<()> {
term::format::negative("missing".to_string())
}
}
);
));
}
}
Target::Policy => {
@ -111,19 +111,19 @@ pub fn run(args: Args, ctx: impl term::Context) -> anyhow::Result<()> {
let seed = policies.seed_policy(&rid)?;
match seed.policy {
SeedingPolicy::Allow { scope } => {
println!(
term::print(format_args!(
"Repository {} is {} with scope {}",
term::format::tertiary(&rid),
term::format::positive("being seeded"),
term::format::dim(format!("`{scope}`"))
);
));
}
SeedingPolicy::Block => {
println!(
term::print(format_args!(
"Repository {} is {}",
term::format::tertiary(&rid),
term::format::negative("not being seeded"),
);
));
}
}
}
@ -132,19 +132,19 @@ pub fn run(args: Args, ctx: impl term::Context) -> anyhow::Result<()> {
let aliases = profile.aliases();
for did in doc.delegates().iter() {
if let Some(alias) = aliases.alias(did) {
println!(
term::print(format_args!(
"{} {}",
term::format::tertiary(&did),
term::format::parens(term::format::dim(alias))
);
));
} else {
println!("{}", term::format::tertiary(&did));
term::print(term::format::tertiary(&did));
}
}
}
Target::Visibility => {
let (_, doc) = repo(rid, storage)?;
println!("{}", term::format::visibility(doc.visibility()));
term::print(term::format::visibility(doc.visibility()));
}
Target::History => {
let (repo, _) = repo(rid, storage)?;
@ -177,22 +177,22 @@ pub fn run(args: Args, ctx: impl term::Context) -> anyhow::Result<()> {
.with_timezone(&timezone)
.to_rfc2822();
println!(
term::print(format_args!(
"{} {}",
term::format::yellow("commit"),
term::format::yellow(oid),
);
));
if let Ok(parent) = tip.parent_id(0) {
println!("parent {parent}");
term::print(format_args!("parent {parent}"));
}
println!("blob {}", revision.blob);
println!("date {time}");
println!();
term::print(format_args!("blob {}", revision.blob));
term::print(format_args!("date {time}"));
term::blank();
if let Some(msg) = tip.message() {
for line in msg.lines() {
if line.is_empty() {
println!();
term::blank();
} else {
term::indented(term::format::dim(line));
}
@ -200,10 +200,10 @@ pub fn run(args: Args, ctx: impl term::Context) -> anyhow::Result<()> {
term::blank();
}
for line in json::to_pretty(&doc, Path::new("radicle.json"))? {
println!(" {line}");
term::print(format_args!(" {line}"));
}
println!();
term::blank();
}
}
Target::RepoId => {
@ -232,7 +232,7 @@ fn refs(repo: &radicle::storage::git::Repository) -> anyhow::Result<()> {
}
}
print!("{}", tree(refs));
term::print_inline(tree(refs));
Ok(())
}

2
crates/radicle-cli/src/commands/node.rs
View File

@ -88,7 +88,7 @@ pub fn run(args: Args, ctx: impl term::Context) -> anyhow::Result<()> {
Command::Inventory { nid } => {
let nid = nid.as_ref().unwrap_or(profile.id());
for rid in profile.routing()?.get_inventory(nid)? {
println!("{}", term::format::tertiary(rid));
term::print(term::format::tertiary(rid));
}
}
Command::Status {

6
crates/radicle-cli/src/commands/node/control.rs
View File

@ -170,7 +170,7 @@ pub fn logs(lines: usize, follow: Option<time::Duration>, profile: &Profile) ->
}
tail.reverse();
print!("{}", term::format::dim(String::from_utf8_lossy(&tail)));
term::print_inline(term::format::dim(String::from_utf8_lossy(&tail)));
if let Some(timeout) = follow {
file.seek(SeekFrom::End(0))?;
@ -184,7 +184,7 @@ pub fn logs(lines: usize, follow: Option<time::Duration>, profile: &Profile) ->
if len == 0 {
thread::sleep(time::Duration::from_millis(250));
} else {
print!("{}", term::format::dim(line));
term::print_inline(term::format::dim(line));
}
}
}
@ -414,7 +414,7 @@ pub fn config(node: &Node) -> anyhow::Result<()> {
let cfg = node.config()?;
let cfg = serde_json::to_string_pretty(&cfg)?;
println!("{cfg}");
term::print(cfg);
Ok(())
}

4
crates/radicle-cli/src/commands/node/events.rs
View File

@ -2,6 +2,8 @@ use std::time;
use radicle::node::{Event, Handle};
use crate::terminal as term;
pub fn run<H>(node: H, count: usize, timeout: time::Duration) -> anyhow::Result<()>
where
H: Handle<Event = Result<Event, <H as Handle>::Error>>,
@ -11,7 +13,7 @@ where
let event = event?;
let obj = serde_json::to_string(&event)?;
println!("{obj}");
term::print(&obj);
// Only output up to `count` events.
if i + 1 >= count {

2
crates/radicle-cli/src/commands/node/routing.rs
View File

@ -44,6 +44,6 @@ fn print_table(entries: impl IntoIterator<Item = (RepoId, NodeId)>) {
fn print_json(entries: impl IntoIterator<Item = (RepoId, NodeId)>) {
for (rid, nid) in entries {
println!("{}", serde_json::json!({ "rid": rid, "nid": nid }));
term::print(serde_json::json!({ "rid": rid, "nid": nid }));
}
}

2
crates/radicle-cli/src/commands/path.rs
View File

@ -9,7 +9,7 @@ pub use args::Args;
pub fn run(_args: Args, _ctx: impl term::Context) -> anyhow::Result<()> {
let home = profile::home()?;
println!("{}", home.path().display());
term::print(home.path().display());
Ok(())
}

3
crates/radicle-cli/src/lib.rs
View File

@ -1,4 +1,7 @@
#![allow(clippy::too_many_arguments)]
// Prevent use of `println!` and `print!` which panic on broken pipes.
// Use `terminal::io::println` or `terminal::io::print` instead.
#![deny(clippy::print_stdout)]
pub mod commands;
pub mod git;
pub mod node;

102
crates/radicle-cli/src/main.rs
View File

@ -118,6 +118,25 @@ fn main() {
.homepage(env!("CARGO_PKG_HOMEPAGE"))
.support("Open a support request at https://radicle.zulipchat.com/ or file an issue via Radicle itself, or e-mail to team@radicle.dev"));
// Install a panic hook that intercepts panics caused by broken pipes and exits
// cleanly. This is a backstop for any uses of `println!` (in our code or
// dependencies like `clap`) that were not converted to `term::print`.
//
// `println!` panics with "failed printing to stdout: Broken pipe" when
// failing to write to a closed standard output. We chain our hook in front
// of `human_panic`'s hook so that panics not caused by broken pipes are
// still handled by `human_panic`.
//
// See also <https://github.com/rust-lang/rust/issues/62569>.
#[cfg(unix)]
{
let default_hook = std::panic::take_hook();
std::panic::set_hook(Box::new(move |info| {
handle_broken_pipe(info);
default_hook(info);
}));
}
if let Some(lvl) = radicle::logger::env_level() {
let logger = Box::new(radicle::logger::Logger::new(lvl));
log::set_boxed_logger(logger).expect("no other logger should have been set already");
@ -146,12 +165,95 @@ fn run(command: Command, ctx: impl term::Context) -> ! {
match run_command(command, ctx) {
Ok(()) => process::exit(0),
Err(err) => {
// If the error is a broken pipe, exit cleanly. This happens when
// output is piped to a command that exits before reading all our
// output, e.g. `rad config | head`.
//
// Rust ignores `SIGPIPE` by default (since 1.62), so broken pipes
// and instead returns `io::ErrorKind::BrokenPipe` errors on writes.
// We want to catch these and exit cleanly.
//
// See <https://github.com/rust-lang/rust/issues/62569>.
#[cfg(unix)]
if is_broken_pipe(&err) {
process::exit(0);
}
term::fail(&err);
process::exit(1);
}
}
}
/// Handle an error of kind [`ErrorKind::BrokenPipe`] during a panic, and
/// exit the process with exit code 0.
///
/// # Debug
///
/// If compiled with `debug_assertions` enabled, then the panic is written to
/// [`std::io::stderr`].
#[cfg(unix)]
fn handle_broken_pipe(info: &std::panic::PanicHookInfo<'_>) {
if !is_broken_pipe_panic(info) {
return;
}
if cfg!(debug_assertions) {
let thread = std::thread::current();
let thread = thread.name().unwrap_or("<unnamed>");
let mut stderr = std::io::stderr().lock();
match info.location() {
Some(location) => {
let _ = writeln!(
stderr,
"broken pipe in thread '{thread}' at: {}:{}",
location.file(),
location.line(),
);
}
None => {
let _ = writeln!(stderr, "broken pipe in thread '{thread}'");
}
}
#[cfg(feature = "backtrace")]
let backtrace = format!("{:?}", backtrace::Backtrace::new());
#[cfg(not(feature = "backtrace"))]
let backtrace = "(no backtrace available)";
let _ = writeln!(stderr, "{backtrace}");
}
process::exit(0);
}
/// Check if any error in the [`anyhow::Error::chain`] of `err` is of kind
/// [`ErrorKind::BrokenPipe`].
#[cfg(unix)]
fn is_broken_pipe(err: &anyhow::Error) -> bool {
err.chain()
.filter_map(|cause| cause.downcast_ref::<io::Error>())
.any(|io_err| io_err.kind() == ErrorKind::BrokenPipe)
}
/// Check whether a panic was caused by writing to a broken pipe.
///
/// The standard library panics with a [`String`] payload containing
/// "Broken pipe" when [`println!`] or [`print!`] fail to write because standard
/// output is closed. This is stable behaviour across all Unix platforms, since
/// it is adopted from the description of `EPIPE` in [`errno.h` in POSIX.1-2024].
///
/// [`errno.h` in POSIX.1-2024]: https://pubs.opengroup.org/onlinepubs/9799919799.2024edition/basedefs/errno.h.html
#[cfg(unix)]
fn is_broken_pipe_panic(info: &std::panic::PanicHookInfo<'_>) -> bool {
info.payload()
.downcast_ref::<&'static str>()
.copied()
.or(info.payload().downcast_ref::<String>().map(|s| s.as_str()))
.is_some_and(|message| message.contains("Broken pipe"))
}
fn run_command(command: Command, ctx: impl term::Context) -> Result<(), anyhow::Error> {
match command {
Command::Auth(args) => auth::run(args, ctx),

2
crates/radicle-node/Cargo.toml
View File

@ -17,7 +17,7 @@ test = ["radicle/test", "radicle-crypto/test", "radicle-crypto/cyphernet", "radi
tor = ["cyphernet/tor", "radicle/tor", "radicle-protocol/tor"]
[dependencies]
backtrace = { version = "0.3.75", optional = true }
backtrace = { workspace = true, optional = true }
bytes = { workspace = true }
crossbeam-channel = { workspace = true }
cyphernet = { workspace = true, features = ["dns", "ed25519", "p2p-ed25519", "noise-framework", "noise_sha2"] }