Invariant-parameterize `Ptr` and make `is_bit_valid` safe #699

jswrenn · 2023-12-08T23:14:57Z

Closes #715.

src/util.rs

joshlf

Some invariants are now invalid - e.g., in as_ref, the safety comment assumes that self.ptr is aligned even though it's inside a block in which ASSUME_ALIGN can be either true or false.

src/util.rs

joshlf · 2023-12-11T20:29:50Z

src/util.rs

+    /// The argument to `TryFromBytes::is_bit_valid`.
+    pub type MaybeValid<'a, T> = Ptr<'a, T, false>;


IMO the name MaybeValid implies that the only thing that's not guaranteed is validity, but here lifetime and aliasing are both not guaranteed. I think we should just use Ptr directly since it's more clear that the user should go look at the type rather than guess its semantics.

Ptr does guarantee lifetime (item 8):

/// INVARIANTS: /// 1. `ptr` is derived from some valid Rust allocation, `A` /// 2. `ptr` has the same provenance as `A` /// 3. `ptr` addresses a byte range which is entirely contained in `A` /// 4. `ptr` addresses a byte range whose length fits in an `isize` /// 5. `ptr` addresses a byte range which does not wrap around the address /// space /// 6. If `ASSUME_ALIGNED`, `ptr` is validly-aligned for `T` /// 7. `A` is guaranteed to live for at least `'a` /// 8. `T: 'a`

...but not aliasing. However, this type only appears as an argument to is_bit_valid (and is documented as such), and is_bit_valid, as a pre-condition, guarantees the appropriate aliasing requirements. The name MaybeValid, then, does convey the salient quality of the argument.

src/util.rs

src/pointer/ptr.rs

src/macros.rs

joshlf · 2023-12-27T14:00:38Z

src/pointer.rs

+
+/// A shorthand for a maybe-valid, maybe-aligned reference. Used as the argument
+/// to [`TryFromBytes::is_bit_valid`][crate::TryFromBytes::is_bit_valid].
+pub type MaybeValid<'a, T> =


Can we call this MaybeAlignedValid or something? I was confused by the naming when reading lib.rs until I came and looked at this doc comment.

What do you think about Maybe<T>? Its reading is its meaning.

Renamed to Maybe<T>. >:]

I'm signing off for the night, but feel free to bikeshed the name without me!

Can we add a separate #[doc(hidden)] attribute here (so that it remains even if we remove the outer one on the module) and leave a TODO comment to bikeshed this name if we're planning on making this public?

src/pointer.rs

src/pointer/ptr.rs

joshlf · 2023-12-27T14:09:37Z

src/lib.rs

-            // SAFETY: We uphold the safety contract of `is_bit_valid(elem)`, by
-            // precondition on the surrounding call to `is_bit_valid`. The
-            // memory referenced by `elem` is contained entirely within `c`, and
-            // satisfies the preconditions satisfied by `c`. By axiom, we assume
-            // that `Iterator:all` does not invalidate these preconditions
-            // (e.g., by writing to `elem`.) Since `elem` is derived from `c`,
-            // it is only possible for uninitialized bytes to occur in `elem` at
-            // the same bytes they occur within `c`.


src/pointer/ptr.rs

src/pointer.rs

src/pointer/ptr.rs

joshlf · 2023-12-27T21:47:33Z

src/pointer/ptr.rs

+        /// Note that this method does not consume `self`. Callers should be
+        /// aware that safe code may assume that the aliasing invariants of
+        /// `self` are not violated, and that the `NonNull` may be used in ways
+        /// that violate those assumptions.


Gotcha. I usually expect to see "assume" used in the context of unsafe code because it's the behavior of the code that matters (by contrast, with safe code, even things unrelated to the code's behavior matter because the compiler itself can make assumptions that could be broken).

I also think that the "Non null may be used" phrasing is confusing - I read it as "is allowed to be used", but I think you meant it to be read "could be used".

Maybe just something like:

Note that this method does not consume self. The caller should watch out for unsafe code which uses the returned NonNull in a way that violates the safety invariants of self.

(I originally said "it is the caller's responsibility...", but that feels weird given that this method is safe, and safe methods are unsound if their callers are required to do anything in order to uphold safety.)

joshlf · 2023-12-27T23:22:22Z

src/pointer/ptr.rs

+        mod sealed {
+            pub trait Sealed {}

-    /// No aliasing invariant.
-    pub const AnyAliasing: Aliasing = 1 << 0;
+            impl<$($set,)*> Sealed for ($($set,)*)
+            where
+                $($set: super::$set,)*
+            {}


We could also just put a single Sealed trait in util::sealed or something. We already have a top-level sealed module which we could move to util::sealed.

src/pointer/mod.rs

src/lib.rs

src/pointer/mod.rs

joshlf · 2024-01-17T20:37:28Z

src/util.rs

+            let raw = self.ptr.ptr.as_ptr();
+            // SAFETY: By invariant on `MaybeAligned`, `raw` contains
+            // validly-initialized data for `T`. The value is safe to read and
+            // return, because `T` is copy.
+            unsafe { core::ptr::read_unaligned(raw) }


Can you leave a TODO comment on read_unaligned to consider loosening the requirements at some point in the future? In particular, I could see us introducing a trait which represents "at least shared" aliasing, implementing it for both invariant::Shared and invariant::Exclusive, and using that trait (rather than a particular concrete type) as the aliasing bound.

src/pointer/mod.rs

joshlf · 2024-01-17T20:42:32Z

src/pointer.rs

+        #[inline]
+        pub fn read_unaligned(self) -> T
+        where
+            T: Copy,


Maybe let's open an issue to track having Ptr store the "original" type of its data? Probably not worth holding up this PR for that, and I suspect there are a lot of subtleties.

Closes #715.

jswrenn changed the title ~~[wip] Don't Ptrs point to validly-aligned data~~ [wip] Don't require Ptrs point to validly-aligned data Dec 8, 2023

jswrenn force-pushed the ptr-unaligned branch 2 times, most recently from a6b1487 to c1141bf Compare December 11, 2023 19:29

jswrenn commented Dec 11, 2023

View reviewed changes

src/util.rs Outdated Show resolved Hide resolved

joshlf requested changes Dec 11, 2023

View reviewed changes

jswrenn force-pushed the ptr-unaligned branch from c1141bf to e7534bc Compare December 26, 2023 23:23

jswrenn changed the title ~~[wip] Don't require Ptrs point to validly-aligned data~~ Invariant-parameterize Ptr and make is_bit_valid safe Dec 26, 2023

jswrenn force-pushed the ptr-unaligned branch 4 times, most recently from b3a0929 to 200757e Compare December 26, 2023 23:48

jswrenn marked this pull request as ready for review December 26, 2023 23:48

jswrenn commented Dec 26, 2023

View reviewed changes

src/pointer/ptr.rs Outdated Show resolved Hide resolved

jswrenn force-pushed the ptr-unaligned branch from 200757e to a02f5ce Compare December 26, 2023 23:58

jswrenn requested a review from joshlf December 26, 2023 23:58

jswrenn commented Dec 27, 2023

View reviewed changes

src/pointer/ptr.rs Outdated Show resolved Hide resolved

joshlf requested changes Dec 27, 2023

View reviewed changes

src/pointer/ptr.rs Outdated Show resolved Hide resolved

joshlf requested changes Dec 27, 2023

View reviewed changes

jswrenn force-pushed the ptr-unaligned branch from a02f5ce to a11d01c Compare December 27, 2023 19:45

joshlf requested changes Dec 27, 2023

View reviewed changes

jswrenn force-pushed the ptr-unaligned branch from 4268cf9 to c2f0b92 Compare December 28, 2023 00:09

jswrenn changed the title ~~Invariant-parameterize Ptr and make is_bit_valid safe~~ Invariant-parameterize Ptr and make is_bit_valid safe Dec 28, 2023

jswrenn force-pushed the ptr-unaligned branch 6 times, most recently from 2439de6 to f8fb38a Compare January 3, 2024 18:18

jswrenn mentioned this pull request Jan 9, 2024

Support TryFromBytes - conditional conversion analogous to FromBytes #5

Open

26 tasks

joshlf reviewed Jan 17, 2024

View reviewed changes

Invariant-parameterize Ptr and make is_bit_valid safe

925ec0a

Closes #715.

jswrenn force-pushed the ptr-unaligned branch from f8fb38a to 925ec0a Compare January 18, 2024 19:51

jswrenn requested a review from joshlf January 18, 2024 19:52

joshlf approved these changes Jan 18, 2024

View reviewed changes

joshlf enabled auto-merge January 18, 2024 19:53

joshlf added this pull request to the merge queue Jan 18, 2024

Merged via the queue into main with commit 644cdb8 Jan 18, 2024
127 checks passed

joshlf deleted the ptr-unaligned branch January 18, 2024 20:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Invariant-parameterize `Ptr` and make `is_bit_valid` safe #699

Invariant-parameterize `Ptr` and make `is_bit_valid` safe #699

jswrenn commented Dec 8, 2023 •

edited

joshlf left a comment

joshlf Dec 11, 2023

jswrenn Dec 11, 2023

joshlf Dec 27, 2023

jswrenn Dec 27, 2023

jswrenn Dec 28, 2023

joshlf Jan 17, 2024

joshlf Dec 27, 2023

jswrenn Jan 18, 2024

joshlf Dec 27, 2023

joshlf Dec 27, 2023

joshlf Jan 17, 2024

joshlf Jan 17, 2024

		/// The argument to `TryFromBytes::is_bit_valid`.
		pub type MaybeValid<'a, T> = Ptr<'a, T, false>;

Invariant-parameterize Ptr and make is_bit_valid safe #699

Invariant-parameterize Ptr and make is_bit_valid safe #699

Conversation

jswrenn commented Dec 8, 2023 • edited

joshlf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Invariant-parameterize `Ptr` and make `is_bit_valid` safe #699

Invariant-parameterize `Ptr` and make `is_bit_valid` safe #699

jswrenn commented Dec 8, 2023 •

edited