Conversation
diff writes the full result buffer (and side-by-side writes each line) with a bare .unwrap(), so a reader closing early (diff big1 big2 | head) turns a BrokenPipe error into a panic and a core dump (exit 134). GNU diff dies quietly to SIGPIPE (exit 141) in the same situation. Handle BrokenPipe explicitly at both write sites and exit(141); any other write error still panics since it would be a genuine bug. Fixes uutils#244
| ); | ||
| } else { | ||
| io::stdout().write_all(&result).unwrap(); | ||
| } else if let Err(e) = io::stdout().write_all(&result) { |
There was a problem hiding this comment.
the println! for --brief just above will still panic on a closed pipe, no?
| } else { | ||
| io::stdout().write_all(&result).unwrap(); | ||
| } else if let Err(e) = io::stdout().write_all(&result) { | ||
| exit_on_broken_pipe_or_panic(e); |
There was a problem hiding this comment.
please add a test in tests/integration.rs (closed stdout pipe, and /dev/full)
| Result::Left(left_ln) => push_output(left_ln, b"", b'<', output, &config).unwrap(), | ||
| Result::Right(right_ln) => push_output(b"", right_ln, b'>', output, &config).unwrap(), | ||
| Result::Left(left_ln) => { | ||
| if let Err(e) = push_output(left_ln, b"", b'<', output, &config) { |
There was a problem hiding this comment.
almost the same code 3 times.
could side_diff::diff return io::Result and let diff.rs handle it once?
| /// Any other write error is a genuine bug, so it still panics. | ||
| pub fn exit_on_broken_pipe_or_panic(e: std::io::Error) -> ! { | ||
| if e.kind() == std::io::ErrorKind::BrokenPipe { | ||
| std::process::exit(141); |
There was a problem hiding this comment.
we don't want process::exit in the lib, diff should stay embeddable.
could you please propagate the error and return ExitCode::from(141) from diff::main instead?
| std::process::exit(141); | ||
| } | ||
| panic!("{e}"); | ||
| } |
There was a problem hiding this comment.
same for panic
other write errors aren't bugs, e.g. diff a b > /dev/full.
GNU prints "write error" and exits 2, please do the same instead of panicking
2 participants
Summary
diff's output write is a bare.unwrap()(normal/context/unified/ed path insrc/diff.rs:94, side-by-side path insrc/side_diff.rs), so when the reader closes the pipe early (diff big1 big2 | head) theBrokenPipeerror is unwrapped and the process panics/aborts (exit 134) instead of dying quietly toSIGPIPElike GNU (exit 141).Fix
Added
exit_on_broken_pipe_or_panicinsrc/utils.rs: onErrorKind::BrokenPipeit exits with 141 (matching GNU); any other write error still panics, since that would be a genuine bug. Wired it into both write sites.Test plan
cargo build— cleancargo test— 21 passedcargo fmt -- --check/cargo clippy --all-targets— clean (pre-existing warnings in unrelated test code untouched)diff big1 big2 | head -1— before: panic, exit 134; after: exit 141diff -y big1 big2 | head -1Fixes #244
🤖 Generated with Claude Code