Ensure the read_to_end buffer always has enough room to fit a single UTF-8 code point

[ChrisDenton]

This is a quick fix to resolve #142847. So long as the buffer we read into has space for at least one UTF-8 code point then we can avoid any issues caused by splitting between code point boundaries (thus avoiding a lot of complexity). I think this is also good in general as it avoids some unnecessarily short reads.

[rustbot]

r? @ibraheemdev

rustbot has assigned @ibraheemdev.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[a1phyr]

I think that this the condition could even be replaced by an unconditional call to try_reserve(PROBE_SIZE).

[the8472]

That might lead to some unnecessary reallocations around some tipping-points, see #89165 (also mentioned in the function's top comment). It's tricky to get these tradeoffs right.

Maybe we can limit this to cfg(windows)? Specialization could work too but feels a bit overkill here.

[the8472]

Also, please add the issue to the list of masters this function is serving. Anyone who wants to touch it will have to keep track of all these things.

[the8472]

Since default_read_to_end is a crate-private we could also add a "min read capacity" option which can then be passed down from the stdin impl

[ChrisDenton]

My reason for thinking this is more general useful is that currently if a read is just short of the available buffer then the next read will have a buffer size of one or two bytes. If we've not reached the end then this is wasteful.

Another alternative is to use small_read_probe, which at least is better than using a one byte buffer or so.

[the8472]

The user may have passed a buffer or size hint with exactly the right size and block-sizes just happen to align so that the last read will be small. In that case it might lead to a huge reallocation just to read the last few bytes.

[ChrisDenton]

Right but reading into a stack buffer would side-step that issue. We could also unconditionally realloc if we've already reallocated at least once.

[the8472]

yeah, that could work.

[ChrisDenton]

Ok I'd appreciate someone checking my logic here but I've changed it so that if we've not reallocated yet and there's less than PROBE_SIZE bytes in the spare capacity then we keep using the stack buffer until either we reach the end or we need to reallocate. Otherwise we just reserve the space directly in the Vec.

[the8472]

I can't think of any case where this would be problematic. The use of the stack buffer can trigger OOMs (while the main loop will turn those into errors), but the extra looping just happens in a window where it also could have happened before anyway.

For a fix this is fine, though I think this impl is growing in ugliness and could use some cleanup and more tests but that can be done separately.

[the8472]

Hrrmm, actually... isn't Stdin buffered? Shouldn't that buffer prevent that tearing? I don't see how read_to_end bypasses that.

[ChrisDenton]

StdinRaw has an implementation of read_to_end and BufReader will defer to that once its drained its own buffer.

rust/library/std/src/io/buffered/bufreader.rs

Lines 408 to 410 in 22be76b

	// The inner reader might have an optimized `read_to_end`. Drain our buffer and then
	// delegate to the inner implementation.
	fn read_to_end(&mut self, buf: &mut Vec<u8>) -> io::Result<usize> {

[a1phyr]

I thought that it would be a good idea too, but small_probe_read is designed to check for EOF so taking this branch if we still have space in the vector does not seem right.

[ChrisDenton]

It will be a tight loop until either EOF or it exceeds the spare space. It'll only ever get past that point once a reallocation is required.

[a1phyr]

Having thought a bit about that, your solution avoids an issue with small reads if the vector was preallocated.

[a1phyr]

This version looks great! Thanks!

Side note: I think that these lines are no longer relevant, now that default_read_to_end uses the unstable read_buf API.

rust/library/std/src/io/mod.rs

Lines 404 to 405 in ae2fc97

	// - avoid passing large buffers to readers that always initialize the free capacity if they perform short reads (#23815, #23820)
	// - pass large buffers to readers that do not initialize the spare capacity. this can amortize per-call overheads

[the8472]

Not all readers implement readbuf, the default impl will still initialize the spare capacity.