[WIP] Try from bytes #279

joshlf · 2023-08-22T22:30:21Z

Feel free to review or comment, but DO NOT MERGE; field projection needs to be landed in a separate PR first before this can be merged.

kupiakos · 2023-08-25T18:33:18Z

src/derive_util.rs

+                    // are valid, which means that the struct itself is valid.
+                    // That is the only precondition of `assume_valid_ref`.
+                    let slf = unsafe { bytes.assume_valid_ref() };
+                    // TODO: What about interior mutability? One approach would


For now, we just forbid interior mutability for TryFromBytes

Updated the comment.

While we're on the subject, though, how do we actually ban interior mutability in MaybeValid? Currently, we implement AsMaybeUninit for all sized types and slices of sized types, which means that impls like the following exist automatically thanks to those blanket impls:

unsafe impl<T> AsMaybeUninit for UnsafeCell<T> { type MaybeUninit = MaybeUninit<UnsafeCell<T>>; }

That, in turn, means that it's valid to write MaybeValid<UnsafeCell<T>>.

Maybe we could add safety preconditions to methods like assume_valid_ref that T::MaybeUninit doesn't contain any UnsafeCells? Then, so long as we only implement TryFromBytes for types without UnsafeCells, and we only ever emit calls to assume_valid_ref in such derived impls, then we should be okay.

Any other ideas? It'd be nice to not have this be so viral.

Update: See rust-lang/unsafe-code-guidelines#455 for more discussion.

how do we actually ban interior mutability in MaybeValid

There's no reason to. The only place where shared mutability gets involved in MaybeValid (through e.g. try_into_ref(&self) -> &T) is through the TryFromBytes::is_bit_valid method - that's the one that needs guarantees that there is only a single reference, either through unsafe or through banning UnsafeCell for TryFromBytes entirely.

That, in turn, means that it's valid to write MaybeValid<UnsafeCell>.

Yes, you can go from &mut MaybeValid<T> to &mut T on any type via a .write(). That's fine though - this doesn't involve shared mutability due to &mut.

Maybe we could add safety preconditions to methods like assume_valid_ref that T::MaybeUninit doesn't contain any UnsafeCells

Again, I don't see why this is in any way necessary.

See rust-lang/unsafe-code-guidelines#455 for more discussion.

That's essentially everything I told you offline - SB bans it, TB is fine. MaybeUninit does not interact with shared mutability, and neither should the AsMaybeUninit trait.

Okay, I believe I've completely handled the interior mutability issue with this update. Lmk if this looks sound to you?

For a bit more detail on my thinking (I think we're both on the same page now, but just to be sure), I had two concerns:

Is it sound to have &MaybeValid<T> and &T at the same time? This is required in order to call custom validators.

This is now sound thanks to the requirement that T and T::MaybeUninit have UnsafeCells at the same offsets.

Is it sound to have &MaybeValid<T> and &[u8] at the same time? This is required in order to implement try_from_ref and friends since they take a &[u8] and convert it to a &MaybeValid<T> in order to call is_bit_valid.

This is sound so long as T::MaybeUninit doesn't contain any UnsafeCells. Thanks to the UnsafeCells-at-the-same-locations requirement, this is sound so long as T doesn't contain any UnsafeCells. Since we only implement FromZeroes and FromBytes for MaybeValid<T> where T: TryFromBytes (and since T: TryFromBytes bans UnsafeCells), we're okay. Given those impls, we only use safe code (namely, Ref and FromBytes methods) to convert &[u8] to &MaybeValid<T>.

joshlf · 2023-08-29T04:25:27Z

src/macros.rs

+            // SAFETY: TODO
+            // TODO: Why is this `allow` necessary? Why is it acceptable?
+            #[allow(clippy::as_conversions)]
+            let $candidate = unsafe { &*(candidate as *const MaybeValid<Self> as *const $repr) };


@kupiakos since you're the as-casting expert, would you mind taking a look at the as casts in this PR and confirming that they're kosher? I'm pretty sure they are, but Clippy is making me question my sanity...

I'm not able to reproduce the clippy warning on the playground. Are you running a version of clippy with a bug?

It's only produced in 1.61 (our MSRV, which is tested in CI).

kupiakos · 2023-08-29T14:54:53Z

src/macros.rs

+///
+/// When implementing `TryFromBytes`:
+/// - If no `is_bit_valid` impl is provided, then it must be valid for
+///   `is_bit_valid` to unconditionally return `true`.


Why allow there to be no is_bit_valid impl at all? Why would true implementations be common enough to justify a default method?

It's used whenever you're implementing TryFromBytes for a type which is also FromBytes - in this PR, ~30 times.

Ah, I would instead have that be covered by the implementing macro instead of exposing that as an extra thing for external users to consider

We could do that, it'd just make the usage sites a lot more verbose. Currently, you can do:

unsafe_impl!(u8: TryFromBytes, FromZeroes, FromBytes, AsBytes, Unaligned); unsafe_impl!(i8: TryFromBytes, FromZeroes, FromBytes, AsBytes, Unaligned); unsafe_impl!(u16: TryFromBytes, FromZeroes, FromBytes, AsBytes); unsafe_impl!(i16: TryFromBytes, FromZeroes, FromBytes, AsBytes); unsafe_impl!(u32: TryFromBytes, FromZeroes, FromBytes, AsBytes); unsafe_impl!(i32: TryFromBytes, FromZeroes, FromBytes, AsBytes); unsafe_impl!(u64: TryFromBytes, FromZeroes, FromBytes, AsBytes); unsafe_impl!(i64: TryFromBytes, FromZeroes, FromBytes, AsBytes); unsafe_impl!(u128: TryFromBytes, FromZeroes, FromBytes, AsBytes); unsafe_impl!(i128: TryFromBytes, FromZeroes, FromBytes, AsBytes); unsafe_impl!(usize: TryFromBytes, FromZeroes, FromBytes, AsBytes); unsafe_impl!(isize: TryFromBytes, FromZeroes, FromBytes, AsBytes);

If you needed to supply a closure, you'd have to do:

unsafe_impl!(u8: FromZeroes, FromBytes, AsBytes, Unaligned); unsafe_impl!(i8: FromZeroes, FromBytes, AsBytes, Unaligned); unsafe_impl!(u16: FromZeroes, FromBytes, AsBytes); unsafe_impl!(i16: FromZeroes, FromBytes, AsBytes); unsafe_impl!(u32: FromZeroes, FromBytes, AsBytes); unsafe_impl!(i32: FromZeroes, FromBytes, AsBytes); unsafe_impl!(u64: FromZeroes, FromBytes, AsBytes); unsafe_impl!(i64: FromZeroes, FromBytes, AsBytes); unsafe_impl!(u128: FromZeroes, FromBytes, AsBytes); unsafe_impl!(i128: FromZeroes, FromBytes, AsBytes); unsafe_impl!(usize: FromZeroes, FromBytes, AsBytes); unsafe_impl!(isize: FromZeroes, FromBytes, AsBytes); unsafe_impl!(u8: TryFromBytes; |_: &MaybeValid<u8>| true); unsafe_impl!(i8: TryFromBytes; |_: &MaybeValid<i8>| true); unsafe_impl!(u16: TryFromBytes; |_: &MaybeValid<u16>| true); unsafe_impl!(i16: TryFromBytes; |_: &MaybeValid<i16>| true); unsafe_impl!(u32: TryFromBytes; |_: &MaybeValid<u32>| true); unsafe_impl!(i32: TryFromBytes; |_: &MaybeValid<i32>| true); unsafe_impl!(u64: TryFromBytes; |_: &MaybeValid<u64>| true); unsafe_impl!(i64: TryFromBytes; |_: &MaybeValid<i64>| true); unsafe_impl!(u128: TryFromBytes; |_: &MaybeValid<u128>| true); unsafe_impl!(i128: TryFromBytes; |_: &MaybeValid<i128>| true); unsafe_impl!(usize: TryFromBytes; |_: &MaybeValid<usize>| true); unsafe_impl!(isize: TryFromBytes; |_: &MaybeValid<isize>| true);

Since we require safety comments on all impls, you're forced to write down a justification for why a type is TryFromBytes. IMO it'd be pretty hard for both you and the reviewer to fail to notice that you'd implemented TryFromBytes unconditionally on a type that isn't actually TryFromBytes. Even if you didn't know how the macro works (namely, that omitting the validator means all bit patterns are valid), you presumably would be unsure of the conditions under which the type was TryFromBytes and would go read the macro docs to see how it behaves.

src/project.rs

src/lib.rs

Makes progress on #196

TODO: - Finish writing safety comments - Finish writing documentation - Add tests Makes progress on #5

joshlf · 2023-09-03T21:11:02Z

src/lib.rs

+
+impl<T> Default for MaybeValid<T> {
+    fn default() -> MaybeValid<T> {
+        MaybeValid { inner: MaybeUninit::uninit() }


This is unsound! It violates the initialization invariant of MaybeValid. This should be MaybeUninit::zeroed instead.

joshlf mentioned this pull request Aug 22, 2023

List of TODOs blocked on MSRV #67

Open

joshlf force-pushed the try-from-bytes branch from 422bcc1 to 9a8c96a Compare August 22, 2023 22:37

joshlf marked this pull request as draft August 22, 2023 22:41

joshlf force-pushed the try-from-bytes branch 7 times, most recently from de67836 to 72ddaae Compare August 23, 2023 07:45

kupiakos reviewed Aug 25, 2023

View reviewed changes

joshlf force-pushed the try-from-bytes branch 4 times, most recently from 38d943c to cc0a52d Compare August 29, 2023 03:03

joshlf mentioned this pull request Aug 29, 2023

[project] Initial commit #292

Draft

joshlf force-pushed the try-from-bytes branch from cc0a52d to 7e1eb18 Compare August 29, 2023 04:23

joshlf commented Aug 29, 2023

View reviewed changes

joshlf force-pushed the try-from-bytes branch 2 times, most recently from 79826fb to 1ecc13a Compare August 29, 2023 08:29

kupiakos reviewed Aug 29, 2023

View reviewed changes

src/project.rs Show resolved Hide resolved

joshlf mentioned this pull request Aug 29, 2023

MaybeUninit impls are unsound #299

Closed

joshlf commented Aug 29, 2023

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

joshlf force-pushed the try-from-bytes branch 6 times, most recently from 2e68ab3 to ac56c20 Compare August 31, 2023 23:12

[project] Initial commit

ca3cf29

Makes progress on #196

joshlf force-pushed the try-from-bytes branch from ac56c20 to 0c35d00 Compare September 2, 2023 04:46

[WIP] TryFromBytes

b042fa0

TODO: - Finish writing safety comments - Finish writing documentation - Add tests Makes progress on #5

joshlf force-pushed the try-from-bytes branch from 0c35d00 to b042fa0 Compare September 2, 2023 04:51

joshlf mentioned this pull request Sep 3, 2023

Support TryFromBytes - conditional conversion analogous to FromBytes #5

Closed

26 tasks

joshlf commented Sep 3, 2023

View reviewed changes

This was referenced Sep 3, 2023

Minimal implementation of TryFromBytes #316

Closed

Minimal implementation of TryFromBytes #317

Closed

joshlf closed this Nov 27, 2023

joshlf deleted the try-from-bytes branch November 27, 2023 18:48

Uh oh!

[WIP] Try from bytes #279

[WIP] Try from bytes #279

Uh oh!

Conversation

joshlf commented Aug 22, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kupiakos Aug 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

joshlf Sep 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kupiakos Aug 29, 2023 •

edited

Loading

joshlf Sep 3, 2023 •

edited

Loading