peek-poke is unsound

[nox]

Method peek_from_uninit creates a &mut T where the T is uninitialised, this is UB AFAIK.

webrender/peek-poke/src/lib.rs

Lines 33 to 39 in efec276

	/// Peek helper for constructing a `T` by `Copy`ing into an uninitialized stack
	/// allocation.
	pub unsafe fn peek_from_uninit<T: Copy + Peek>(bytes: *const u8) -> (T, *const u8) {
	let mut val = MaybeUninitShim { uninit: () };
	let bytes = <T>::peek_from(bytes, &mut val.init);
	(val.init, bytes)
	}

[nox]

@SimonSapin says "I think the lang team is not decided yet whether &mut T is UB, when T is valid for any bit pattern", though.

[nox]

Cc @RalfJung

[RalfJung]

Right, so this is UB right now to keep our options open.

However, for the specific case of uninhabited T, we likely do want to actually make this UB "for real" as that would help greatly with dead-code removal. Does/could ! implement Peek? If yes, this could would be UB and risk miscompilation in future compiler versions. So Peek might have to be an unsafe trait for this to be sound.

See rust-lang/unsafe-code-guidelines#77 for the UCG discussion on this. If you think it is impossible to do what you are doing here when &mut T always requires a fully valid T, that would be an interesting data point for us.

[jrmuizel]

Thoughts @Gankra?

[Gankra]

Yeah it's "Documentation UB" in that it's reserved just to be conservative but it's pretty obvious this pattern needs to be doable somehow, so it's only UB insofar as the lang team has failed to design a mechanism that they're happy with for expressing this pattern.

Which is to say, this code is fine today. I maintain the docs that say this is UB, and I reviewed the code, so this code was accepted with full awareness of the situation. Ralf is also aware of my stance on this problem from the discussion we've had on the aforementionned docs.

Some day, the lang team might either:

• Decide it's fine (no changes needed)
• Design a new thing to express this pattern (like &raw or offsetof) and then we can update the code to use that.

Note that I made peek_from only work with *mut T so this is literally the &raw/offsetof usecase (transiently creating an &mut simply because we lack another way to compute an address).

On impls:

We control all the users, but it's ~impractical to keep all devs who work on firefox aware of this situation, so we don't generally have tight control, in the fullness of time. That said, there's no point in implementing this for !. Anyone who implemented it so that they could deserialize SomeEnum<!> or whatever (very improbable) is probably introducing a blatant security hole in our IPC anyway.

[RalfJung]

Note that I made peek_from only work with *mut T so this is literally the &raw/offsetof usecase (transiently creating an &mut simply because we lack another way to compute an address).

Ah, that wasn't clear. (Sometimes type inference isn't helping... like when reading a snippet of 3 lines of code without having any context.^^)

But then, why doesn't this work?

     let mut val = MaybeUninit::uninit(); 
     let bytes = <T>::peek_from(bytes, val.as_mut_ptr()); 
     (val.assume_init(), bytes) 

[Gankra]

That's just the same code hidden behind a function call :)

[RalfJung]

Yes and no. Code in libstd is special and may make assumptions that code outside libstd may not.

But also, for both of them, there's actually no reason to wait for &raw: You could just do

 pub unsafe fn peek_from_uninit<T: Copy + Peek>(bytes: *const u8) -> (T, *const u8) { 
     let mut val = MaybeUninitShim { uninit: () }; 
     let bytes = <T>::peek_from(bytes, &mut val as *mut _ as *mut T); 
     (val.init, bytes) 
 } 

Note that this relies on MaybeUninitShim being #[repr(C)] (I do not know if it is).

[Gankra]

The code predates MaybeUninit's stabilization, so yeah we could reasonably update to use it so we can "forget" about the issue and have it solved for us later.

Just not a big deal in my mind.