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

API Soundness issue in fill_buf() and read_up_to() #1

Open
Qwaz opened this issue Dec 27, 2020 · 0 comments
Open

API Soundness issue in fill_buf() and read_up_to() #1

Qwaz opened this issue Dec 27, 2020 · 0 comments

Comments

@Qwaz
Copy link

Qwaz commented Dec 27, 2020

Hello fellow Rustacean,
we (Rust group @sslab-gatech) found a memory-safety/soundness issue in this crate while scanning Rust code on crates.io for potential vulnerabilities.

Issue Description

acc_reader/src/lib.rs

Lines 244 to 271 in 95a54aa

impl<R: Read> BufRead for AccReader<R> {
fn fill_buf(&mut self) -> io::Result<&[u8]> {
let available = self.buf.len() - self.pos; // self.buf.len() >= pos
if available == 0 {
let old_len = self.buf.len();
self.buf.reserve(self.inc);
unsafe { self.buf.set_len(old_len + self.inc); }
let (read, error) = match self.source.read(&mut self.buf[self.pos..]) {
Ok(n) => (n, None),
Err(e) => (0, Some(e)),
};
unsafe { self.buf.set_len(old_len + read); }
if let Some(e) = error {
Err(e)
} else {
Ok(&self.buf[self.pos..])
}
} else {
Ok(&self.buf[self.pos..])
}
}
fn consume(&mut self, amt: usize) {
self.pos = cmp::min(self.pos + amt, self.buf.len());
}
}

acc_reader/src/lib.rs

Lines 190 to 219 in 95a54aa

// Read from the stream into the internal buffer as much as possible,
// but no more than the provided number of bytes.
// Updates the buffer length to the actual number of bytes read, even
// in case of errors.
fn read_up_to(&mut self, n: u64) -> io::Result<()> {
let old_len = self.buf.len();
self.buf.reserve(n as usize);
unsafe { self.buf.set_len(old_len + n as usize); }
let mut error = None;
let mut read = 0;
{
let mut target = &mut self.buf[old_len..];
while !target.is_empty() {
match self.source.read(target) {
Ok(0) => break,
Ok(n) => { read += n; let tmp = target; target = &mut tmp[n..]; }
Err(ref e) if e.kind() == io::ErrorKind::Interrupted => {}
Err(e) => { error = Some(e); break; },
}
}
}
unsafe { self.buf.set_len(old_len + read as usize); }
if let Some(e) = error {
Err(e)
} else {
Ok(())
}
}

fill_buf() and read_up_to() methods create an uninitialized buffer and pass it to user-provided Read implementation. This is unsound, because it allows safe Rust code to exhibit an undefined behavior (read from uninitialized memory).

This part from the Read trait documentation explains the issue:

It is your responsibility to make sure that buf is initialized before calling read. Calling read with an uninitialized buf (of the kind one obtains via MaybeUninit<T>) is not safe, and can lead to undefined behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant