Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
saethlin opened this issue May 24, 2023 · 6 comments · Fixed by #117
Closed

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

saethlin opened this issue May 24, 2023 · 6 comments · Fixed by #117
Assignees
Labels
bug Something isn't working high priority High priority
Milestone

Comments

@saethlin
Copy link

saethlin commented May 24, 2023

This code executes UB:

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
Copy link
Owner

Alexhuszagh commented Sep 9, 2024

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() };

@Alexhuszagh Alexhuszagh added this to the 1.0 milestone Sep 9, 2024
@workingjubilee
Copy link

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
Copy link
Owner

Alexhuszagh commented Sep 9, 2024

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
Copy link

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
Copy link
Owner

Alexhuszagh commented Sep 9, 2024

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 added a commit that referenced this issue Sep 9, 2024
This used `::assume_init` which invoked undefined behavior.

- Closes #95
Alexhuszagh added a commit that referenced this issue Sep 9, 2024
This used `::assume_init` which invoked undefined behavior.

- Closes #95
@Alexhuszagh
Copy link
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working high priority High priority
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants