Skip to content
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

Jump to bottom

Weak refcounted pointers can dangle #80407

Closed
wants to merge 1 commit into from
Closed

Weak refcounted pointers can dangle #80407

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
20 changes: 9 additions & 11 deletions library/alloc/src/rc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1780,11 +1780,11 @@ impl<T: ?Sized> Weak<T> {
let ptr: *mut RcBox<T> = NonNull::as_ptr(self.ptr);

// SAFETY: we must offset the pointer manually, and said pointer may be
// a dangling weak (usize::MAX) if T is sized. data_offset is safe to call,
// because we know that a pointer to unsized T was derived from a real
// unsized T, as dangling weaks are only created for sized T. wrapping_offset
// is used so that we can use the same code path for the non-dangling
// unsized case and the potentially dangling sized case.
// a dangling weak (usize::MAX). data_offset is safe to call, because we know
// that a pointer to unsized T was acquired by unsize coercion from an Rc/Weak
// of sized T. (Weak::new can only construct dangling pointers for sized T).
// wrapping_offset is used so that we can use the same code path for the
// dangling and non-dangling cases.
unsafe {
let offset = data_offset(ptr as *mut T);
set_data_ptr(ptr as *mut T, (ptr as *mut u8).wrapping_offset(offset))
Expand Down Expand Up @@ -1874,7 +1874,7 @@ impl<T: ?Sized> Weak<T> {
let offset = unsafe { data_offset(ptr) };

// Reverse the offset to find the original RcBox.
// SAFETY: we use wrapping_offset here because the pointer may be dangling (but only if T: Sized).
// SAFETY: we use wrapping_offset here because the pointer may be dangling.
let ptr = unsafe {
set_data_ptr(ptr as *mut RcBox<T>, (ptr as *mut u8).wrapping_offset(-offset))
};
Expand Down Expand Up @@ -2208,13 +2208,11 @@ impl<T: ?Sized> Unpin for Rc<T> {}
/// This has the same safety requirements as `align_of_val_raw`. In effect:
///
/// - This function is safe for any argument if `T` is sized, and
/// - if `T` is unsized, the pointer must have appropriate pointer metadata
/// acquired from the real instance that you are getting this offset for.
/// - if `T` is unsized, the pointer must have been acquired from an unsize
/// coercion (but may be invalid, such as from Weak::new).
Copy link
Member

Choose a reason for hiding this comment

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

This is not "the same safety requirements as align_of_val_raw", though. align_of_val_raw quite deliberately only talks slices, trait objects, and extern types -- for all other unsiezd tails, "it is not allowed to call this function".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is super awkward to specify cleanly without copying over the entire docs from align_of_val_raw. When I made this adjustment, I figured that "comes from an unsize coercion" would be a tighter bound than align_of_val_raw spells out, since the only types of unsize coercion are for slices and trait objects. "Comes from unsize coercion" is in fact a simpler spelling of the requirement for trait objects (valid vptr). For slices, it's exactly equivalent iff it's impossible to name a fixed-size array type that has a layout close to the size of isize::MAX (such that RcInner<T> is too big). I think that the error "values of the type rc::RcBox<[u128; N]> are too big for the current architecture" is enough for that, though I do think relying on said error for soundness is suboptimal.

(Also, we could potentially relax align_of_val_raw to limit the magnitude of alignment (which it probably should anyway) rather than size.)

Copy link
Member

@RalfJung RalfJung Dec 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

The reason I don't like "comes from an unsize coercion" is that it is open-ended -- it is true for the unsizing coercions that currently exist, but that set might change in the future. The align_of_val_raw docs are quite deliberately stated in a way that is not open-ended in this way.

That's also why "comes from an unsize coercion" is not tighter than the bound on align_of_val_raw.

unsafe fn data_offset<T: ?Sized>(ptr: *const T) -> isize {
// Align the unsized value to the end of the `RcBox`.
// Because it is ?Sized, it will always be the last field in memory.
// Note: This is a detail of the current implementation of the compiler,
// and is not a guaranteed language detail. Do not rely on it outside of std.
// As RcBox is #[repr(C)], it will always be the last field in memory.
unsafe { data_offset_align(align_of_val_raw(ptr)) }
}

Expand Down
14 changes: 7 additions & 7 deletions library/alloc/src/sync.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,6 @@ pub struct Weak<T: ?Sized> {
// `Weak::new` sets this to `usize::MAX` so that it doesn’t need
// to allocate space on the heap. That's not a value a real pointer
// will ever have because RcBox has alignment at least 2.
// This is only possible when `T: Sized`; unsized `T` never dangle.
ptr: NonNull<ArcInner<T>>,
}

Expand Down Expand Up @@ -1566,11 +1565,11 @@ impl<T: ?Sized> Weak<T> {
let ptr: *mut ArcInner<T> = NonNull::as_ptr(self.ptr);

// SAFETY: we must offset the pointer manually, and said pointer may be
// a dangling weak (usize::MAX) if T is sized. data_offset is safe to call,
// because we know that a pointer to unsized T was derived from a real
// unsized T, as dangling weaks are only created for sized T. wrapping_offset
// is used so that we can use the same code path for the non-dangling
// unsized case and the potentially dangling sized case.
// a dangling weak (usize::MAX). data_offset is safe to call, because we know
// that a pointer to unsized T was acquired by unsize coercion from an Rc/Weak
// of sized T. (Weak::new can only construct dangling pointers for sized T).
// wrapping_offset is used so that we can use the same code path for the
// dangling and non-dangling cases.
unsafe {
let offset = data_offset(ptr as *mut T);
set_data_ptr(ptr as *mut T, (ptr as *mut u8).wrapping_offset(offset))
Expand Down Expand Up @@ -1628,6 +1627,7 @@ impl<T: ?Sized> Weak<T> {
/// takes ownership of one weak reference currently represented as a raw pointer (the weak
/// count is not modified by this operation) and therefore it must be paired with a previous
/// call to [`into_raw`].
///
/// # Examples
///
/// ```
Expand Down Expand Up @@ -1660,7 +1660,7 @@ impl<T: ?Sized> Weak<T> {
let offset = unsafe { data_offset(ptr) };

// Reverse the offset to find the original ArcInner.
// SAFETY: we use wrapping_offset here because the pointer may be dangling (but only if T: Sized)
// SAFETY: we use wrapping_offset here because the pointer may be dangling.
let ptr = unsafe {
set_data_ptr(ptr as *mut ArcInner<T>, (ptr as *mut u8).wrapping_offset(-offset))
};
Expand Down