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 regress 1 benchmark

⚡ 10 improved benchmarks
❌ 1 regressed benchmark
✅ 388 untouched benchmarks
⏩ 54 skipped benchmarks¹

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

	Mode	Benchmark	`BASE`	`HEAD`	Efficiency
❌	Simulation	`three_39_bit_primes`	371.3 ms	452.5 ms	-17.93%
⚡	Simulation	`split_lines`	11.1 ms	8.5 ms	+29.92%
⚡	Simulation	`split_numeric_suffix`	11.3 ms	8.7 ms	+29.22%
⚡	Simulation	`join_french_locale`	26.7 ms	24 ms	+11.08%
⚡	Simulation	`join_full_match`	26.7 ms	24 ms	+11.05%
⚡	Simulation	`five_38_bit_primes`	1.7 s	1.6 s	+8.77%
⚡	Simulation	`join_partial_overlap`	22.1 ms	20.4 ms	+8.35%
⚡	Simulation	`join_unicode_locale`	2.6 ms	2.4 ms	+6.64%
⚡	Simulation	`join_custom_separator`	25.9 ms	24.4 ms	+6.09%
⚡	Simulation	`unexpand_large_file[10]`	286.3 ms	274.8 ms	+4.18%
⚡	Simulation	`unexpand_many_lines[100000]`	136.9 ms	131.4 ms	+4.17%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.

_{Comparing abendrothj:pipes-discard-leftover (054e5ef) with main (8cf2e4f)}

54 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.

abendrothj · 2026-10-06T07:32:16Z

The tee change is now #15132, and the test helper and dd change are in #15131. #15132 and this PR are both based on #15131. What's left here is pipes.rs and the cp test.

I tried cat but kept cp. cat has one stdout, and once a write to it fails with EFBIG or ENOSPC every later write fails too, so the leftover bytes never show up: main and this branch give the same exit code, stderr, size and md5 for cat big small, cat big big and cat small big small into a 100 KiB-capped file. It only shows with a temporary error (EAGAIN on a non-blocking stdout, then a fifo), and that needs a fifo, O_NONBLOCK and sleeps, which is more timing-dependent than the cp test. With cp the next destination is a separate file, so the leftover is easy to see.

abendrothj · 2026-10-06T09:20:50Z

Sign in to view

+    thread_local! {
+        static PIPE_CACHE: Cell<Option<(PipeReader, PipeWriter)>> = const { Cell::new(None) };
+    }
+    let Some(pipe) = PIPE_CACHE.take().or_else(|| pipe::<false>().ok()) else {


Yes, changed.

Copilot

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

abendrothj · 2026-10-06T09:40:55Z

Sign in to view

-    static PIPE_CACHE: OnceLock<Option<(PipeReader, PipeWriter)>> = OnceLock::new();
-    let Some((pipe_rd, pipe_wr)) = PIPE_CACHE.get_or_init(|| pipe::<false>().ok()) else {
+    thread_local! {
+        static PIPE_CACHE: Cell<Option<(PipeReader, PipeWriter)>> = const { Cell::new(None) };


Yes, moved it out so both use it.

Copilot

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

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.

oech3 · 2026-10-07T00:23:27Z

+#[cfg_attr(
+    wasi_runner,
+    ignore = "WASI: the file size limit also applies to wasmtime, which then fails to start"
+)]


Suggested change

#[cfg_attr(

wasi_runner,

ignore = "WASI: the file size limit also applies to wasmtime, which then fails to start"

)]

#[cfg(not(wasi_runner))] // linux specific

oech3 · 2026-10-07T00:28:13Z

+    /// An empty pipe kept for the next copy. A copy takes it and puts it back only
+    /// when it succeeded, since a failed write can leave bytes in it.


Suggested change

/// An empty pipe kept for the next copy. A copy takes it and puts it back only

/// when it succeeded, since a failed write can leave bytes in it.

/// Cache empty pipe pair to avoid calling `pipe2` at each copy.

Document usefullness of cache itself. Warning about non-enpty pipes are already done at plate it is used.

oech3 · 2026-10-07T00:30:56Z

        return Ok(Err(()));
    };
    // GNU cat catches all strace injections for 2nd+ splice
+    // if this fails part-way, the rest stays in the pipe, so callers drop the pipe on error


Suggested change

// if this fails part-way, the rest stays in the pipe, so callers drop the pipe on error

Doc of fn itself might enough. We actually have L75's read_to_end too.

oech3 · 2026-10-07T00:32:41Z

+    res
+}

+fn splice_unbounded_with(


This fn is used just once. How about removing it?

oech3 · 2026-10-07T00:33:31Z

+    res
+}
+
+fn send_n_bytes_with(


This fn is used just once. How about removing it?

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 7053f9b to dadbe4d Compare October 6, 2026 07:15

Copilot AI reviewed Oct 6, 2026

This was referenced Oct 6, 2026

tests: dd: ignore SIGXFSZ in the child instead of the test process #15131

Merged

tee: replace the pipe a failed output used #15132

Merged

oech3 reviewed Oct 6, 2026

View reviewed changes

Copilot AI balanced review requested due to automatic review settings October 6, 2026 09:20

Copilot AI reviewed Oct 6, 2026

oech3 reviewed Oct 6, 2026

View reviewed changes

Copilot AI balanced review requested due to automatic review settings October 6, 2026 09:40

Copilot AI reviewed Oct 6, 2026

abendrothj added 3 commits October 6, 2026 15:27

uucore: take the cached pipe as a Result

e92a533

uucore: share one pipe cache between the pipe copies

054e5ef

Copilot AI balanced review requested due to automatic review settings October 6, 2026 23:47

abendrothj force-pushed the pipes-discard-leftover branch from cff190a to 054e5ef Compare October 6, 2026 23:47

Copilot AI reviewed Oct 6, 2026

oech3 reviewed Oct 7, 2026

View reviewed changes

		/// An empty pipe kept for the next copy. A copy takes it and puts it back only
		/// when it succeeded, since a failed write can leave bytes in it.

	/// An empty pipe kept for the next copy. A copy takes it and puts it back only
	/// when it succeeded, since a failed write can leave bytes in it.
	/// Cache empty pipe pair to avoid calling `pipe2` at each copy.

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 regress 1 benchmark

Performance Changes

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!

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

abendrothj commented Oct 6, 2026 •

edited

Loading