Undefined Behavior when T is zero-sized

[TyPR124]

stowaway/src/lib.rs

Lines 212 to 217 in fb1d8d1

	pub fn new(value: T) -> Self {
	let storage = match Self::size_class() {
	SizeClass::Zero => {
	mem::forget(value);
	ptr::null_mut()
	}

stowaway/src/lib.rs

Line 283 in fb1d8d1

SizeClass::Zero => unsafe { ptr::read(storage) },

Per ptr::read: Note that even if T has size 0, the pointer must be non-NULL and properly aligned.

https://doc.rust-lang.org/std/ptr/fn.read.html

[TyPR124]

I also want to point out that this is very much NOT fine, as ALL references must be non-null.

stowaway/src/lib.rs

Lines 537 to 540 in fb1d8d1

	fn as_ref(&self) -> &T {
	let ptr_to_storage = match Self::size_class() {
	// In the ZST case, ptr::read is a no-op, so the null ptr here is fine
	SizeClass::Zero => self.storage,

stowaway/src/lib.rs

Line 547 in fb1d8d1

unsafe { &*ptr_to_storage }

[Lucretiel]

I also want to point out that this is very much NOT fine, as ALL references must be non-null.

Are you certain this is the case? This contradicts the nomicon:

So if the allocator API doesn't support zero-sized allocations, what on earth do we store as our allocation? Unique::empty() of course! Almost every operation with a ZST is a no-op since ZSTs have exactly one value, and therefore no state needs to be considered to store or load them. This actually extends to ptr::read and ptr::write: they won't actually look at the pointer at all. As such we never need to change the pointer.

[Lucretiel]

Opened rust-lang/nomicon#198

[TyPR124]

Not at a computer, but I'm pretty sure if you look into the Unique::empty implementation, it actually does create a properly aligned pointer. Feel free to correct me if I'm wrong, but that is what I remember from looking at it myself in the past.

…

On Fri, Feb 7, 2020, 13:11 Nathan West ***@***.***> wrote: I also want to point out that this is very much NOT fine, as ALL references must be non-null. I'm fixing this anyway, but this contradicts the nomicon <https://doc.rust-lang.org/nomicon/vec-zsts.html#allocating-zero-sized-types> : So if the allocator API doesn't support zero-sized allocations, what on earth do we store as our allocation? Unique::empty() of course! Almost every operation with a ZST is a no-op since ZSTs have exactly one value, and therefore no state needs to be considered to store or load them. This actually extends to ptr::read and ptr::write: they won't actually look at the pointer at all. As such we never need to change the pointer. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#5?email_source=notifications&email_token=AHDX4NSEZEJJ5WARXTU5KBTRBWP4BA5CNFSM4KRKXAE2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELEARNY#issuecomment-583534775>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHDX4NWBEGZUPN63JUHXHQ3RBWP4BANCNFSM4KRKXAEQ> .

[TyPR124]

https://github.com/rust-lang/rust/blob/fb29dfcc9ada6ed10308b6e7f405569f61a9af0b/src/libcore/ptr/unique.rs#L76

…

On Fri, Feb 7, 2020, 13:11 Nathan West ***@***.***> wrote: I also want to point out that this is very much NOT fine, as ALL references must be non-null. I'm fixing this anyway, but this contradicts the nomicon <https://doc.rust-lang.org/nomicon/vec-zsts.html#allocating-zero-sized-types> : So if the allocator API doesn't support zero-sized allocations, what on earth do we store as our allocation? Unique::empty() of course! Almost every operation with a ZST is a no-op since ZSTs have exactly one value, and therefore no state needs to be considered to store or load them. This actually extends to ptr::read and ptr::write: they won't actually look at the pointer at all. As such we never need to change the pointer. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#5?email_source=notifications&email_token=AHDX4NSEZEJJ5WARXTU5KBTRBWP4BA5CNFSM4KRKXAE2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELEARNY#issuecomment-583534775>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHDX4NWBEGZUPN63JUHXHQ3RBWP4BANCNFSM4KRKXAEQ> .

[TyPR124]

Also from the nomicon https://doc.rust-lang.org/nomicon/exotic-sizes.html Note that references to ZSTs (including empty slices), just like all other references, must be non-null and suitably aligned. Dereferencing a null or unaligned pointer to a ZST is undefined behavior <https://doc.rust-lang.org/nomicon/what-unsafe-does.html>, just like for any other type.

…

On Fri, Feb 7, 2020, 13:26 Nathan West ***@***.***> wrote: Opened rust-lang/nomicon#198 <rust-lang/nomicon#198> — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#5?email_source=notifications&email_token=AHDX4NWPCZC3DJDAIE4K3LLRBWRXDA5CNFSM4KRKXAE2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELECHIQ#issuecomment-583541666>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHDX4NTJL2FFNQQSMNFSBODRBWRXDANCNFSM4KRKXAEQ> .

[TyPR124]

Here is an assembly comparison between Box and Stowaway using mem::align_of::<T>() for ZSTs, per discussion in #4.

https://rust.godbolt.org/z/cFsRij