Skip to content

Clarify safety of layout_for_ptr #117991

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Clarify safety of layout_for_ptr #117991

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 25 additions & 17 deletions library/core/src/alloc/layout.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,27 +174,35 @@ impl Layout {
/// allocate backing structure for `T` (which could be a trait
/// or other unsized type like a slice).
///
/// This is an unchecked version of [`for_value`][Self::for_value]
/// which takes a raw pointer instead of a reference.
///
/// # Safety
///
/// This function is only safe to call if the following conditions hold:
///
/// - If `T` is `Sized`, this function is always safe to call.
/// - If the unsized tail of `T` is:
/// - a [slice], then the length of the slice tail must be an initialized
/// integer, and the size of the *entire value*
/// (dynamic tail length + statically sized prefix) must fit in `isize`.
/// - a [trait object], then the vtable part of the pointer must point
/// to a valid vtable for the type `T` acquired by an unsizing coercion,
/// and the size of the *entire value*
/// (dynamic tail length + statically sized prefix) must fit in `isize`.
/// - an (unstable) [extern type], then this function is always safe to
/// call, but may panic or otherwise return the wrong value, as the
/// extern type's layout is not known. This is the same behavior as
/// [`Layout::for_value`] on a reference to an extern type tail.
/// - otherwise, it is conservatively not allowed to call this function.
/// The provided (possibly wide) pointer must describe a valid value layout.
/// Specifically:
///
/// - If `T` is a `Sized` type, this function is always safe to call and is
/// equivalent to [`new::<T>()`][Self::new]. The pointer is unused.
/// - If the unsized tail of `T` is a [slice], then the size of the *entire
/// value* (statically sized prefix plus dynamic tail) must fit in `isize`.
/// The pointer does not need to be [valid](crate::ptr#safety) for access,
/// as only the pointer metadata is used.
/// - If the unsized tail of `T` is a [trait object], then the wide pointer
/// metadata (the vtable reference) must originate from an unsizing or trait
/// upcasting coercion to this trait object tail, and the size of the *entire
/// value* (statically sized prefix plus dynamic tail) must fit in `isize`.
/// The pointer does not need to be [valid](crate::ptr#safety) for access,
/// as only the pointer metadata is used.
/// - For any other unsized tail kind (for example, unstable [extern types]),
/// it is *undefined behavior* to call this function. Unknown unsized tail
/// kinds may impose arbitrary requirements unknowable to current code.
///
/// [trait object]: ../../book/ch17-02-trait-objects.html
/// [extern type]: ../../unstable-book/language-features/extern-types.html
/// [extern types]: ../../unstable-book/language-features/extern-types.html
///
/// It is important to note that the last point means that it would be *unsound*
/// to implement `for_value` as an unconditional call to `for_value_raw`.
#[unstable(feature = "layout_for_ptr", issue = "69835")]
#[rustc_const_unstable(feature = "const_alloc_layout", issue = "67521")]
#[must_use]
Expand Down
91 changes: 49 additions & 42 deletions library/core/src/mem/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -346,43 +346,44 @@ pub const fn size_of_val<T: ?Sized>(val: &T) -> usize {

/// Returns the size of the pointed-to value in bytes.
///
/// This is usually the same as [`size_of::<T>()`]. However, when `T` *has* no
/// statically-known size, e.g., a slice [`[T]`][slice] or a [trait object],
/// then `size_of_val_raw` can be used to get the dynamically-known size.
/// This is an unchecked version of [`size_of_val`] which takes a raw pointer
/// instead of a reference.
///
/// # Safety
///
/// This function is only safe to call if the following conditions hold:
///
/// - If `T` is `Sized`, this function is always safe to call.
/// - If the unsized tail of `T` is:
/// - a [slice], then the length of the slice tail must be an initialized
/// integer, and the size of the *entire value*
/// (dynamic tail length + statically sized prefix) must fit in `isize`.
/// - a [trait object], then the vtable part of the pointer must point
/// to a valid vtable acquired by an unsizing coercion, and the size
/// of the *entire value* (dynamic tail length + statically sized prefix)
/// must fit in `isize`.
/// - an (unstable) [extern type], then this function is always safe to
/// call, but may panic or otherwise return the wrong value, as the
/// extern type's layout is not known. This is the same behavior as
/// [`size_of_val`] on a reference to a type with an extern type tail.
/// - otherwise, it is conservatively not allowed to call this function.
/// The provided (possibly wide) pointer must describe a valid value layout.
/// Specifically:
///
/// - If `T` is a `Sized` type, this function is always safe to call and is
/// equivalent to [`size_of::<T>()`][size_of]. The pointer is unused.
/// - If the unsized tail of `T` is a [slice], then the size of the *entire
/// value* (statically sized prefix plus dynamic tail) must fit in `isize`.
/// The pointer does not need to be [valid](crate::ptr#safety) for access,
/// as only the pointer metadata is used.
/// - If the unsized tail of `T` is a [trait object], then the wide pointer
/// metadata (the vtable reference) must originate from an unsizing or trait
/// upcasting coercion to this trait object tail, and the size of the *entire
/// value* (statically sized prefix plus dynamic tail) must fit in `isize`.
/// The pointer does not need to be [valid](crate::ptr#safety) for access,
/// as only the pointer metadata is used.
/// - For any other unsized tail kind (for example, unstable [extern types]),
/// it is *undefined behavior* to call this function. Unknown unsized tail
/// kinds may impose arbitrary requirements unknowable to current code.
///
/// [`size_of::<T>()`]: size_of
/// [trait object]: ../../book/ch17-02-trait-objects.html
/// [extern type]: ../../unstable-book/language-features/extern-types.html
/// [extern types]: ../../unstable-book/language-features/extern-types.html
///
/// It is important to note that the last point means that it would be *unsound*
/// to implement `size_of_val` as an unconditional call to `size_of_val_raw`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not seem desirable. Shouldn't we guarantee that if the argument would be sound to cast to a reference, then it may also be passed to size_of_val_raw and behavior is equivalent to size_of_val?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do agree it's unfortunate, but the reasoning for this is hedging with respect to future unsized tail kinds. It's certainly sufficient for any MetaSized type, and Rust can't yet manipulate properly !MetaSized types, but exotic size kinds can cause issues.

My go-to example is UnsafeCell<ThinCStr>. Getting the size of a value requires reading the value, which is a potential data race depending on whatever the external synchronization is.

This is, to be honest, "I don't want to deal with it" flavored UB. Even if size_of_val panics or is a post-monomorphization error, there does need to be an option which actually gets the layout (if it's knowable).

The lower floor is that if you can make a Box holding it, size_of_val_raw on a uniquely owned allocation with a valid (but potentially dropped) value is allowed (e.g. when dropping the last Weak<T> and deallocating). Beyond that is as far as I'm aware still undecided.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think size_of_val_raw should keep exposing the same underlying intrinsic as size_of_val. For these hypothetical future custom DST, whatever they do to keep size_of_val sound should also apply to size_of_val_raw. They will have to introduce new mechanisms anyway, so those should be used to deal with the extra constraints / provide new even-less-safe operations.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems prudent to me not to make any guarantees about those hypothetical types. If we call it UB now, we can still change that to some defined behavior later, and even now the implementation could use the same intrinsic and possibly work, or explode. Am I missing something?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already made the guarantee that the size of every &T can be computed in safe code -- by providing size_of_val. I don't see what we'd lose by saying that the same can be done for every *const T that satisfies all the requirements for an &T. Not making that guarantee, OTOH, sounds like it would create a lot of potential for confusion and uncertainty.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I've slowly come around to agreeing that guaranteeing that {if size_of_val(&*p) is valid, size_of_val_raw(p) is also valid and produces the same result} is reasonable. It is the "raw" version of the same operation, after all. The "future unsized tail kind" clause would then defer to this general case and call out that this requires the pointer to be valid for reads that may occur, unlike the other unsize kinds.

What may have convinced me the most was a realization that the "more raw" version should probably live in the ptr module and not the mem module anyway, or be a method of raw pointers like it is of the DynMetadata type. Having three versions of the same functionality isn't great (especially with Layout versions existing as well as the split size/align), but I don't think that's bad enough to justify

What's mostly being said in guaranteeing that imo is a soft & loose commitment to one of:

  • future unsized tailed types can always be passed to size_of_val (i.e. they satisfy Self: ?edition2015#Sized), causing erroneous behavior (complains to sanitizers) if they don't provide a way to get size from &self, then
    • panicking1 or
    • causing a postmono error if known at mono time with a panicking shim in the vtable; or that
  • future unsized tailed types cannot be passed to size_of_val (i.e. they do not satisfy Self: ?edition2015#Sized) unless they provide a way to get size from &self (e.g. Self: DynSized).

Unfortunately, the desire to avoid even implying any decision there is a primary part of why feature(layout_for_ptr) has been stuck in unstable limbo and why this PR proposes to weaken it to the minimally useful baseline (i.e. explicitly not saying anything about unsized tail kinds that do not yet exist).

"It's just size_of_val with these relaxations for 'MetaSized' types" does also feel conservative, but I've yet to find a way to word that without implying that the functionality works for unsize kinds that haven't been mentioned. And due to all that, size_of_val::<&UnsafeCell<ThinCStr>> haunts me.

Footnotes

  1. Returning wrong results is very scary. Consider a version of mem::swap or even take_mut that works for T: ?Sized by using a dynamically allocated heap buffer instead of a statically sized stack slot, checking the runtime queried size/align/metadata to ensure that it all lines up. Or even just feature(unsized_locals). These turn extern type claiming a layout of (0, 1) into a nicely hidden soundness footgun. ↩

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we make size_of_val panic / mono-time-error for such hypothetical future types, we can do the same for size_of_val_raw. If we find a way to statically prevent them from being passed to size_of_val, we can likewise do the same for size_of_val_raw. I think that's what you are saying?

///
/// # Examples
///
/// ```
/// #![feature(layout_for_ptr)]
/// use std::mem;
///
/// assert_eq!(4, mem::size_of_val(&5i32));
///
/// let x: [u8; 13] = [0; 13];
/// let y: &[u8] = &x;
/// let y: *const [u8] = &x;
/// assert_eq!(13, unsafe { mem::size_of_val_raw(y) });
/// ```
#[inline]
Expand Down Expand Up @@ -493,31 +494,37 @@ pub const fn align_of_val<T: ?Sized>(val: &T) -> usize {
/// Returns the [ABI]-required minimum alignment of the type of the value that `val` points to in
/// bytes.
///
/// Every reference to a value of the type `T` must be a multiple of this number.
/// This is an unchecked version of [`align_of_val`] which takes a raw pointer
/// instead of a reference.
///
/// [ABI]: https://en.wikipedia.org/wiki/Application_binary_interface
///
/// # Safety
///
/// This function is only safe to call if the following conditions hold:
///
/// - If `T` is `Sized`, this function is always safe to call.
/// - If the unsized tail of `T` is:
/// - a [slice], then the length of the slice tail must be an initialized
/// integer, and the size of the *entire value*
/// (dynamic tail length + statically sized prefix) must fit in `isize`.
/// - a [trait object], then the vtable part of the pointer must point
/// to a valid vtable acquired by an unsizing coercion, and the size
/// of the *entire value* (dynamic tail length + statically sized prefix)
/// must fit in `isize`.
/// - an (unstable) [extern type], then this function is always safe to
/// call, but may panic or otherwise return the wrong value, as the
/// extern type's layout is not known. This is the same behavior as
/// [`align_of_val`] on a reference to a type with an extern type tail.
/// - otherwise, it is conservatively not allowed to call this function.
/// The provided (possibly wide) pointer must describe a valid value layout.
/// Specifically:
///
/// - If `T` is a `Sized` type, this function is always safe to call and is
/// equivalent to [`size_of::<T>()`][size_of]. The pointer is unused.
/// - If the unsized tail of `T` is a [slice], then the size of the *entire
/// value* (statically sized prefix plus dynamic tail) must fit in `isize`.
/// The pointer does not need to be [valid](crate::ptr#safety) for access,
/// as only the pointer metadata is used.
/// - If the unsized tail of `T` is a [trait object], then the wide pointer
/// metadata (the vtable reference) must originate from an unsizing or trait
/// upcasting coercion to this trait object tail, and the size of the *entire
/// value* (statically sized prefix plus dynamic tail) must fit in `isize`.
/// The pointer does not need to be [valid](crate::ptr#safety) for access,
/// as only the pointer metadata is used.
/// - For any other unsized tail kind (for example, unstable [extern types]),
/// it is *undefined behavior* to call this function. Unknown unsized tail
/// kinds may impose arbitrary requirements unknowable to current code.
///
/// [trait object]: ../../book/ch17-02-trait-objects.html
/// [extern type]: ../../unstable-book/language-features/extern-types.html
/// [extern types]: ../../unstable-book/language-features/extern-types.html
///
/// It is important to note that the last point means that it would be *unsound*
/// to implement `align_of_val` as an unconditional call to `align_of_val_raw`.
///
/// # Examples
///
Expand Down
Loading