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

Implement AnyBitPattern for MaybeUninit<T> #152

Closed
wants to merge 1 commit into from

Conversation

zachs18
Copy link
Contributor

@zachs18 zachs18 commented Dec 3, 2022

... where MaybeUninit<T>: Copy + 'static.

Resolves #108 .

A feature flag is required because MaybeUninit was unstable in bytemuck's MSRV. I just used the #[cfg(feature = "zeroable_maybe_uninit")] flag since that's required anyway, and it didn't seem reasonable to add another feature flag for this.

The where MaybeUninit<T>: Copy + 'static bound is required because of AnyBitPattern's supertrait bounds Copy + 'static. A simpler bound would be where T: Copy + 'static (i.e. have the bound on T instead of MaybeUninit<T>). These are currently (and probably always will be) equivalent, but if std ever relaxes the Copy impl of MaybeUninit, this bound will reflect that, whereas where T: Copy + 'static would not.

@zachs18
Copy link
Contributor Author

zachs18 commented Dec 3, 2022

Note about MaybeUninit<InteriorMutableType>: AnyBitPattern requires no interior mutability, but there's no safe way to access the interior mutability with just MaybeUninit, so I'm inclined to say it's fine. The docs for AnyBitPattern say

More precisely: A shared reference to the type must allow reads, and only reads.

This is true of &MaybeUninit<T> when only considering its safe API. fn as_ptr gives you a *const T, which is unsafe to dereference (though once dereferenced could access interior mutability of T).


Edit: Thinking about this more, I'm less sure, but still pretty confident this PR is sound. I don't think interior mutable types that are Sync can ever implement Copy, because Copy implies that bytewise, non-atomic, non-synchronized memcpys are allows from a shared reference, which would race with any possible interior mutability on another thread. I'm less sure if Copy + !Sync can ever have interior mutability, but I still think they cannot (e.g. std::cell::Cell<T> does not implement Copy, even if T: Copy).

@zachs18
Copy link
Contributor Author

zachs18 commented Dec 3, 2022

So it turns out that Cell<T> isn't Copy probably not for soundness reasons, but just as a lint. So this PR is probably unsound as written. Closing for now.

rust-lang/rust#55207

TODO: Maybe where MaybeUninit<T>: Sync + Copy + 'static would be valid, for the reasoning above? Alternately this could be added under a new unsound_maybeuninit_anybitpattern_impl as per unsound_ptr_pod_impl, though IIUC that was only added as a semver exception.

@zachs18 zachs18 closed this Dec 3, 2022
@zachs18
Copy link
Contributor Author

zachs18 commented Dec 3, 2022

This might still be sound if AnyBitPattern's requirements for interior mutability are clarified to only apply to safe code (since accessing the T in a MaybeUninit<T> requires unsafe code), otherwise this is unsound, e.g.

// Assume we are in a hypothetical world where `Cell<T>: Copy where T: Copy`
// https://github.com/zachs18/rust/tree/cell_copy
use std::cell::Cell;
fn main() {
    let a: u32 = 0;
    let b: &MaybeUninit<Cell<u32>> = bytemuck::cast_ref(&a);
    let c: &Cell<u32> = unsafe { b.assume_init_ref() }; // the only unsafe code is here
    c.set(4); // UB: writing to a readonly location
    dbg!(a);
}

Actually, under current miri Stacked Borrows (with library modifications to make Cell<T>: Copy where T: Copy), this errors on the bytemuck::cast_ref, since IIUC shared references to (types containing) UnsafeCell always have SharedReadWrite provenance, but &u32 only has SharedReadOnly provenance.

Miri output on above code
error: Undefined Behavior: trying to retag from <14149> for SharedReadWrite permission at alloc5677[0x0], but that tag only grants SharedReadOnly permission for this location
   --> /home/zachary/.cargo/git/checkouts/bytemuck-b67854ce243a90d2/9cdc314/src/internal.rs:291:17
    |
291 |     Ok(unsafe { &*(a as *const A as *const B) })
    |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |                 |
    |                 trying to retag from <14149> for SharedReadWrite permission at alloc5677[0x0], but that tag only grants SharedReadOnly permission for this location
    |                 this error occurs as part of retag at alloc5677[0x0..0x4]
    |
    = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
    = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
help: <14149> was created by a SharedReadOnly retag at offsets [0x0..0x4]
   --> src/main.rs:43:38
    |
43  |     let b: &MaybeUninit<Cell<u32>> = bytemuck::cast_ref(&a);
    |                                      ^^^^^^^^^^^^^^^^^^^^^^
    = note: BACKTRACE:
    = note: inside `bytemuck::internal::try_cast_ref::<u32, std::mem::MaybeUninit<std::cell::Cell<u32>>>` at /home/zachary/.cargo/git/checkouts/bytemuck-b67854ce243a90d2/9cdc314/src/internal.rs:291:17
    = note: inside `bytemuck::internal::cast_ref::<u32, std::mem::MaybeUninit<std::cell::Cell<u32>>>` at /home/zachary/.cargo/git/checkouts/bytemuck-b67854ce243a90d2/9cdc314/src/internal.rs:215:11
    = note: inside `bytemuck::cast_ref::<u32, std::mem::MaybeUninit<std::cell::Cell<u32>>>` at /home/zachary/.cargo/git/checkouts/bytemuck-b67854ce243a90d2/9cdc314/src/lib.rs:262:12
note: inside `main` at src/main.rs:43:38
   --> src/main.rs:43:38
    |
43  |     let b: &MaybeUninit<Cell<u32>> = bytemuck::cast_ref(&a);
    |                                      ^^^^^^^^^^^^^^^^^^^^^^

note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace

error: aborting due to previous error

@Lokathor
Copy link
Owner

Lokathor commented Dec 3, 2022

Pod itself also assumes Freeze without saying so.

Really, Freeze needs to be a trait that code can talk about directly

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.

Could MaybeUninit<T: Copy> implement AnyBitPattern?
2 participants