Mark try_from_bytes function as unsafe

[kitcatier]

Hello, I found a soundness issue in this crate.

bytemuck/src/checked.rs

Lines 249 to 259 in b19f8ab

	pub fn try_from_bytes<T: CheckedBitPattern>(
	s: &[u8],
	) -> Result<&T, CheckedCastError> {
	let pod = unsafe { internal::try_from_bytes(s) }?;
	if <T as CheckedBitPattern>::is_valid_bit_pattern(pod) {
	Ok(unsafe { &*(pod as *const <T as CheckedBitPattern>::Bits as *const T) })
	} else {
	Err(CheckedCastError::InvalidBitPattern)
	}
	}

bytemuck/src/internal.rs

Lines 140 to 150 in b19f8ab

	pub(crate) unsafe fn try_from_bytes<T: Copy>(
	s: &[u8],
	) -> Result<&T, PodCastError> {
	if s.len() != size_of::<T>() {
	Err(PodCastError::SizeMismatch)
	} else if (s.as_ptr() as usize) % align_of::<T>() != 0 {
	Err(PodCastError::TargetAlignmentGreaterAndInputNotAligned)
	} else {
	Ok(unsafe { &*(s.as_ptr() as *const T) })
	}
	}

The unsafe function called needs to ensure that the parameter must be :

• If the slice isn't aligned for the new type

• If the slice's length isn’t exactly the size of the new type

and the developer who calls the try_from_bytes function may not notice this safety requirement.
Marking them unsafe also means that callers must make sure they know what they're doing.

[kitcatier]

A similar situation to the above is as follows:

bytemuck/src/lib.rs

Lines 170 to 172 in b19f8ab

	pub fn bytes_of<T: NoUninit>(t: &T) -> &[u8] {
	unsafe { internal::bytes_of(t) }
	}

bytemuck/src/lib.rs

Lines 179 to 181 in b19f8ab

	pub fn bytes_of_mut<T: NoUninit + AnyBitPattern>(t: &mut T) -> &mut [u8] {
	unsafe { internal::bytes_of_mut(t) }
	}

bytemuck/src/lib.rs

Lines 189 to 191 in b19f8ab

	pub fn from_bytes<T: AnyBitPattern>(s: &[u8]) -> &T {
	unsafe { internal::from_bytes(s) }
	}

bytemuck/src/lib.rs

Lines 208 to 212 in b19f8ab

	pub fn try_pod_read_unaligned<T: AnyBitPattern>(
	bytes: &[u8],
	) -> Result<T, PodCastError> {
	unsafe { internal::try_pod_read_unaligned(bytes) }
	}

bytemuck/src/lib.rs

Lines 199 to 201 in b19f8ab

	pub fn from_bytes_mut<T: NoUninit + AnyBitPattern>(s: &mut [u8]) -> &mut T {
	unsafe { internal::from_bytes_mut(s) }
	}

bytemuck/src/lib.rs

Lines 219 to 221 in b19f8ab

	pub fn pod_read_unaligned<T: AnyBitPattern>(bytes: &[u8]) -> T {
	unsafe { internal::pod_read_unaligned(bytes) }
	}

bytemuck/src/lib.rs

Lines 230 to 232 in b19f8ab

	pub fn try_from_bytes<T: AnyBitPattern>(s: &[u8]) -> Result<&T, PodCastError> {
	unsafe { internal::try_from_bytes(s) }
	}

bytemuck/src/lib.rs

Lines 241 to 245 in b19f8ab

	pub fn try_from_bytes_mut<T: NoUninit + AnyBitPattern>(
	s: &mut [u8],
	) -> Result<&mut T, PodCastError> {
	unsafe { internal::try_from_bytes_mut(s) }
	}

bytemuck/src/lib.rs

Lines 253 to 255 in b19f8ab

	pub fn cast<A: NoUninit, B: AnyBitPattern>(a: A) -> B {
	unsafe { internal::cast(a) }
	}

[zachs18]

The unsafe function called needs to ensure that the parameter must be:

• If the slice isn't aligned for the new type
• If the slice's length isn’t exactly the size of the new type

and the developer who calls the try_from_bytes function may not notice this safety requirement

These are listed as "Failure" cases for internal::try_from_bytes

bytemuck/src/internal.rs

Lines 133 to 140 in b19f8ab

	/// Re-interprets `&[u8]` as `&T`.
	///
	/// ## Failure
	///
	/// * If the slice isn't aligned for the new type
	/// * If the slice's length isn’t exactly the size of the new type
	#[inline]
	pub(crate) unsafe fn try_from_bytes<T: Copy>(

I.e. they are situations where the function will return an error (guaranteed). They are not safety preconditions of internal::try_from_bytes, and failing one of them will not cause unsoundness (since it will always be caught).

Note that all of the internal::* functions are pub(crate), meaning they cannot be called from outside bytemuck itself, and are essentially implementation details.

The publicly-exported functions in bytemuck do not need to be marked unsafe fn because they uphold their safety preconditions through their trait bounds; e.g. casting from u8 to bool would be unsound in general, but let x: bool = bytemuck::cast(3u8); does not compile because bool does not implement AnyBitPattern. Thus, it is up to the implementors of AnyBitPattern/NoUninit to ensure soundness (which is valid, since they are unsafe traits, which require unsafe impl to implement).

(Perhaps the documentation for the internal::* functions could be updated to contain their safety preconditions for clarity, and // SAFETY comments could be added to their usages, but this would have no effect on the public API of the crate, so it would not be necessary to mark any publicly-exported functions as unsafe).

I do not believe any of these snippets indicate unsoundness in this crate. Do you have a concrete example of unsoundness caused by this crate (e.g. a program that has undefined behavior because of this crate)?

[kitcatier]

The unsafe function called needs to ensure that the parameter must be:

• If the slice isn't aligned for the new type
• If the slice's length isn’t exactly the size of the new type

and the developer who calls the try_from_bytes function may not notice this safety requirement

These are listed as "Failure" cases for internal::try_from_bytes

bytemuck/src/internal.rs

Lines 133 to 140 in b19f8ab

	/// Re-interprets `&[u8]` as `&T`.
	///
	/// ## Failure
	///
	/// * If the slice isn't aligned for the new type
	/// * If the slice's length isn’t exactly the size of the new type
	#[inline]
	pub(crate) unsafe fn try_from_bytes<T: Copy>(

I.e. they are situations where the function will return an error (guaranteed). They are not safety preconditions of internal::try_from_bytes, and failing one of them will not cause unsoundness (since it will always be caught).
Note that all of the internal::* functions are pub(crate), meaning they cannot be called from outside bytemuck itself, and are essentially implementation details.

The publicly-exported functions in bytemuck do not need to be marked unsafe fn because they uphold their safety preconditions through their trait bounds; e.g. casting from u8 to bool would be unsound in general, but let x: bool = bytemuck::cast(3u8); does not compile because bool does not implement AnyBitPattern. Thus, it is up to the implementors of AnyBitPattern/NoUninit to ensure soundness (which is valid, since they are unsafe traits, which require unsafe impl to implement).

(Perhaps the documentation for the internal::* functions could be updated to contain their safety preconditions for clarity, and // SAFETY comments could be added to their usages, but this would have no effect on the public API of the crate, so it would not be necessary to mark any publicly-exported functions as unsafe).

I do not believe any of these snippets indicate unsoundness in this crate. Do you have a concrete example of unsoundness caused by this crate (e.g. a program that has undefined behavior because of this crate)?

That's right, I see, thanks

[Lokathor]

If you're satisfied we could close the issue, but I'd also be happy if you wanted to send in a PR that better explains the safety invariants of the internal functions.