dd: keep short reads from corrupting re-blocked output#13459
Conversation
| unix, | ||
| not(target_os = "macos"), | ||
| not(target_os = "freebsd"), | ||
| feature = "printf" |
There was a problem hiding this comment.
Good catch. This test drives the writer through sh -c, where printf is a shell builtin, not the uutils printf utility, so the feature dependency was unnecessary.
I removed feature = "printf" from the regression test and widened the cfgs of the two shared imports it uses (get_tests_binary and std::process::Command) by their actual use, so a dd-only build still compiles. Verified that the test is now discovered and passes with --no-default-features --features dd (it was silently compiled out there before), and that dd,printf, feat_os_unix and the default feature set are unchanged.
The two neighbouring FIFO tests carry the same historical gate even though they also only use the shell builtin. I left them untouched to keep this PR focused on the correctness fix; they are the same kind of test-configuration cleanup and can follow separately.
b9bd209 to
7193662
Compare
Merging this PR will improve performance by 3.04%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | Simulation | du_max_depth_balanced_tree[(6, 4, 10)] |
25.9 ms | 25.1 ms | +3.04% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing relative23:dd-partial-read-gathering (8199a64) with main (50b8ead)
Footnotes
-
46 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
|
GNU testsuite comparison: |
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. The regression test drives the writer through `sh -c`, where printf is a shell builtin, so it does not need the printf utility feature; only the shared imports it uses were gated on it. Widen those gates by use instead, so the test also runs in a dd-only build. Fixes uutils#13458.
7193662 to
8199a64
Compare
Fixes #13458.
Two related defects made short reads (pipes, FIFOs, sockets — no
iflag=fullblock) corrupt or mis-block the output whenobs > ibs:fill_consecutivekept reading after a short read, so the next read landed at the followingibs-aligned chunk of the copy-loop buffer. The gap in between held stale buffer bytes, and the finaltruncate(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 match GNU (reproducer and analysis in dd: a short read corrupts the output when obs > ibs (stale filler byte written, real bytes dropped) #13458). The fix ends the fill at the first short read: the bytes received so far form one partial record, which is how GNU dd processes it. Completeibsreads continue to fill the buffer exactly as before, so regular-file behavior is unchanged.Fixing that exposed a latent defect in
BufferedOutput::write_blocks: when the pending partial block plus the incoming bytes still do not fill oneobsblock, the split index saturates to zero and the pending bytes were handed to the inner writer as a premature partial write (visible as0+2 records outwhere GNU reports0+1). They are now buffered until a block completes or the stream ends. This path was practically unreachable before this change, because sub-obsfills used to happen only once, at EOF.Testing
ibs=3 obs=6, two deliberately short reads): fails onmainwithab\xDDc(stale byte injected,ddropped), passes withabcdafter the fix; the stats0+2 records in / 0+1 records outmatch GNU dd (verified against GNU on Debian stable). Ran 10× in a loop to guard against timing flakiness.BufferedOutput: two writes that together still do not fill one block are both buffered instead of being flushed prematurely.cargo test -p uu_dd(89 tests) and the dd integration suite (127 passed, including the existing FIFO partial-read tests withibs == obs, which are unaffected) pass; fmt, clippy and cspell are clean.Note
The changed lines in
fill_consecutiveoverlap textually with the refactor in #13373; the conflict is trivial and I am happy to rebase either PR once the other lands.