uucore: drop the pipe after a failed splice write by abendrothj · Pull Request #15086 · uutils/coreutils

abendrothj · 2026-10-05T10:16:36Z

cat, cp, install, mv, head, tail and tac copy through cached pipes that get reused for more than one copy. If a write to the destination failed part-way (ENOSPC, EFBIG), the bytes still in the pipe stayed there, and the next copy wrote them at the start of its own destination. With cp, a file that failed to copy put the rest of its data into the next file.

A cached pipe is now put back only when the copy through it succeeded. After a failure it's dropped, and the next copy makes a new one. test_cp_failed_write_does_not_spill_into_next_file fails on main and passes here.

tee has the same problem with its own pipes; that's #15132.

Closes #15085

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

oech3 · 2026-10-05T17:40:22Z

Sign in to view

+        match splice(pipe, dest, len) {
+            Ok(s) => len -= s,
+            Err(e) => {
+                let _ = std::io::copy(&mut pipe.take(len as u64), &mut std::io::sink());


Oh wait... it is probally read(2) to user space RAM. Though std might optimize it with splice to /dev/null in the future... We would not see EIO or OOM at here. But I still think we should discard & regenerate pipe to avoid it.

Right, sink() doesn't open anything: it's a unit struct whose write just returns the length, so this is a read into a buffer that gets thrown away.

I'd still drain rather than recreate the pipe. tee also calls drain_pipe, with its own two pipes that aren't in PIPE_CACHE, so dropping the cached pipe wouldn't fix it, and tee has the same bug: on main, when one output fails part-way, the bytes it didn't take end up in the other outputs. test_tee_failed_output_does_not_spill_into_others in 55a4ca3 covers that. Draining in drain_pipe fixes every caller in one place, and either way it only happens after a failed write.

Recalling pipe is much cheaper than repeating read internally. At least, we should have .unwrap() to catch failure.

Agreed, switched to recreating the pipe, so there's no read and nothing to unwrap. tee replaces the pipe a failed output was using with a new one of the same size.

A correction to my reply above: tee doesn't make the new pipe "the same size" anymore. The 2nd pipe is always 1 MiB and the 1st is only enlarged if the 2nd was. The old pair is closed before the new one is made, and if no new pipe can be made tee falls back to read/write. That's in #15132 now.

correction: in #15132 tee no longer falls back to read/write, if it can't make a new pipe it reports the error and stops.

oech3 · 2026-10-05T10:41:01Z

+    fn ignore() -> Self {
+        // SAFETY: signal() with SIG_IGN is async-signal-safe and `drop` puts
+        // the old handler back.
+        Self(unsafe { libc::signal(libc::SIGXFSZ, libc::SIG_IGN) })


Would you rewrite the test with strace to drop unsafes?

Since the unsafe operations are properly encapsulated in SigxfszGuard I think unsafe is fine for FFI.

I prefer strace also for readability.

It matches the approach used in tests/by-util/test_dd.rs; I think it’s OK for this PR.

The tests have no unsafe now: 4179394 adds UCommand::ignore_sigxfsz(), which ignores SIGXFSZ in the child's pre_exec, next to the existing limit and umask ones. The old guard changed SIGXFSZ for the whole test binary, and test_dd's two tests that used it were already killing each other's dd under cargo test (143 of 300 runs with two threads). They use the new method now too.

I tried strace with cp as well: inject=splice:error=EIO:when=4 with --reflink=never reproduces it reliably. But strace isn't in the runner images, and only the script-check jobs install it, so in the test jobs it would return early without checking anything. The fsize limit runs on every Linux job.

I think 4179394 deserves it's own PR.

Split out into #15131 (c44a370). This PR is based on it, because the cp test uses ignore_sigxfsz. A correction to my earlier numbers: the 143/300 came from a different setup. With --test-threads=2 on the two dd Fsize tests, main fails 26 of 1000 runs and the new method 0 of 1000.

codspeed · 2026-10-05T12:20:39Z

Merging this PR will not alter performance

⚠️

124 benchmarks spent significant time in system calls

System calls cannot be consistently instrumented, so they are not included in the measure, which understates the real cost. Please switch to the Walltime instrument to accurately measure system calls.

Measurement and system calls

du_all_wide_tree[(5000, 500)]

sort_unique_utf8_locale

du_wide_tree[(5000, 500)]

factor_multiple_u64s[2]

dd_copy_64k_blocks

df_deep_directory

cksum_raw_output

cksum_sysv

cksum_default

cksum_multiple_files

…and 114 more

ℹ️ 1 benchmark has no flame graph

The flamegraph could not be generated because too much time was spent in runtime internals, making the visualization not useful for performance analysis.

sort_dictionary_order[500000]

✅ 401 untouched benchmarks
⏩ 70 skipped benchmarks¹

_{Comparing abendrothj:pipes-discard-leftover (d65be17) with main (a770f03)}

70 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. ↩

xtqqczze · 2026-10-05T15:22:50Z

+/// `dest` fails part-way, the bytes still in `pipe` are read out and dropped
+/// before the error is returned, so that they do not end up at the start of
+/// the next destination.
+#[inline]


#[inline] shouldn't be necessary for a function that isn't public.

Gone along with splice_all.

abendrothj · 2026-10-05T21:29:11Z

Sign in to view

+/// before the error is returned, so that they do not end up at the start of
+/// the next destination.
+#[inline]
+fn splice_all(pipe: &PipeReader, dest: &impl AsFd, mut len: usize) -> std::io::Result<()> {


Done in 55a4ca3: the loops are back as they were, with .inspect_err(|_| discard(pipe, remaining))?. I kept a one-line discard so the drain is written once.

This one is outdated now: there's no discard and no draining after a failed write anymore. The pipe is dropped instead, so the loops don't need an inspect_err at all.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

abendrothj · 2026-10-05T22:14:48Z

Sign in to view

+///
+/// If writing to `dest` fails part-way, the bytes still in `pipe` are read out
+/// and dropped before the error is returned: callers reuse their pipes, and
+/// those bytes must not end up at the start of the next destination.


Moved it under the loop in 7053f9b.

Correction: in dadbe4d the comment sits directly above the loop it's about, and the function doc states the rule for callers.

oech3 · 2026-10-05T21:14:41Z

+    // When writing to one output fails part-way, the bytes it did not take must
+    // not end up in the other outputs.
+    #[test]
+    fn test_tee_failed_output_does_not_spill_into_others() {


One utility is enough to avoid regression.

With the pipe recreated, tee has its own code for this (replace_pipe), which the cp test doesn't reach, so I kept the tee test and made it go through both of tee's pipes. It fails if either one isn't replaced.

This PR now only has the cp test, test_cp_failed_write_does_not_spill_into_next_file. The tee tests went to #15132 with the tee change, since that's the code they exercise.

abendrothj · 2026-10-05T22:14:52Z

Sign in to view


+/// Read out and drop the `len` bytes left in `pipe`.
+fn discard(pipe: &PipeReader, len: usize) {
+    let _ = std::io::copy(&mut pipe.take(len as u64), &mut std::io::sink());


Done in 7053f9b: both caches are a thread-local Cell now. The pipe is taken out for the copy and put back only if the copy succeeded, so after a failed write it's dropped and the next copy makes a new one. discard is gone. One side effect: if creating the pipe fails, the next call tries again instead of using the fallback for the rest of the process.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

abendrothj · 2026-10-06T07:31:55Z

Sign in to view

        #[cfg(any(target_os = "linux", target_os = "android"))]
-        if let Ok((pipe_read, pipe_write)) = io::pipe()
-            && let Ok((pipe2_read, pipe2_write)) = io::pipe()
+        if let Ok(mut pipe) = io::pipe()


Right, and no * is needed since they're plain locals: the names are back (pipe_read/pipe_write, pipe2_read/pipe2_write), and the macro reassigns them with (r, w) = .... This is in the tee PR now, #15132.

abendrothj · 2026-10-06T07:31:58Z

Sign in to view

+#[cfg(any(target_os = "linux", target_os = "android"))]
+fn replace_pipe(pipe: &mut (io::PipeReader, io::PipeWriter)) -> io::Result<()> {
+    use rustix::pipe::{fcntl_getpipe_size, fcntl_setpipe_size};
+    let size = fcntl_getpipe_size(&pipe.0)?;


replace_pipe and the F_GETPIPE_SZ are gone (in #15132). A new 2nd pipe always comes from pipe::<true>(). The 1st one doesn't, because tee only grows its pipes when growing the 2nd one worked at startup, and growing the 1st can fail on its own when the user's pipe pages run out, which would make tee abort where it now goes on. So a new 1st pipe is pipe::<false>() when the 2nd is 1 MiB, and plain io::pipe() otherwise.

outdated too: in #15132 the macro now makes both new pipes the same way, enlarged only if the 2nd was at startup, so a new 1st pipe that can't be enlarged stops tee as well.

oech3 · 2026-10-05T22:23:15Z


 /// splice `len` bytes from `pipe` into `dest`.
+///
+/// On error, `pipe` may still hold bytes that were not written: don't reuse it.


Can we make drain_pipe itself safe instead of asking to reflesh pipe?

I kept the contract: drain_pipe can leave bytes behind after a partial splice error, its doc says not to reuse the pipe after an error, and the owners act on that. The cache only gets a pipe back when the copy succeeded, and tee replaces the pair a failed output used (that's in #15132). Making it safe in place means discarding the rest on error, which is more I/O on the error path that can fail too, so we'd still need the drop as a backstop. It also wouldn't cover the later-chunk loop in splice_unbounded_with, which calls splice directly and not drain_pipe.

OK. It seems best currently. I have no idea how to make code simpler.

abendrothj · 2026-10-06T07:31:51Z

Sign in to view

Moved to #15131. This PR no longer changes test_dd.rs, apart from carrying that commit as its base until it lands.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

xtqqczze · 2026-10-10T09:45:58Z


+thread_local! {
+    /// Cache empty pipe pair to avoid calling `pipe2` at each copy.
+    static PIPE_CACHE: Cell<Option<(PipeReader, PipeWriter)>> = const { Cell::new(None) };


what is the purpose of the const block here?

It lets thread_local! skip the lazy initialization check; clippy's missing_const_for_thread_local asks for it.

oech3 · 2026-10-11T03:46:34Z

 pub fn splice_unbounded_auto(source: &impl AsFd, dest: &mut impl AsFd) -> PipeRes {
-    static PIPE_CACHE: OnceLock<Option<(PipeReader, PipeWriter)>> = OnceLock::new();
-    let Some((pipe_rd, pipe_wr)) = PIPE_CACHE.get_or_init(|| pipe::<false>().ok()) else {
+    let Ok(pipe) = PIPE_CACHE.take().map_or_else(pipe::<false>, Ok) else {


Suggested change

let Ok(pipe) = PIPE_CACHE.take().map_or_else(pipe::<false>, Ok) else {

let Ok(&(pipe_rd, pipe_wr)) = PIPE_CACHE.take().map_or_else(pipe::<false>, Ok) else {

Is this possible to remove let (pipe_rd, pipe_wr) = &pipe;?

Done, by value: take() gives the pair itself, so &(..) doesn't match. Same in send_n_bytes.

Thanks for the suggestion, much cleaner.

abendrothj · 2026-10-11T06:26:26Z

Sign in to view

-            if drain_pipe(pipe_rd, dest, n)?.is_err() {
-                return Ok(Err(()));
+    // an error returns without caching the pipe: a failed write can leave bytes in it
+    let res = 'copy: {


Done: the loop moved into the Ok(n) arm, so the match gives the result and there's one PIPE_CACHE.set after it.

abendrothj · 2026-10-11T06:26:27Z

Sign in to view

+    let (broker_r, broker_w) = &pipe;
    let mut n = n;
    let mut bytes_written: u64 = 0;
+    // an error returns without caching the pipe: a failed write can leave bytes in it


Done, and the same in splice_unbounded_auto.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

github-actions · 2026-10-11T06:43:38Z

GNU testsuite comparison:

Skipping an intermittent issue tests/date/resolution (passes in this run but fails in the 'main' branch)

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing. Review the Copilot code review billing settings.

eduardomourar · 2026-10-17T09:35:43Z

+
 #[cfg(any(target_os = "linux", target_os = "android"))]
 #[test]
 #[cfg_attr(wasi_runner, ignore = "WASI: --sparse is only supported on linux")]


The --sparse test here uses #[cfg_attr(wasi_runner, ignore = "...")]
while the one you added uses #[cfg(not(wasi_runner))]. Both exclude WASI, but one skips
visibly and one compiles out. Matching the style between the two adjacent tests
would avoid a future reader wondering why they differ.

We should use compiling out if test is essentially Linux only.
We should use ignore if feature is coming from wasi's limitation and it can be supported in the future.

We can mix them.

I recommend to not compile tests using strace to avoid some side-effect for wasmtime.

In this particular case, they are both Linux specific tests that should not be compiled to WASI. Therefore, we should set to #[cfg(not(wasi_runner))].

Switched test_cp_sparse_never_empty to #[cfg(not(wasi_runner))] so the two match. The other 30 --sparse tests in this file still use the ignore; I can switch them in a follow-up PR if you'd like.

sylvestre · 2026-10-17T12:21:00Z

+                {
+                    while n > 0 {
+                        n -= splice(&pipe_rd, dest, n)
+                            .map_err(|error| PipeError::Write(error.into()))?;


now that the cache is shared, a default-size pipe made here for a small n can be reused by splice_unbounded_auto, which expects 1 MiB, no?
is that fine?

Sorry. I was not able to understand your question.

We can use both of pipe size or size we want to send for splice's length. So n is fine.
If extending pipe size failed, but 1 MiB was passed, that's also fine as splice stops at default pipe size.

If you are considering to omit next fcntl syscall for pipe when n is small, I don't think we need to do that because dropping pipe is not a hot code path.

It works either way: each splice into the empty pipe moves at most what fits and drain_pipe writes exactly that, so a 64 KiB pipe only means smaller chunks (only tail uses both functions). Still, the cached pipe's size shouldn't depend on which function happened to make it, so send_n_bytes now makes its pipe with pipe::<false>() too and the cached pipe always gets the 1 MiB resize.

cat, cp, install, mv, head, tail and tac move data through pipes that are reused for more than one copy. When writing to the destination failed part-way, for example with ENOSPC or EFBIG, the bytes still in the pipe stayed there and the next copy wrote them at the start of its own destination. With cp, a file that failed to copy put the rest of its data into the next file copied. Keep a cached pipe only when the copy through it succeeded, so one that a failed write left data in is dropped and the next copy makes a new one.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing. Review the Copilot code review billing settings.

oech3 · 2026-10-18T00:54:36Z

        let _ = fcntl_setpipe_size(&target, pipe_size);
    }
-    let Some((broker_r, broker_w)) = PIPE_CACHE.get_or_init(|| {
-        // use std::io::pipe to avoid unnecessary fcntl


Cannot we keep this optimization by any way? This is still important if input is just 1 file.

Done in #15516.

sylvestre · 2026-10-18T08:03:46Z

Thanks for your PR

uutils#15086 had send_n_bytes size every new pipe at 1 MiB, adding an fcntl to small copies like head -c 10. Only resize when the copy is larger than the default pipe size, as before.

Copilot AI balanced review requested due to automatic review settings October 5, 2026 10:16

Copilot AI reviewed Oct 5, 2026

This comment was marked as resolved.

Sign in to view

oech3 reviewed Oct 5, 2026

View reviewed changes

oech3 suggested changes Oct 5, 2026

View reviewed changes

oech3 reviewed Oct 5, 2026

View reviewed changes

This comment was marked as outdated.

Sign in to view

xtqqczze reviewed Oct 5, 2026

View reviewed changes

This comment was marked as outdated.

Sign in to view

oech3 reviewed Oct 5, 2026

View reviewed changes

abendrothj force-pushed the pipes-discard-leftover branch from cf614c3 to 87ceeff Compare October 5, 2026 21:11

Copilot AI balanced review requested due to automatic review settings October 5, 2026 21:11

Copilot AI reviewed Oct 5, 2026

oech3 reviewed Oct 5, 2026

View reviewed changes

abendrothj force-pushed the pipes-discard-leftover branch from 87ceeff to 55a4ca3 Compare October 5, 2026 21:26

Copilot AI balanced review requested due to automatic review settings October 5, 2026 21:26

Copilot AI reviewed Oct 5, 2026

abendrothj force-pushed the pipes-discard-leftover branch from 55a4ca3 to 7053f9b Compare October 5, 2026 22:14

Copilot AI balanced review requested due to automatic review settings October 5, 2026 22:14

abendrothj changed the title ~~uucore: drop leftover pipe data after a failed splice write~~ uucore: drop the pipe after a failed splice write Oct 5, 2026

Copilot AI reviewed Oct 5, 2026

oech3 reviewed Oct 5, 2026

View reviewed changes

This comment was marked as resolved.

Sign in to view

Copilot AI balanced review requested due to automatic review settings October 6, 2026 07:15

abendrothj force-pushed the pipes-discard-leftover branch from 054e5ef to 6f612c0 Compare October 8, 2026 01:18

Copilot AI reviewed Oct 8, 2026

Copilot AI balanced review requested due to automatic review settings October 10, 2026 04:26

abendrothj force-pushed the pipes-discard-leftover branch from 6f612c0 to 5f22581 Compare October 10, 2026 04:26

Copilot AI reviewed Oct 10, 2026

xtqqczze reviewed Oct 10, 2026

View reviewed changes

oech3 reviewed Oct 11, 2026

View reviewed changes

abendrothj force-pushed the pipes-discard-leftover branch from 5f22581 to 1a35ff9 Compare October 11, 2026 06:26

Copilot AI balanced review requested due to automatic review settings October 11, 2026 06:26

Copilot AI reviewed Oct 11, 2026

Copilot AI balanced review requested due to automatic review settings October 12, 2026 10:31

abendrothj force-pushed the pipes-discard-leftover branch from 1a35ff9 to ec9a55a Compare October 12, 2026 10:31

Copilot AI reviewed Oct 12, 2026

Copilot AI balanced review requested due to automatic review settings October 16, 2026 08:01

abendrothj force-pushed the pipes-discard-leftover branch from ec9a55a to 8e5344f Compare October 16, 2026 08:01

Copilot AI reviewed Oct 16, 2026

eduardomourar suggested changes Oct 17, 2026

View reviewed changes

sylvestre reviewed Oct 17, 2026

View reviewed changes

Copilot AI balanced review requested due to automatic review settings October 18, 2026 00:50

abendrothj force-pushed the pipes-discard-leftover branch from 8e5344f to d65be17 Compare October 18, 2026 00:50

Copilot AI reviewed Oct 18, 2026

oech3 reviewed Oct 18, 2026

View reviewed changes

sylvestre merged commit 6ba6658 into uutils:main Oct 18, 2026
190 checks passed

abendrothj mentioned this pull request Oct 19, 2026

uucore: skip the pipe resize for small send_n_bytes copies again #15516

Open

	let Ok(pipe) = PIPE_CACHE.take().map_or_else(pipe::<false>, Ok) else {
	let Ok(&(pipe_rd, pipe_wr)) = PIPE_CACHE.take().map_or_else(pipe::<false>, Ok) else {

Conversation

abendrothj commented Oct 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

This comment was marked as outdated.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xtqqczze Oct 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codspeed Bot commented Oct 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging this PR will not alter performance

Footnotes

Uh oh!

This comment was marked as outdated.

xtqqczze Oct 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

abendrothj commented Oct 5, 2026 •

edited

Loading

xtqqczze Oct 5, 2026 •

edited

Loading

codspeed Bot commented Oct 5, 2026 •

edited

Loading

xtqqczze Oct 5, 2026 •

edited

Loading