From 322c2b4df6266ba6bfc4fce7213791c9ee9b130a Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Sat, 16 Mar 2024 10:24:55 +0100 Subject: [PATCH 1/6] shred: simplify the code The formatting directive {:2.0} will handle both cases (single-digit and double-digit numbers) by ensuring at least two characters wide with no decimal places --- src/uu/shred/src/shred.rs | 24 +++++++----------------- 1 file changed, 7 insertions(+), 17 deletions(-) diff --git a/src/uu/shred/src/shred.rs b/src/uu/shred/src/shred.rs index b142e2e94..bb938c73d 100644 --- a/src/uu/shred/src/shred.rs +++ b/src/uu/shred/src/shred.rs @@ -477,23 +477,13 @@ fn wipe_file( for (i, pass_type) in pass_sequence.into_iter().enumerate() { if verbose { let pass_name = pass_name(&pass_type); - if total_passes < 10 { - show_error!( - "{}: pass {}/{} ({})...", - path.maybe_quote(), - i + 1, - total_passes, - pass_name - ); - } else { - show_error!( - "{}: pass {:2.0}/{:2.0} ({})...", - path.maybe_quote(), - i + 1, - total_passes, - pass_name - ); - } + show_error!( + "{}: pass {:2}/{} ({})...", + path.maybe_quote(), + i + 1, + total_passes, + pass_name + ); } // size is an optional argument for exactly how many bytes we want to shred // Ignore failed writes; just keep trying From f410d0967fe0a8f1ade553de7443460985dd8dd7 Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Sat, 16 Mar 2024 10:48:19 +0100 Subject: [PATCH 2/6] shred: if the file is empty, don't run passes on it --- src/uu/shred/src/shred.rs | 53 ++++++++++++++++++++----------------- tests/by-util/test_shred.rs | 20 ++++++++++++++ 2 files changed, 48 insertions(+), 25 deletions(-) diff --git a/src/uu/shred/src/shred.rs b/src/uu/shred/src/shred.rs index bb938c73d..8a73eb1f1 100644 --- a/src/uu/shred/src/shred.rs +++ b/src/uu/shred/src/shred.rs @@ -406,9 +406,10 @@ fn wipe_file( )); } + let metadata = fs::metadata(path).map_err_context(String::new)?; + // If force is true, set file permissions to not-readonly. if force { - let metadata = fs::metadata(path).map_err_context(String::new)?; let mut perms = metadata.permissions(); #[cfg(unix)] #[allow(clippy::useless_conversion, clippy::unnecessary_cast)] @@ -428,35 +429,37 @@ fn wipe_file( // Fill up our pass sequence let mut pass_sequence = Vec::new(); + if metadata.len() != 0 { + // Only add passes if the file is non-empty - if n_passes <= 3 { - // Only random passes if n_passes <= 3 - for _ in 0..n_passes { - pass_sequence.push(PassType::Random); - } - } else { - // First fill it with Patterns, shuffle it, then evenly distribute Random - let n_full_arrays = n_passes / PATTERNS.len(); // How many times can we go through all the patterns? - let remainder = n_passes % PATTERNS.len(); // How many do we get through on our last time through? + if n_passes <= 3 { + // Only random passes if n_passes <= 3 + for _ in 0..n_passes { + pass_sequence.push(PassType::Random); + } + } else { + // First fill it with Patterns, shuffle it, then evenly distribute Random + let n_full_arrays = n_passes / PATTERNS.len(); // How many times can we go through all the patterns? + let remainder = n_passes % PATTERNS.len(); // How many do we get through on our last time through? - for _ in 0..n_full_arrays { - for p in PATTERNS { - pass_sequence.push(PassType::Pattern(p)); + for _ in 0..n_full_arrays { + for p in PATTERNS { + pass_sequence.push(PassType::Pattern(p)); + } + } + for pattern in PATTERNS.into_iter().take(remainder) { + pass_sequence.push(PassType::Pattern(pattern)); + } + let mut rng = rand::thread_rng(); + pass_sequence.shuffle(&mut rng); // randomize the order of application + + let n_random = 3 + n_passes / 10; // Minimum 3 random passes; ratio of 10 after + // Evenly space random passes; ensures one at the beginning and end + for i in 0..n_random { + pass_sequence[i * (n_passes - 1) / (n_random - 1)] = PassType::Random; } } - for pattern in PATTERNS.into_iter().take(remainder) { - pass_sequence.push(PassType::Pattern(pattern)); - } - let mut rng = rand::thread_rng(); - pass_sequence.shuffle(&mut rng); // randomize the order of application - - let n_random = 3 + n_passes / 10; // Minimum 3 random passes; ratio of 10 after - // Evenly space random passes; ensures one at the beginning and end - for i in 0..n_random { - pass_sequence[i * (n_passes - 1) / (n_random - 1)] = PassType::Random; - } } - // --zero specifies whether we want one final pass of 0x00 on our file if zero { pass_sequence.push(PassType::Pattern(PATTERNS[0])); diff --git a/tests/by-util/test_shred.rs b/tests/by-util/test_shred.rs index d5de7882f..03aeef0c4 100644 --- a/tests/by-util/test_shred.rs +++ b/tests/by-util/test_shred.rs @@ -143,3 +143,23 @@ fn test_hex() { ucmd.arg("--size=0x10").arg(file).succeeds(); } + +#[test] +fn test_shred_empty() { + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + + let file_a = "test_shred_remove_a"; + + at.touch(file_a); + + // Shred file_a and verify that, as it is empty, it doesn't have "pass 1/3 (random)" + scene + .ucmd() + .arg("-uv") + .arg(file_a) + .succeeds() + .stdout_does_not_contain("pass 1/3 (random)"); + + assert!(!at.file_exists(file_a)); +} From 844f0774018028b8e65e28445af4ac3dcd19d618 Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Sat, 16 Mar 2024 10:56:15 +0100 Subject: [PATCH 3/6] shred: as we already got the metadata info, use it directly --- src/uu/shred/src/shred.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/uu/shred/src/shred.rs b/src/uu/shred/src/shred.rs index 8a73eb1f1..12cd67acd 100644 --- a/src/uu/shred/src/shred.rs +++ b/src/uu/shred/src/shred.rs @@ -474,7 +474,7 @@ fn wipe_file( let size = match size { Some(size) => size, - None => get_file_size(path)?, + None => metadata.len(), }; for (i, pass_type) in pass_sequence.into_iter().enumerate() { @@ -532,10 +532,6 @@ fn do_pass( Ok(()) } -fn get_file_size(path: &Path) -> Result { - Ok(fs::metadata(path)?.len()) -} - // Repeatedly renames the file with strings of decreasing length (most likely all 0s) // Return the path of the file after its last renaming or None if error fn wipe_name(orig_path: &Path, verbose: bool, remove_method: RemoveMethod) -> Option { From bb5111cc71754495730142feb63e558e93f909b8 Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Sat, 16 Mar 2024 11:33:58 +0100 Subject: [PATCH 4/6] shred: fails in case of permissions issue --- src/uu/shred/src/shred.rs | 4 +++- tests/by-util/test_shred.rs | 24 ++++++++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/src/uu/shred/src/shred.rs b/src/uu/shred/src/shred.rs index 12cd67acd..5265f29f0 100644 --- a/src/uu/shred/src/shred.rs +++ b/src/uu/shred/src/shred.rs @@ -365,6 +365,7 @@ fn get_size(size_str_opt: Option) -> Option { .or_else(|| { if let Some(size) = size_str_opt { show_error!("invalid file size: {}", size.quote()); + // TODO: replace with our error management std::process::exit(1); } None @@ -578,7 +579,8 @@ fn wipe_name(orig_path: &Path, verbose: bool, remove_method: RemoveMethod) -> Op new_path.quote(), e ); - return None; + // TODO: replace with our error management + std::process::exit(1); } } } diff --git a/tests/by-util/test_shred.rs b/tests/by-util/test_shred.rs index 03aeef0c4..8dfd403b7 100644 --- a/tests/by-util/test_shred.rs +++ b/tests/by-util/test_shred.rs @@ -6,6 +6,7 @@ // spell-checker:ignore wipesync use crate::common::util::TestScenario; +use std::path::Path; #[test] fn test_invalid_arg() { @@ -163,3 +164,26 @@ fn test_shred_empty() { assert!(!at.file_exists(file_a)); } + +#[test] +#[cfg(all(unix, feature = "chmod"))] +fn test_shred_fail_no_perm() { + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + let dir = "dir"; + + let file = "test_shred_remove_a"; + + let binding = Path::new("dir").join(file); + let path = binding.to_str().unwrap(); + at.mkdir(dir); + at.touch(path); + scene.ccmd("chmod").arg("a-w").arg(dir).succeeds(); + + scene + .ucmd() + .arg("-uv") + .arg(path) + .fails() + .stderr_contains("Couldn't rename to"); +} From cf3fe0e566c93e2e9ab754132cb7971d66632cc5 Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Sat, 16 Mar 2024 11:54:01 +0100 Subject: [PATCH 5/6] shred: small improv on the tests --- tests/by-util/test_shred.rs | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/tests/by-util/test_shred.rs b/tests/by-util/test_shred.rs index 8dfd403b7..1ff847afc 100644 --- a/tests/by-util/test_shred.rs +++ b/tests/by-util/test_shred.rs @@ -6,7 +6,6 @@ // spell-checker:ignore wipesync use crate::common::util::TestScenario; -use std::path::Path; #[test] fn test_invalid_arg() { @@ -160,7 +159,19 @@ fn test_shred_empty() { .arg("-uv") .arg(file_a) .succeeds() - .stdout_does_not_contain("pass 1/3 (random)"); + .stderr_does_not_contain("1/3 (random)"); + + assert!(!at.file_exists(file_a)); + + // if the file isn't empty, we should have random + at.touch(file_a); + at.write(file_a, "1"); + scene + .ucmd() + .arg("-uv") + .arg(file_a) + .succeeds() + .stderr_contains("1/3 (random)"); assert!(!at.file_exists(file_a)); } @@ -168,6 +179,8 @@ fn test_shred_empty() { #[test] #[cfg(all(unix, feature = "chmod"))] fn test_shred_fail_no_perm() { + use std::path::Path; + let scene = TestScenario::new(util_name!()); let at = &scene.fixtures; let dir = "dir"; From 6d89f96d860416a34c03c37021caca40b6c8a6a9 Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Sat, 23 Mar 2024 10:21:20 +0100 Subject: [PATCH 6/6] shred: only run zero when the file isn't empty --- src/uu/shred/src/shred.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/uu/shred/src/shred.rs b/src/uu/shred/src/shred.rs index 5265f29f0..d023b6210 100644 --- a/src/uu/shred/src/shred.rs +++ b/src/uu/shred/src/shred.rs @@ -460,10 +460,11 @@ fn wipe_file( pass_sequence[i * (n_passes - 1) / (n_random - 1)] = PassType::Random; } } - } - // --zero specifies whether we want one final pass of 0x00 on our file - if zero { - pass_sequence.push(PassType::Pattern(PATTERNS[0])); + + // --zero specifies whether we want one final pass of 0x00 on our file + if zero { + pass_sequence.push(PassType::Pattern(PATTERNS[0])); + } } let total_passes = pass_sequence.len();