Fixes UB in stack::generator: Aliasing violations and references to uninit memory.

[oliver-giersch]

While looking through the code I noticed some issues with unsound usage of MaybeUninit as well as one aliasing violation. Instead of opening an issue I decided to try and fix it myself. I have not looked at the the other modules, so there may be similar issues there.

Issues

genawaiter/src/stack/generator.rs

Lines 92 to 93 in 3e9652a

	// Safety: Build the struct in place, by writing each field in place.
	let p = &mut *shelf.0.as_mut_ptr() as *mut State<Y, R, F>;

Here as_mut_ptr returns a *mut T to uninitialized memory. De-referencing it is UB, which is also unnecessary, since it is turned back into a raw pointer anyways in the same line.
Lines 96 and 99 cause UB in a similar manner, i.e., by de-referencing a pointer to uninitialized memory.

genawaiter/src/stack/generator.rs

Lines 88 to 99 in 3e9652a

	pub unsafe fn new(
	shelf: &'s mut Shelf<Y, R, F>,
	producer: impl FnOnce(Co<'s, Y, R>) -> F,
	) -> Self {
	// Safety: Build the struct in place, by writing each field in place.
	let p = &mut *shelf.0.as_mut_ptr() as *mut State<Y, R, F>;
	let airlock = Airlock::default();
	ptr::write(&mut (*p).airlock, airlock);
	let future = producer(Co::new(&(*p).airlock));
	ptr::write(&mut (*p).future, future);

The aliasing issue is more subtle: Gen::new receives a mutable reference to a Shelf and stores it within itself as a Pin<&mut _>, however, the future that is created by producer receives a shared reference to the Airlock, which it may store within itself. This is a bit weird, since it is a reference into the same struct, but I believe it constitutes a violation of the aliasing rules, because there exists at the same time a (pinned) mutable reference to the Shelf and a shared reference to the Airlock within the same Shelf.

Changes

• the Airlock is initialized regularly, without the use of MaybeUninit
• the future is initialized correctly through a raw pointer write
• the shelf borrow is split into a mutable and pinned borrow of future and a shared borrow of airlock, which means the aliasing rules are maintained (I think)

Discussion

I am sure, if Gen::new has to be an unsafe function. I assume that it would be possible to violate memory safety if it were possible to find a way to get the producer closure to move the Co instance out and in between the Shelf and the Gen like so:

let mut shelf = Shelf::new();
let mut outer_co = None;
let gen = Gen::new(&mut shelf, |co| {
    outer_co = Some(co);
    // ...
});
// ... complete generator, then use outer_co somehow

I have not found a way to get something like this to compile however, so maybe Gen::new could be declared as safe?

EDIT:
Having read through the other PR about making Co::yield_ take &mut self I've seen it is indeed possible to let the co instance escape, but since airlock is now initialized regularly and only dropped alongside the shelf, I think it is safe to let it escape the closure and try to yield stuff from it after the the generator is complete, the code will simply panic without any violations of memory safety.

[whatisaphone]

Thanks for the PR! It's great to have more eyes on the unsafe code.

The first issue is definitely technically UB – though for now it's generally accepted as the best way to express the specific case of &x.field as *const _ until RFC 2582 (&raw) is implemented. But you found a way to avoid that dark corner of the language entirely, which is great!

The second one I'm not so sure is UB. It's perfectly fine to have aliasing shared references pointing inside a mutable reference (example). The airlock field isn't mutably borrowed at all anywhere (until after future is dropped), so the mutable aliasing rules don't ever come into effect.

I'm happy to accept this PR, since either way it's a nice simplification of the code. I think you are right about Gen::new no longer being unsafe with these changes, and that is a huge win!

I resolved the merge conflict and rebased your changes onto master. Thanks for contributing!

[oliver-giersch]

Thanks for merging and apologies for not resolving the conflict myself, but I could not figure out what the issue was, since I actually had run rustfmt on everything.

I was also not totally certain if the way the borrow of airlock and future is handled constituted UB or not, but it seemed that way. This example is a closer analogy to what is going on with the two borrows and I was surprised that it actually compiled, so perhaps you are right.