Skip to content

Section on uninitialized memory #5

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

Merged
merged 19 commits into from
Mar 12, 2023
Merged

Section on uninitialized memory #5

merged 19 commits into from
Mar 12, 2023

Conversation

Manishearth
Copy link
Collaborator

No description provided.

@Manishearth Manishearth marked this pull request as ready for review March 4, 2023 20:41
@Manishearth
Copy link
Collaborator Author

Ready for review!


unsafe {
// reads from moved-from memory
let ghost = ptr::read(ptr);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't the issue here that ptr is dangling after x is moved, not that the value at ptr is necessarily uninitialized?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like it's both, right? It can be analyzed through the framework of dangling pointers or through the framework of uninit.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is misleading to talk about deallocated and uninitialized memory in the same way. Reads from deallocated memory are insta-UB, whereas reads from uninitialized memory might not be UB if you're reading it as a type such as MaybeUninit<u8>.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I'm going to rewrite this section. I'll keep it in since it's similar to uninitialized memory, so worth thinking about, but not the same.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've reworked this a bit, thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would reword this to start by talking about freed allocations. Then end the section by saying that moving out of a local variable is like deallocating the memory because the compiler might reuse that location on the stack for another local.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm hesitant to do too much here because I suspect that coverage may live better in a separate chapter. I mentioned freed memory a bit, though.

@Manishearth Manishearth requested a review from djkoloski March 5, 2023 05:58

If you explicitly wish to work with uninitialized and partially-initialized types, [`MaybeUninit<T>`] is a useful abstraction since it can be constructed with no overhead and then written to in parts.

## Safely working with uninitialized memory
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not super happy with the flow of the chapter here, I think it's important to talk about this but I'm not sure where it should go. We have spent a lot of time talking about where uninitialized values come from, but not talking about what actually is safe vs unsafe about it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happier with it now but I think "Safely working with uninit" is now not the right name for the section

@Manishearth Manishearth changed the title Start section on uninitialized memory Section on uninitialized memory Mar 5, 2023
@Manishearth Manishearth requested a review from Darksonn March 5, 2023 22:30
Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't yet looked at anything below ## When you might end up making an uninitialized value


unsafe {
// reads from moved-from memory
let ghost = ptr::read(ptr);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would reword this to start by talking about freed allocations. Then end the section by saying that moving out of a local variable is like deallocating the memory because the compiler might reuse that location on the stack for another local.

@Manishearth Manishearth requested a review from djkoloski March 6, 2023 00:53
@Darksonn
Copy link
Contributor

Darksonn commented Mar 6, 2023

Here is some guidance on specific situations that involve uninit memory:

  1. When using Vec::set_len to add elements to a vector, you are supposed to initialize the memory before you call set_len.
  2. Given an &mut [MaybeUninit<u8>] where some of the bytes are already initialized, its possible to deinitialize them. This means that passing the &mut [MaybeUninit<u8>] to unknown user code and expecting those bytes to still be initialized is not sound. This is the origin of things like UninitSlice.
  3. If you use MaybeUninit on a field and then later do let var = var.assume_init(); when you've initialized it can sometimes optimize poorly. In these cases, you can create a &mut T that points into the MaybeUninit and just use the reference directly. However, this requires extra care if T has a destructor.
  4. When mixing MaybeUninit with UnsafeCell, you should do UnsafeCell<MaybeUninit<T>> rather than MaybeUninit<UnsafeCell<T>>.
  5. When going from *mut Struct to *mut Field, it's best practice to use addr_of! or addr_of_mut! for the conversion if the struct is uninitialized.

@djkoloski djkoloski merged commit 4e188e3 into google:main Mar 12, 2023
@Manishearth Manishearth deleted the undef branch March 12, 2023 18:36
@Manishearth
Copy link
Collaborator Author

Probably can incorporate some of @Darksonn's feedback in a separate PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants