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

Amend Rc/Arc::from_raw() docs regarding unsafety #68099

Merged
merged 3 commits into from
Mar 22, 2020
Merged

Amend Rc/Arc::from_raw() docs regarding unsafety #68099

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
becebf3
Ammend Rc/Arc::from_raw() docs regarding unsafety
lukaslueg Jan 10, 2020
b4c96a9
Refine [Arc/Rc]::from_raw() docs
lukaslueg Jan 28, 2020
586c7e3
Make rc::RcBox and sync::ArcInner repr(C)
lukaslueg Feb 3, 2020
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
7 changes: 4 additions & 3 deletions src/liballoc/rc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -573,10 +573,11 @@ impl<T: ?Sized> Rc<T> {
/// Constructs an `Rc` from a raw pointer.
///
/// The raw pointer must have been previously returned by a call to a
/// [`Rc::into_raw`][into_raw].
/// [`Rc::into_raw`][into_raw] using the same `T`.
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 this is not quite true, or at least is likely over-constraining. For example, I believe it is fine to go between the unsized representation and the sized representation via pointer casts (e.g,. Rc<T> unsizes to Rc<dyn Debug> and then you into_raw, cast to T, and from_raw).

I'm not sure what exact wording we want to guarantee here, though, maybe just same T is in practice both sufficient and good enough for most use cases. Maybe something like "layout and alignment equivalent to T; note that this is essentially a reference to reference transmute if the T differs from the one used by into_raw, and so the same conditions apply"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I thought about that as well, it could be done. My thinking, however, is that it's too flaky to try to document under which conditions this is safe (and therefore binds all future changes to Rc/Arc).

Copy link
Member

Choose a reason for hiding this comment

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

How about this? (basically the same what @Mark-Simulacrum suggested)

The raw pointer must have been previously returned by a call to a Rc<U>::into_raw, where U must have the same alignment and memory layout as T (which is trivially satisfied if T == U). Note that if U is not T, this is basically like transmuting references of different types. See mem::transmute for more information on what restrictions apply in that case.

I usually wouldn't mind over-constraining, but if we document this exactly, this PR is just a doc bug fix.

///
/// This function is unsafe because improper use may lead to memory problems. For example, a
/// double-free may occur if the function is called twice on the same raw pointer.
/// This function is unsafe because improper use may lead to memory unsafety,
/// even if `T` is never accessed. For example, a double-free may occur if the function is
Copy link
Member

Choose a reason for hiding this comment

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

I guess this means "if Rc<T> is never accessed"`?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True as well, although the pitfall here is to assume accessing Rc<T> itself was safe if T is never touched.

even if Rc<T> or T is never accessed.

?

Copy link
Member

Choose a reason for hiding this comment

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

This whole paragraph seems a bit strange to me. Underspecified in a way. So what are the conditions for this method to be safe?

  • What's specified in the first paragraph (from a previous into_raw call)
  • Only call from_raw once per raw pointer.
  • T must (still) be initialized so that it can be dropped.

That's it, right? Shouldn't we explicitly document that? We can still mention the double free later.

Copy link
Member

Choose a reason for hiding this comment

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

It's worth noting (in my opinion) that the from_raw once per raw pointer makes relatively little sense, and IMO represents more hassle than it's worth. It means you can't technically store a pointer and then clone off copies of it, i.e., something like this would not be valid, but I think should be.

Other than that, I don't see any issues with the other points.

unsafe fn clone_arc(ptr: *const T) -> Arc<T> {
    let a = Arc::from_raw(ptr);
    let copy = a.clone();
    mem::forget(a);
    copy
}

let ptr = Arc::into_raw(some_arc);
clone_arc(ptr);
clone_arc(ptr);
clone_arc(ptr);

Copy link
Member

Choose a reason for hiding this comment

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

Good point. I guess we can just say "the user of from_raw has to make sure a specific T is only dropped once". Then it's the users responsibility.

/// called twice on the same raw pointer.
///
/// [into_raw]: struct.Rc.html#method.into_raw
///
Expand Down
7 changes: 4 additions & 3 deletions src/liballoc/sync.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -553,10 +553,11 @@ impl<T: ?Sized> Arc<T> {
/// Constructs an `Arc` from a raw pointer.
///
/// The raw pointer must have been previously returned by a call to a
/// [`Arc::into_raw`][into_raw].
/// [`Arc::into_raw`][into_raw], using the same `T`.
///
/// This function is unsafe because improper use may lead to memory problems. For example, a
/// double-free may occur if the function is called twice on the same raw pointer.
/// This function is unsafe because improper use may lead to memory unsafety,
/// even if `T` is never accessed. For example, a double-free may occur if the function is
/// called twice on the same raw pointer.
///
/// [into_raw]: struct.Arc.html#method.into_raw
///
Expand Down