tail: report a read/seek failure instead of panicking - #14298
Open
arbelonson-source wants to merge 1 commit into
Open
tail: report a read/seek failure instead of panicking#14298arbelonson-source wants to merge 1 commit into
arbelonson-source wants to merge 1 commit into
Conversation
Once `bounded_tail` decides a file is seekable, every seek and read it
does to find the start of the last N lines/bytes assumed success and
unwrapped the result. A seekable file that then fails to read -- a
character device that times out or errors mid-read, for instance --
crashed instead of reporting the failure:
$ tail /dev/drm_dp_aux2
thread 'main' panicked at src/uu/tail/src/chunks.rs:92:40:
called `Result::unwrap()` on an `Err` value: Os { code: 5, ... }
GNU reports it as a read error and exits 1:
$ tail /dev/drm_dp_aux2
tail: error reading '/dev/drm_dp_aux2': Input/output error
`ReverseChunks` (used by `-n`'s negative case) and every seek in
`bounded_tail` (used by all of `-n`/`-c`, positive and negative) now
return `io::Result` instead of unwrapping, converted at the one call
site into the same message `tail` already uses for an unreadable
directory.
Verified against the same device as root, across every `bounded_tail`
mode (default, `-n N`, `-n +N`, `-c N`, `-c +N`): each now matches GNU's
message and exit code exactly, where each previously panicked (the `-c`
cases already avoided a panic before this change, by going through a
different, already-fallible copy path, but reported a bare
"Input/output error" without the filename; that gap is unchanged here
and is a separate, pre-existing issue).
No portable, root-free way to reproduce the underlying condition in the
test suite -- it needs a real device whose read genuinely fails after
open -- so this is verified manually rather than by an added test;
cargo test --features tail --test tests -- test_tail (161 pre-existing,
0 new) is unaffected.
|
GNU testsuite comparison: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #13124.
Once
bounded_taildecides a file is seekable, every seek and read it does to find the start of the last N lines/bytes assumed success and unwrapped the result. A seekable file that then fails to read — a character device that times out or errors mid-read, for instance — crashed instead of reporting the failure:This isn't specific to
-n(the mode in the original report) or to this one device — it's every mode that takes the seekable/bounded path, since they all shared the same unwrap-on-seek pattern:What changed
ReverseChunks::newand itsIterator::next(used by-n's negative case) now returnio::Resultinstead of unwrapping every seek/read.backwards_thru_filepropagates that with?instead of.unwrap().bounded_tail's seeking logic (covering all of-n/-c, positive and negative) is split into a smallio::Result-returning helper, so every one of its seeks propagates instead of unwrapping.io::Errorfrom that helper into the sametail-error-reading-filemessagetailalready uses for an unreadable directory, so the wording and exit code (1) match GNU exactly.Not covered
The
-ccases didn't panic before this change —print_target_section's copy path is already fallible — but they report a bareInput/output errorwithout the filename, where GNU sayserror reading 'FILE': .... That gap is unchanged by this PR; fixing it means threading the display name intoprint_target_sectiontoo, which felt like a separate, smaller follow-up rather than something to fold in here.Testing
Verified against
/dev/drm_dp_aux2as root (the device from the report), across everybounded_tailmode: default,-n N,-n +N,-c N,-c +N. Each now matches GNU's message and exit code exactly, where all but the-ccases previously panicked.I couldn't find a portable, root-free way to reproduce the underlying condition (a seekable file whose read genuinely fails) in the shared test suite, so this is verified manually rather than by an added regression test.
-n/-csign/zero combinations, small and large files, nonexistent files): all match, confirming no regression to the normal path.cargo test --features tail --test tests -- test_tail: 161 passed, 0 failed (all pre-existing; unaffected by this change).cargo test --test testsbuild.cargo fmt --checkandcargo clippy -p uu_tail --all-targets -- -D warnings: clean.Disclosure
Prepared with AI assistance (Claude Opus 5, via Claude Code), per the AI policy in CONTRIBUTING.md. GNU's behaviour was established by running the installed GNU binary as a black box; I did not read GNU coreutils source. All testing was run locally.