UB due to misuse of mem::uninitialized (will soon lead to panic)

[RalfJung]

Here, this crate causes UB by "Producing an invalid value". Concretely, it produces a value of type basic_dsp_vector::DspVec<std::vec::Vec<f32>, f32, basic_dsp_vector::meta::Complex, basic_dsp_vector::meta::Time> with mem::uninitialized() (in the crates.io version) or mem::MaybeUninit::uninit().assume_init() (in master), which contains a Vec<f32> field, which has a data pointer that must always be non-NULL (and in particular may not remain uninitialized). In the near future the call to mem::uninitialized() will panic to avoid UB, and our crater run determined that this crate will be affected.

mem::uninitialized() is deprecated since Rust 1.39. The intended replacement is MaybeUninit, which tracks the possibility of uninitialized values at the type level to make sure the compiler does not make any false assumptions.

I see that this commit contains lots of code like

-            let first = mem::replace(&mut self[0], mem::uninitialized());
-            let second = mem::replace(&mut self[1], mem::uninitialized());
-            let third = mem::replace(&mut self[2], mem::uninitialized());
+            let first = mem::replace(&mut self[0], mem::MaybeUninit::uninit().assume_init());
+            let second = mem::replace(&mut self[1], mem::MaybeUninit::uninit().assume_init());
+            let third = mem::replace(&mut self[2], mem::MaybeUninit::uninit().assume_init());

This silenced the warning on earlier compiler versions, but it does not fix the bug, and newer compiler versions should also detect this code and warn about it. Read the assume_init docs for details about what you as the unsafe code author must ensure before calling that method. Creating an uninitialized NonNull is UB no matter how you go about it. If you want to just read from self[0], why don't you ptr::read(&self[0])? You need to be careful of double-drops, but dropping uninitialized data is also UB so it's not like this can make things any worse.

[liebharc]

Hi, thanks for your detailed report and the solution hints. ptr::read(&self[0]) indeed seems to be the better solution here and the only reason it wasn't used is, that I didn't know about this option. Based on your feedback I've pushed a change to master. As you seem to work on the Rust compiler I don't expect that you will find time to take a look at the change, but I'll keep this issue open for a couple of days in case that you have some further feedback.

As a background what's happening in the code: A matrix is implemented as an array of DSP vectors. A lot of the functionality in the basic_dsp_vector crate transforms a DSP vector. This way it models meta information e.g. if the vector contains time or frequency domain data. E.g. if a Fourier transformation is applied to a time domain vector it will consume this vector and return a new vector in frequency domain. Consumes means that it takes ownership of the argument and returns a new value of a different type but same size (internally most of the argument is reused to avoid unnecessary copy operations). For the matrix crate - and the code you've pointed out - this means that you want to take an array of e.g. two DSP vectors in time domain and return a new array of two DSP vectors in frequency domain.

And an unrelated question to the compiler team: If I take a look at the download chart at crates.io (https://crates.io/crates/basic_dsp) for a long time I've seen a pattern that someone downloads each version of the crate once in regular intervals. It was once mentioned in a post that the compiler team takes crates from crates.io for compiler testing. Are those downloads related to these tests?

Thanks once more for your report and stay healthy!

[RalfJung]

Thanks for the quick fix and response! Unfortunately I couldn't follow most of your DSP-level explanations, but that's okay. :D

I looked at your patch, and it looks like there is no more use of assume_init or uninitialized here now, which is great. :) I saw you added a bunch of mem::forget; please note that we generally recommend people use ManuallyDrop instead of mem::forget as it is easier to use correctly.

And an unrelated question to the compiler team: If I take a look at the download chart at crates.io (https://crates.io/crates/basic_dsp) for a long time I've seen a pattern that someone downloads each version of the crate once in regular intervals. It was once mentioned in a post that the compiler team takes crates from crates.io for compiler testing. Are those downloads related to these tests?

I'm a regular compiler contributor but not part of the compiler team. But it is indeed possible that you are seeing crater (though I think it only tests the latest version); see https://github.com/rust-lang/crater for more information. Or maybe someone is just scraping all of crates.io.