From a540ab3563a28caad49753ca18c84aa4107e173c Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Sun, 8 Mar 2026 15:42:23 +0100 Subject: [PATCH] split: fix I/O error handling for device full conditions Should fix tests/split/split-io-err.sh --- src/uu/split/locales/en-US.ftl | 4 + src/uu/split/locales/fr-FR.ftl | 5 ++ src/uu/split/src/platform/unix.rs | 16 +++- src/uu/split/src/split.rs | 118 +++++++++++++++++++++++++++--- tests/by-util/test_split.rs | 43 +++++++++++ 5 files changed, 172 insertions(+), 14 deletions(-) diff --git a/src/uu/split/locales/en-US.ftl b/src/uu/split/locales/en-US.ftl index 629b8956d75..05a751f52cb 100644 --- a/src/uu/split/locales/en-US.ftl +++ b/src/uu/split/locales/en-US.ftl @@ -38,6 +38,10 @@ split-error-cannot-determine-input-size = { $input }: cannot determine input siz split-error-cannot-determine-file-size = { $input }: cannot determine file size split-error-cannot-read-from-input = { $input }: cannot read from input : { $error } split-error-input-output-error = input/output error +split-error-flush-before-file-switch = flush error before file switch +split-error-flush-already-reported = flush error already reported +split-error-write-already-reported = write error already reported +split-error-flush-in-chunk-writer = flush error in ByteChunkWriter split-error-unable-to-open-file = unable to open { $file }; aborting split-error-unable-to-reopen-file = unable to re-open { $file }; aborting split-error-file-descriptor-limit = at file descriptor limit, but no file descriptor left to close. Closed { $count } writers before. diff --git a/src/uu/split/locales/fr-FR.ftl b/src/uu/split/locales/fr-FR.ftl index e8e70c280fa..ef40289669b 100644 --- a/src/uu/split/locales/fr-FR.ftl +++ b/src/uu/split/locales/fr-FR.ftl @@ -38,11 +38,16 @@ split-error-cannot-determine-input-size = { $input } : impossible de déterminer split-error-cannot-determine-file-size = { $input } : impossible de déterminer la taille du fichier split-error-cannot-read-from-input = { $input } : impossible de lire depuis l'entrée : { $error } split-error-input-output-error = erreur d'entrée/sortie +split-error-flush-before-file-switch = erreur de vidage avant changement de fichier +split-error-flush-already-reported = erreur de vidage déjà signalée +split-error-write-already-reported = erreur d'écriture déjà signalée +split-error-flush-in-chunk-writer = erreur de vidage dans ByteChunkWriter split-error-unable-to-open-file = impossible d'ouvrir { $file } ; abandon split-error-unable-to-reopen-file = impossible de rouvrir { $file } ; abandon split-error-file-descriptor-limit = limite de descripteurs de fichiers atteinte, mais aucun descripteur de fichier à fermer. { $count } écrivains fermés auparavant. split-error-shell-process-returned = Le processus shell a retourné { $code } split-error-shell-process-terminated = Le processus shell a été terminé par un signal +split-error-is-a-directory = { $dir } : Est un répertoire # Messages d'aide pour les options de ligne de commande split-help-bytes = mettre TAILLE octets par fichier de sortie diff --git a/src/uu/split/src/platform/unix.rs b/src/uu/split/src/platform/unix.rs index 656bd0109be..8d6cb7f365f 100644 --- a/src/uu/split/src/platform/unix.rs +++ b/src/uu/split/src/platform/unix.rs @@ -143,19 +143,27 @@ pub fn instantiate_current_writer( ErrorKind::IsADirectory => Error::other( translate!("split-error-is-a-directory", "dir" => filename), ), - _ => Error::other( + ErrorKind::PermissionDenied => Error::other( translate!("split-error-unable-to-open-file", "file" => filename), ), + _ => Error::other(format!( + "split: {filename}: {}", + uucore::error::strip_errno(&e) + )), })? } else { // re-open file that we previously created to append to it std::fs::OpenOptions::new() .append(true) .open(Path::new(&filename)) - .map_err(|_| { - Error::other( + .map_err(|e| match e.kind() { + ErrorKind::PermissionDenied => Error::other( translate!("split-error-unable-to-reopen-file", "file" => filename), - ) + ), + _ => Error::other(format!( + "split: {filename}: {}", + uucore::error::strip_errno(&e) + )), })? }; Ok(BufWriter::new(Box::new(file) as Box)) diff --git a/src/uu/split/src/split.rs b/src/uu/split/src/split.rs index 923f66ac134..25640388034 100644 --- a/src/uu/split/src/split.rs +++ b/src/uu/split/src/split.rs @@ -21,7 +21,9 @@ use std::io::{BufRead, BufReader, BufWriter, ErrorKind, Read, Seek, SeekFrom, Wr use std::path::Path; use thiserror::Error; use uucore::display::Quotable; -use uucore::error::{FromIo, UIoError, UResult, USimpleError, UUsageError}; +use uucore::error::{ + FromIo, UIoError, UResult, USimpleError, UUsageError, set_exit_code, strip_errno, +}; use uucore::translate; use uucore::parser::parse_size::parse_size_u64; @@ -565,11 +567,48 @@ fn ignorable_io_error(error: &io::Error, settings: &Settings) -> bool { /// If ignorable io error occurs, return number of bytes as if all bytes written /// Should not be used for Kth chunk number sub-strategies /// as those do not work with `--filter` option -fn custom_write(bytes: &[u8], writer: &mut T, settings: &Settings) -> io::Result { +/// +/// When `error_context` is None, acts as simple write wrapper +/// When `error_context` is Some((filename, error_reported)), enables error reporting and immediate flushing +fn custom_write( + bytes: &[u8], + writer: &mut T, + settings: &Settings, + error_context: Option<(&str, &mut bool)>, +) -> io::Result { match writer.write(bytes) { - Ok(n) => Ok(n), + Ok(n) => { + // If error reporting is enabled, flush immediately to catch buffered I/O errors + if let Some((filename, error_reported)) = error_context { + match writer.flush() { + Ok(()) => Ok(n), + Err(e) if ignorable_io_error(&e, settings) => Ok(bytes.len()), + Err(e) => { + if !*error_reported { + uucore::show_error!("{}: {}", filename, strip_errno(&e)); + set_exit_code(1); + *error_reported = true; + } + Err(io::Error::other("")) + } + } + } else { + Ok(n) + } + } Err(e) if ignorable_io_error(&e, settings) => Ok(bytes.len()), - Err(e) => Err(e), + Err(e) => { + if let Some((filename, error_reported)) = error_context { + if !*error_reported { + uucore::show_error!("{}: {}", filename, strip_errno(&e)); + set_exit_code(1); + *error_reported = true; + } + Err(io::Error::other("")) + } else { + Err(e) + } + } } } @@ -713,6 +752,12 @@ struct ByteChunkWriter<'a> { /// Iterator that yields filenames for each chunk. filename_iterator: FilenameIterator<'a>, + + /// Current filename being written to. + current_filename: String, + + /// Whether an error has already been reported for the current file. + error_reported: bool, } impl<'a> ByteChunkWriter<'a> { @@ -732,10 +777,28 @@ impl<'a> ByteChunkWriter<'a> { num_chunks_written: 0, inner, filename_iterator, + current_filename: filename, + error_reported: false, }) } } +impl Drop for ByteChunkWriter<'_> { + fn drop(&mut self) { + // Ensure final flush to catch any buffered write errors, but only report if not already reported + if !self.error_reported { + if let Err(e) = self.inner.flush() { + uucore::show_error!( + "split: {}: final flush failed: {}", + self.current_filename, + strip_errno(&e) + ); + set_exit_code(1); + } + } + } +} + impl Write for ByteChunkWriter<'_> { /// Implements `--bytes=SIZE` fn write(&mut self, mut buf: &[u8]) -> io::Result { @@ -751,6 +814,13 @@ impl Write for ByteChunkWriter<'_> { } if self.num_bytes_remaining_in_current_chunk == 0 { + // Flush the current writer before switching to a new file to catch any delayed write errors + if let Err(e) = self.inner.flush() { + uucore::show_error!("{}: {}", self.current_filename, strip_errno(&e)); + set_exit_code(1); + return Err(io::Error::other("")); + } + // Increment the chunk number, reset the number of bytes remaining, and instantiate the new underlying writer. self.num_chunks_written += 1; self.num_bytes_remaining_in_current_chunk = self.chunk_size; @@ -763,6 +833,8 @@ impl Write for ByteChunkWriter<'_> { println!("creating file {}", filename.quote()); } self.inner = self.settings.instantiate_current_writer(&filename, true)?; + self.current_filename = filename; + self.error_reported = false; } // If the capacity of this chunk is greater than the number of @@ -771,7 +843,12 @@ impl Write for ByteChunkWriter<'_> { // the chunk number and repeat. let buf_len = buf.len(); if (buf_len as u64) < self.num_bytes_remaining_in_current_chunk { - let num_bytes_written = custom_write(buf, &mut self.inner, self.settings)?; + let num_bytes_written = custom_write( + buf, + &mut self.inner, + self.settings, + Some((&self.current_filename, &mut self.error_reported)), + )?; self.num_bytes_remaining_in_current_chunk -= num_bytes_written as u64; return Ok(carryover_bytes_written + num_bytes_written); } @@ -782,7 +859,12 @@ impl Write for ByteChunkWriter<'_> { // self.num_bytes_remaining_in_current_chunk is lower than // n, which is already usize. let i = self.num_bytes_remaining_in_current_chunk as usize; - let num_bytes_written = custom_write(&buf[..i], &mut self.inner, self.settings)?; + let num_bytes_written = custom_write( + &buf[..i], + &mut self.inner, + self.settings, + Some((&self.current_filename, &mut self.error_reported)), + )?; self.num_bytes_remaining_in_current_chunk -= num_bytes_written as u64; // It's possible that the underlying writer did not @@ -799,7 +881,14 @@ impl Write for ByteChunkWriter<'_> { } } fn flush(&mut self) -> io::Result<()> { - self.inner.flush() + match self.inner.flush() { + Ok(()) => Ok(()), + Err(e) => { + uucore::show_error!("{}: {}", self.current_filename, strip_errno(&e)); + set_exit_code(1); + Err(io::Error::other("")) + } + } } } @@ -891,7 +980,8 @@ impl Write for LineChunkWriter<'_> { // Write the line, starting from *after* the previous // separator character and ending *after* the current // separator character. - let num_bytes_written = custom_write(&buf[prev..=i], &mut self.inner, self.settings)?; + let num_bytes_written = + custom_write(&buf[prev..=i], &mut self.inner, self.settings, None)?; total_bytes_written += num_bytes_written; prev = i + 1; self.num_lines_remaining_in_current_chunk -= 1; @@ -907,7 +997,7 @@ impl Write for LineChunkWriter<'_> { self.num_lines_remaining_in_current_chunk = self.chunk_size; } let num_bytes_written = - custom_write(&buf[prev..buf.len()], &mut self.inner, self.settings)?; + custom_write(&buf[prev..buf.len()], &mut self.inner, self.settings, None)?; total_bytes_written += num_bytes_written; } Ok(total_bytes_written) @@ -1606,7 +1696,15 @@ fn split(settings: &Settings) -> UResult<()> { // allowable filenames, we use `ErrorKind::Other` to // indicate that. A special error message needs to be // printed in that case. - ErrorKind::Other => Err(USimpleError::new(1, format!("{e}"))), + ErrorKind::Other => { + let error_msg = format!("{e}"); + if error_msg.is_empty() { + // This is a handled error, return error to stop processing + Err(USimpleError::new(1, "")) + } else { + Err(USimpleError::new(1, error_msg)) + } + } _ => Err(uio_error!( e, "{}", diff --git a/tests/by-util/test_split.rs b/tests/by-util/test_split.rs index d9dac54ca21..3fc69bfbc8f 100644 --- a/tests/by-util/test_split.rs +++ b/tests/by-util/test_split.rs @@ -2091,3 +2091,46 @@ fn test_split_directory_already_exists() { .no_stdout() .stderr_is("split: xaa: Is a directory\n"); } + +#[test] +#[cfg(target_os = "linux")] +fn test_split_io_error() { + use std::process::Command; + + let (at, mut ucmd) = at_and_ucmd!(); + + // Use ln command to create a symlink to /dev/full, like the GNU test does + // This is more reliable in various test environments + let ln_result = Command::new("ln") + .args(["-sf", "/dev/full", "xaa"]) + .current_dir(at.as_string()) + .status(); + + if ln_result.is_err() || !ln_result.unwrap().success() { + // Skip the test if we can't create the symlink (might happen in some test environments) + eprintln!("Skipping I/O error test: cannot create symlink to /dev/full"); + return; + } + + // Create input with 2 bytes to be split into 1-byte chunks + at.write("input", "ab"); + + // split should fail with exit code 1 when writing to /dev/full + ucmd.args(&["-b", "1", "input"]) + .fails_with_code(1) + .no_stdout() + .stderr_contains("split: xaa: No space left on device"); + + // The symlink should still exist after the failed split + // Note: at.file_exists() doesn't handle symlinks properly, so we use Path::exists() as fallback + let xaa_path = at.as_string().clone() + "/xaa"; + assert!( + at.file_exists("xaa") || Path::new(&xaa_path).exists(), + "Expected xaa symlink to exist after failed split" + ); + // But split should not have continued to create additional files + assert!( + !at.file_exists("xab"), + "Expected xab to not exist after split failure" + ); +}