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

Access to unitialized data in read_from #33

Open
mpdn opened this issue Jan 16, 2024 · 1 comment
Open

Access to unitialized data in read_from #33

mpdn opened this issue Jan 16, 2024 · 1 comment

Comments

@mpdn
Copy link

mpdn commented Jan 16, 2024

Hi. Nice library, thanks for putting effort into making this.

I think I've discovered an issue allowing use of uninitialized data from safe code in the read_from function:

/// Reads at most `count` bytes from `Read` instance and appends them to the ring buffer.
/// If `count` is `None` then as much as possible bytes will be read.
///
/// Returns:
/// + `None`: ring buffer is empty or `count` is `0`. In this case `read` isn't called at all.
/// + `Some(Ok(n))`: `read` succeeded. `n` is number of bytes been read. `n == 0` means that `read` also returned `0`.
/// + `Some(Err(e))` `read` is failed and `e` is original error. In this case it is guaranteed that no items was read from the reader.
/// To achieve this we read only one contiguous slice at once. So this call may read less than `vacant_len` items in the buffer even if the reader is ready to provide more.
fn read_from<S: Read>(&mut self, reader: &mut S, count: Option<usize>) -> Option<io::Result<usize>>
where
Self: Producer<Item = u8>,
{
let (left, _) = self.vacant_slices_mut();
let count = cmp::min(count.unwrap_or(left.len()), left.len());
if count == 0 {
return None;
}
let left_init = unsafe { slice_assume_init_mut(&mut left[..count]) };
let read_count = match reader.read(left_init) {
Ok(n) => n,
Err(e) => return Some(Err(e)),
};
assert!(read_count <= count);
unsafe { self.advance_write_index(read_count) };
Some(Ok(read_count))
}
}

The issue is that a (safe) implementation of Read can read the uninitialized data in the buffer. This test:

#[test]
fn test_read_touching_buf() {

    struct Reader;

    impl Read for Reader {
        fn read(&mut self, buf: &mut [u8]) -> io::Result<usize> {
            assert!(buf[0] == 0);
            Ok(0)
        }
    }

    let (mut producer, _consumer) = HeapRb::<u8>::new(10).split();
    producer.read_from(&mut Reader, None).unwrap();
}

Triggers this error when executing in Miri:

test foo ... error: Undefined Behavior: using uninitialized data, but this operation requires initialized memory
  --> src/main.rs:73:21
   |
73 |             assert!(buf[0] == 0);
   |                     ^^^^^^ using uninitialized data, but this operation requires initialized memory
   |
   = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
   = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
@agerasev
Copy link
Owner

agerasev commented Jan 17, 2024

Hi!
Thank you for finding and reporting such a subtle issue!

I've made a fast fix by initializing destination memory before calling read. But this could result in a huge overhead when, for example, reader reads just a single byte, but the whole buffer needs to be initialized every time.

Waiting for Read::read_buf stabilization :)

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

2 participants