Incorrect use of assume_init

[RalfJung]

I noticed by coincidence some incorrect uses of assume_init in this crate:

generator-rs/src/yield_.rs

Line 32 in 9f45261

unsafe { ret.assume_init() }

generator-rs/src/stack/unix.rs

Line 97 in cd677f7

let mut limit = unsafe { limit.assume_init() };

Quoting from the assume_init docs (emphasis mine):

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

It does not matter whether the value is dropped, or never used, or anything like that. Calling assume_init when the data is not yet initialized is a safety violation in all cases.

getrlimit is easy to fix, it should use MaybeUninit::as_mut_ptr. The other function looks very suspicious but I do not understand enough of what is going on to suggest a fix.

(The other two uses of assume_init in this crate also look suspicious, but I can't tell where the values are coming from -- they sure look like they have not actually been properly initialized yet, though.)

[Xudong-Huang]

Thanks for point out! I refine the code.
I believe it's correct and safe for this line

generator-rs/src/yield_.rs

Line 32 in 9f45261

unsafe { ret.assume_init() }

[RalfJung]

I believe it's correct and safe for this line

That depends on the T this function will be used for. If T can ever be something like bool, this is definitely wrong. assume_init must only be called after the value has been fully initialized, and this function does not actually seem to do anything to initialize its value -- so I can't quite see how this can be correct. The fact that this value will not be dropped is irrelevant here, just creating an uninitialized bool is already an error. If you work with uninitialized data you have to make this explicit in the types, e.g. by returning MaybeUninit<bool>.

And it seems like this function is publicly exposed through a macro, so a user could use this in a way that it returns a bool, right?

It is correct when T is (), but if that is the intent then MaybeUninit could be avoided entirely and the function should only allow T as return value.

[RalfJung]

In fact given that T is user-controlled, the user could put ! as the type (once never_type is stabilized), which can very easily lead to miscompilations. A function that returns ! must not return, and yet done::<!>() would return.

[Xudong-Huang]

The done macro is just an indicator within the generator to force it to finish. It's dummy value would never return to user. you can ref the logic here
https://github.com/Xudong-Huang/generator-rs/blob/master/src/gen_impl.rs#L348-L353

For the ! type, as you point out the return instruction would never happen, so it's ok. Actually you can panic within a generator just like panic in a function.

[RalfJung]

It doesn't matter if the value is ever returned to the user. What matters is that the value is even constructed.

So the only way this is okay is if that code is never executed. In that case, I suggest you remove the unsafe code and just replace it by abort() or panic!(). If that does not work, then the code clearly is executed, and in that case the assume_init is wrong.

---

The following code is wrong:

fn done_and_forget<T>() {
  let ret = std::mem::MaybeUninit::uninit();
  mem::forget::<T>(unsafe { ret.assume_init() });
}

I think this roughly corresponds to what your code is doing, and hence, that code is also wrong. It is clearly documented as wrong in the documentation of assume_init:

Calling this when the content is not yet fully initialized causes immediate undefined behavior.

"immediate" means exactly that -- immediate. There is no exception for "the next thing we do is forget that value", at that point, it is already too late.

You can also see this by running this example code: it panics, because it executes a wrong assume_init.
Or by running this code in Miri (select "Tools - Miri").

[Xudong-Huang]

I see, thanks for the example!

[Xudong-Huang]

I think this could be closed