Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions benches/bench_tar.rs
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,7 @@ fn extract_archive_20_files(bencher: divan::Bencher) {
input,
&archive_path,
false,
false,
CompressionMode::None,
)
.unwrap();
Expand Down
4 changes: 4 additions & 0 deletions src/uu/tar/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,10 @@ pub enum TarError {
#[error("tar: {path}: Member name contains '..'")]
MemberNameContainsDotDot { path: PathBuf },

/// A member that `-k` keeps from replacing an existing file
#[error("tar: {path}: Cannot open: File exists")]
FileExists { path: PathBuf },

/// Errors that were reported earlier and recovered from
#[error("tar: Exiting with failure status due to previous errors")]
PreviousErrors,
Expand Down
33 changes: 32 additions & 1 deletion src/uu/tar/src/operations/extract.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,10 @@
use crate::compression::open_archive_reader;
use crate::errors::{finish_after_previous_errors, report_recoverable, TarError};
use crate::CompressionMode;
use std::fs;
use std::io::Read;
use std::io::{self, BufWriter, Write};
use std::path::Path;
use std::path::{Component, Path, PathBuf};
use tar::Archive;
use uucore::error::UResult;

Expand All @@ -18,6 +19,7 @@ use uucore::error::UResult;
///
/// * `archive_path` - Path to the tar archive to extract
/// * `verbose` - Whether to print verbose output during extraction
/// * `keep_old` - Don't replace existing files; each one is reported as an error
///
/// # Errors
///
Expand All @@ -29,6 +31,7 @@ pub fn extract_archive(
input: impl Read,
archive_path: &Path,
verbose: bool,
keep_old: bool,
compression: CompressionMode,
) -> UResult<()> {
let reader = open_archive_reader(input, compression)?;
Expand Down Expand Up @@ -58,6 +61,19 @@ pub fn extract_archive(
writeln!(out, "{}", path.display()).map_err(TarError::Io)?;
}

// -k: like GNU tar, an existing file is an error and is left as it is;
// an existing directory is fine.
if keep_old && !entry.header().entry_type().is_dir() {
if let Some(target) = unpack_target(&path) {
if fs::symlink_metadata(&target).is_ok() {
out.flush().map_err(TarError::Io)?;
report_recoverable(&TarError::FileExists { path: target })?;
refused_member = true;
continue;
}
}
}

// Unpack the entry. `unpack_in` skips, and returns false for, a member whose
// name contains a `..` component, as it would escape the target directory.
let unpacked = entry.unpack_in(".").map_err(|e| TarError::CannotExtract {
Expand All @@ -78,6 +94,21 @@ pub fn extract_archive(
Ok(())
}

/// The path `unpack_in(".")` writes *path* to: leading `/` and `.` components
/// are dropped. `None` when it writes nothing, for a member with a `..`
/// component (refused) or no name left.
fn unpack_target(path: &Path) -> Option<PathBuf> {
let mut target = PathBuf::new();
for component in path.components() {
match component {
Component::Prefix(_) | Component::RootDir | Component::CurDir => {}
Component::ParentDir => return None,
Component::Normal(part) => target.push(part),
}
}
(!target.as_os_str().is_empty()).then_some(target)
}

#[cfg(test)]
#[path = "extract_tests.rs"]
mod tests;
2 changes: 1 addition & 1 deletion src/uu/tar/src/operations/extract_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ fn test_extract_archive_with_zstd() {

let _guard = crate::operations::TestDirGuard::enter(tempdir.path());
let input = fs::File::open(&archive_path).unwrap();
let result = extract_archive(input, &archive_path, true, CompressionMode::Zstd);
let result = extract_archive(input, &archive_path, true, false, CompressionMode::Zstd);

result.unwrap();
assert_eq!(
Expand Down
12 changes: 10 additions & 2 deletions src/uu/tar/src/tar.rs
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,7 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {

let verbose = matches.get_flag("verbose");
let allow_absolute = matches.get_flag("absolute-names");
let keep_old = matches.get_flag("keep-old-files");
let recursive = !matches.get_flag("no-recursion");
let explicit_compression = if matches.get_flag("gzip") {
Some(CompressionMode::Gzip)
Expand All @@ -162,11 +163,17 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {

let compression = explicit_compression.unwrap_or(CompressionMode::Auto);
return if archive_path == Path::new("-") {
operations::extract::extract_archive(io::stdin(), archive_path, verbose, compression)
operations::extract::extract_archive(
io::stdin(),
archive_path,
verbose,
keep_old,
compression,
)
} else {
let file =
File::open(archive_path).map_err(|e| TarError::from_io_error(e, archive_path))?;
operations::extract::extract_archive(file, archive_path, verbose, compression)
operations::extract::extract_archive(file, archive_path, verbose, keep_old, compression)
};
}

Expand Down Expand Up @@ -279,6 +286,7 @@ pub fn uu_app() -> Command {
arg!(--zstd "Filter through zstd").conflicts_with("gzip"),
// Common options
arg!(-v --verbose "Verbosely list files processed"),
arg!(-k --"keep-old-files" "Don't replace existing files when extracting"),
arg!(--"no-recursion" "Avoid descending automatically in directories"),
// arg!(-h --dereference "Follow symlinks"),
// arg!(-p --"preserve-permissions" "Extract information about file permissions"),
Expand Down
93 changes: 92 additions & 1 deletion tests/by-util/test_tar.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@ use std::io::{Cursor, Read, Write};
use std::path::{self, PathBuf};

use tar_rs_crate::{Archive as TarRsArchive, Builder as TarRsBuilder, Header as TarRsHeader};
use uutests::{at_and_ucmd, new_ucmd};
use uutests::util::TestScenario;
use uutests::{at_and_ucmd, new_ucmd, util_name};

/// Size of a single tar block in bytes (per POSIX specification).
// TODO: This should be exported from the tar crate instead of being redefined here.
Expand Down Expand Up @@ -86,6 +87,96 @@ fn test_create_dir_verbose() {
.stdout_contains(file2_path);
}

#[test]
fn test_extract_keep_old_files() {
let ts = TestScenario::new(util_name!());
let at = &ts.fixtures;

at.write("a.txt", "from archive");
at.write("b.txt", "also from archive");
ts.ucmd()
.args(&["-cf", "archive.tar", "a.txt", "b.txt"])
.succeeds();
at.write("a.txt", "already here");
at.remove("b.txt");

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.

the asserts already say this, the comment can go

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Removed.

ts.ucmd()
.args(&["-xkf", "archive.tar"])
.fails()
.code_is(2)
.stderr_contains("tar: a.txt: Cannot open: File exists")
.stderr_contains("Exiting with failure status due to previous errors");
assert_eq!(at.read("a.txt"), "already here");
assert_eq!(at.read("b.txt"), "also from archive");
}

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.

please also add a test where the member is a directory that already exists (it should not be an error)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added test_extract_keep_old_files_existing_directory.


#[test]
fn test_extract_keep_old_files_existing_directory() {
let ts = TestScenario::new(util_name!());
let at = &ts.fixtures;

at.mkdir("d");
at.write("d/f.txt", "from archive");
ts.ucmd().args(&["-cf", "archive.tar", "d"]).succeeds();
at.remove("d/f.txt");

ts.ucmd()
.args(&["-xkf", "archive.tar"])
.succeeds()
.no_output();
assert_eq!(at.read("d/f.txt"), "from archive");
}

fn keep_old_members_tar(names: &[&str]) -> Vec<u8> {

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.

almost the same code as in test_extract_reports_member_with_parent_dir_and_continues, could you please use this helper there too?
and maybe a less -k specific name, like tar_with_members

let mut tar_bytes = Vec::new();
{
let mut builder = TarRsBuilder::new(&mut tar_bytes);
for name in names {
let mut header = TarRsHeader::new_gnu();
header.as_old_mut().name[..name.len()].copy_from_slice(name.as_bytes());
header.set_mode(0o644);
header.set_size(1);
header.set_cksum();
builder.append(&header, Cursor::new("x")).unwrap();
}
builder.finish().unwrap();
}
tar_bytes
}

#[test]
fn test_extract_keep_old_files_checks_the_unpacked_path() {
let (at, mut ucmd) = at_and_ucmd!();
at.mkdir("out");
at.write("out/kept.txt", "already here");
// A leading `/` is dropped when extracting, so `/kept.txt` lands on
// out/kept.txt, which -k must keep.
at.write_bytes("members.tar", &keep_old_members_tar(&["/kept.txt"]));

ucmd.args(&["-xkf", "../members.tar"])
.current_dir(at.plus("out"))
.fails()
.code_is(2)
.stderr_contains("tar: kept.txt: Cannot open: File exists");
assert_eq!(at.read("out/kept.txt"), "already here");
}

#[test]
fn test_extract_keep_old_files_reports_parent_dir_member() {
let (at, mut ucmd) = at_and_ucmd!();
at.mkdir("out");
at.write("item", "outside");
at.write_bytes("members.tar", &keep_old_members_tar(&["../item"]));

ucmd.args(&["-xkf", "../members.tar"])
.current_dir(at.plus("out"))
.fails()
.code_is(2)
.stderr_contains("../item: Member name contains '..'")
.stderr_does_not_contain("File exists");
assert_eq!(at.read("item"), "outside");
}

#[test]
fn test_create_no_recursion() {
let (at, mut ucmd) = at_and_ucmd!();
Expand Down
Loading