Skip to content

Commit

Permalink
Restrict MaybeUninit trait impls to fix soundness (#308)
Browse files Browse the repository at this point in the history
Previously, we implemented `FromZeroes` and `FromBytes` for
`MaybeUninit<T>` with no bound on `T`. This resulted in a soundness hole
in which `T` - and thus `MaybeUninit<T>` - could contain an
`UnsafeCell`, which is a violation of the contracts of `FromZeroes` and
`FromBytes`.

This is a breaking change, but it's very unlikely to be one that code is
currently relying on, especially given that the 0.7.x release train was
published very recently. Thus, in this commit, we publish 0.7.3, and we
will yank 0.7.{0,1,2} as soon as 0.7.3 is published.

Fixes #299
  • Loading branch information
joshlf authored Sep 2, 2023
1 parent cb20ba0 commit 62f76d2
Show file tree
Hide file tree
Showing 3 changed files with 20 additions and 10 deletions.
8 changes: 4 additions & 4 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
[package]
edition = "2021"
name = "zerocopy"
version = "0.7.2"
version = "0.7.3"
authors = ["Joshua Liebow-Feeser <[email protected]>"]
description = "Utilities for zero-copy parsing and serialization"
license = "BSD-2-Clause"
Expand Down Expand Up @@ -55,7 +55,7 @@ simd-nightly = ["simd"]
__internal_use_only_features_that_work_on_stable = ["alloc", "simd"]

[dependencies]
zerocopy-derive = { version = "=0.7.2", path = "zerocopy-derive", optional = true }
zerocopy-derive = { version = "=0.7.3", path = "zerocopy-derive", optional = true }

[dependencies.byteorder]
version = "1.3"
Expand All @@ -66,7 +66,7 @@ optional = true
# zerocopy-derive remain equal, even if the 'derive' feature isn't used.
# See: https://github.com/matklad/macro-dep-test
[target.'cfg(any())'.dependencies]
zerocopy-derive = { version = "=0.7.2", path = "zerocopy-derive" }
zerocopy-derive = { version = "=0.7.3", path = "zerocopy-derive" }

[dev-dependencies]
rand = "0.6"
Expand All @@ -78,4 +78,4 @@ static_assertions = "1.1"
# CI test failures.
trybuild = "=1.0.80"
# In tests, unlike in production, zerocopy-derive is not optional
zerocopy-derive = { version = "=0.7.2", path = "zerocopy-derive" }
zerocopy-derive = { version = "=0.7.3", path = "zerocopy-derive" }
20 changes: 15 additions & 5 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -996,13 +996,23 @@ safety_comment! {
//
/// SAFETY:
/// - `FromZeroes`, `FromBytes`: `MaybeUninit<T>` has no restrictions on its
/// contents.
/// contents. Unfortunately, in addition to bit validity, `FromZeroes` and
/// `FromBytes` also require that implementers contain no `UnsafeCell`s.
/// Thus, we require `T: FromZeroes` and `T: FromBytes` in order to ensure
/// that `T` - and thus `MaybeUninit<T>` - contains to `UnsafeCell`s.
/// Thus, requiring that `T` implement each of these traits is sufficient
/// - `Unaligned`: `MaybeUninit<T>` is guaranteed by its documentation [1]
/// to have the same alignment as `T`.
///
/// [1] https://doc.rust-lang.org/nightly/core/mem/union.MaybeUninit.html#layout-1
unsafe_impl!(T => FromZeroes for MaybeUninit<T>);
unsafe_impl!(T => FromBytes for MaybeUninit<T>);
/// [1]
/// https://doc.rust-lang.org/nightly/core/mem/union.MaybeUninit.html#layout-1
///
/// TODO(https://github.com/google/zerocopy/issues/251): If we split
/// `FromBytes` and `RefFromBytes`, or if we introduce a separate
/// `NoCell`/`Freeze` trait, we can relax the trait bounds for `FromZeroes`
/// and `FromBytes`.
unsafe_impl!(T: FromZeroes => FromZeroes for MaybeUninit<T>);
unsafe_impl!(T: FromBytes => FromBytes for MaybeUninit<T>);
unsafe_impl!(T: Unaligned => Unaligned for MaybeUninit<T>);
assert_unaligned!(MaybeUninit<()>, MaybeUninit<u8>);
}
Expand Down Expand Up @@ -4059,7 +4069,7 @@ mod tests {
assert_impls!(ManuallyDrop<[NotZerocopy]>: !FromZeroes, !FromBytes, !AsBytes, !Unaligned);

assert_impls!(MaybeUninit<u8>: FromZeroes, FromBytes, Unaligned, !AsBytes);
assert_impls!(MaybeUninit<NotZerocopy>: FromZeroes, FromBytes, !AsBytes, !Unaligned);
assert_impls!(MaybeUninit<NotZerocopy>: !FromZeroes, !FromBytes, !AsBytes, !Unaligned);

assert_impls!(Wrapping<u8>: FromZeroes, FromBytes, AsBytes, Unaligned);
assert_impls!(Wrapping<NotZerocopy>: !FromZeroes, !FromBytes, !AsBytes, !Unaligned);
Expand Down
2 changes: 1 addition & 1 deletion zerocopy-derive/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
[package]
edition = "2021"
name = "zerocopy-derive"
version = "0.7.2"
version = "0.7.3"
authors = ["Joshua Liebow-Feeser <[email protected]>"]
description = "Custom derive for traits from the zerocopy crate"
license = "BSD-2-Clause"
Expand Down

0 comments on commit 62f76d2

Please sign in to comment.