Skip to content

uucore: emulate GNU close_stdout / close_stream behaviour - #13999

Open
sylvestre wants to merge 1 commit into
uutils:mainfrom
sylvestre:close-stdout-fix
Open

uucore: emulate GNU close_stdout / close_stream behaviour#13999
sylvestre wants to merge 1 commit into
uutils:mainfrom
sylvestre:close-stdout-fix

Conversation

@sylvestre

Copy link
Copy Markdown
Contributor

GNU coreutils use close_stdout() (from gnulib) to flush stdout at exit and report write errors. Key behaviours:

  • Exit 1 if flush fails (e.g. writing to /dev/full)
  • Silence EBADF when stdout was already closed at startup (prog >&-) and the program produced no output
  • Silence BrokenPipe (normal pipe shutdown)

In Rust the bin! macro previously only printed a message on flush failure without exiting non-zero. Fix this by:

  1. Detecting whether fd 1 is open at startup via /proc/self/fd/1 (before any allocation can claim fd 1)
  2. Flushing stdout in bin_inner! after uumain and exiting 1 on error, unless the error is BrokenPipe or EBADF-on-initially-closed-stdout

The flush logic is moved from bin! into bin_inner! so stdout_initially_open is in scope.

This fixes the /dev/full cases in tests/misc/close-stdout.sh.

GNU coreutils use close_stdout() (from gnulib) to flush stdout at exit
and report write errors. Key behaviours:
- Exit 1 if flush fails (e.g. writing to /dev/full)
- Silence EBADF when stdout was already closed at startup (prog >&-)
  and the program produced no output
- Silence BrokenPipe (normal pipe shutdown)

In Rust the bin! macro previously only printed a message on flush
failure without exiting non-zero. Fix this by:
1. Detecting whether fd 1 is open at startup via /proc/self/fd/1
   (before any allocation can claim fd 1)
2. Flushing stdout in bin_inner! after uumain and exiting 1 on error,
   unless the error is BrokenPipe or EBADF-on-initially-closed-stdout

This fixes the /dev/full cases in tests/misc/close-stdout.sh.
Copilot AI lite review requested due to automatic review settings August 18, 2026 08:55

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Comment thread src/uucore/src/lib/lib.rs
// start). We use /proc/self/fd/1 to avoid a libc dependency.
#[cfg(unix)]
let stdout_initially_open: bool =
std::path::Path::new("/proc/self/fd/1").exists();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not work if /proc is masked.

@RenjiSann

Copy link
Copy Markdown
Collaborator

This reintroduce a bug in cksum, when doing cksum /dev/null > /dev/full where the "write error" message is printed twice when it should not:

(GNU) $ cksum /dev/null > /dev/full                   
cksum: write error
(uutils) $ cksum /dev/null > /dev/full                       
cksum: write error: No space left on device
error writing to stdout: No space left on device (os error 28)

Additionally, it renders the split between bin! and bin_inner! useless

@github-actions

Copy link
Copy Markdown

GNU testsuite comparison:

Skip an intermittent issue tests/cut/bounded-memory (fails in this run but passes in the 'main' branch)
Skip an intermittent issue tests/date/resolution (fails in this run but passes in the 'main' branch)
Skip an intermittent issue tests/tail/tail-n0f (fails in this run but passes in the 'main' branch)
Skipping an intermittent issue tests/date/date-locale-hour (passes in this run but fails in the 'main' branch)
Skipping an intermittent issue tests/tail/symlink (passes in this run but fails in the 'main' branch)
Congrats! The gnu test tests/cp/link-heap is now passing!
Congrats! The gnu test tests/rm/many-dir-entries-vs-OOM is now passing!
Congrats! The gnu test tests/seq/seq-epipe is now passing!

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.

4 participants