From 45d63236401aab81d0d9951e7b3ab8bca8d0e080 Mon Sep 17 00:00:00 2001 From: relative23 <62724298+relative23@users.noreply.github.com> Date: Sun, 19 Jul 2026 16:12:56 +0200 Subject: [PATCH] dd: keep short reads from corrupting re-blocked output Two related defects made short reads (pipes, FIFOs, sockets, no iflag=fullblock) corrupt or mis-block the output when obs > ibs: fill_consecutive kept reading after a short read, so the next read landed at the following ibs-aligned chunk of the copy-loop buffer. The gap in between held stale buffer bytes, and the final truncate(bytes_total) kept that gap in the output while cutting the same number of real bytes off the end -- silent corruption, since the record counts still matched GNU. End the fill at the first short read instead: the bytes received so far form one partial record, as in GNU dd. That exposed a latent defect in BufferedOutput::write_blocks: when the pending partial block plus the incoming bytes still do not fill one obs block, the split index saturated to zero and the pending bytes were handed to the inner writer as a premature partial write. Buffer them instead until a block completes or the final flush. Fixes #13458. --- src/uu/dd/src/bufferedoutput.rs | 30 +++++++++++++++++++++ src/uu/dd/src/dd.rs | 10 ++++++- tests/by-util/test_dd.rs | 47 +++++++++++++++++++++++++++++++++ 3 files changed, 86 insertions(+), 1 deletion(-) diff --git a/src/uu/dd/src/bufferedoutput.rs b/src/uu/dd/src/bufferedoutput.rs index 7f4f92b8528..e0e5b371a56 100644 --- a/src/uu/dd/src/bufferedoutput.rs +++ b/src/uu/dd/src/bufferedoutput.rs @@ -74,6 +74,14 @@ impl<'a> BufferedOutput<'a> { // If `buf` does not include enough bytes to form a full block, // just buffer the whole thing and write zero blocks. let n = self.buf.len() + buf.len(); + if n < self.inner.settings.obs { + // Not even one complete block can be formed: buffer everything. + // Falling through would pass the pending bytes to the inner + // writer and emit them as a premature partial write (reachable + // once short reads end a fill early, see issue #13458). + self.buf.extend_from_slice(buf); + return Ok(WriteStat::default()); + } let rem = n % self.inner.settings.obs; let i = buf.len().saturating_sub(rem); let (to_write, to_buffer) = buf.split_at(i); @@ -144,6 +152,28 @@ mod tests { assert_eq!(output.buf, b"ab"); } + #[test] + fn test_buffered_output_write_blocks_lone_partial_accumulates() { + let settings = Settings { + obs: 6, + ..Default::default() + }; + let inner = Output { + dst: Dest::Sink, + settings: &settings, + }; + let mut output = BufferedOutput::new(inner).unwrap(); + // Two writes that together still do not fill one block must both + // be buffered; the second call must not flush the pending partial + // block prematurely. + output.write_blocks(b"ab").unwrap(); + let wstat = output.write_blocks(b"cd").unwrap(); + assert_eq!(wstat.writes_complete, 0); + assert_eq!(wstat.writes_partial, 0); + assert_eq!(wstat.bytes_total, 0); + assert_eq!(output.buf, b"abcd"); + } + #[test] fn test_buffered_output_write_blocks_complete() { let settings = Settings { diff --git a/src/uu/dd/src/dd.rs b/src/uu/dd/src/dd.rs index 0599963e6e3..163d10c9423 100644 --- a/src/uu/dd/src/dd.rs +++ b/src/uu/dd/src/dd.rs @@ -515,7 +515,9 @@ impl Input<'_> { /// Fills a given buffer. /// Reads in increments of 'self.ibs'. - /// The start of each ibs-sized read follows the previous one. + /// The start of each ibs-sized read follows the previous one; a short + /// read ends the fill, so the bytes received so far form one partial + /// record for the copy loop (as in GNU dd). fn fill_consecutive(&mut self, buf: &mut Vec) -> io::Result { let mut reads_complete = 0; let mut reads_partial = 0; @@ -530,6 +532,12 @@ impl Input<'_> { rlen if rlen > 0 => { bytes_total += rlen; reads_partial += 1; + // A short read must end this fill: the next read would + // start at the following ibs-aligned chunk, leaving a + // gap of stale bytes inside `buf` that the `truncate` + // below would keep in the output while dropping the + // same number of real trailing bytes (issue #13458). + break; } _ => break, } diff --git a/tests/by-util/test_dd.rs b/tests/by-util/test_dd.rs index 849c47e227f..6ba3bb82572 100644 --- a/tests/by-util/test_dd.rs +++ b/tests/by-util/test_dd.rs @@ -1761,6 +1761,53 @@ fn test_reading_partial_blocks_from_fifo_unbuffered() { assert!(output.stderr.starts_with(expected)); } +/// Regression test for : +/// two deliberately short reads (ibs=3 sees only 2 bytes each) must be +/// gathered into a single obs=6 output block without pulling stale bytes +/// from the ibs-aligned gap into the output. +/// +/// The writer below runs `printf` inside `sh`, where it is a builtin, so this +/// needs no `printf` feature. +#[test] +#[cfg(all(unix, not(target_os = "macos"), not(target_os = "freebsd")))] +fn test_reading_partial_blocks_from_fifo_gathered_into_larger_obs() { + // Create the FIFO. + let ts = TestScenario::new(util_name!()); + let at = &ts.fixtures; + at.mkfifo("fifo"); + let fifoname = at.plus_as_string("fifo"); + + // Start a `dd` process that reads from the fifo (so it will wait + // until the writer process starts). + let mut reader_command = Command::new(get_tests_binary()); + let child = reader_command + .args(["dd", "ibs=3", "obs=6", &format!("if={fifoname}")]) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .env("LC_ALL", "C") + .env("LANG", "C") + .env("LANGUAGE", "C") + .spawn() + .unwrap(); + + // Start different processes to write to the FIFO, with a small + // pause in between. + let mut writer_command = Command::new("sh"); + let _ = writer_command + .args([ + "-c", + &format!("(printf \"ab\"; sleep 0.1; printf \"cd\") > {fifoname}"), + ]) + .spawn() + .unwrap() + .wait(); + + let output = child.wait_with_output().unwrap(); + assert_eq!(output.stdout, b"abcd"); + let expected = b"0+2 records in\n0+1 records out\n4 bytes copied"; + assert!(output.stderr.starts_with(expected)); +} + #[test] #[cfg(any(target_os = "linux", target_os = "android"))] fn test_iflag_directory_fails_when_file_is_passed_via_std_in() {