From 09e7fcf82f0ae1220b4d1bb6aaaaa9c7403888d1 Mon Sep 17 00:00:00 2001 From: can1357 Date: Thu, 19 Mar 2026 00:43:46 +0100 Subject: [PATCH 1/7] shred: validate file type on opened fd --- src/uu/shred/locales/en-US.ftl | 1 + src/uu/shred/locales/fr-FR.ftl | 1 + src/uu/shred/src/shred.rs | 210 +++++++++++++++++++++++++++------ tests/by-util/test_shred.rs | 25 ++++ 4 files changed, 199 insertions(+), 38 deletions(-) diff --git a/src/uu/shred/locales/en-US.ftl b/src/uu/shred/locales/en-US.ftl index 41af9150ad7..0d36987b5fb 100644 --- a/src/uu/shred/locales/en-US.ftl +++ b/src/uu/shred/locales/en-US.ftl @@ -43,6 +43,7 @@ shred-cannot-open-random-source = cannot open random source: {$source} shred-invalid-file-size = invalid file size: {$size} shred-no-such-file-or-directory = {$file}: No such file or directory shred-not-a-file = {$file}: Not a file +shred-invalid-file-type = {$file}: invalid file type # Option help text shred-force-help = change permissions to allow writing if necessary diff --git a/src/uu/shred/locales/fr-FR.ftl b/src/uu/shred/locales/fr-FR.ftl index aa248254a35..6917e812e98 100644 --- a/src/uu/shred/locales/fr-FR.ftl +++ b/src/uu/shred/locales/fr-FR.ftl @@ -42,6 +42,7 @@ shred-cannot-open-random-source = impossible d'ouvrir la source aléatoire : {$s shred-invalid-file-size = taille de fichier invalide : {$size} shred-no-such-file-or-directory = {$file} : Aucun fichier ou répertoire de ce type shred-not-a-file = {$file} : N'est pas un fichier +shred-invalid-file-type = {$file} : type de fichier invalide # Texte d'aide des options shred-force-help = modifier les permissions pour permettre l'écriture si nécessaire diff --git a/src/uu/shred/src/shred.rs b/src/uu/shred/src/shred.rs index 7d19a42fc58..b918dd084d4 100644 --- a/src/uu/shred/src/shred.rs +++ b/src/uu/shred/src/shred.rs @@ -12,9 +12,9 @@ use rand::{RngExt as _, rngs::StdRng, seq::SliceRandom}; use std::cell::RefCell; use std::ffi::OsString; use std::fs::{self, File, OpenOptions}; -use std::io::{self, Read, Seek, SeekFrom, Write}; #[cfg(unix)] -use std::os::unix::prelude::PermissionsExt; +use std::io::IsTerminal; +use std::io::{self, Read, Seek, SeekFrom, Write}; use std::path::{Path, PathBuf}; use uucore::display::Quotable; use uucore::error::{FromIo, UResult, USimpleError, UUsageError}; @@ -613,6 +613,81 @@ fn create_compatible_sequence( } } +#[cfg(unix)] +fn reject_unsupported_file_type(path: &Path, file: &File) -> UResult<()> { + use std::os::unix::fs::FileTypeExt; + + // Validate the open fd with fstat instead of trusting the path check, + // which an attacker could swap between check and open. + let fd_metadata = file + .metadata() + .map_err_context(|| translate!("shred-failed-to-get-metadata"))?; + let file_type = fd_metadata.file_type(); + + if file_type.is_fifo() || file_type.is_socket() { + return Err(USimpleError::new( + 1, + translate!("shred-invalid-file-type", "file" => path.maybe_quote()), + )); + } + + // Reject TTY character devices. + if file_type.is_char_device() && file.is_terminal() { + return Err(USimpleError::new( + 1, + translate!("shred-invalid-file-type", "file" => path.maybe_quote()), + )); + } + + Ok(()) +} + +fn target_size(file: &mut File, metadata: &fs::Metadata, size: Option) -> UResult> { + if let Some(size) = size { + return Ok(Some(size)); + } + + if metadata.file_type().is_file() { + return Ok(Some(metadata.len())); + } + + match file.seek(SeekFrom::End(0)) { + Ok(size) => { + file.rewind() + .map_err_context(|| translate!("shred-failed-to-seek-file"))?; + + if size == 0 { + // Non-regular files can report an offset of 0 even when they accept writes + // (for example, character devices like /dev/zero). Use one bounded write + // block so we still run overwrite passes without looping forever. + Ok(Some(OPTIMAL_IO_BLOCK_SIZE as u64)) + } else { + Ok(Some(size)) + } + } + Err(_) => Ok(None), + } +} + +fn filesystem_block_size(metadata: &fs::Metadata) -> u64 { + #[cfg(unix)] + { + use std::os::unix::fs::MetadataExt; + + let block_size = metadata.blksize(); + if block_size > 0 { + block_size + } else { + OPTIMAL_IO_BLOCK_SIZE as u64 + } + } + #[cfg(not(unix))] + { + let _ = metadata; + OPTIMAL_IO_BLOCK_SIZE as u64 + } +} + #[allow(clippy::too_many_arguments)] #[allow(clippy::cognitive_complexity)] fn wipe_file( @@ -634,6 +709,7 @@ fn wipe_file( translate!("shred-no-such-file-or-directory", "file" => path.maybe_quote()), )); } + #[cfg(not(unix))] if !path.is_file() { return Err(USimpleError::new( 1, @@ -641,15 +717,19 @@ fn wipe_file( )); } - let metadata = + // Read path metadata for permission changes only. + // Type checks happen on the opened fd. + let path_metadata = fs::metadata(path).map_err_context(|| translate!("shred-failed-to-get-metadata"))?; // If force is true, set file permissions to not-readonly. if force { - let mut perms = metadata.permissions(); + let mut perms = path_metadata.permissions(); #[cfg(unix)] #[allow(clippy::useless_conversion, clippy::unnecessary_cast)] { + use std::os::unix::fs::PermissionsExt; + // NOTE: set_readonly(false) makes the file world-writable on Unix. // NOTE: S_IWUSR type is u16 on macOS, i32 on Redox. if (perms.mode() & (S_IWUSR as u32)) == 0 { @@ -664,10 +744,52 @@ fn wipe_file( .map_err_context(|| translate!("shred-failed-to-set-permissions"))?; } + // Pre-open fast-reject for FIFOs and sockets: opening a FIFO for writing + // blocks until a reader connects, so we must reject before open(). The + // post-open fstat check below closes the TOCTOU window for races. + #[cfg(unix)] + { + use std::os::unix::fs::FileTypeExt; + + let ft = path_metadata.file_type(); + if ft.is_fifo() || ft.is_socket() { + return Err(USimpleError::new( + 1, + translate!("shred-invalid-file-type", "file" => path.maybe_quote()), + )); + } + } + + let mut open_opts = OpenOptions::new(); + open_opts.write(true).truncate(false); + #[cfg(unix)] + { + use std::os::unix::fs::OpenOptionsExt; + + open_opts.custom_flags(libc::O_NOCTTY); + } + let mut file = open_opts.open(path).map_err_context( + || translate!("shred-failed-to-open-for-writing", "file" => path.maybe_quote()), + )?; + #[cfg(unix)] + reject_unsupported_file_type(path, &file)?; + + let metadata = file + .metadata() + .map_err_context(|| translate!("shred-failed-to-get-metadata"))?; + let size = target_size(&mut file, &metadata, size)?.unwrap_or(metadata.len()); + let is_regular_file = metadata.file_type().is_file(); + let should_sync_data = is_regular_file; + let io_block_size = if is_regular_file { + filesystem_block_size(&metadata) + } else { + OPTIMAL_IO_BLOCK_SIZE as u64 + }; + // 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 size != 0 { + // Only add passes if the target contains bytes to overwrite if n_passes <= 3 { // Only random passes if n_passes <= 3 @@ -683,29 +805,17 @@ fn wipe_file( } } - // --zero specifies whether we want one final pass of 0x00 on our file + // If `zero` is set, append a final pass with 0x00 bytes. if zero { pass_sequence.push(PassType::Pattern(PATTERNS[0])); } } let total_passes = pass_sequence.len(); - let mut file = OpenOptions::new() - .write(true) - .truncate(false) - .open(path) - .map_err_context( - || translate!("shred-failed-to-open-for-writing", "file" => path.maybe_quote()), - )?; - let size = match size { - Some(size) => size, - None => metadata.len(), - }; - - for (i, pass_type) in pass_sequence.into_iter().enumerate() { + for (i, pass_type) in pass_sequence.iter().enumerate() { if verbose { - let pass_name = pass_name(&pass_type); + let pass_name = pass_name(pass_type); let msg = translate!("shred-pass-progress", "file" => path.maybe_quote()); show_error!( "{msg} {}/{total_passes} ({pass_name})...", @@ -713,7 +823,16 @@ fn wipe_file( ); } // size is an optional argument for exactly how many bytes we want to shred - do_pass(&mut file, &pass_type, exact, random_source, size).map_err_context( + do_pass( + &mut file, + pass_type, + exact, + random_source, + size, + should_sync_data, + io_block_size, + ) + .map_err_context( || translate!("shred-file-write-pass-failed", "file" => path.maybe_quote()), )?; } @@ -726,7 +845,7 @@ fn wipe_file( Ok(()) } -fn split_on_blocks(file_size: u64, exact: bool) -> (u64, u64) { +fn split_on_blocks(file_size: u64, exact: bool, io_block_size: u64) -> (u64, u64) { // OPTIMAL_IO_BLOCK_SIZE must not exceed BLOCK_SIZE. Violating this may cause overflows due // to alignment or performance issues.This kind of misconfiguration is // highly unlikely but would indicate a serious error. @@ -735,10 +854,11 @@ fn split_on_blocks(file_size: u64, exact: bool) -> (u64, u64) { let file_size = if exact { file_size } else { - // The main idea here is to align the file size to the OPTIMAL_IO_BLOCK_SIZE, and then + let io_block_size = io_block_size.max(1); + // The main idea here is to align the file size to the io_block_size, and then // split it into BLOCK_SIZE + remaining bytes. Since the input data is already aligned to N - // * OPTIMAL_IO_BLOCK_SIZE, the output file size will also be aligned and correct. - file_size.div_ceil(OPTIMAL_IO_BLOCK_SIZE as u64) * OPTIMAL_IO_BLOCK_SIZE as u64 + // * io_block_size, the output file size will also be aligned and correct. + file_size.div_ceil(io_block_size) * io_block_size }; (file_size / BLOCK_SIZE as u64, file_size % BLOCK_SIZE as u64) } @@ -749,12 +869,14 @@ fn do_pass( exact: bool, random_source: Option<&RefCell>, file_size: u64, + should_sync_data: bool, + io_block_size: u64, ) -> Result<(), io::Error> { // We might be at the end of the file due to a previous iteration, so rewind. file.rewind()?; let mut writer = BytesWriter::from_pass_type(pass_type, random_source)?; - let (number_of_blocks, bytes_left) = split_on_blocks(file_size, exact); + let (number_of_blocks, bytes_left) = split_on_blocks(file_size, exact, io_block_size); // We start by writing BLOCK_SIZE times as many time as possible. for _ in 0..number_of_blocks { @@ -762,11 +884,13 @@ fn do_pass( file.write_all(block)?; } - // Then we write remaining data which is smaller than the BLOCK_SIZE + // Then we write remaining data which is smaller than the BLOCK_SIZE. let block = writer.bytes_for_pass(bytes_left as usize)?; file.write_all(block)?; - file.sync_data()?; + if should_sync_data { + file.sync_data()?; + } Ok(()) } @@ -864,28 +988,38 @@ mod tests { fn test_align_non_exact_control_values() { // Note: This test only makes sense for the default values of BLOCK_SIZE and // OPTIMAL_IO_BLOCK_SIZE. - assert_eq!(split_on_blocks(1, false), (0, 4096)); - assert_eq!(split_on_blocks(4095, false), (0, 4096)); - assert_eq!(split_on_blocks(4096, false), (0, 4096)); - assert_eq!(split_on_blocks(4097, false), (0, 8192)); - assert_eq!(split_on_blocks(65535, false), (1, 0)); - assert_eq!(split_on_blocks(65536, false), (1, 0)); - assert_eq!(split_on_blocks(65537, false), (1, 4096)); + assert_eq!(split_on_blocks(1, false, OPTIMAL_IO_BLOCK_SIZE as u64), (0, 4096)); + assert_eq!(split_on_blocks(4095, false, OPTIMAL_IO_BLOCK_SIZE as u64), (0, 4096)); + assert_eq!(split_on_blocks(4096, false, OPTIMAL_IO_BLOCK_SIZE as u64), (0, 4096)); + assert_eq!(split_on_blocks(4097, false, OPTIMAL_IO_BLOCK_SIZE as u64), (0, 8192)); + assert_eq!(split_on_blocks(65535, false, OPTIMAL_IO_BLOCK_SIZE as u64), (1, 0)); + assert_eq!(split_on_blocks(65536, false, OPTIMAL_IO_BLOCK_SIZE as u64), (1, 0)); + assert_eq!(split_on_blocks(65537, false, OPTIMAL_IO_BLOCK_SIZE as u64), (1, 4096)); } #[test] fn test_align_non_exact_cycle() { for size in 1..BLOCK_SIZE as u64 * 2 { - let (number_of_blocks, bytes_left) = split_on_blocks(size, false); + let (number_of_blocks, bytes_left) = + split_on_blocks(size, false, OPTIMAL_IO_BLOCK_SIZE as u64); let test_size = number_of_blocks * BLOCK_SIZE as u64 + bytes_left; assert_eq!(test_size % OPTIMAL_IO_BLOCK_SIZE as u64, 0); } } + + #[test] + fn test_align_non_exact_uses_provided_block_size() { + assert_eq!(split_on_blocks(1, false, 512), (0, 512)); + assert_eq!(split_on_blocks(512, false, 512), (0, 512)); + assert_eq!(split_on_blocks(513, false, 512), (0, 1024)); + } + #[test] fn test_align_exact_cycle() { for size in 1..BLOCK_SIZE as u64 * 2 { - let (number_of_blocks, bytes_left) = split_on_blocks(size, true); + let (number_of_blocks, bytes_left) = + split_on_blocks(size, true, OPTIMAL_IO_BLOCK_SIZE as u64); let test_size = number_of_blocks * BLOCK_SIZE as u64 + bytes_left; assert_eq!(test_size, size); } diff --git a/tests/by-util/test_shred.rs b/tests/by-util/test_shred.rs index 81f69fb5c66..29f3886fd0e 100644 --- a/tests/by-util/test_shred.rs +++ b/tests/by-util/test_shred.rs @@ -214,6 +214,31 @@ fn test_shred_fail_no_perm() { .stderr_contains("Couldn't rename to"); } +#[test] +#[cfg(unix)] +fn test_shred_char_device_with_explicit_size() { + new_ucmd!() + .args(&["-n1", "--size=1", "/dev/null"]) + .succeeds(); +} + +#[test] +#[cfg(unix)] +fn test_shred_rejects_fifo() { + use std::time::Duration; + + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + at.mkfifo("fifo"); + + scene + .ucmd() + .arg("fifo") + .timeout(Duration::from_secs(2)) + .fails() + .stderr_contains("invalid file type"); +} + #[test] fn test_shred_verbose_no_padding_1() { let (at, mut ucmd) = at_and_ucmd!(); From 63c92fe9189b53a363251dfa0db16a99b2c72918 Mon Sep 17 00:00:00 2001 From: can1357 Date: Thu, 19 Mar 2026 00:44:21 +0100 Subject: [PATCH 2/7] shred: use atomic rename in wipe-name --- src/uu/shred/src/shred.rs | 98 ++++++++++++++++++++++++++++++++++++--- 1 file changed, 91 insertions(+), 7 deletions(-) diff --git a/src/uu/shred/src/shred.rs b/src/uu/shred/src/shred.rs index b918dd084d4..63c861e137c 100644 --- a/src/uu/shred/src/shred.rs +++ b/src/uu/shred/src/shred.rs @@ -10,6 +10,8 @@ use clap::{Arg, ArgAction, Command}; use libc::S_IWUSR; use rand::{RngExt as _, rngs::StdRng, seq::SliceRandom}; use std::cell::RefCell; +#[cfg(any(target_vendor = "apple", target_os = "linux", target_os = "android"))] +use std::ffi::CString; use std::ffi::OsString; use std::fs::{self, File, OpenOptions}; #[cfg(unix)] @@ -895,10 +897,96 @@ fn do_pass( Ok(()) } +#[cfg(target_vendor = "apple")] +fn rename_new(old_path: &Path, new_path: &Path) -> io::Result<()> { + let old_path = cstring_from_path(old_path)?; + let new_path = cstring_from_path(new_path)?; + + // SAFETY: Both pointers come from owned `CString` values that remain alive + // for the duration of this call, are NUL-terminated, and point to valid + // path bytes for the OS API. + let ret = unsafe { libc::renamex_np(old_path.as_ptr(), new_path.as_ptr(), libc::RENAME_EXCL) }; + if ret == 0 { + return Ok(()); + } + + Err(io::Error::last_os_error()) +} + +#[cfg(any(target_os = "linux", target_os = "android"))] +fn rename_new(old_path: &Path, new_path: &Path) -> io::Result<()> { + let old_path_c = cstring_from_path(old_path)?; + let new_path_c = cstring_from_path(new_path)?; + + // SAFETY: Both pointers come from owned `CString` values that remain alive + // for the duration of this call, are NUL-terminated, and point to valid + // path bytes for the OS API. `AT_FDCWD` targets the current working + // directory and `RENAME_NOREPLACE` requests kernel-enforced no-overwrite + // semantics atomically. + let ret = unsafe { + libc::renameat2( + libc::AT_FDCWD, + old_path_c.as_ptr(), + libc::AT_FDCWD, + new_path_c.as_ptr(), + libc::RENAME_NOREPLACE, + ) + }; + if ret == 0 { + return Ok(()); + } + + let err = io::Error::last_os_error(); + if matches!( + err.raw_os_error(), + Some(libc::ENOSYS) | Some(libc::EINVAL) | Some(libc::EOPNOTSUPP) + ) { + // Fall back to check-then-rename when atomic no-overwrite is unsupported + // by kernel/filesystem. This is racy but preserves functionality. + if new_path.exists() { + return Err(io::Error::new( + io::ErrorKind::AlreadyExists, + "destination already exists", + )); + } + return fs::rename(old_path, new_path); + } + + Err(err) +} + +#[cfg(not(any(target_vendor = "apple", target_os = "linux", target_os = "android")))] +fn rename_new(old_path: &Path, new_path: &Path) -> io::Result<()> { + // No kernel-level atomic no-overwrite rename available; fall back to + // check-then-rename. Racy, but functional and matches the non-Unix path. + if new_path.exists() { + return Err(io::Error::new( + io::ErrorKind::AlreadyExists, + "destination already exists", + )); + } + fs::rename(old_path, new_path) +} + +#[cfg(any(target_vendor = "apple", target_os = "linux", target_os = "android"))] +fn cstring_from_path(path: &Path) -> io::Result { + use std::os::unix::ffi::OsStrExt; + + CString::new(path.as_os_str().as_bytes()).map_err(|_| { + io::Error::new( + io::ErrorKind::InvalidInput, + "path contains interior NUL byte", + ) + }) +} + /// 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 in case of an error fn wipe_name(orig_path: &Path, verbose: bool, remove_method: RemoveMethod) -> PathBuf { - let file_name_len = orig_path.file_name().unwrap().len(); + let file_name_len = orig_path + .file_name() + .expect("wipe_name called on a path with no filename component") + .len(); let mut last_path = PathBuf::from(orig_path); @@ -907,12 +995,7 @@ fn wipe_name(orig_path: &Path, verbose: bool, remove_method: RemoveMethod) -> Pa // If every possible filename already exists, just reduce the length and try again for name in FilenameIter::new(length) { let new_path = orig_path.with_file_name(name); - // We don't want the filename to already exist (don't overwrite) - // If it does, find another name that doesn't - if new_path.exists() { - continue; - } - match fs::rename(&last_path, &new_path) { + match rename_new(&last_path, &new_path) { Ok(()) => { if verbose { show_error!( @@ -935,6 +1018,7 @@ fn wipe_name(orig_path: &Path, verbose: bool, remove_method: RemoveMethod) -> Pa last_path = new_path; break; } + Err(e) if e.kind() == io::ErrorKind::AlreadyExists => {} Err(e) => { let msg = translate!("shred-couldnt-rename", "file" => last_path.maybe_quote(), "new_name" => new_path.quote(), "error" => e); show_error!("{msg}"); From e72caef853517136b519b47c5f29e27f2b833299 Mon Sep 17 00:00:00 2001 From: can1357 Date: Thu, 19 Mar 2026 00:44:46 +0100 Subject: [PATCH 3/7] shred: sync parent directory for metadata durability --- src/uu/shred/src/shred.rs | 43 ++++++++++++++++++++++++++++++++------- 1 file changed, 36 insertions(+), 7 deletions(-) diff --git a/src/uu/shred/src/shred.rs b/src/uu/shred/src/shred.rs index 63c861e137c..31d99fdb06f 100644 --- a/src/uu/shred/src/shred.rs +++ b/src/uu/shred/src/shred.rs @@ -980,6 +980,19 @@ fn cstring_from_path(path: &Path) -> io::Result { }) } +fn sync_parent_dir(path: &Path) -> io::Result<()> { + if let Some(parent) = path.parent() { + let dir_path = if parent.as_os_str().is_empty() { + Path::new(".") + } else { + parent + }; + let dir = OpenOptions::new().read(true).open(dir_path)?; + dir.sync_all()?; + } + Ok(()) +} + /// 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 in case of an error fn wipe_name(orig_path: &Path, verbose: bool, remove_method: RemoveMethod) -> PathBuf { @@ -1007,12 +1020,16 @@ fn wipe_name(orig_path: &Path, verbose: bool, remove_method: RemoveMethod) -> Pa } if remove_method == RemoveMethod::WipeSync { - // Sync every file rename - let new_file = OpenOptions::new() - .write(true) - .open(new_path.clone()) - .expect("Failed to open renamed file for syncing"); - new_file.sync_all().expect("Failed to sync renamed file"); + // Sync the parent directory to ensure the rename is durable. + // GNU shred does this via fdatasync/fsync on the directory fd. + if let Err(err) = sync_parent_dir(&new_path) { + show_error!( + "failed to sync directory after rename from {} to {}: {}", + last_path.maybe_quote().to_string(), + new_path.maybe_quote().to_string(), + err + ); + } } last_path = new_path; @@ -1051,7 +1068,19 @@ fn do_remove( wipe_name(path, verbose, remove_method) }; - fs::remove_file(remove_path)?; + fs::remove_file(&remove_path)?; + + // Sync the parent directory after unlink to ensure removal is durable + // (matches GNU shred's directory sync after unlink). + if remove_method == RemoveMethod::WipeSync { + if let Err(err) = sync_parent_dir(&remove_path) { + show_error!( + "failed to sync directory after removing {}: {}", + orig_filename.maybe_quote(), + err + ); + } + } if verbose { show_error!( From f6c2405685131782c90b3c63d6bb2461c8ba7e3f Mon Sep 17 00:00:00 2001 From: can1357 Date: Thu, 19 Mar 2026 00:45:17 +0100 Subject: [PATCH 4/7] shred: truncate before remove --- src/uu/shred/locales/en-US.ftl | 1 + src/uu/shred/locales/fr-FR.ftl | 1 + src/uu/shred/src/shred.rs | 11 +++++++++++ tests/by-util/test_shred.rs | 13 ++++++++++--- 4 files changed, 23 insertions(+), 3 deletions(-) diff --git a/src/uu/shred/locales/en-US.ftl b/src/uu/shred/locales/en-US.ftl index 0d36987b5fb..07290cdeb33 100644 --- a/src/uu/shred/locales/en-US.ftl +++ b/src/uu/shred/locales/en-US.ftl @@ -66,6 +66,7 @@ shred-couldnt-rename = {$file}: Couldn't rename to {$new_name}: {$error} shred-failed-to-open-for-writing = {$file}: failed to open for writing shred-file-write-pass-failed = {$file}: File write pass failed shred-failed-to-remove-file = {$file}: failed to remove file +shred-failed-to-truncate-file = {$file}: failed to truncate file # File I/O error messages shred-failed-to-clone-file-handle = failed to clone file handle diff --git a/src/uu/shred/locales/fr-FR.ftl b/src/uu/shred/locales/fr-FR.ftl index 6917e812e98..0566512e0b1 100644 --- a/src/uu/shred/locales/fr-FR.ftl +++ b/src/uu/shred/locales/fr-FR.ftl @@ -65,6 +65,7 @@ shred-couldnt-rename = {$file} : Impossible de renommer en {$new_name} : {$error shred-failed-to-open-for-writing = {$file} : impossible d'ouvrir pour l'écriture shred-file-write-pass-failed = {$file} : Échec du passage d'écriture de fichier shred-failed-to-remove-file = {$file} : impossible de supprimer le fichier +shred-failed-to-truncate-file = {$file} : impossible de tronquer le fichier # Messages d'erreur E/S de fichier shred-failed-to-clone-file-handle = échec du clonage du descripteur de fichier diff --git a/src/uu/shred/src/shred.rs b/src/uu/shred/src/shred.rs index 31d99fdb06f..b3d0186505e 100644 --- a/src/uu/shred/src/shred.rs +++ b/src/uu/shred/src/shred.rs @@ -839,6 +839,17 @@ fn wipe_file( )?; } + if remove_method != RemoveMethod::None && metadata.file_type().is_file() { + file.set_len(0).map_err_context( + || translate!("shred-failed-to-truncate-file", "file" => path.maybe_quote()), + )?; + if should_sync_data { + file.sync_data().map_err_context( + || translate!("shred-failed-to-truncate-file", "file" => path.maybe_quote()), + )?; + } + } + if remove_method != RemoveMethod::None { do_remove(path, path_str, verbose, remove_method).map_err_context( || translate!("shred-failed-to-remove-file", "file" => path.maybe_quote()), diff --git a/tests/by-util/test_shred.rs b/tests/by-util/test_shred.rs index 29f3886fd0e..af733b6e9a3 100644 --- a/tests/by-util/test_shred.rs +++ b/tests/by-util/test_shred.rs @@ -192,7 +192,7 @@ fn test_shred_empty() { #[test] #[cfg(all(unix, feature = "chmod"))] fn test_shred_fail_no_perm() { - use std::path::Path; + use std::{fs, path::Path}; let scene = TestScenario::new(util_name!()); let at = &scene.fixtures; @@ -201,9 +201,11 @@ fn test_shred_fail_no_perm() { let file = "test_shred_remove_a"; let binding = Path::new("dir").join(file); - let path = binding.to_str().unwrap(); + let path = binding + .to_str() + .expect("fixture path should be valid UTF-8"); at.mkdir(dir); - at.touch(path); + at.write(path, "SECRETSECRET"); // cspell:disable-line scene.ccmd("chmod").arg("a-w").arg(dir).succeeds(); scene @@ -212,6 +214,11 @@ fn test_shred_fail_no_perm() { .arg(path) .fails() .stderr_contains("Couldn't rename to"); + assert!(at.file_exists(path)); + let file_len = fs::metadata(at.plus(path)) + .expect("test file should still exist after failed remove") + .len(); + assert_eq!(file_len, 0); } #[test] From 28f2502dfa50d856c70ce754e15745f6e5908359 Mon Sep 17 00:00:00 2001 From: can1357 Date: Thu, 19 Mar 2026 00:46:55 +0100 Subject: [PATCH 5/7] shred: continue overwrite past EIO bad sectors --- src/uu/shred/src/shred.rs | 136 +++++++++++++++++++++++++++++++----- tests/by-util/test_shred.rs | 5 +- 2 files changed, 124 insertions(+), 17 deletions(-) diff --git a/src/uu/shred/src/shred.rs b/src/uu/shred/src/shred.rs index b3d0186505e..f6f0a035941 100644 --- a/src/uu/shred/src/shred.rs +++ b/src/uu/shred/src/shred.rs @@ -57,6 +57,10 @@ const PATTERN_BUFFER_SIZE: usize = BLOCK_SIZE + PATTERN_LENGTH - 1; /// example, `std::os::unix::fs::MetadataExt::blksize()`. const OPTIMAL_IO_BLOCK_SIZE: usize = 1 << 12; +/// Sector size for recovering after write failures, matching GNU shred. +/// Used to skip past bad sectors on EIO and continue overwriting. +const SECTOR_SIZE: u64 = 512; + /// Patterns that appear in order for the passes /// /// A single-byte pattern is equivalent to a multi-byte pattern of that byte three times. @@ -188,6 +192,7 @@ impl BytesWriter { pass: &PassType, random_source: Option<&RefCell>, ) -> Result { + match pass { PassType::Random => match random_source { None => Ok(Self::Random { @@ -242,6 +247,12 @@ impl BytesWriter { } } } + + fn sync_pattern_offset(&mut self, file_offset: u64) { + if let Self::Pattern { offset, .. } = self { + *offset = (file_offset % PATTERN_LENGTH as u64) as usize; + } + } } #[uucore::main] @@ -779,7 +790,7 @@ fn wipe_file( let metadata = file .metadata() .map_err_context(|| translate!("shred-failed-to-get-metadata"))?; - let size = target_size(&mut file, &metadata, size)?.unwrap_or(metadata.len()); + let size = target_size(&mut file, &metadata, size)?; let is_regular_file = metadata.file_type().is_file(); let should_sync_data = is_regular_file; let io_block_size = if is_regular_file { @@ -790,7 +801,7 @@ fn wipe_file( // Fill up our pass sequence let mut pass_sequence = Vec::new(); - if size != 0 { + if size != Some(0) { // Only add passes if the target contains bytes to overwrite if n_passes <= 3 { @@ -876,33 +887,90 @@ fn split_on_blocks(file_size: u64, exact: bool, io_block_size: u64) -> (u64, u64 (file_size / BLOCK_SIZE as u64, file_size % BLOCK_SIZE as u64) } + +fn is_eio(err: &io::Error) -> bool { + #[cfg(unix)] + { + err.raw_os_error() == Some(libc::EIO) + } + #[cfg(not(unix))] + { + let _ = err; + false + } +} + fn do_pass( file: &mut File, pass_type: &PassType, exact: bool, random_source: Option<&RefCell>, - file_size: u64, + file_size: Option, should_sync_data: bool, io_block_size: u64, ) -> Result<(), io::Error> { - // We might be at the end of the file due to a previous iteration, so rewind. - file.rewind()?; + // Known-size passes must restart from the beginning each iteration. + if file_size.is_some() { + file.rewind()?; + } let mut writer = BytesWriter::from_pass_type(pass_type, random_source)?; - let (number_of_blocks, bytes_left) = split_on_blocks(file_size, exact, io_block_size); - // We start by writing BLOCK_SIZE times as many time as possible. - for _ in 0..number_of_blocks { - let block = writer.bytes_for_pass(BLOCK_SIZE)?; - file.write_all(block)?; + if let Some(file_size) = file_size { + let (number_of_blocks, bytes_left) = split_on_blocks(file_size, exact, io_block_size); + let total_bytes = number_of_blocks * BLOCK_SIZE as u64 + bytes_left; + + let mut offset: u64 = 0; + while offset < total_bytes { + writer.sync_pattern_offset(offset); + let remaining = (total_bytes - offset) as usize; + let chunk_size = remaining.min(BLOCK_SIZE); + let block = writer.bytes_for_pass(chunk_size)?; + + loop { + match file.write_all(block) { + Ok(()) => { + offset += chunk_size as u64; + break; + } + Err(err) if is_eio(&err) => { + // Bad sector: continue from the next sector after the bytes + // the kernel accepted before reporting EIO. + let current_pos = file.stream_position()?; + let next_sector = (current_pos | (SECTOR_SIZE - 1)) + 1; + if next_sector >= total_bytes { + break; + } + file.seek(SeekFrom::Start(next_sector))?; + offset = next_sector; + break; + } + Err(err) => return Err(err), + } + } + } + } else { + loop { + let block = writer.bytes_for_pass(BLOCK_SIZE)?; + match file.write_all(block) { + Ok(()) => {} + Err(err) + if err.kind() == io::ErrorKind::WriteZero + || err.kind() == io::ErrorKind::StorageFull => + { + break; + } + Err(err) => return Err(err), + } + } } - // Then we write remaining data which is smaller than the BLOCK_SIZE. - let block = writer.bytes_for_pass(bytes_left as usize)?; - file.write_all(block)?; - if should_sync_data { - file.sync_data()?; + if let Err(err) = file.sync_data() { + if !is_eio(&err) { + return Err(err); + } + } } Ok(()) @@ -1106,7 +1174,9 @@ fn do_remove( #[cfg(test)] mod tests { - use crate::{BLOCK_SIZE, OPTIMAL_IO_BLOCK_SIZE, split_on_blocks}; + use crate::{ + BLOCK_SIZE, BytesWriter, OPTIMAL_IO_BLOCK_SIZE, PassType, Pattern, split_on_blocks, + }; #[test] fn test_align_non_exact_control_values() { @@ -1139,6 +1209,40 @@ mod tests { assert_eq!(split_on_blocks(513, false, 512), (0, 1024)); } + + #[test] + fn test_pattern_writer_syncs_to_file_offset() { + let mut writer = BytesWriter::from_pass_type( + &PassType::Pattern(Pattern::Multi([b'A', b'B', b'C'])), + None, + ) + .expect("pattern writer should be created"); + + writer.sync_pattern_offset(0); + assert_eq!( + writer.bytes_for_pass(1).expect("pattern byte at offset 0"), + b"A" + ); + + writer.sync_pattern_offset(1); + assert_eq!( + writer.bytes_for_pass(1).expect("pattern byte at offset 1"), + b"B" + ); + + writer.sync_pattern_offset(2); + assert_eq!( + writer.bytes_for_pass(1).expect("pattern byte at offset 2"), + b"C" + ); + + writer.sync_pattern_offset(3); + assert_eq!( + writer.bytes_for_pass(1).expect("pattern byte at offset 3"), + b"A" + ); + } + #[test] fn test_align_exact_cycle() { for size in 1..BLOCK_SIZE as u64 * 2 { diff --git a/tests/by-util/test_shred.rs b/tests/by-util/test_shred.rs index af733b6e9a3..f60944ce27b 100644 --- a/tests/by-util/test_shred.rs +++ b/tests/by-util/test_shred.rs @@ -286,7 +286,9 @@ fn test_all_patterns_present() { fn test_random_source_regular_file() { let (at, mut ucmd) = at_and_ucmd!(); - // Currently, our block size is 4096. If it changes, this test has to be adapted. + // Use --size=4096 so the bytes consumed per pass are deterministic + // regardless of the filesystem's blksize() (which split_on_blocks uses + // for alignment when --exact/--size is not set). let mut many_bytes = Vec::with_capacity(4096 * 4); for i in 0..4096u32 { @@ -301,6 +303,7 @@ fn test_random_source_regular_file() { ucmd .arg("-vn3") + .arg("--size=4096") .arg("--random-source=source_long") .arg(file) .succeeds() From 5d6b4798433643354c98ff532e0e490fe7f9d1d5 Mon Sep 17 00:00:00 2001 From: can1357 Date: Thu, 19 Mar 2026 00:47:31 +0100 Subject: [PATCH 6/7] shred: add direct I/O to bypass page cache --- src/uu/shred/src/shred.rs | 61 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/src/uu/shred/src/shred.rs b/src/uu/shred/src/shred.rs index f6f0a035941..f369cce1fc0 100644 --- a/src/uu/shred/src/shred.rs +++ b/src/uu/shred/src/shred.rs @@ -888,6 +888,48 @@ fn split_on_blocks(file_size: u64, exact: bool, io_block_size: u64) -> (u64, u64 } +/// Enable or disable direct I/O on a file descriptor to bypass the page cache. +/// This is a performance optimization matching GNU shred behavior. +/// Silently does nothing if the platform or filesystem doesn't support it. +#[cfg(any(target_os = "linux", target_os = "android"))] +fn direct_mode(file: &File, enable: bool) { + use std::os::unix::io::AsRawFd; + + let fd = file.as_raw_fd(); + + // SAFETY: fd is a valid open file descriptor from a File we hold a reference to. + let flags = unsafe { libc::fcntl(fd, libc::F_GETFL) }; + if flags >= 0 { + let new_flags = if enable { + flags | libc::O_DIRECT + } else { + flags & !libc::O_DIRECT + }; + if new_flags != flags { + // SAFETY: fd is valid, F_SETFL with an int argument is safe. + unsafe { libc::fcntl(fd, libc::F_SETFL, new_flags) }; + } + } +} + +#[cfg(target_vendor = "apple")] +fn direct_mode(file: &File, enable: bool) { + use std::os::unix::io::AsRawFd; + + let fd = file.as_raw_fd(); + + // SAFETY: fd is a valid open file descriptor. F_NOCACHE with 0 or 1 is safe. + unsafe { libc::fcntl(fd, libc::F_NOCACHE, i32::from(enable)) }; +} + +#[cfg(all( + not(target_os = "linux"), + not(target_os = "android"), + not(target_vendor = "apple") +))] +fn direct_mode(_file: &File, _enable: bool) {} + + fn is_eio(err: &io::Error) -> bool { #[cfg(unix)] { @@ -920,6 +962,13 @@ fn do_pass( let (number_of_blocks, bytes_left) = split_on_blocks(file_size, exact, io_block_size); let total_bytes = number_of_blocks * BLOCK_SIZE as u64 + bytes_left; + // As a performance tweak, avoid direct I/O for small sizes. + // Direct I/O can be unsupported for small non-aligned sizes. + let mut try_without_direct_io = total_bytes < BLOCK_SIZE as u64; + if !try_without_direct_io { + direct_mode(file, true); + } + let mut offset: u64 = 0; while offset < total_bytes { writer.sync_pattern_offset(offset); @@ -933,6 +982,16 @@ fn do_pass( offset += chunk_size as u64; break; } + Err(err) + if err.kind() == io::ErrorKind::InvalidInput && !try_without_direct_io => + { + // Direct I/O may not be supported on this filesystem or + // for this alignment. Retry without it. + direct_mode(file, false); + try_without_direct_io = true; + // Retry the same write at the same offset. + file.seek(SeekFrom::Start(offset))?; + } Err(err) if is_eio(&err) => { // Bad sector: continue from the next sector after the bytes // the kernel accepted before reporting EIO. @@ -949,6 +1008,8 @@ fn do_pass( } } } + + direct_mode(file, false); } else { loop { let block = writer.bytes_for_pass(BLOCK_SIZE)?; From 86616b59fb43692bafce723f30b3bfd76524a34d Mon Sep 17 00:00:00 2001 From: can1357 Date: Thu, 19 Mar 2026 00:48:53 +0100 Subject: [PATCH 7/7] shred: check close errors before remove --- src/uu/shred/locales/en-US.ftl | 1 + src/uu/shred/locales/fr-FR.ftl | 1 + src/uu/shred/src/shred.rs | 208 +++++++++++++++++++++++---------- tests/by-util/test_shred.rs | 3 +- 4 files changed, 150 insertions(+), 63 deletions(-) diff --git a/src/uu/shred/locales/en-US.ftl b/src/uu/shred/locales/en-US.ftl index 07290cdeb33..0e413aaef09 100644 --- a/src/uu/shred/locales/en-US.ftl +++ b/src/uu/shred/locales/en-US.ftl @@ -65,6 +65,7 @@ shred-pass-progress = {$file}: pass shred-couldnt-rename = {$file}: Couldn't rename to {$new_name}: {$error} shred-failed-to-open-for-writing = {$file}: failed to open for writing shred-file-write-pass-failed = {$file}: File write pass failed +shred-failed-to-close-file = {$file}: failed to close file shred-failed-to-remove-file = {$file}: failed to remove file shred-failed-to-truncate-file = {$file}: failed to truncate file diff --git a/src/uu/shred/locales/fr-FR.ftl b/src/uu/shred/locales/fr-FR.ftl index 0566512e0b1..7fc747c598d 100644 --- a/src/uu/shred/locales/fr-FR.ftl +++ b/src/uu/shred/locales/fr-FR.ftl @@ -66,6 +66,7 @@ shred-failed-to-open-for-writing = {$file} : impossible d'ouvrir pour l'écritur shred-file-write-pass-failed = {$file} : Échec du passage d'écriture de fichier shred-failed-to-remove-file = {$file} : impossible de supprimer le fichier shred-failed-to-truncate-file = {$file} : impossible de tronquer le fichier +shred-failed-to-close-file = {$file} : échec de la fermeture du fichier # Messages d'erreur E/S de fichier shred-failed-to-clone-file-handle = échec du clonage du descripteur de fichier diff --git a/src/uu/shred/src/shred.rs b/src/uu/shred/src/shred.rs index f369cce1fc0..6a47e998621 100644 --- a/src/uu/shred/src/shred.rs +++ b/src/uu/shred/src/shred.rs @@ -3,7 +3,7 @@ // For the full copyright and license information, please view the LICENSE // file that was distributed with this source code. -// spell-checker:ignore (words) wipesync prefill couldnt fillpattern +// spell-checker:ignore (words) wipesync prefill couldnt fillpattern renameat renameatx noreplace nocache renamex FDCWD use clap::{Arg, ArgAction, Command}; #[cfg(unix)] @@ -192,7 +192,6 @@ impl BytesWriter { pass: &PassType, random_source: Option<&RefCell>, ) -> Result { - match pass { PassType::Random => match random_source { None => Ok(Self::Random { @@ -629,7 +628,6 @@ fn create_compatible_sequence( #[cfg(unix)] fn reject_unsupported_file_type(path: &Path, file: &File) -> UResult<()> { use std::os::unix::fs::FileTypeExt; - // Validate the open fd with fstat instead of trusting the path check, // which an attacker could swap between check and open. let fd_metadata = file @@ -644,7 +642,7 @@ fn reject_unsupported_file_type(path: &Path, file: &File) -> UResult<()> { )); } - // Reject TTY character devices. + // Reject TTY character devices if file_type.is_char_device() && file.is_terminal() { return Err(USimpleError::new( 1, @@ -655,7 +653,11 @@ fn reject_unsupported_file_type(path: &Path, file: &File) -> UResult<()> { Ok(()) } -fn target_size(file: &mut File, metadata: &fs::Metadata, size: Option) -> UResult> { +fn target_size( + file: &mut File, + metadata: &fs::Metadata, + size: Option, +) -> UResult> { if let Some(size) = size { return Ok(Some(size)); } @@ -681,26 +683,6 @@ fn target_size(file: &mut File, metadata: &fs::Metadata, size: Option) -> U Err(_) => Ok(None), } } - -fn filesystem_block_size(metadata: &fs::Metadata) -> u64 { - #[cfg(unix)] - { - use std::os::unix::fs::MetadataExt; - - let block_size = metadata.blksize(); - if block_size > 0 { - block_size - } else { - OPTIMAL_IO_BLOCK_SIZE as u64 - } - } - #[cfg(not(unix))] - { - let _ = metadata; - OPTIMAL_IO_BLOCK_SIZE as u64 - } -} - #[allow(clippy::too_many_arguments)] #[allow(clippy::cognitive_complexity)] fn wipe_file( @@ -742,7 +724,6 @@ fn wipe_file( #[allow(clippy::useless_conversion, clippy::unnecessary_cast)] { use std::os::unix::fs::PermissionsExt; - // NOTE: set_readonly(false) makes the file world-writable on Unix. // NOTE: S_IWUSR type is u16 on macOS, i32 on Redox. if (perms.mode() & (S_IWUSR as u32)) == 0 { @@ -763,7 +744,6 @@ fn wipe_file( #[cfg(unix)] { use std::os::unix::fs::FileTypeExt; - let ft = path_metadata.file_type(); if ft.is_fifo() || ft.is_socket() { return Err(USimpleError::new( @@ -778,21 +758,22 @@ fn wipe_file( #[cfg(unix)] { use std::os::unix::fs::OpenOptionsExt; - open_opts.custom_flags(libc::O_NOCTTY); } let mut file = open_opts.open(path).map_err_context( || translate!("shred-failed-to-open-for-writing", "file" => path.maybe_quote()), )?; + // Validate file type on the opened fd so path swaps cannot bypass checks. #[cfg(unix)] reject_unsupported_file_type(path, &file)?; + // Get metadata from the fd for size/type decisions. let metadata = file .metadata() .map_err_context(|| translate!("shred-failed-to-get-metadata"))?; + let size = target_size(&mut file, &metadata, size)?; let is_regular_file = metadata.file_type().is_file(); - let should_sync_data = is_regular_file; let io_block_size = if is_regular_file { filesystem_block_size(&metadata) } else { @@ -803,7 +784,6 @@ fn wipe_file( let mut pass_sequence = Vec::new(); if size != Some(0) { // Only add passes if the target contains bytes to overwrite - if n_passes <= 3 { // Only random passes if n_passes <= 3 for _ in 0..n_passes { @@ -824,6 +804,7 @@ fn wipe_file( } } + let should_sync_data = is_regular_file; let total_passes = pass_sequence.len(); for (i, pass_type) in pass_sequence.iter().enumerate() { @@ -861,14 +842,68 @@ fn wipe_file( } } + finalize_file(file, path, path_str, verbose, remove_method) +} + +fn finalize_file( + file: File, + path: &Path, + path_str: &OsString, + verbose: bool, + remove_method: RemoveMethod, +) -> UResult<()> { + checked_close(file).map_err_context( + || translate!("shred-failed-to-close-file", "file" => path.maybe_quote()), + )?; + if remove_method != RemoveMethod::None { do_remove(path, path_str, verbose, remove_method).map_err_context( || translate!("shred-failed-to-remove-file", "file" => path.maybe_quote()), )?; } + Ok(()) } +#[cfg(unix)] +fn checked_close(file: File) -> io::Result<()> { + use std::os::fd::IntoRawFd; + let raw_fd = file.into_raw_fd(); + // SAFETY: `raw_fd` came from `File::into_raw_fd`, which transfers ownership of an + // open descriptor to this function. We call `close` exactly once here and never use + // `raw_fd` again afterward. + let close_result = unsafe { libc::close(raw_fd) }; + if close_result == 0 { + Ok(()) + } else { + Err(io::Error::last_os_error()) + } +} + +#[cfg(not(unix))] +fn checked_close(file: File) -> io::Result<()> { + file.sync_all() +} + +fn filesystem_block_size(metadata: &fs::Metadata) -> u64 { + #[cfg(unix)] + { + use std::os::unix::fs::MetadataExt; + + let block_size = metadata.blksize(); + if block_size > 0 { + block_size + } else { + OPTIMAL_IO_BLOCK_SIZE as u64 + } + } + #[cfg(not(unix))] + { + let _ = metadata; + OPTIMAL_IO_BLOCK_SIZE as u64 + } +} + fn split_on_blocks(file_size: u64, exact: bool, io_block_size: u64) -> (u64, u64) { // OPTIMAL_IO_BLOCK_SIZE must not exceed BLOCK_SIZE. Violating this may cause overflows due // to alignment or performance issues.This kind of misconfiguration is @@ -884,17 +919,16 @@ fn split_on_blocks(file_size: u64, exact: bool, io_block_size: u64) -> (u64, u64 // * io_block_size, the output file size will also be aligned and correct. file_size.div_ceil(io_block_size) * io_block_size }; + (file_size / BLOCK_SIZE as u64, file_size % BLOCK_SIZE as u64) } - /// Enable or disable direct I/O on a file descriptor to bypass the page cache. /// This is a performance optimization matching GNU shred behavior. /// Silently does nothing if the platform or filesystem doesn't support it. #[cfg(any(target_os = "linux", target_os = "android"))] fn direct_mode(file: &File, enable: bool) { use std::os::unix::io::AsRawFd; - let fd = file.as_raw_fd(); // SAFETY: fd is a valid open file descriptor from a File we hold a reference to. @@ -915,7 +949,6 @@ fn direct_mode(file: &File, enable: bool) { #[cfg(target_vendor = "apple")] fn direct_mode(file: &File, enable: bool) { use std::os::unix::io::AsRawFd; - let fd = file.as_raw_fd(); // SAFETY: fd is a valid open file descriptor. F_NOCACHE with 0 or 1 is safe. @@ -929,6 +962,20 @@ fn direct_mode(file: &File, enable: bool) { ))] fn direct_mode(_file: &File, _enable: bool) {} +/// Sync the parent directory of `path` to ensure directory entry changes +/// (renames, unlinks) are durable. +fn sync_parent_dir(path: &Path) -> io::Result<()> { + if let Some(parent) = path.parent() { + let dir_path = if parent.as_os_str().is_empty() { + Path::new(".") + } else { + parent + }; + let dir = OpenOptions::new().read(true).open(dir_path)?; + dir.sync_all()?; + } + Ok(()) +} fn is_eio(err: &io::Error) -> bool { #[cfg(unix)] @@ -941,7 +988,6 @@ fn is_eio(err: &io::Error) -> bool { false } } - fn do_pass( file: &mut File, pass_type: &PassType, @@ -1098,7 +1144,7 @@ fn rename_new(old_path: &Path, new_path: &Path) -> io::Result<()> { #[cfg(not(any(target_vendor = "apple", target_os = "linux", target_os = "android")))] fn rename_new(old_path: &Path, new_path: &Path) -> io::Result<()> { // No kernel-level atomic no-overwrite rename available; fall back to - // check-then-rename. Racy, but functional and matches the non-Unix path. + // check-then-rename. Racy, but functional and matches the non-Unix path. if new_path.exists() { return Err(io::Error::new( io::ErrorKind::AlreadyExists, @@ -1111,7 +1157,6 @@ fn rename_new(old_path: &Path, new_path: &Path) -> io::Result<()> { #[cfg(any(target_vendor = "apple", target_os = "linux", target_os = "android"))] fn cstring_from_path(path: &Path) -> io::Result { use std::os::unix::ffi::OsStrExt; - CString::new(path.as_os_str().as_bytes()).map_err(|_| { io::Error::new( io::ErrorKind::InvalidInput, @@ -1120,19 +1165,6 @@ fn cstring_from_path(path: &Path) -> io::Result { }) } -fn sync_parent_dir(path: &Path) -> io::Result<()> { - if let Some(parent) = path.parent() { - let dir_path = if parent.as_os_str().is_empty() { - Path::new(".") - } else { - parent - }; - let dir = OpenOptions::new().read(true).open(dir_path)?; - dir.sync_all()?; - } - Ok(()) -} - /// 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 in case of an error fn wipe_name(orig_path: &Path, verbose: bool, remove_method: RemoveMethod) -> PathBuf { @@ -1159,9 +1191,9 @@ fn wipe_name(orig_path: &Path, verbose: bool, remove_method: RemoveMethod) -> Pa ); } + // Sync the parent directory to ensure the rename is durable. + // GNU shred does this via fdatasync/fsync on the directory fd. if remove_method == RemoveMethod::WipeSync { - // Sync the parent directory to ensure the rename is durable. - // GNU shred does this via fdatasync/fsync on the directory fd. if let Err(err) = sync_parent_dir(&new_path) { show_error!( "failed to sync directory after rename from {} to {}: {}", @@ -1238,18 +1270,42 @@ mod tests { use crate::{ BLOCK_SIZE, BytesWriter, OPTIMAL_IO_BLOCK_SIZE, PassType, Pattern, split_on_blocks, }; - + #[cfg(unix)] + use crate::{RemoveMethod, finalize_file}; + #[cfg(unix)] + use std::os::fd::AsRawFd; #[test] fn test_align_non_exact_control_values() { // Note: This test only makes sense for the default values of BLOCK_SIZE and // OPTIMAL_IO_BLOCK_SIZE. - assert_eq!(split_on_blocks(1, false, OPTIMAL_IO_BLOCK_SIZE as u64), (0, 4096)); - assert_eq!(split_on_blocks(4095, false, OPTIMAL_IO_BLOCK_SIZE as u64), (0, 4096)); - assert_eq!(split_on_blocks(4096, false, OPTIMAL_IO_BLOCK_SIZE as u64), (0, 4096)); - assert_eq!(split_on_blocks(4097, false, OPTIMAL_IO_BLOCK_SIZE as u64), (0, 8192)); - assert_eq!(split_on_blocks(65535, false, OPTIMAL_IO_BLOCK_SIZE as u64), (1, 0)); - assert_eq!(split_on_blocks(65536, false, OPTIMAL_IO_BLOCK_SIZE as u64), (1, 0)); - assert_eq!(split_on_blocks(65537, false, OPTIMAL_IO_BLOCK_SIZE as u64), (1, 4096)); + assert_eq!( + split_on_blocks(1, false, OPTIMAL_IO_BLOCK_SIZE as u64), + (0, 4096) + ); + assert_eq!( + split_on_blocks(4095, false, OPTIMAL_IO_BLOCK_SIZE as u64), + (0, 4096) + ); + assert_eq!( + split_on_blocks(4096, false, OPTIMAL_IO_BLOCK_SIZE as u64), + (0, 4096) + ); + assert_eq!( + split_on_blocks(4097, false, OPTIMAL_IO_BLOCK_SIZE as u64), + (0, 8192) + ); + assert_eq!( + split_on_blocks(65535, false, OPTIMAL_IO_BLOCK_SIZE as u64), + (1, 0) + ); + assert_eq!( + split_on_blocks(65536, false, OPTIMAL_IO_BLOCK_SIZE as u64), + (1, 0) + ); + assert_eq!( + split_on_blocks(65537, false, OPTIMAL_IO_BLOCK_SIZE as u64), + (1, 4096) + ); } #[test] @@ -1262,7 +1318,6 @@ mod tests { } } - #[test] fn test_align_non_exact_uses_provided_block_size() { assert_eq!(split_on_blocks(1, false, 512), (0, 512)); @@ -1270,7 +1325,6 @@ mod tests { assert_eq!(split_on_blocks(513, false, 512), (0, 1024)); } - #[test] fn test_pattern_writer_syncs_to_file_offset() { let mut writer = BytesWriter::from_pass_type( @@ -1303,7 +1357,39 @@ mod tests { b"A" ); } + #[cfg(unix)] + #[test] + fn test_finalize_file_does_not_remove_on_close_error() { + let temp_dir = + std::env::temp_dir().join(format!("uu_shred_close_error_{}", std::process::id())); + std::fs::create_dir_all(&temp_dir).expect("test temporary directory is created"); + + let file_path = temp_dir.join("target"); + std::fs::write(&file_path, b"content").expect("test file is created"); + + let file = std::fs::OpenOptions::new() + .write(true) + .open(&file_path) + .expect("test file opens for writing"); + let raw_fd = file.as_raw_fd(); + + // SAFETY: `raw_fd` belongs to `file` and is currently valid. Closing it here + // intentionally simulates a late close failure when `finalize_file` performs its + // checked close on the same descriptor. + let close_result = unsafe { libc::close(raw_fd) }; + assert_eq!(close_result, 0, "test setup closes descriptor once"); + + let file_name = std::ffi::OsString::from("target"); + let result = finalize_file(file, &file_path, &file_name, false, RemoveMethod::Unlink); + assert!(result.is_err(), "close failure is reported"); + assert!( + file_path.exists(), + "remove phase is skipped when close fails" + ); + std::fs::remove_file(&file_path).expect("test file cleanup succeeds"); + std::fs::remove_dir(&temp_dir).expect("test directory cleanup succeeds"); + } #[test] fn test_align_exact_cycle() { for size in 1..BLOCK_SIZE as u64 * 2 { diff --git a/tests/by-util/test_shred.rs b/tests/by-util/test_shred.rs index f60944ce27b..1dd717017ff 100644 --- a/tests/by-util/test_shred.rs +++ b/tests/by-util/test_shred.rs @@ -3,7 +3,7 @@ // For the full copyright and license information, please view the LICENSE // file that was distributed with this source code. -// spell-checker:ignore wipesync +// spell-checker:ignore wipesync SECRETSECRET use uutests::at_and_ucmd; use uutests::new_ucmd; @@ -245,7 +245,6 @@ fn test_shred_rejects_fifo() { .fails() .stderr_contains("invalid file type"); } - #[test] fn test_shred_verbose_no_padding_1() { let (at, mut ucmd) = at_and_ucmd!();