[BUG] Safety comments for MaybeUninit::assume_init calls are wrong, calls are UB

[saethlin]

This code executes UB:

rust-lexical/lexical-write-float/src/radix.rs

Lines 70 to 72 in 09c686b

	let buffer: mem::MaybeUninit<[u8; SIZE]> = mem::MaybeUninit::uninit();
	// SAFETY: safe, since we never read bytes that weren't written.
	let mut buffer = unsafe { buffer.assume_init() };

The docs for MaybeUninit::uninit do not have an exception for this use case. This code is UB, because the MaybeUninit is not initialized.

The safety comment is also technically wrong; the value is read by the assignment and return from MaybeUninit::assume_init.

This problem is reliably reported by running cargo +nightly miri test --all-features.

---

The existing MaybeUninit APIs are not exactly elegant, but I think they can be slotted in pretty neatly with the existing abstractions you have here. I can take a shot at fixing this in the coming days/weeks.

[Alexhuszagh]

I'll prioritize this later today. I did not realize this was the case. I'll likely add in miri checks into our CI pipeline. I'll see if migrating it to zeroed affects anything. I was (incorrectly) under the assumption ptr.write() was defined behavior, such as shown in the examples but didn't realize the assignment affected that.

use std::mem::MaybeUninit;

// Create an explicitly uninitialized reference. The compiler knows that data inside
// a `MaybeUninit<T>` may be invalid, and hence this is not UB:
let mut x = MaybeUninit::<&i32>::uninit();
// Set it to a valid value.
x.write(&0);
// Extract the initialized data -- this is only allowed *after* properly
// initializing `x`!
let x = unsafe { x.assume_init() };

[workingjubilee]

I was (incorrectly) under the assumption ptr.write() was defined behavior

What do you mean by "ptr.write() was defined behavior"?
Did you mean you assumed that uninit.assume_init() and ptr.write() were allowed to be ordering-independent?

[Alexhuszagh]

I was (incorrectly) under the assumption ptr.write() was defined behavior

What do you mean by "ptr.write() was defined behavior"? Did you mean you assumed that uninit.assume_init() and ptr.write() were allowed to be ordering-independent?

Correct, that's my mistake. I'm doing a comprehensive analysis of all unsafe behavior within lexical and will be adding much more comprehensive testing and ensure safety guarantees are more stringent, removing more unsafe behavior, and ensuring that code quality is up to scratch for the next major release. I'll probably be removing all MaybeUninit and explicitly 0-assigning it.

I apologize for this and my late response.

[workingjubilee]

It is fine if you write first.

Mostly, we are currently debating how we can improve our MaybeUninit::assume_init docs to make this clearer.

[Alexhuszagh]

I greatly appreciate that, it makes sense on a closer inspection but I made some major mistakes here in my initial assessment. That would be a huge help I'm sure to many additional developers.

I've done some benchmarks and found I can zero-initialize with comparable performance so I'll be removing the use of MaybeUninit in these locations (significantly faster than dtoa, identical performance to ryu in most cases, and significantly faster in some edge cases).

[Alexhuszagh]

I'll be rushing out a major release by the end of the week, and I'll be slowly ensuring more safety invariants are upheld and yanking previous versions as required.

Also, on another note: I highly appreciate how helpful both of you have been in explaining this to me. I've added Miri workflows to our pipelines so any time unsafe code is modified we can ensure safety with this and fuzzing. I know this has been a major issue and I'm glad to have had some help in this process.