From 892cba5458464f1a86205fb8af7a9d3846ae75cd Mon Sep 17 00:00:00 2001 From: Jack Wrenn Date: Wed, 26 Jun 2024 16:57:47 -0400 Subject: [PATCH] Use `copy_nonoverlapping` instead of `slice::copy_from` (#1448) In doing so, we eliminate a potential panic path. Although I was not able to observe panic paths emitted in toy examples, they might be emitted in complex examples in which the optimizer is low on gas. Regardless, I expect this change will ease our future adoption of call-graph analysis techniques of potential panic paths. Ref https://github.com/google/zerocopy/issues/200#issuecomment-2181544061 --- src/lib.rs | 60 ++++++++++++++++++++++++++++++++++------------------- src/util.rs | 23 ++++++++++++++++++++ 2 files changed, 62 insertions(+), 21 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 52357ab5d7..5c72f5676a 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -3934,9 +3934,9 @@ pub unsafe trait IntoBytes { unsafe { slice::from_raw_parts_mut(slf.cast::(), len) } } - /// Writes a copy of `self` to `bytes`. + /// Writes a copy of `self` to `dst`. /// - /// If `bytes.len() != size_of_val(self)`, `write_to` returns `Err`. + /// If `dst.len() != size_of_val(self)`, `write_to` returns `Err`. /// /// # Examples /// @@ -3982,21 +3982,27 @@ pub unsafe trait IntoBytes { /// ``` #[must_use = "callers should check the return value to see if the operation succeeded"] #[inline] - fn write_to(&self, bytes: &mut [u8]) -> Result<(), SizeError<&Self, &mut [u8]>> + fn write_to(&self, dst: &mut [u8]) -> Result<(), SizeError<&Self, &mut [u8]>> where Self: Immutable, { - if bytes.len() != mem::size_of_val(self) { - return Err(SizeError::new(self)); + let src = self.as_bytes(); + if dst.len() == src.len() { + // SAFETY: Within this branch of the conditional, we have ensured + // that `dst.len()` is equal to `src.len()`. Neither the size of the + // source nor the size of the destination change between the above + // size check and the invocation of `copy_unchecked`. + unsafe { util::copy_unchecked(src, dst) } + Ok(()) + } else { + Err(SizeError::new(self)) } - bytes.copy_from_slice(self.as_bytes()); - Ok(()) } - /// Writes a copy of `self` to the prefix of `bytes`. + /// Writes a copy of `self` to the prefix of `dst`. /// /// `write_to_prefix` writes `self` to the first `size_of_val(self)` bytes - /// of `bytes`. If `bytes.len() < size_of_val(self)`, it returns `Err`. + /// of `dst`. If `dst.len() < size_of_val(self)`, it returns `Err`. /// /// # Examples /// @@ -4042,24 +4048,29 @@ pub unsafe trait IntoBytes { /// ``` #[must_use = "callers should check the return value to see if the operation succeeded"] #[inline] - fn write_to_prefix(&self, bytes: &mut [u8]) -> Result<(), SizeError<&Self, &mut [u8]>> + fn write_to_prefix(&self, dst: &mut [u8]) -> Result<(), SizeError<&Self, &mut [u8]>> where Self: Immutable, { - let size = mem::size_of_val(self); - match bytes.get_mut(..size) { - Some(bytes) => { - bytes.copy_from_slice(self.as_bytes()); + let src = self.as_bytes(); + match dst.get_mut(..src.len()) { + Some(dst) => { + // SAFETY: Within this branch of the `match`, we have ensured + // through fallible subslicing that `dst.len()` is equal to + // `src.len()`. Neither the size of the source nor the size of + // the destination change between the above subslicing operation + // and the invocation of `copy_unchecked`. + unsafe { util::copy_unchecked(src, dst) } Ok(()) } None => Err(SizeError::new(self)), } } - /// Writes a copy of `self` to the suffix of `bytes`. + /// Writes a copy of `self` to the suffix of `dst`. /// /// `write_to_suffix` writes `self` to the last `size_of_val(self)` bytes of - /// `bytes`. If `bytes.len() < size_of_val(self)`, it returns `Err`. + /// `dst`. If `dst.len() < size_of_val(self)`, it returns `Err`. /// /// # Examples /// @@ -4112,17 +4123,18 @@ pub unsafe trait IntoBytes { /// ``` #[must_use = "callers should check the return value to see if the operation succeeded"] #[inline] - fn write_to_suffix(&self, bytes: &mut [u8]) -> Result<(), SizeError<&Self, &mut [u8]>> + fn write_to_suffix(&self, dst: &mut [u8]) -> Result<(), SizeError<&Self, &mut [u8]>> where Self: Immutable, { - let start = if let Some(start) = bytes.len().checked_sub(mem::size_of_val(self)) { + let src = self.as_bytes(); + let start = if let Some(start) = dst.len().checked_sub(src.len()) { start } else { return Err(SizeError::new(self)); }; - let bytes = if let Some(bytes) = bytes.get_mut(start..) { - bytes + let dst = if let Some(dst) = dst.get_mut(start..) { + dst } else { // get_mut() should never return None here. We return a `SizeError` // rather than .unwrap() because in the event the branch is not @@ -4130,7 +4142,13 @@ pub unsafe trait IntoBytes { // than panicking. return Err(SizeError::new(self)); }; - bytes.copy_from_slice(self.as_bytes()); + // SAFETY: Through fallible subslicing of `dst`, we have ensured that + // `dst.len()` is equal to `src.len()`. Neither the size of the source + // nor the size of the destination change between the above subslicing + // operation and the invocation of `copy_unchecked`. + unsafe { + util::copy_unchecked(src, dst); + } Ok(()) } diff --git a/src/util.rs b/src/util.rs index 2f069fc872..0b5188857f 100644 --- a/src/util.rs +++ b/src/util.rs @@ -610,6 +610,29 @@ pub(crate) const fn min(a: NonZeroUsize, b: NonZeroUsize) -> NonZeroUsize { } } +/// Copies `src` into the prefix of `dst`. +/// +/// # Safety +/// +/// The caller guarantees that `src.len() <= dst.len()`. +#[inline(always)] +pub(crate) unsafe fn copy_unchecked(src: &[u8], dst: &mut [u8]) { + debug_assert!(src.len() <= dst.len()); + // SAFETY: This invocation satisfies the safety contract of + // copy_nonoverlapping [1]: + // - `src.as_ptr()` is trivially valid for reads of `src.len()` bytes + // - `dst.as_ptr()` is valid for writes of `src.len()` bytes, because the + // caller has promised that `src.len() <= dst.len()` + // - `src` and `dst` are, trivially, properly aligned + // - the region of memory beginning at `src` with a size of `src.len()` + // bytes does not overlap with the region of memory beginning at `dst` + // with the same size, because `dst` is derived from an exclusive + // reference. + unsafe { + core::ptr::copy_nonoverlapping(src.as_ptr(), dst.as_mut_ptr(), src.len()); + }; +} + /// Since we support multiple versions of Rust, there are often features which /// have been stabilized in the most recent stable release which do not yet /// exist (stably) on our MSRV. This module provides polyfills for those