Closed RalfJung closed 2 years ago
Thanks for point out! I refine the code. I believe it's correct and safe for this line https://github.com/Xudong-Huang/generator-rs/blob/9f45261a075f0ecac8928e40fb37ca650319bd46/src/yield_.rs#L32
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.
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.
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.
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").
I see, thanks for the example!
I think this could be closed
I noticed by coincidence some incorrect uses of assume_init in this crate:
https://github.com/Xudong-Huang/generator-rs/blob/9f45261a075f0ecac8928e40fb37ca650319bd46/src/yield_.rs#L32
https://github.com/Xudong-Huang/generator-rs/blob/cd677f7d13696e840d312f5e3658ded2de2fb4bb/src/stack/unix.rs#L97
Quoting from the
assume_init
docs (emphasis mine):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 useMaybeUninit::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.)