Skip to content

Commit

Permalink
Fix unsoundness in impl_for_transparent_wrapper!
Browse files Browse the repository at this point in the history 
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 <[email protected]>
  • Loading branch information
@joshlf @josephlr
joshlf and josephlr committed Aug 9, 2024
1 parent b37557f commit 37305af
Show file tree
Hide file tree
Showing 2 changed files with 111 additions and 140 deletions.
19 changes: 11 additions & 8 deletions src/impls.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
232 changes: 100 additions & 132 deletions src/macros.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<I, T, W>` 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<I, $tyvar, $ty>`,
// 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<I, W>` requires:
// - `W: TransparentWrapper<I>`
// - `W::Inner: $trait`
// - `f` is generic over `I: Invariants`, and in its body, calls
// `is_transparent_wrapper::<I, $ty>()`. Thus, this code will only
// compile if, for all `I: Invariants`:
// - `$ty: TransparentWrapper<I>`
// - `$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<I>` 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<I: Invariants, $tyvar $(: $(? $optbound +)* $($bound +)*)?>() {
is_transparent_wrapper::<I, $tyvar, $ty>();
fn f<I: Invariants, $($tyvar $(: $(? $optbound +)* $($bound +)*)?)?>() {
is_transparent_wrapper::<I, $ty>();
}
}

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<I, T, W>` 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<I, $inner, $ty>`,
// 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<I: Invariants>() {
is_transparent_wrapper::<I, $inner, $ty>();
}
}

impl_for_transparent_wrapper!(@is_bit_valid $trait for $ty);
}
};
(@is_transparent_wrapper Immutable) => {
// SAFETY: `W: TransparentWrapper<UnsafeCellVariance=Covariant>`
(@define_is_transparent_wrapper Immutable) => {
// SAFETY: `W: TransparentWrapper<UnsafeCellVariance = Covariant>`
// 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<I: Invariants, T: ?Sized, W: TransparentWrapper<I, Inner=T, UnsafeCellVariance=Covariant> + ?Sized>() {}
};
(@is_transparent_wrapper FromZeros) => {
// SAFETY: `W: TransparentWrapper<ValidityVariance=Covariant>` 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<I: Invariants, T: ?Sized, W: TransparentWrapper<I, Inner=T, ValidityVariance=Covariant> + ?Sized>() {}
};
(@is_transparent_wrapper FromBytes) => {
// SAFETY: `W: TransparentWrapper<ValidityVariance=Covariant>` 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<I: Invariants, T: ?Sized, W: TransparentWrapper<I, Inner=T, ValidityVariance=Covariant> + ?Sized>() {}
};
(@is_transparent_wrapper IntoBytes) => {
// SAFETY: `W: TransparentWrapper<ValidityVariance=Covariant>` 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<I: Invariants, T: ?Sized, W: TransparentWrapper<I, Inner=T, ValidityVariance=Covariant> + ?Sized>() {}
};
(@is_transparent_wrapper Unaligned) => {
// SAFETY: `W: TransparentWrapper<AlignmentVariance=Covariant>` 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<I: Invariants, T: ?Sized, W: TransparentWrapper<I, Inner=T, AlignmentVariance=Covariant> + ?Sized>() {}
};
(@is_transparent_wrapper TryFromBytes) => {
// SAFETY: `W: TransparentWrapper<ValidityVariance=Covariant>` requires
// that `W` has the same bit validity as `W::Inner = T`. `T:
// TryFromBytes` implies that `<T as TryFromBytes>::is_bit_valid(c)`
// only returns `true` if `c` references a bit-valid instance of `T`.
// Thus, `<T as TryFromBytes>::is_bit_valid(c)` only returns `true` if
// `c` references a bit-valid instance of `W`. Below, we implement `<W
// as TryFromBytes>::is_bit_valid` by deferring to `<T as
// TryFromBytes>::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<ValidityVariance = Covariant>`
// 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<ValidityVariance = Covariant>`
// 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<ValidityVariance = Covariant>`
// 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<AlignmentVariance = Covariant>`
// 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<ValidityVariance = Covariant>`
// requires that `W` has the same bit validity as `W::Inner`. `W::Inner:
// TryFromBytes` implies that `<W::Inner as
// TryFromBytes>::is_bit_valid(c)` only returns `true` if `c` references
// a bit-valid instance of `W::Inner`. Thus, `<W::Inner as
// TryFromBytes>::is_bit_valid(c)` only returns `true` if `c` references
// a bit-valid instance of `W`. Below, we implement `<W as
// TryFromBytes>::is_bit_valid` by deferring to `<W::Inner as
// TryFromBytes>::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<I: Invariants, T: ?Sized, W: TransparentWrapper<I, Inner=T, ValidityVariance=Covariant> + ?Sized>() {}
fn is_transparent_wrapper<I: Invariants, W: TransparentWrapper<I, $variance = Covariant> + ?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]
Expand Down

0 comments on commit 37305af

Please sign in to comment.