UB in PagedVec::read_range_generic

[gio256]

Summary

The following tests both trigger undefined behavior in openvm_circuit::system::memory::PagedVec::read_range_generic.

#[test]
fn test_ub_clone_from() {
    let v = PagedVec::<Vec<u8>, 4>::new(2);
    let _ = v.range_vec(0..2);
}

#[test]
fn test_ub_drop() {
    #[derive(Default, Clone)]
    struct Byte(u8);

    impl Drop for Byte {
        fn drop(&mut self) {
            let _uninit = self.0;
        }
    }

    let v = PagedVec::<Byte, 4>::new(2);
    let _ = v.range_vec(0..2);
}

This can be seen by running with Miri.

cargo +nightly miri test -p openvm-circuit --no-default-features system::memory::paged_vec::tests::test_ub_drop

Discussion

When called from PagedVec::range_vec, read_range_generic attempts to fill an uninitialized Vec with data from the PagedVec via a raw pointer. If the requested page is not initialized, the method instead fills the Vec with the result of T::default(). The first instance of this occurs here.

std::slice::from_raw_parts_mut(dst, len).fill(T::default());

The safety docs for std::slice::from_raw_parts_mut state that dst: *mut T must point to len consecutive properly initialized values of type T. It is not clear from the docs whether this is a validity invariant or a safety invariant.

While it is technically UB to construct a reference to uninitialized data (see, e.g., the std::ptr::addr_of docs), the rules here are not yet finalized (unsafe-code-guidelines#412). Therefore, creating a reference to uninitialized data using std::slice::from_raw_parts_mut is unlikely to result in miscompilation on its own, and I would assume the same for the typed copy needed to pass this reference to <[T]>::fill.

However, any "use" of this uninitialized data is clearly UB. As shown in the tests above, there are two cases that result in a use of the data in the destination slice within <[T]>::fill:

• T implements Drop.
• <T as Clone>::clone_from reads from self (e.g. Vec::clone_from).

Assuming that the generic parameter T in PagedVec<T, PAGE_SIZE> is typically a field element, and considering that p3_field::Field: Copy, both of these cases seem unlikely to occur in practice.

Fix

I believe the best fix here is to use MaybeUninit::fill. Unfortunately, this method is unstable, so it may be necessary to reimplement it using the approach shown here.

[jonathanpwang]

We are likely going to rewrite large parts of Memory and thus PagedVec, so will take this into account during the rewrite. Thanks for filing!