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

Fix unsoundness in iterator Clone impl (crudely) #120

Merged
merged 1 commit into from Jan 4, 2022

Conversation

saethlin
Copy link
Contributor

@saethlin saethlin commented Jan 4, 2022

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.

We need something to put into the contents of the new GenericArray to
back the iterator upon clone. The perfect solution would be to change
the element type to MaybeUninit, but that invades the trait bounds too
much. This solution is somewhat disconcerting, but unlike the previous
implementation's use of assume_init(), I cannot get miri to reject it.
@saethlin saethlin changed the title Fully initialize new iterator upon clone (crudely) Fix unsoundness in iterator Clone impl (crudely) Jan 4, 2022
@novacrazy
Copy link
Collaborator

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 novacrazy merged commit 1e5b9b0 into fizyk20:master Jan 4, 2022
@novacrazy
Copy link
Collaborator

Published in 0.14.5

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

Successfully merging this pull request may close these issues.

None yet

2 participants