fizyk20 / generic-array

Generic array types in Rust
MIT License
404 stars 77 forks source link

Fix unsoundness in iterator Clone impl (crudely) #120

Closed saethlin closed 2 years ago

saethlin commented 2 years ago

This example program is rejected by Miri, no MIRIFLAGS required:

fn main() {
    let mut it = generic_array::arr![&'static i32; &1].into_iter();
    it.next();
    it.clone();
}
error: Undefined Behavior: type validation failed at .value.data.data: encountered uninitialized reference
   --> /home/ben/.cargo/registry/src/github.com-1ecc6299db9ec823/generic-array-0.14.4/src/iter.rs:111:42
    |
111 |                 array: ManuallyDrop::new(array.assume_init()),
    |                                          ^^^^^^^^^^^^^^^^^^^ type validation failed at .value.data.data: encountered uninitialized reference

This is a problem for all types, it just requires no additional flags to Miri to trip the bug with a reference. The documentation for MaybeUninit says:

It is up to the caller to guarantee that the MaybeUninit<T> really is in an initialized state. Calling this when the content is not yet fully initialized causes immediate undefined behavior.

The problem is that the copy loop may not initialize all the contents of array before .assume_init() is called on it. In this example, none of the array is initialized.

The theoretical proper fix is to keep the array elements as MaybeUninit<T>, but as far as I can tell that conflicts with the type bounds, and I can't find a way to resolve that. Therefore I've replaced the MaybeUninit dance with just a bit-copy of the old array, then an element-wise clone of the elements we actually intend to copy. The ManuallyDrop wrapper prevents this from turning into a double-drop.

I'm not excited about this implementation, but I can't come up with anything else that compiles and is accepted by Miri.

novacrazy commented 2 years ago

Looks good to me. ptr::read keeps array within a ManuallyDrop, so there are no risks for double-drops if .clone() panics. There are technically duplicate items in the array, but that's better than being uninitialized, and existing logic prevents double-drops there.

novacrazy commented 2 years ago

Published in 0.14.5