Add missing safety requirement

[yilin0518]

Objective

bevy_ptr exposes several unsafe fns that hand out a plain &T (or &mut T) derived from memory that is, or was, wrapped in an UnsafeCell. Their # Safety sections document only part of what's actually required for soundness, omitting that the referenced memory must not be accessed through a raw pointer for the duration of the reference's lifetime. A caller who does nothing more than satisfy the contract as literally written can still produce undefined behavior.

This affects two independent call sites:

1. ThinSlicePtr::get_unchecked

/// # Safety
///
/// `index` must be in-bounds.
pub unsafe fn get_unchecked(&self, index: usize) -> &'a T { ... }

The only stated precondition is bounds-checking. ThinSlicePtr<UnsafeCell<T>> also exposes a fully safe cast() that returns a ThinSlicePtr<T> by discarding the UnsafeCell from the pointee's type via UnsafeCell::raw_get. Chaining cast() (safe) with get_unchecked() (unsafe, contract satisfied by bounds-checking alone) yields a &T while the original UnsafeCell handle is still around and can be legally written to — nothing in get_unchecked's contract forbids it.

I reproduced this under Miri in a standalone crate (path dependency on bevy_ptr, no modification to this crate needed to trigger it):

use bevy_ptr::ThinSlicePtr;
use core::cell::UnsafeCell;

fn main() {
    let data = [UnsafeCell::new(1i32), UnsafeCell::new(2i32)];
    let cell_ptr: ThinSlicePtr<UnsafeCell<i32>> = ThinSlicePtr::from(&data[..]);
    let plain_ptr: ThinSlicePtr<i32> = cell_ptr.cast();          // safe
    let shared_ref: &i32 = unsafe { plain_ptr.get_unchecked(0) }; // contract: index in-bounds
    unsafe { *data[0].get() = 42; }                               // legal UnsafeCell write
    println!("{shared_ref}");                                     // UB
}

error: Undefined Behavior: trying to retag from <470> for SharedReadOnly permission at alloc181[0x0], but that tag does not exist in the borrow stack for this location
    --> C:\Users\14798\.rustup\toolchains\nightly-x86_64-pc-windows-gnu\lib\rustlib\src\rust\library\core\src\fmt\mod.rs:2872:71
     |
2872 |             fn fmt(&self, f: &mut Formatter<'_>) -> Result { $tr::fmt(&**self, f) }
     |                                                                       ^^^^^^^ this error occurs as part of retag at alloc181[0x0..0x4]
...
2882 | fmt_refs! { Debug, Display, Octal, Binary, LowerHex, UpperHex, LowerExp, UpperExp }
     | ----------------------------------------------------------------------------------- in this macro invocation
     |
     = 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: <470> was created by a SharedReadOnly retag at offsets [0x0..0x4]
    --> src\main.rs:28:37
     |
  28 |     let shared_ref: &i32 = unsafe { plain_ptr.get_unchecked(0) };
     |                                     ^^^^^^^^^^^^^^^^^^^^^^^^^^
help: <470> was later invalidated at offsets [0x0..0x4] by a write access
    --> src\main.rs:35:9
     |
  35 |         *data[0].get() = 42;
     |         ^^^^^^^^^^^^^^^^^^^
error: aborting due to 1 previous error

2. UnsafeCellDeref::deref and UnsafeCellDeref::deref_mut

/// # Safety
/// - The returned value must be unique and not alias any mutable or immutable references to the contents of the [`UnsafeCell`].
/// - At all times, you must avoid data races. ...
unsafe fn deref_mut(self) -> &'a mut T;

/// # Safety
/// - For the lifetime `'a` of the returned value you must not construct a mutable reference to the contents of the [`UnsafeCell`].
/// - At all times, you must avoid data races. ...
unsafe fn deref(self) -> &'a T;

Both contracts are phrased in terms of references. deref's wording forbids only constructing a &mut T; deref_mut's wording forbids only aliasing references. Neither says anything about a raw pointer obtained from a different alias of the same UnsafeCell (e.g. via UnsafeCell::get/ raw_get) — which is not a "reference" under either wording, and is the ordinary, intended way to use an UnsafeCell. A caller can satisfy both contracts to the letter while still causing UB.

For deref — a raw-pointer write through a different alias, never
constructing a &mut T:

use bevy_ptr::UnsafeCellDeref;
use core::cell::UnsafeCell;

fn main() {
    let cell = UnsafeCell::new(1i32);
    let a: &UnsafeCell<i32> = &cell;
    let b: &UnsafeCell<i32> = &cell;

    let shared: &i32 = unsafe { a.deref() };   // contract: never construct a `&mut`
    unsafe { *b.get() = 42; }                  // legal UnsafeCell write via a different alias
    println!("{shared}");                      // UB
}

error: Undefined Behavior: trying to retag from <441> for SharedReadOnly permission
       at alloc180[0x0], but that tag does not exist in the borrow stack for this location
help: <441> was created by a SharedReadOnly retag at offsets [0x0..0x4]
    --> examples\deref.rs:19:33
     |  let shared: &i32 = unsafe { a.deref() };
help: <441> was later invalidated at offsets [0x0..0x4] by a write access
    --> examples\deref.rs:25:9
     |  *b.get() = 42;

For deref_mut the same gap exists but is sharper: a mutable reference forbids any other access — not just writes, reads too — yet the wording only mentions "references", so a raw-pointer read through a different alias also violates the real contract while complying with the stated one:

use bevy_ptr::UnsafeCellDeref;
use core::cell::UnsafeCell;

fn main() {
    let cell = UnsafeCell::new(1i32);
    let a: &UnsafeCell<i32> = &cell;
    let b: &UnsafeCell<i32> = &cell;

    let exclusive: &mut i32 = unsafe { a.deref_mut() }; // contract: no aliasing *references*
    *exclusive = 10;
    let snooped = unsafe { *b.get() };                  // not a reference, just a raw-pointer read
    *exclusive += 1;                                     // UB
}

error: Undefined Behavior: attempting a read access using <444> at alloc180[0x0],
       but that tag does not exist in the borrow stack for this location
  --> examples\deref_mut.rs:26:5
   |  *exclusive += 1;
help: <444> was created by a Unique retag at offsets [0x0..0x4]
  --> examples\deref_mut.rs:18:40
   |  let exclusive: &mut i32 = unsafe { a.deref_mut() };
help: <444> was later invalidated at offsets [0x0..0x4] by a read access
  --> examples\deref_mut.rs:22:28
   |  let snooped = unsafe { *b.get() };

Note the invalidating event here is a read, not a write. The read at *b.get() is a plain raw-pointer access — it does not construct any reference — but it still counts as "access... through another pointer" under Rust's aliasing rule for the mutable reference exclusive already holds, and that rule is what the old wording ("not alias any... references") failed to make explicit:

• Shared reference: "while this reference exists, the memory it points to must not get mutated (except inside UnsafeCell)."
• Mutable reference: "while this reference exists, the memory it points to must not get accessed (read or written) through any other pointer or reference not derived from this reference."

Both are "public unsafe contract gap" issues: not a currently-exploited bug in this crate's own callers, but an incomplete # Safety section that a compliant downstream caller could follow to undefined behavior.

Solution

Complete the # Safety documentation on all three already-unsafe fns so they state the actual precondition Rust's aliasing rules require, not just "don't construct a &mut" / "don't alias a reference":

• ThinSlicePtr::get_unchecked: added the missing no-concurrent-mutation clause, explicitly calling out that this also covers writes through an UnsafeCell the pointer was cast() from.
• ThinSlicePtr<UnsafeCell<T>>::cast(): added an explanatory (non-# Safety, since the function itself does no dereference) doc note connecting it to the contract above, so a reader doesn't have to piece the two together.
• UnsafeCellDeref::deref: replaced "must not construct a mutable reference" with the full "must not be mutated by any means, including a raw-pointer write" wording.
• UnsafeCellDeref::deref_mut: replaced "must be unique and not alias any mutable or immutable references" with wording that also covers raw-pointer reads and writes, matching the "no access at all" requirement of a real mutable reference.

This is a documentation-only change.

Testing

• cargo check -p bevy_ptr — passes.
• All three pocs prove trigger UB.

[hymm]

The UB comes from violating the safety of raw pointers, not these methods.
in the first example:
i.e. unsafe { *data[0].get() = 42; }
This is getting a raw pointer and then dereferencing it to a &mut T. Which you've now aliased the same memory location.
See https://doc.rust-lang.org/std/ptr/index.html
Specifically:

You must enforce Rust’s aliasing rules. The exact aliasing rules are not decided yet, so we only give a rough overview here. The rules also depend on whether a mutable or a shared reference is being created.

• When creating a mutable reference, then while this reference exists, the memory it points to must not get accessed (read or written) through any other pointer or reference not derived from this reference.
• When creating a shared reference, then while this reference exists, the memory it points to must not get mutated (except inside UnsafeCell).

[yilin0518]

Thanks for your opinion!

The UB comes from violating the safety of raw pointers, not these methods. in the first example: i.e. unsafe { *data[0].get() = 42; } This is getting a raw pointer and then dereferencing it to a &mut T. Which you've now aliased the same memory location.

Unsafecell::get directly return a *mut T. Because the type of data is Vec<UnsafeCell<i32>>, we can use the internal mutability to change the internal value. But I think the missing part of original documentation is: It should remind the user that there may exist the UnsafeCell<T> reference and then change the internal value, but this behaviour should be considered UB. The above pocs are what I want to prove that only have the original requirement is not enough.

I suspect the PoCs I provide might contain some intentionally triggered UB, and I'm not sure if it's appropiate to add these extra information. Looking forward to your suggestion.