Skip to content

Commit dd70b7f

Browse files
joshlfjoshlf
committed
[pointer] Relax UnsafeCell requirements
Previously, we required there to always be "`UnsafeCell` agreement" between any `Ptr`s or references referencing a given region of memory. This was based on an overly-strict interpretation of the semantics of `UnsafeCell`. In this commit, we relax `Ptr` to only require "`UnsafeCell` agreement" when the aliasing model is `Shared`. All of the places that our internal invariants are "consumed" - ie, where we use them as safety preconditions for calling unsafe functions defined outside our crate - this relaxation is sufficient. This is based on what we (@jswrenn and I) believe to be a more accurate model of the semantics of `UnsafeCell`s. In particular, `UnsafeCell`s do not affect the semantics of loads or stores in Rust. All they do is affect the semantics of shared references. In particular, Rust assumes that the referent of a shared reference will not be stored to during the lifetime of any shared reference, but this assumption is not made for bytes which are covered by an `UnsafeCell`. This is entirely a runtime property. If two references refer to the same memory, but disagree on whether that memory is covered by an `UnsafeCell`, this results in UB if the `UnsafeCell` is used to store to the memory, violating the expectations of the non-`UnsafeCell` shared reference. This commit is consistent with the runtime nature of this property, but is inconsistent with Stacked Borrows (rust-lang/unsafe-code-guidelines#455). However, this is considered to be a bug in Stacked Borrows.
1 parent f405033 commit dd70b7f

File tree

8 files changed

+172
-277
lines changed

8 files changed

+172
-277
lines changed

‎src/lib.rs

+11-6
Original file line numberDiff line numberDiff line change
@@ -3935,12 +3935,17 @@ unsafe impl<T: TryFromBytes> TryFromBytes for UnsafeCell<T> {
39353935
// We wrap in `Unalign` here so that we can get a vanilla Rust reference
39363936
// below, which in turn allows us to call `UnsafeCell::get_mut`.
39373937
//
3938-
// SAFETY: `Unalign` and `MaybeUninit` both have the same size as the
3939-
// types they wrap [1]. Thus, this cast will preserve the size of the
3940-
// pointer. Since both the source and destination types are wrapped in
3941-
// `UnsafeCell`, all bytes of both types are inside of `UnsafeCell`s,
3942-
// and so the byte ranges covered by `UnsafeCell`s are identical in both
3943-
// types.
3938+
// SAFETY:
3939+
// - `.cast` preserves address. `Unalign` and `MaybeUninit` both have
3940+
// the same size as the types they wrap [1]. Thus, this cast will
3941+
// preserve the size of the pointer. As a result, the cast will
3942+
// address the same bytes as `c`.
3943+
// - `.cast` preserves provenance.
3944+
// - Since both the source and destination types are wrapped in
3945+
// `UnsafeCell`, all bytes of both types are inside of `UnsafeCell`s,
3946+
// and so the byte ranges covered by `UnsafeCell`s are identical in
3947+
// both types. Since the pointers refer to the same byte ranges,
3948+
// the same is true of the pointers' referents as well.
39443949
//
39453950
// [1] Per https://doc.rust-lang.org/stable/core/mem/union.MaybeUninit.html#layout-1:
39463951
//

‎src/macros.rs

+10-8
Original file line numberDiff line numberDiff line change
@@ -142,10 +142,11 @@ macro_rules! unsafe_impl {
142142
#[inline]
143143
fn is_bit_valid<AA: invariant::Aliasing + invariant::AtLeast<invariant::Shared>>(candidate: Maybe<'_, Self, AA>) -> bool {
144144
// SAFETY:
145-
// - The argument to `cast_unsized` is `|p| p as *mut _` as required
146-
// by that method's safety precondition.
147-
// - The caller has promised that the cast results in an object of
148-
// equal or lesser size.
145+
// - The cast preserves address. The caller has promised that the
146+
// cast results in an object of equal or lesser size, and so the
147+
// cast returns a pointer which references a subset of the bytes
148+
// of `p`.
149+
// - The cast preserves provenance.
149150
// - The caller has promised that the destination type has
150151
// `UnsafeCell`s at the same byte ranges as the source type.
151152
#[allow(clippy::as_conversions)]
@@ -164,10 +165,11 @@ macro_rules! unsafe_impl {
164165
#[inline]
165166
fn is_bit_valid<AA: invariant::Aliasing + invariant::AtLeast<invariant::Shared>>(candidate: Maybe<'_, Self, AA>) -> bool {
166167
// SAFETY:
167-
// - The argument to `cast_unsized` is `|p| p as *mut _` as required
168-
// by that method's safety precondition.
169-
// - The caller has promised that the cast results in an object of
170-
// equal or lesser size.
168+
// - The cast preserves address. The caller has promised that the
169+
// cast results in an object of equal or lesser size, and so the
170+
// cast returns a pointer which references a subset of the bytes
171+
// of `p`.
172+
// - The cast preserves provenance.
171173
// - The caller has promised that the destination type has
172174
// `UnsafeCell`s at the same byte ranges as the source type.
173175
#[allow(clippy::as_conversions)]

‎src/pointer/ptr.rs

+88-202
Large diffs are not rendered by default.

‎src/util.rs

+20-25
Original file line numberDiff line numberDiff line change
@@ -28,10 +28,8 @@ use crate::{
2828
///
2929
/// `T: TransparentWrapper` implies that `T` has the same size as [`T::Inner`].
3030
/// Further, `T: TransparentWrapper<I>` implies that:
31-
/// - If `T::UnsafeCellVariance = Covariant`, then:
32-
/// - `T` has `UnsafeCell`s covering the same byte ranges as `T::Inner`
33-
/// - `T` has zero-sized `UnsafeCell`s (e.g., `UnsafeCell<()>`,
34-
/// `[UnsafeCell<u8>; 0]`, etc) at the same byte offsets as `T::Inner`
31+
/// - If `T::UnsafeCellVariance = Covariant`, then `T` has `UnsafeCell`s
32+
/// covering the same byte ranges as `T::Inner`.
3533
/// - If a `T` pointer satisfies the alignment invariant `I::Alignment`, then
3634
/// that same pointer, cast to `T::Inner`, satisfies the alignment invariant
3735
/// `<T::AlignmentVariance as AlignmentVariance<I::Alignment>>::Applied`.
@@ -112,8 +110,8 @@ impl<I: invariant::Validity> ValidityVariance<I> for Invariant {
112110
unsafe impl<T, I: Invariants> TransparentWrapper<I> for MaybeUninit<T> {
113111
type Inner = T;
114112

115-
// SAFETY: Per [1], `MaybeUninit<T>` has `UnsafeCell`s at the same byte
116-
// ranges as `Inner = T`, and `UnsafeCell`s at the same byte offsets as `T`.
113+
// SAFETY: Per [1], `MaybeUninit<T>` has `UnsafeCell`s covering the same
114+
// byte ranges as `Inner = T`.
117115
//
118116
// [1] TODO(#896): Write a safety proof before the next stable release.
119117
type UnsafeCellVariance = Covariant;
@@ -154,8 +152,8 @@ unsafe impl<T, I: Invariants> TransparentWrapper<I> for MaybeUninit<T> {
154152
unsafe impl<T: ?Sized, I: Invariants> TransparentWrapper<I> for ManuallyDrop<T> {
155153
type Inner = T;
156154

157-
// SAFETY: Per [1], `ManuallyDrop<T>` has `UnsafeCell`s at the same byte
158-
// ranges as `Inner = T`, and `UnsafeCell`s at the same byte offsets as `T`.
155+
// SAFETY: Per [1], `ManuallyDrop<T>` has `UnsafeCell`s covering the same
156+
// byte ranges as `Inner = T`.
159157
//
160158
// [1] TODO(#896): Write a safety proof before the next stable release.
161159
type UnsafeCellVariance = Covariant;
@@ -199,8 +197,8 @@ unsafe impl<T: ?Sized, I: Invariants> TransparentWrapper<I> for ManuallyDrop<T>
199197
unsafe impl<T, I: Invariants> TransparentWrapper<I> for Wrapping<T> {
200198
type Inner = T;
201199

202-
// SAFETY: Per [1], `Wrapping<T>` has `UnsafeCell`s at the same byte ranges
203-
// as `Inner = T`, and `UnsafeCell`s at the same byte offsets as `T`.
200+
// SAFETY: Per [1], `Wrapping<T>` has `UnsafeCell`s covering the same byte
201+
// ranges as `Inner = T`.
204202
//
205203
// [1] TODO(#896): Write a safety proof before the next stable release.
206204
type UnsafeCellVariance = Covariant;
@@ -249,8 +247,8 @@ unsafe impl<T, I: Invariants> TransparentWrapper<I> for Wrapping<T> {
249247
//
250248
// [1] Per https://doc.rust-lang.org/core/cell/struct.UnsafeCell.html#memory-layout:
251249
//
252-
// `UnsafeCell<T>`` has the same in-memory representation as its inner
253-
// type `T`.
250+
// `UnsafeCell<T>` has the same in-memory representation as its inner type
251+
// `T`.
254252
unsafe impl<T: ?Sized, I: Invariants> TransparentWrapper<I> for UnsafeCell<T> {
255253
type Inner = T;
256254

@@ -268,9 +266,9 @@ unsafe impl<T: ?Sized, I: Invariants> TransparentWrapper<I> for UnsafeCell<T> {
268266
//
269267
// [1] Per https://doc.rust-lang.org/std/cell/struct.UnsafeCell.html#memory-layout:
270268
//
271-
// `UnsafeCell<T>`` has the same in-memory representation as its inner
272-
// type `T`. A consequence of this guarantee is that it is possible to
273-
// convert between `T` and `UnsafeCell<T>`.
269+
// `UnsafeCell<T>` has the same in-memory representation as its inner type
270+
// `T`. A consequence of this guarantee is that it is possible to convert
271+
// between `T` and `UnsafeCell<T>`.
274272
type ValidityVariance = Covariant;
275273

276274
fn cast_into_inner(ptr: *mut UnsafeCell<T>) -> *mut T {
@@ -292,25 +290,22 @@ unsafe impl<T: ?Sized, I: Invariants> TransparentWrapper<I> for UnsafeCell<T> {
292290
}
293291
}
294292

295-
// SAFETY: We define `Unalign<T>` to be a `#[repr(C, packed)]` type wrapping a
296-
// single `T` field. Thus, `Unalign<T>` has the same size as `T`.
293+
// SAFETY: `Unalign<T>` promises to have the same size as `T`.
297294
//
298295
// See inline comments for other safety justifications.
299296
unsafe impl<T, I: Invariants> TransparentWrapper<I> for Unalign<T> {
300297
type Inner = T;
301298

302-
// SAFETY: `Unalign<T>` is a `#[repr(C, packed)]` type wrapping a single `T`
303-
// field, and so has `UnsafeCell`s at the same byte ranges as `Inner = T`,
304-
// and `UnsafeCell`s at the same byte offsets as `T`.
299+
// SAFETY: `Unalign<T>` promises to have `UnsafeCell`s covering the same
300+
// byte ranges as `Inner = T`.
305301
type UnsafeCellVariance = Covariant;
306302

307-
// SAFETY: Since `Unalign<T>` is `repr(packed)`, it has the alignment 1
308-
// regardless of `T`'s alignment. Thus, an aligned pointer to `Unalign<T>`
309-
// is not necessarily an aligned pointer to `T`.
303+
// SAFETY: Since `Unalign<T>` promises to have alignment 1 regardless of
304+
// `T`'s alignment. Thus, an aligned pointer to `Unalign<T>` is not
305+
// necessarily an aligned pointer to `T`.
310306
type AlignmentVariance = Invariant;
311307

312-
// SAFETY: `Unalign<T>` is a `#[repr(C, packed)]` type wrapping a single `T`
313-
// field, and so has the same validity as `T`.
308+
// SAFETY: `Unalign<T>` promises to have the same validity as `T`.
314309
type ValidityVariance = Covariant;
315310

316311
fn cast_into_inner(ptr: *mut Unalign<T>) -> *mut T {

‎src/wrappers.rs

+8-2
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,12 @@ use super::*;
3939
/// [`try_deref_mut`]: Unalign::try_deref_mut
4040
/// [`deref_unchecked`]: Unalign::deref_unchecked
4141
/// [`deref_mut_unchecked`]: Unalign::deref_mut_unchecked
42+
///
43+
/// # Safety
44+
///
45+
/// `Unalign<T>` is guaranteed to have the same size and bit validity as `T`,
46+
/// and to have [`UnsafeCell`]s covering the same byte ranges as `T`.
47+
/// `Unalign<T>` is guaranteed to have alignment 1.
4248
// NOTE: This type is sound to use with types that need to be dropped. The
4349
// reason is that the compiler-generated drop code automatically moves all
4450
// values to aligned memory slots before dropping them in-place. This is not
@@ -63,8 +69,8 @@ impl_known_layout!(T => Unalign<T>);
6369

6470
safety_comment! {
6571
/// SAFETY:
66-
/// - `Unalign<T>` is `repr(packed)`, so it is unaligned regardless of the
67-
/// alignment of `T`, and so we don't require that `T: Unaligned`
72+
/// - `Unalign<T>` promises to have alignment 1, and so we don't require
73+
/// that `T: Unaligned`.
6874
/// - `Unalign<T>` has the same bit validity as `T`, and so it is
6975
/// `FromZeros`, `FromBytes`, or `IntoBytes` exactly when `T` is as well.
7076
/// - `Immutable`: `Unalign<T>` has the same fields as `T`, so it contains

‎zerocopy-derive/src/lib.rs

+19-13
Original file line numberDiff line numberDiff line change
@@ -395,11 +395,13 @@ fn derive_try_from_bytes_struct(ast: &DeriveInput, strct: &DataStruct) -> proc_m
395395
mut candidate: ::zerocopy::Maybe<Self, A>
396396
) -> bool {
397397
true #(&& {
398-
// SAFETY: `project` is a field projection of `candidate`,
399-
// and `Self` is a struct type. The candidate will have
400-
// `UnsafeCell`s at exactly the same ranges as its
401-
// projection, because the projection is a field of the
402-
// candidate struct.
398+
// SAFETY:
399+
// - `project` is a field projection, and so it addresses a
400+
// subset of the bytes addressed by `slf`
401+
// - ..., and so it preserves provenance
402+
// - ..., and `*slf` is a struct, so `UnsafeCell`s exist at
403+
// the same byte ranges in the returned pointer's referent
404+
// as they do in `*slf`
403405
let field_candidate = unsafe {
404406
let project = |slf: *mut Self|
405407
::zerocopy::macro_util::core_reexport::ptr::addr_of_mut!((*slf).#field_names);
@@ -444,11 +446,13 @@ fn derive_try_from_bytes_union(ast: &DeriveInput, unn: &DataUnion) -> proc_macro
444446
mut candidate: ::zerocopy::Maybe<Self, A>
445447
) -> bool {
446448
false #(|| {
447-
// SAFETY: `project` is a field projection of `candidate`,
448-
// and `Self` is a union type. The candidate and projection
449-
// agree exactly on where their `UnsafeCell` ranges are,
450-
// because `Self: Immutable` is enforced by
451-
// `self_type_trait_bounds`.
449+
// SAFETY:
450+
// - `project` is a field projection, and so it addresses a
451+
// subset of the bytes addressed by `slf`
452+
// - ..., and so it preserves provenance
453+
// - Since `Self: Immutable` is enforced by
454+
// `self_type_trait_bounds`, neither `*slf` nor the
455+
// returned pointer's referent contain any `UnsafeCell`s
452456
let field_candidate = unsafe {
453457
let project = |slf: *mut Self|
454458
::zerocopy::macro_util::core_reexport::ptr::addr_of_mut!((*slf).#field_names);
@@ -514,9 +518,11 @@ fn derive_try_from_bytes_enum(ast: &DeriveInput, enm: &DataEnum) -> proc_macro2:
514518
quote!(
515519
use ::zerocopy::macro_util::core_reexport;
516520
// SAFETY:
517-
// - `cast` is implemented as required.
518-
// - By definition, `*mut Self` and `*mut [u8; size_of::<Self>()]`
519-
// are types of the same size.
521+
// - The closure is a pointer cast, and `Self` and `[u8;
522+
// size_of::<Self>()]` have the same size, so the returned pointer
523+
// addresses the same bytes as `p` subset of the bytes addressed
524+
// by `slf`
525+
// - ..., and so it preserves provenance
520526
// - Since we validate that this type is a field-less enum, it
521527
// cannot contain any `UnsafeCell`s. Neither does `[u8; N]`.
522528
let discriminant = unsafe { candidate.cast_unsized(|p: *mut Self| p as *mut [core_reexport::primitive::u8; core_reexport::mem::size_of::<Self>()]) };

‎zerocopy-derive/tests/struct_try_from_bytes.rs

+8-11
Original file line numberDiff line numberDiff line change
@@ -74,11 +74,10 @@ fn two_bad() {
7474
let candidate = unsafe { candidate.assume_initialized() };
7575

7676
// SAFETY:
77-
// - The cast `cast(p)` is implemented exactly as follows: `|p: *mut T| p as
78-
// *mut U`.
79-
// - The size of the object referenced by the resulting pointer is equal to
80-
// the size of the object referenced by `self`.
81-
// - `Two` does not contain any `UnsafeCell`s.
77+
// - The cast preserves address and size. As a result, the cast will address
78+
// the same bytes as `c`.
79+
// - The cast preserves provenance.
80+
// - Neither the input nor output types contain any `UnsafeCell`s.
8281
let candidate = unsafe { candidate.cast_unsized(|p| p as *mut Two) };
8382

8483
// SAFETY: `candidate`'s referent is as-initialized as `Two`.
@@ -105,12 +104,10 @@ fn un_sized() {
105104
let candidate = unsafe { candidate.assume_initialized() };
106105

107106
// SAFETY:
108-
// - The cast `cast(p)` is implemented exactly as follows: `|p: *mut T| p as
109-
// *mut U`.
110-
// - The size of the object referenced by the resulting pointer is equal to
111-
// the size of the object referenced by `self`.
112-
// - The alignment of `Unsized` is equal to the alignment of `[u8]`.
113-
// - `Unsized` does not contain any `UnsafeCell`s.
107+
// - The cast preserves address and size. As a result, the cast will address
108+
// the same bytes as `c`.
109+
// - The cast preserves provenance.
110+
// - Neither the input nor output types contain any `UnsafeCell`s.
114111
let candidate = unsafe { candidate.cast_unsized(|p| p as *mut Unsized) };
115112

116113
// SAFETY: `candidate`'s referent is as-initialized as `Two`.

‎zerocopy-derive/tests/union_try_from_bytes.rs

+8-10
Original file line numberDiff line numberDiff line change
@@ -69,11 +69,10 @@ fn two_bad() {
6969
let candidate = unsafe { candidate.assume_initialized() };
7070

7171
// SAFETY:
72-
// - The cast `cast(p)` is implemented exactly as follows: `|p: *mut T| p as
73-
// *mut U`.
74-
// - The size of the object referenced by the resulting pointer is equal to
75-
// the size of the object referenced by `self`.
76-
// - `Two` does not contain any `UnsafeCell`s.
72+
// - The cast preserves address and size. As a result, the cast will address
73+
// the same bytes as `c`.
74+
// - The cast preserves provenance.
75+
// - Neither the input nor output types contain any `UnsafeCell`s.
7776
let candidate = unsafe { candidate.cast_unsized(|p| p as *mut Two) };
7877

7978
// SAFETY: `candidate`'s referent is as-initialized as `Two`.
@@ -99,11 +98,10 @@ fn bool_and_zst() {
9998
let candidate = unsafe { candidate.assume_initialized() };
10099

101100
// SAFETY:
102-
// - The cast `cast(p)` is implemented exactly as follows: `|p: *mut T| p as
103-
// *mut U`.
104-
// - The size of the object referenced by the resulting pointer is equal to
105-
// the size of the object referenced by `self`.
106-
// - `BoolAndZst` does not contain any `UnsafeCell`s.
101+
// - The cast preserves address and size. As a result, the cast will address
102+
// the same bytes as `c`.
103+
// - The cast preserves provenance.
104+
// - Neither the input nor output types contain any `UnsafeCell`s.
107105
let candidate = unsafe { candidate.cast_unsized(|p| p as *mut BoolAndZst) };
108106

109107
// SAFETY: `candidate`'s referent is fully initialized.

0 commit comments

Comments
 (0)