vmsplice in yes can lead to errors.

[halyavin]

The utility yes uses vmsplice to write data to the pipe. This leads to Linux kernel having links to buffer memory in user space. Once the utility gets a write error to the pipe, it exits and frees the buffer. This means that buffer memory can be reused for other allocations and so can be changed. If there are kernel links to the buffer memory in user space, reading from such kernel buffer will get a wrong result.

One might think that since pipe is closed, there are no kernel links to the buffer memory anymore. Unfortunately, this is not always the case. If another program uses splice to read yes data from the pipe, it can move the kernel link to the other pipe. Now such program can close the pipe from yes first and cause yes to exit, corrupting inflight data in the second pipe which points to the buffer memory in yes program.

This wouldn't happen if the buffer was allocated with mmap outside of memory manager control. Such memory can be unmapped on function exit preventing any chance of it being corrupted.

[karlmcdowall]

Is it worth the complexity and risk of using the splice APIs for such a simple app?
Should we just update the code to do a simple write to (a buffered) stdout?

[karlmcdowall]

Related discussion? https://lore.kernel.org/lkml/20230629155433.4170837-1-dhowells@redhat.com/

[halyavin]

Is it worth the complexity and risk of using the splice APIs for such a simple app? Should we just update the code to do a simple write to (a buffered) stdout?

I think reliability is more important for such fundamental utilities like yes. It already has the mode which does simple write to stdout.

[halyavin]

No other utility uses vmsplice and so no other utility is affected by this kind of issues.

[tavianator]

It makes sense to mmap() anyway to get a page-aligned buffer

[halyavin]

It makes sense to mmap() anyway to get a page-aligned buffer

This buffer has dynamic size. It is not so easy to mmap.

[tavianator]

The buffer seems to be hardcoded to 16k?

coreutils/src/uu/yes/src/yes.rs

Line 26 in 7eb267d

const BUF_SIZE: usize = 16 * 1024;

[halyavin]

The buffer seems to be hardcoded to 16k?

coreutils/src/uu/yes/src/yes.rs

Line 26 in 7eb267d

const BUF_SIZE: usize = 16 * 1024;

This is only a capacity: https://github.com/uutils/coreutils/blob/7eb267ddcd0d53cf2ece8653cec9b725b095a798/src/uu/yes/src/yes.rs#L32C22-L32C50

[tavianator]

... but then the buffer is filled up to the largest multiple of the line length that fits in the buffer:

coreutils/src/uu/yes/src/yes.rs

Lines 106 to 112 in 7eb267d

	let target_size = line_len * (BUF_SIZE / line_len);
	while buf.len() < target_size {
	let to_copy = std::cmp::min(target_size - buf.len(), buf.len());
	debug_assert_eq!(to_copy % line_len, 0);
	buf.extend_from_within(..to_copy);
	}

[halyavin]

... but then the buffer is filled up to the largest multiple of the line length that fits in the buffer:

coreutils/src/uu/yes/src/yes.rs

Lines 106 to 112 in 7eb267d

let target_size = line_len * (BUF_SIZE / line_len);

while buf.len() < target_size {
let to_copy = std::cmp::min(target_size - buf.len(), buf.len());
debug_assert_eq!(to_copy % line_len, 0);
buf.extend_from_within(..to_copy);
}

The problem is that buffer can become larger than BUF_SIZE. It happens when command line parameters are larger than BUF_SIZE.

[karlmcdowall]

Hey folks, I've posted the changes that I think are required for this here.
I'd appreciate any feedback folks have to let me know if I understood the request properly 👍

[halyavin]

Hey folks, I've posted the changes that I think are required for this here. I'd appreciate any feedback folks have to let me know if I understood the request properly 👍

Yes, you understood it correctly.