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.

Sign up for GitHub

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

Fix handling of malicious Readers in read_to_end #80895

Merged
merged 4 commits into from
Jan 14, 2021
Merged

Fix handling of malicious Readers in read_to_end #80895

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ebe402d
Fix handling of malicious Readers in read_to_end
sfackler Jan 11, 2021
a9ef798
clean up control flow
sfackler Jan 11, 2021
5cb8303
make check a bit more clear
sfackler Jan 11, 2021
e6c07b0
clarify docs a bit
sfackler Jan 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 9 additions & 11 deletions library/std/src/io/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -366,7 +366,6 @@ where
{
let start_len = buf.len();
let mut g = Guard { len: buf.len(), buf };
let ret;
loop {
if g.len == g.buf.len() {
unsafe {
Expand All @@ -386,20 +385,19 @@ where
}

match r.read(&mut g.buf[g.len..]) {
Ok(0) => {
ret = Ok(g.len - start_len);
break;
Ok(0) => return Ok(g.len - start_len),
Ok(n) => {
// We can't let g.len overflow which would result in the vec shrinking when the function returns. In
// particular, that could break read_to_string if the shortened buffer doesn't end on a UTF-8 boundary.
// The minimal check would just be a checked_add, but this assert is a bit more precise and should be
// just about the same cost.
assert!(n <= g.buf.len() - g.len);
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to slice the slice that's been passed into the read call in the first place and then take its length? As in:

let buffer = &mut g.buf[g.len..];
match r.read(buffer) { 
    // ...
    Ok(n) => g.len += buffer[..n].len(),
}

or something along those lines? It has an added benefit of also detecting when returned n is greater than the length of the length of the buffer provided, and may result in slightly less code generated overall.

Copy link
Member Author

Choose a reason for hiding this comment

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

This check already is looking for n being greater than the length of the buffer, but agreed that structuring it like that is a bit more clear. Will update later today.

g.len += n;
}
Ok(n) => g.len += n,
Err(ref e) if e.kind() == ErrorKind::Interrupted => {}
Err(e) => {
ret = Err(e);
break;
}
Err(e) => return Err(e),
}
}

ret
}

pub(crate) fn default_read_vectored<F>(read: F, bufs: &mut [IoSliceMut<'_>]) -> Result<usize>
Expand Down