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 2 benchmarks

⚠️

120 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

cksum_default

cksum_multiple_files

dd_copy_partial

du_all_wide_tree[(5000, 500)]

df_deep_directory

cksum_raw_output

cksum_crc

cksum_crc32b

du_deep_tree[(100, 3)]

wc_bytes_synthetic[1]

…and 110 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]

⚠️

Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

⚡ 1 improved benchmark
❌ 2 regressed benchmarks
✅ 398 untouched benchmarks
⏩ 54 skipped benchmarks¹

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

	Mode	Benchmark	`BASE`	`HEAD`	Efficiency
❌	Simulation	`five_38_bit_primes`	1.6 s	1.7 s	-8.22%
❌	Simulation	`thirteen_39_bit_primes`	9.1 s	9.6 s	-4.62%
⚡	Simulation	`three_39_bit_primes`	583.2 ms	429.8 ms	+35.69%

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 (ec9a55a) with main (6292c03)}

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

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

I'd keep cfg_attr(wasi_runner, ignore = ...), as wasi.yml documents and the rest of test_cp.rs does. It's not about linux (the cfg above handles that): the size limit also applies to wasmtime.

In this case, this should no be compiled since it is strictly linux specific test.

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?

Done, inlined. A labeled block keeps one place that caches the pipe, and errors still return without it.

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

+    res
+}
+
+fn send_n_bytes_with(


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

Done, inlined. It caches the pipe at its two exits.

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)

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.

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

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

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

Copilot AI balanced review requested due to automatic review settings October 8, 2026 01:18

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

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

	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 regress 2 benchmarks

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