Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

io: extend Buf length only after having read into it #7054

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

paolobarbolini
Copy link
Contributor

@paolobarbolini paolobarbolini commented Dec 29, 2024

Motivation

I've discovered that Buf::ensure_capacity_for is unsound because it uses Vec::set_len to grow the Vec past the init area. This violates the documented safety invariants of Vec::set_len, which say The elements at old_len..new_len must be initialized.

This can be tested very easily by adding for &byte in &self.buf {} after the self.buf.set_len call.

Solution

Have reads use the spare capacity of buf and Vec::set_len afterwards.

@paolobarbolini paolobarbolini force-pushed the unsound-ensure-capacity-for branch from e64152a to a2a4b8a Compare December 29, 2024 17:48
@Darksonn
Copy link
Contributor

Please add a test that would catch the unsoundness in question if run under miri.

@Darksonn Darksonn added A-tokio Area: The main tokio crate M-io Module: tokio/io labels Dec 29, 2024
@paolobarbolini paolobarbolini force-pushed the unsound-ensure-capacity-for branch 3 times, most recently from 5759994 to 064afef Compare December 29, 2024 19:03
@Darksonn
Copy link
Contributor

I don't think this qualifies as a security issue, because Blocking is only used internally with stdlib types that don't read from the buffer passed to read. It's a similar situation to this:

// Safety: The caller must ensure that `E` can read into uninitialized memory
pub(crate) unsafe fn poll_read<'a>(

Still, I agree that the particular implementation is not the greatest. We should avoid calling set_len until after actually filling the data with read. The constructor of Buf should also unsafely assert that the IO resource doesn't read from the slice in read.

@paolobarbolini paolobarbolini force-pushed the unsound-ensure-capacity-for branch from 064afef to 8ec1726 Compare January 2, 2025 09:55
@paolobarbolini paolobarbolini changed the title io: fix unsound Buf::ensure_capacity_for io: extend via Vec::set_len only after having read into it Jan 2, 2025
Copy link
Contributor Author

@paolobarbolini paolobarbolini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not too happy about the current &mut [MaybeUninit<u8>] -> &mut [u8] hacks but given that everything else also relies on it I've changed it here too

tokio/src/io/blocking.rs Outdated Show resolved Hide resolved
@paolobarbolini paolobarbolini force-pushed the unsound-ensure-capacity-for branch from 8ec1726 to c95f8a6 Compare January 2, 2025 10:08
@paolobarbolini paolobarbolini changed the title io: extend via Vec::set_len only after having read into it io: extend Buf length only after having read into it Jan 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate M-io Module: tokio/io
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants