From 37305afab5646ae7c4440cc9bb00f968bf4136a8 Mon Sep 17 00:00:00 2001 From: Joshua Liebow-Feeser Date: Fri, 9 Aug 2024 13:39:59 -0700 Subject: [PATCH] Fix unsoundness in impl_for_transparent_wrapper! Previously, `impl_for_transparent_wrapper!` permitted the following unsound code to compile: impl_for_transparent_wrapper!(FromBytes for AtomicBool); `impl_for_transparent_wrapper!` attempted to ensure that `AtomicBool`'s inner type implemented `FromBytes`, but failed to properly enforce this bound (it should have failed thanks to `bool: !FromBytes`). This commit simplifies `impl_for_transparent_wrapper!` and fixes this soundness hole. This fix is adapted from @josephlr's similar fix in #1091. Credit to @josephlr for discovering this soundness hole. Co-authored-by: Joe Richey --- src/impls.rs | 19 +++-- src/macros.rs | 232 ++++++++++++++++++++++---------------------------- 2 files changed, 111 insertions(+), 140 deletions(-) diff --git a/src/impls.rs b/src/impls.rs index 92ad671edab..19337107238 100644 --- a/src/impls.rs +++ b/src/impls.rs @@ -441,23 +441,26 @@ safety_comment! { } macro_rules! impl_traits_for_atomics { - ($($atomics:ident [$inners:ident]),* $(,)?) => { + ($($atomics:ident),* $(,)?) => { $( - impl_for_transparent_wrapper!(TryFromBytes for $atomics [UnsafeCell<$inners>]); - impl_for_transparent_wrapper!(FromZeros for $atomics [UnsafeCell<$inners>]); - impl_for_transparent_wrapper!(FromBytes for $atomics [UnsafeCell<$inners>]); - impl_for_transparent_wrapper!(IntoBytes for $atomics [UnsafeCell<$inners>]); + impl_for_transparent_wrapper!(=> TryFromBytes for $atomics); + impl_for_transparent_wrapper!(=> FromZeros for $atomics); + impl_for_transparent_wrapper!(=> FromBytes for $atomics); + impl_for_transparent_wrapper!(=> IntoBytes for $atomics); )* }; } #[rustfmt::skip] impl_traits_for_atomics!( - AtomicBool [bool], - AtomicI16 [i16], AtomicI32 [i32], AtomicI8 [i8], AtomicIsize [isize], - AtomicU16 [u16], AtomicU32 [u32], AtomicU8 [u8], AtomicUsize [usize], + AtomicI16, AtomicI32, AtomicI8, AtomicIsize, + AtomicU16, AtomicU32, AtomicU8, AtomicUsize, ); +impl_for_transparent_wrapper!(=> TryFromBytes for AtomicBool); +impl_for_transparent_wrapper!(=> FromZeros for AtomicBool); +impl_for_transparent_wrapper!(=> IntoBytes for AtomicBool); + safety_comment! { /// SAFETY: /// Per [1], `AtomicBool`, `AtomicU8`, and `AtomicI8` have the same size as diff --git a/src/macros.rs b/src/macros.rs index f1d4f50d4d7..abeeb9ed8ba 100644 --- a/src/macros.rs +++ b/src/macros.rs @@ -208,168 +208,136 @@ macro_rules! unsafe_impl { macro_rules! impl_for_transparent_wrapper { ( $(#[$attr:meta])* - $tyvar:ident $(: $(? $optbound:ident $(+)?)* $($bound:ident $(+)?)* )? + $($tyvar:ident $(: $(? $optbound:ident $(+)?)* $($bound:ident $(+)?)* )?)? => $trait:ident for $ty:ty $(; |$candidate:ident $(: MaybeAligned<$ref_repr:ty>)? $(: Maybe<$ptr_repr:ty>)?| $is_bit_valid:expr)? ) => { $(#[$attr])* #[allow(non_local_definitions)] - // SAFETY: - // - We explicitly add a `$tyvar: $trait` bound (regardless of what - // bounds the caller has provided). - // - Inside of `only_derive_is_allowed_to_implement_this_trait`, `f` is - // generic over the any `I: Invariants`, and is generic over the same - // `$tyvar` bounds as the outer impl (with the exception of `$trait`). - // - `f` can only compile if its internal call to - // `is_transparent_wrapper` compiles. + + // This block implements `$trait` for `$ty` under the following + // conditions: + // - `$ty: TransparentWrapper` + // - `$ty::Inner: $trait` + // - For some `Xxx`, `$ty::XxxVariance = Covariant` (`Xxx` is determined + // by the `@define_is_transparent_wrapper` macro arms). This bound + // ensures that some layout property is the same between `$ty` and + // `$ty::Inner`. Which layout property this is depends on the trait + // being implemented (for example, `FromBytes` is not concerned with + // alignment, but is concerned with bit validity). // - // The definition of `is_transparent_wrapper` is generated by - // each `@is_transparent_wrapper` macro arm. Each arm is parameterized - // by `$trait`, and emits bounds which are sufficient to ensure that - // this is a sound implementation of `$trait for W` *so long as* `T: - // $trait`. In `f`, we call `is_transparent_wrapper`, - // and so if this code compiles, that guarantees that `$ty` satisfies - // the same bounds for all `$tyvar: $trait`. Thus, so long as the bounds - // emitted by the `@is_transparent_wrapper` arm are sound, then this - // entire impl is sound. + // In other words, `$ty` is guaranteed to soundly implement `$trait` + // because some property of its layout is the same as `$ty::Inner`, + // which implements `$trait`. Most of the complexity in this macro is to + // ensure that the above-mentioned conditions are actually met, and that + // the proper variance (ie, the proper layout property) is chosen. + + // SAFETY: + // - `is_transparent_wrapper` requires: + // - `W: TransparentWrapper` + // - `W::Inner: $trait` + // - `f` is generic over `I: Invariants`, and in its body, calls + // `is_transparent_wrapper::()`. Thus, this code will only + // compile if, for all `I: Invariants`: + // - `$ty: TransparentWrapper` + // - `$ty::Inner: $trait` // - // Each `@is_transparent_wrapper` arm contains its own safety comment - // which explains why its bounds are sound. - unsafe impl<$tyvar: $($(? $optbound +)* $($bound +)*)? $trait> $trait for $ty { + // These two facts - that `$ty: TransparentWrapper` and that + // `$ty::Inner: $trait` - are the preconditions to the full safety + // proofs, which are completed below in the + // `@define_is_transparent_wrapper` macro arms. The safety proof is + // slightly different for each trait. + unsafe impl<$($tyvar $(: $(? $optbound +)* $($bound +)*)?)?> $trait for $ty { #[allow(dead_code, clippy::missing_inline_in_public_items)] #[cfg_attr(coverage_nightly, coverage(off))] fn only_derive_is_allowed_to_implement_this_trait() { use crate::{pointer::invariant::Invariants, util::*}; - impl_for_transparent_wrapper!(@is_transparent_wrapper $trait); + impl_for_transparent_wrapper!(@define_is_transparent_wrapper $trait); #[cfg_attr(coverage_nightly, coverage(off))] - fn f() { - is_transparent_wrapper::(); + fn f() { + is_transparent_wrapper::(); } } impl_for_transparent_wrapper!( @is_bit_valid - <$tyvar: $($(? $optbound +)* $($bound +)*)?> + $(<$tyvar $(: $(? $optbound +)* $($bound +)*)?>)? $trait for $ty ); } }; - ( - $(#[$attr:meta])* - for $ty:ty [$inner:ty] $(; |$candidate:ident $(: MaybeAligned<$ref_repr:ty>)? $(: Maybe<$ptr_repr:ty>)?| $is_bit_valid:expr)? - ) => {}; - ( - $(#[$attr:meta])* - $trait:ident $(, $traits:ident)* for $ty:ty [$inner:ty] $(; |$candidate:ident $(: MaybeAligned<$ref_repr:ty>)? $(: Maybe<$ptr_repr:ty>)?| $is_bit_valid:expr)? - ) => { - impl_for_transparent_wrapper!( - $(#[$attr])* - $($traits),* for $ty [$inner] $(; |$candidate $(: MaybeAligned<$ref_repr>)? $(: Maybe<$ptr_repr>)?| $is_bit_valid:expr)? - ); - - $(#[$attr])* - #[allow(non_local_definitions)] - // SAFETY: - // - We explicitly add a `$tyvar: $trait` bound (regardless of what - // bounds the caller has provided). - // - Inside of `only_derive_is_allowed_to_implement_this_trait`, `f` is - // generic over the any `I: Invariants`. - // - `f` can only compile if its internal call to - // `is_transparent_wrapper` compiles. - // - // The definition of `is_transparent_wrapper` is generated by - // each `@is_transparent_wrapper` macro arm. Each arm is parameterized - // by `$trait`, and emits bounds which are sufficient to ensure that - // this is a sound implementation of `$trait for W` *so long as* `T: - // $trait`. In `f`, we call `is_transparent_wrapper`, - // and so if this code compiles, that guarantees that `$ty` satisfies - // the same bounds so long as `$inner: $trait`. Thus, so long as the - // bounds emitted by the `@is_transparent_wrapper` arm are sound, then - // this entire impl is sound. - // - // Each `@is_transparent_wrapper` arm contains its own safety comment - // which explains why its bounds are sound. - unsafe impl $trait for $ty { - #[allow(dead_code, clippy::missing_inline_in_public_items)] - #[cfg_attr(coverage_nightly, coverage(off))] - fn only_derive_is_allowed_to_implement_this_trait() { - use crate::{pointer::invariant::Invariants, util::*}; - - impl_for_transparent_wrapper!(@is_transparent_wrapper $trait); - - #[cfg_attr(coverage_nightly, coverage(off))] - fn f() { - is_transparent_wrapper::(); - } - } - - impl_for_transparent_wrapper!(@is_bit_valid $trait for $ty); - } - }; - (@is_transparent_wrapper Immutable) => { - // SAFETY: `W: TransparentWrapper` + (@define_is_transparent_wrapper Immutable) => { + // SAFETY: `W: TransparentWrapper` // requires that `W` has `UnsafeCell`s at the same byte offsets as - // `W::Inner = T`. `T: Immutable` implies that `T` does not contain any - // `UnsafeCell`s, and so `W` does not contain any `UnsafeCell`s. Thus, - // `W` can soundly implement `Immutable`. - #[cfg_attr(coverage_nightly, coverage(off))] - fn is_transparent_wrapper + ?Sized>() {} - }; - (@is_transparent_wrapper FromZeros) => { - // SAFETY: `W: TransparentWrapper` requires - // that `W` has the same bit validity as `W::Inner = T`. `T: FromZeros` - // implies that the all-zeros bit pattern is a bit-valid instance of - // `T`, and so the all-zeros bit pattern is a bit-valid instance of `W`. - // Thus, `W` can soundly implement `FromZeros`. - #[cfg_attr(coverage_nightly, coverage(off))] - fn is_transparent_wrapper + ?Sized>() {} - }; - (@is_transparent_wrapper FromBytes) => { - // SAFETY: `W: TransparentWrapper` requires - // that `W` has the same bit validity as `W::Inner = T`. `T: FromBytes` - // implies that any initialized bit pattern is a bit-valid instance of - // `T`, and so any initialized bit pattern is a bit-valid instance of - // `W`. Thus, `W` can soundly implement `FromBytes`. - #[cfg_attr(coverage_nightly, coverage(off))] - fn is_transparent_wrapper + ?Sized>() {} - }; - (@is_transparent_wrapper IntoBytes) => { - // SAFETY: `W: TransparentWrapper` requires - // that `W` has the same bit validity as `W::Inner = T`. `T: IntoBytes` - // implies that no bit-valid instance of `T` contains uninitialized - // bytes, and so no bit-valid instance of `W` contains uninitialized - // bytes. Thus, `W` can soundly implement `IntoBytes`. - #[cfg_attr(coverage_nightly, coverage(off))] - fn is_transparent_wrapper + ?Sized>() {} - }; - (@is_transparent_wrapper Unaligned) => { - // SAFETY: `W: TransparentWrapper` requires - // that `W` has the same alignment as `W::Inner = T`. `T: Unaligned` - // implies `T`'s alignment is 1, and so `W`'s alignment is 1. Thus, `W` - // can soundly implement `Unaligned`. - #[cfg_attr(coverage_nightly, coverage(off))] - fn is_transparent_wrapper + ?Sized>() {} - }; - (@is_transparent_wrapper TryFromBytes) => { - // SAFETY: `W: TransparentWrapper` requires - // that `W` has the same bit validity as `W::Inner = T`. `T: - // TryFromBytes` implies that `::is_bit_valid(c)` - // only returns `true` if `c` references a bit-valid instance of `T`. - // Thus, `::is_bit_valid(c)` only returns `true` if - // `c` references a bit-valid instance of `W`. Below, we implement `::is_bit_valid` by deferring to `::is_bit_valid`. Thus, it is sound for `W` to implement - // `TryFromBytes` with this implementation of `is_bit_valid`. + // `W::Inner`. `W::Inner: Immutable` implies that `W::Inner` does not + // contain any `UnsafeCell`s, and so `W` does not contain any + // `UnsafeCell`s. Since `W = $ty`, `$ty` can soundly implement + // `Immutable`. + impl_for_transparent_wrapper!(@define_is_transparent_wrapper Immutable, UnsafeCellVariance) + }; + (@define_is_transparent_wrapper FromZeros) => { + // SAFETY: `W: TransparentWrapper` + // requires that `W` has the same bit validity as `W::Inner`. `W::Inner: + // FromZeros` implies that the all-zeros bit pattern is a bit-valid + // instance of `W::Inner`, and so the all-zeros bit pattern is a + // bit-valid instance of `W`. Since `W = $ty`, `$ty` can soundly + // implement `FromZeros`. + impl_for_transparent_wrapper!(@define_is_transparent_wrapper FromZeros, ValidityVariance) + }; + (@define_is_transparent_wrapper FromBytes) => { + // SAFETY: `W: TransparentWrapper` + // requires that `W` has the same bit validity as `W::Inner`. `W::Inner: + // FromBytes` implies that any initialized bit pattern is a bit-valid + // instance of `W::Inner`, and so any initialized bit pattern is a + // bit-valid instance of `W`. Since `W = $ty`, `$ty` can soundly + // implement `FromBytes`. + impl_for_transparent_wrapper!(@define_is_transparent_wrapper FromBytes, ValidityVariance) + }; + (@define_is_transparent_wrapper IntoBytes) => { + // SAFETY: `W: TransparentWrapper` + // requires that `W` has the same bit validity as `W::Inner`. `W::Inner: + // IntoBytes` implies that no bit-valid instance of `W::Inner` contains + // uninitialized bytes, and so no bit-valid instance of `W` contains + // uninitialized bytes. Since `W = $ty`, `$ty` can soundly implement + // `IntoBytes`. + impl_for_transparent_wrapper!(@define_is_transparent_wrapper IntoBytes, ValidityVariance) + }; + (@define_is_transparent_wrapper Unaligned) => { + // SAFETY: `W: TransparentWrapper` + // requires that `W` has the same alignment as `W::Inner`. `W::Inner: + // Unaligned` implies `W::Inner`'s alignment is 1, and so `W`'s + // alignment is 1. Since `W = $ty`, `W` can soundly implement + // `Unaligned`. + impl_for_transparent_wrapper!(@define_is_transparent_wrapper Unaligned, AlignmentVariance) + }; + (@define_is_transparent_wrapper TryFromBytes) => { + // SAFETY: `W: TransparentWrapper` + // requires that `W` has the same bit validity as `W::Inner`. `W::Inner: + // TryFromBytes` implies that `::is_bit_valid(c)` only returns `true` if `c` references + // a bit-valid instance of `W::Inner`. Thus, `::is_bit_valid(c)` only returns `true` if `c` references + // a bit-valid instance of `W`. Below, we implement `::is_bit_valid` by deferring to `::is_bit_valid`. Since `W = $ty`, it is sound for `$ty` + // to implement `TryFromBytes` with this implementation of + // `is_bit_valid`. + impl_for_transparent_wrapper!(@define_is_transparent_wrapper TryFromBytes, ValidityVariance) + }; + (@define_is_transparent_wrapper $trait:ident, $variance:ident) => { #[cfg_attr(coverage_nightly, coverage(off))] - fn is_transparent_wrapper + ?Sized>() {} + fn is_transparent_wrapper + ?Sized>() + where + W::Inner: $trait, + {} }; ( @is_bit_valid $(<$tyvar:ident $(: $(? $optbound:ident $(+)?)* $($bound:ident $(+)?)* )?>)? TryFromBytes for $ty:ty ) => { - // SAFETY: See safety comment in `(@is_transparent_wrapper + // SAFETY: See safety comment in `(@define_is_transparent_wrapper // TryFromBytes)` macro arm for an explanation of why this is a sound // implementation of `is_bit_valid`. #[inline]