Skip to content

Commit

Permalink
[pointer] Fix soundness hole in read_unaligned (#1893)
Browse files Browse the repository at this point in the history 
Makes progress on #1892
  • Loading branch information
@joshlf
joshlf authored Oct 14, 2024
1 parent 2c237a3 commit 0405574
Show file tree
Hide file tree
Showing 6 changed files with 27 additions and 24 deletions.
26 changes: 13 additions & 13 deletions src/impls.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ safety_comment! {
///
/// [4] TODO(#429): Justify this claim.
unsafe_impl!(char: TryFromBytes; |candidate: MaybeAligned<u32>| {
let candidate = candidate.read_unaligned();
let candidate = candidate.read_unaligned::<BecauseImmutable>();
char::from_u32(candidate).is_some()
});
}
Expand Down Expand Up @@ -310,18 +310,18 @@ safety_comment! {
///
/// [2] `NonZeroXxx` self-evidently does not contain `UnsafeCell`s. This is
/// not a proof, but we are accepting this as a known risk per #1358.
unsafe_impl!(NonZeroU8: TryFromBytes; |n: MaybeAligned<u8>| NonZeroU8::new(n.read_unaligned()).is_some());
unsafe_impl!(NonZeroI8: TryFromBytes; |n: MaybeAligned<i8>| NonZeroI8::new(n.read_unaligned()).is_some());
unsafe_impl!(NonZeroU16: TryFromBytes; |n: MaybeAligned<u16>| NonZeroU16::new(n.read_unaligned()).is_some());
unsafe_impl!(NonZeroI16: TryFromBytes; |n: MaybeAligned<i16>| NonZeroI16::new(n.read_unaligned()).is_some());
unsafe_impl!(NonZeroU32: TryFromBytes; |n: MaybeAligned<u32>| NonZeroU32::new(n.read_unaligned()).is_some());
unsafe_impl!(NonZeroI32: TryFromBytes; |n: MaybeAligned<i32>| NonZeroI32::new(n.read_unaligned()).is_some());
unsafe_impl!(NonZeroU64: TryFromBytes; |n: MaybeAligned<u64>| NonZeroU64::new(n.read_unaligned()).is_some());
unsafe_impl!(NonZeroI64: TryFromBytes; |n: MaybeAligned<i64>| NonZeroI64::new(n.read_unaligned()).is_some());
unsafe_impl!(NonZeroU128: TryFromBytes; |n: MaybeAligned<u128>| NonZeroU128::new(n.read_unaligned()).is_some());
unsafe_impl!(NonZeroI128: TryFromBytes; |n: MaybeAligned<i128>| NonZeroI128::new(n.read_unaligned()).is_some());
unsafe_impl!(NonZeroUsize: TryFromBytes; |n: MaybeAligned<usize>| NonZeroUsize::new(n.read_unaligned()).is_some());
unsafe_impl!(NonZeroIsize: TryFromBytes; |n: MaybeAligned<isize>| NonZeroIsize::new(n.read_unaligned()).is_some());
unsafe_impl!(NonZeroU8: TryFromBytes; |n: MaybeAligned<u8>| NonZeroU8::new(n.read_unaligned::<BecauseImmutable>()).is_some());
unsafe_impl!(NonZeroI8: TryFromBytes; |n: MaybeAligned<i8>| NonZeroI8::new(n.read_unaligned::<BecauseImmutable>()).is_some());
unsafe_impl!(NonZeroU16: TryFromBytes; |n: MaybeAligned<u16>| NonZeroU16::new(n.read_unaligned::<BecauseImmutable>()).is_some());
unsafe_impl!(NonZeroI16: TryFromBytes; |n: MaybeAligned<i16>| NonZeroI16::new(n.read_unaligned::<BecauseImmutable>()).is_some());
unsafe_impl!(NonZeroU32: TryFromBytes; |n: MaybeAligned<u32>| NonZeroU32::new(n.read_unaligned::<BecauseImmutable>()).is_some());
unsafe_impl!(NonZeroI32: TryFromBytes; |n: MaybeAligned<i32>| NonZeroI32::new(n.read_unaligned::<BecauseImmutable>()).is_some());
unsafe_impl!(NonZeroU64: TryFromBytes; |n: MaybeAligned<u64>| NonZeroU64::new(n.read_unaligned::<BecauseImmutable>()).is_some());
unsafe_impl!(NonZeroI64: TryFromBytes; |n: MaybeAligned<i64>| NonZeroI64::new(n.read_unaligned::<BecauseImmutable>()).is_some());
unsafe_impl!(NonZeroU128: TryFromBytes; |n: MaybeAligned<u128>| NonZeroU128::new(n.read_unaligned::<BecauseImmutable>()).is_some());
unsafe_impl!(NonZeroI128: TryFromBytes; |n: MaybeAligned<i128>| NonZeroI128::new(n.read_unaligned::<BecauseImmutable>()).is_some());
unsafe_impl!(NonZeroUsize: TryFromBytes; |n: MaybeAligned<usize>| NonZeroUsize::new(n.read_unaligned::<BecauseImmutable>()).is_some());
unsafe_impl!(NonZeroIsize: TryFromBytes; |n: MaybeAligned<isize>| NonZeroIsize::new(n.read_unaligned::<BecauseImmutable>()).is_some());
}
safety_comment! {
/// SAFETY:
Expand Down
4 changes: 2 additions & 2 deletions src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -359,7 +359,7 @@ use core::{
slice,
};

use crate::pointer::{invariant, BecauseExclusive, BecauseImmutable};
use crate::pointer::{invariant, BecauseExclusive};

#[cfg(any(feature = "alloc", test))]
extern crate alloc;
Expand All @@ -371,7 +371,7 @@ use core::alloc::Layout;

// Used by `TryFromBytes::is_bit_valid`.
#[doc(hidden)]
pub use crate::pointer::{Maybe, MaybeAligned, Ptr};
pub use crate::pointer::{BecauseImmutable, Maybe, MaybeAligned, Ptr};
// Used by `KnownLayout`.
#[doc(hidden)]
pub use crate::layout::*;
Expand Down
9 changes: 6 additions & 3 deletions src/pointer/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,14 +41,17 @@ where
/// Reads the value from `MaybeAligned`.
#[must_use]
#[inline]
pub fn read_unaligned(self) -> T
pub fn read_unaligned<R>(self) -> T
where
T: Copy,
R: AliasingSafeReason,
T: AliasingSafe<T, Aliasing, R>,
{
let raw = self.as_non_null().as_ptr();
// SAFETY: By invariant on `MaybeAligned`, `raw` contains
// validly-initialized data for `T`. The value is safe to read and
// return, because `T` is copy.
// validly-initialized data for `T`. By `T: AliasingSafe`, we are
// permitted to perform a read of `raw`'s referent. The value is safe to
// read and return, because `T` is copy.
unsafe { core::ptr::read_unaligned(raw) }
}

Expand Down
4 changes: 2 additions & 2 deletions src/pointer/ptr.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1658,8 +1658,8 @@ mod tests {
assert_eq!(ptr.len(), N);
// SAFETY: `i` is in bounds by construction.
let (l, r) = unsafe { ptr.reborrow().split_at(i) };
let l_sum: usize = l.iter().map(Ptr::read_unaligned).sum();
let r_sum: usize = r.iter().map(Ptr::read_unaligned).sum();
let l_sum: usize = l.iter().map(Ptr::read_unaligned::<BecauseImmutable>).sum();
let r_sum: usize = r.iter().map(Ptr::read_unaligned::<BecauseImmutable>).sum();
assert_eq!(l_sum, i);
assert_eq!(r_sum, N - i);
assert_eq!(l_sum + r_sum, N);
Expand Down
2 changes: 1 addition & 1 deletion zerocopy-derive/src/enum.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -330,7 +330,7 @@ pub(crate) fn derive_is_bit_valid(
// is `Initialized`. Since we have not written uninitialized
// bytes into the referent, `tag_ptr` is also `Initialized`.
let tag_ptr = unsafe { tag_ptr.assume_initialized() };
tag_ptr.bikeshed_recall_valid().read_unaligned()
tag_ptr.bikeshed_recall_valid().read_unaligned::<::zerocopy::BecauseImmutable>()
};

// SAFETY:
Expand Down
6 changes: 3 additions & 3 deletions zerocopy-derive/src/output_tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -599,7 +599,7 @@ fn test_try_from_bytes_enum() {
candidate.reborrow().cast_unsized(|p: *mut Self| { p as *mut ___ZerocopyTagPrimitive })
};
let tag_ptr = unsafe { tag_ptr.assume_initialized() };
tag_ptr.bikeshed_recall_valid().read_unaligned()
tag_ptr.bikeshed_recall_valid().read_unaligned::<::zerocopy::BecauseImmutable>()
};
let raw_enum = unsafe {
candidate.cast_unsized(|p: *mut Self| { p as *mut ___ZerocopyRawEnum<'a, N, X, Y> })
Expand Down Expand Up @@ -893,7 +893,7 @@ fn test_try_from_bytes_enum() {
candidate.reborrow().cast_unsized(|p: *mut Self| { p as *mut ___ZerocopyTagPrimitive })
};
let tag_ptr = unsafe { tag_ptr.assume_initialized() };
tag_ptr.bikeshed_recall_valid().read_unaligned()
tag_ptr.bikeshed_recall_valid().read_unaligned::<::zerocopy::BecauseImmutable>()
};
let raw_enum = unsafe {
candidate.cast_unsized(|p: *mut Self| { p as *mut ___ZerocopyRawEnum<'a, N, X, Y> })
Expand Down Expand Up @@ -1187,7 +1187,7 @@ fn test_try_from_bytes_enum() {
candidate.reborrow().cast_unsized(|p: *mut Self| { p as *mut ___ZerocopyTagPrimitive })
};
let tag_ptr = unsafe { tag_ptr.assume_initialized() };
tag_ptr.bikeshed_recall_valid().read_unaligned()
tag_ptr.bikeshed_recall_valid().read_unaligned::<::zerocopy::BecauseImmutable>()
};
let raw_enum = unsafe {
candidate.cast_unsized(|p: *mut Self| { p as *mut ___ZerocopyRawEnum<'a, N, X, Y> })
Expand Down

0 comments on commit 0405574

Please sign in to comment.