Repository navigation
tar: report members containing '..' and exit with status 2 - #397
Conversation
When extracting, tar-rs refuses a member whose name contains a `..` component, so that it cannot be written outside the current directory, and reports that through the return value of `unpack_in`. That value was ignored: the member was skipped silently and tar exited 0. GNU tar reports such a member, extracts the remaining ones, and exits 2 with "Exiting with failure status due to previous errors". Do the same, and only print "Error is not recoverable: exiting now" when the exit status comes from an error that stopped tar, not from one it recovered from. Fixes uutils#386
| // If exiting with code 2 because of an error, show the "not recoverable" | ||
| // message. An exit code set through `set_exit_code` instead records errors that | ||
| // tar reported and recovered from, such as members it refused to extract. | ||
| if exit_code == 2 && uucore::error::get_exit_code() != 2 { |
There was a problem hiding this comment.
this relies on nobody else calling set_exit_code(2) before a fatal error, not super robust
could you please document the why in one line?
| out.flush().map_err(TarError::Io)?; | ||
| writeln!( | ||
| io::stderr(), | ||
| "tar: {}: Member name contains '..'", |
There was a problem hiding this comment.
please add a variant in TarError (errors.rs) instead of a hardcoded string
| if refused_member { | ||
| writeln!( | ||
| io::stderr(), | ||
| "tar: Exiting with failure status due to previous errors" |
There was a problem hiding this comment.
same here, please move it to errors.rs
this isn't specific to extract either, it might be better in one shared place
|
@sylvestre moved both messages into |
Merging this PR will not alter performance
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #397 +/- ##
==========================================
+ Coverage 96.84% 97.82% +0.97%
==========================================
Files 11 15 +4
Lines 1492 1794 +302
Branches 29 36 +7
==========================================
+ Hits 1445 1755 +310
+ Misses 46 38 -8
Partials 1 1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Thanks for your PR |
2 participants
Summary
Fixes #386.
When extracting an archive containing a member with a
..path component, tar currently skips that member silently and exits successfully.This happens because the return value of
unpack_inis ignored when it refuses to extract an unsafe path.This PR changes that behavior to:
...Testing
Added a regression test that creates an archive containing an unsafe
../member followed by a valid member.The test verifies that:
The regression test fails on the original implementation and passes with the fix.
Additional checks passed:
cargo test --workspace-D warningscargo fmt --all -- --checkNotes
The error reporting follows the existing project's filename-display conventions. Some formatting differences from GNU tar remain, including escaping special characters in member names.
AI Assistance
Claude Code assisted with implementation and testing. I reviewed the changes and test results before submitting.