Skip to content

Conversation

@joshlf
Copy link
Member

@joshlf joshlf commented Feb 20, 2024

Credit to @jswrenn for figuring out how to make this design
more misuse-proof.

Implementing TryFromBytes for UnsafeCell isn't possible without
changes to TryFromBytes. Previously, we passed is_bit_valid a Ptr
with shared aliasing. There is no way to soundly implement
is_bit_valid for UnsafeCell with a shared-aliased pointer, as
there's no way to soundly inspect the bytes of an UnsafeCell when
other references may exist to the same memory. The only way to inspect
the bytes of an UnsafeCell in a generic context is given a mutable
reference to one. This is in principle possible from TryFromBytes.
Notably, try_read_from reads a MaybeUninit<Self> into its local
stack frame, and that has UnsafeCells at the same byte ranges as
Self, so it is sound to construct a mutably-aliased Ptr to that
local variable. Unfortunately, is_bit_valid previously had no way of
reasoning about a mutably-aliased pointer.

In order to lift this restriction, we modify is_bit_valid to be
generic over aliasing using A: invariant::at_least::Shared. When A
is invariant::Exclusive, we can implement TryFromBytes::is_bit_valid
for UnsafeCell.

However, this isn't the whole story. We need to implement TryFromBytes
for UnsafeCell in the general case, even when a shared-aliased Ptr
is passed to is_bit_valid. Our first design was to define calling
UnsafeCell::is_bit_valid with a shared-aliased to be a panic condition
of is_bit_valid. Since we only call is_bit_valid from our own code
(is_bit_valid is #[doc(hidden)]), we can in theory simply avoid
calling UnsafeCell::is_bit_valid with a shared-aliased pointer.
However, that's very error-prone.

@jswrenn had the idea to instead use a post-monomorphization error, which
is implemented in Ptr::into_exclusive_or_post_monomorphization_error.
This allows us to test at compile time to see whether the argument to
UnsafeCell::is_bit_valid is a shared or exclusive pointer, and fail
compilation in the former case.

Some readers might wonder why we couldn't use a NoCell bound to
enforce that is_bit_valid could only be called with a shared-aliased
pointer when Self: NoCell. We experimented with this, but it turns out
that there is some existing code that cannot be made to work with this
pattern - the type system isn't smart enough to infer bounds in some
cases.

Makes progress on #5

@joshlf joshlf force-pushed the try-from-bytes-unsafecell-scratch-space branch 5 times, most recently from df97fb9 to d5351c1 Compare February 23, 2024 21:26
@joshlf joshlf changed the base branch from main to test-impls-uses-box February 23, 2024 21:28
@joshlf joshlf force-pushed the try-from-bytes-unsafecell-scratch-space branch 2 times, most recently from 5cd1351 to 1e9980a Compare February 23, 2024 21:42
@joshlf joshlf marked this pull request as ready for review February 23, 2024 21:42
@joshlf joshlf requested a review from jswrenn February 23, 2024 21:43
Base automatically changed from test-impls-uses-box to main February 23, 2024 21:46
@joshlf joshlf force-pushed the try-from-bytes-unsafecell-scratch-space branch from 1e9980a to 173e1bc Compare February 23, 2024 21:48
@joshlf joshlf force-pushed the try-from-bytes-unsafecell-scratch-space branch from 173e1bc to 49af394 Compare February 24, 2024 04:27
@joshlf joshlf enabled auto-merge February 24, 2024 17:00
Credit to [email protected] for figuring out how to make this design
more misuse-proof.

Implementing `TryFromBytes` for `UnsafeCell` isn't possible without
changes to `TryFromBytes`. Previously, we passed `is_bit_valid` a `Ptr`
with shared aliasing. There is no way to soundly implement
`is_bit_valid` for `UnsafeCell` with a shared-aliased pointer, as
there's no way to soundly inspect the bytes of an `UnsafeCell` when
other references may exist to the same memory. The only way to inspect
the bytes of an `UnsafeCell` in a generic context is given a mutable
reference to one. This is in principle possible from `TryFromBytes`.
Notably, `try_read_from` reads a `MaybeUninit<Self>` into its local
stack frame, and that has `UnsafeCell`s at the same byte ranges as
`Self`, so it is sound to construct a mutably-aliased `Ptr` to that
local variable. Unfortunately, `is_bit_valid` previously had no way of
reasoning about a mutably-aliased pointer.

In order to lift this restriction, we modify `is_bit_valid` to be
generic over aliasing using `A: invariant::at_least::Shared`. When `A`
is `invariant::Exclusive`, we can implement `TryFromBytes::is_bit_valid`
for `UnsafeCell`.

However, this isn't the whole story. We need to implement `TryFromBytes`
for `UnsafeCell` in the general case, even when a shared-aliased `Ptr`
is passed to `is_bit_valid`. Our first design was to define calling
`UnsafeCell::is_bit_valid` with a shared-aliased to be a panic condition
of `is_bit_valid`. Since we only call `is_bit_valid` from our own code
(`is_bit_valid` is `#[doc(hidden)]`), we can in theory simply avoid
calling `UnsafeCell::is_bit_valid` with a shared-aliased pointer.
However, that's very error-prone.

jswrenn had the idea to instead use a post-monomorphization error, which
is implemented in `Ptr::into_exclusive_or_post_monomorphization_error`.
This allows us to test *at compile time* to see whether the argument to
`UnsafeCell::is_bit_valid` is a shared or exclusive pointer, and fail
compilation in the former case.

Some readers might wonder why we couldn't use a `NoCell` bound to
enforce that `is_bit_valid` could only be called with a shared-aliased
pointer when `Self: NoCell`. We experimented with this, but it turns out
that there is some existing code that cannot be made to work with this
pattern - the type system isn't smart enough to infer bounds in some
cases.

Makes progress on #5
@joshlf joshlf force-pushed the try-from-bytes-unsafecell-scratch-space branch from 49af394 to 5d1a8e1 Compare February 26, 2024 14:59
@joshlf joshlf added this pull request to the merge queue Feb 26, 2024
Merged via the queue into main with commit 54c583c Feb 26, 2024
@joshlf joshlf deleted the try-from-bytes-unsafecell-scratch-space branch February 26, 2024 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants