Use pointer offset instead of deref for A/Rc::into_raw#67339
Merged
bors merged 2 commits intorust-lang:masterfrom Jan 16, 2020
Merged
Use pointer offset instead of deref for A/Rc::into_raw#67339bors merged 2 commits intorust-lang:masterfrom
bors merged 2 commits intorust-lang:masterfrom
Conversation
This comment has been minimized.
This comment has been minimized.
rust-highfive
added
the
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
Contributor
Author
|
Oh right, fat pointers... (sorry for the number of force pushes: until I get fully moved over to my new PC the one I'm working on has some messed up dev environment and can't even build rustc fully anymore...) |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Member
|
r? @RalfJung |
This comment has been minimized.
This comment has been minimized.
Centril
reviewed
Dec 16, 2019
Member
|
(I'm sick and my backlog is already quite long, no promises when I'll be able to get around to this.) |
RalfJung
reviewed
Dec 22, 2019
Member
|
The general approach looks good to me, but I feel this is T-libs territory, so r? @SimonSapin |
Contributor
|
Per r? @sfackler |
Member
|
@bors r+ |
Collaborator
|
📌 Commit eb77f7e has been approved by |
bors
removed
the
S-waiting-on-review
bors
added
the
S-waiting-on-bors
|
Oops :D |
|
r? @sfackler |
Collaborator
bors
added a commit
that referenced
this pull request
Jan 16, 2020
Use pointer offset instead of deref for A/Rc::into_raw Internals thread: https://internals.rust-lang.org/t/rc-and-internal-mutability/11463/2?u=cad97 The current implementation of (`A`)`Rc::into_raw` uses the `Deref::deref` implementation to get the pointer-to-data that is returned. This is problematic in the proposed Stacked Borrow rules, as this only gets shared provenance over the data location. (Note that the strong/weak counts are `UnsafeCell` (`Cell`/`Atomic`) so shared provenance can still mutate them, but the data itself is not.) When promoted back to a real reference counted pointer, the restored pointer can be used for mutation through `::get_mut` (if it is the only surviving reference). However, this mutates through a pointer ultimately derived from a `&T` borrow, violating the Stacked Borrow rules. There are three known potential solutions to this issue: - Stacked Borrows is wrong, liballoc is correct. - Fully admit (`A`)`Rc` as an "internal mutability" type and store the data payload in an `UnsafeCell` like the strong/weak counts are. (Note: this is not needed generally since the `RcBox`/`ArcInner` is stored behind a shared `NonNull` which maintains shared write provenance as a raw pointer.) - Adjust `into_raw` to do direct manipulation of the pointer (like `from_raw`) so that it maintains write provenance and doesn't derive the pointer from a reference. This PR implements the third option, as recommended by @RalfJung. Potential future work: provide `as_raw` and `clone_raw` associated functions to allow the [`&T` -> (`A`)`Rc<T>` pattern](https://internals.rust-lang.org/t/rc-and-internal-mutability/11463/2?u=cad97) to be used soundly without creating (`A`)`Rc` from references.
Collaborator
|
☀️ Test successful - checks-azure |
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
11 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Internals thread: https://internals.rust-lang.org/t/rc-and-internal-mutability/11463/2?u=cad97
The current implementation of (
A)Rc::into_rawuses theDeref::derefimplementation to get the pointer-to-data that is returned. This is problematic in the proposed Stacked Borrow rules, as this only gets shared provenance over the data location. (Note that the strong/weak counts areUnsafeCell(Cell/Atomic) so shared provenance can still mutate them, but the data itself is not.) When promoted back to a real reference counted pointer, the restored pointer can be used for mutation through::get_mut(if it is the only surviving reference). However, this mutates through a pointer ultimately derived from a&Tborrow, violating the Stacked Borrow rules.There are three known potential solutions to this issue:
A)Rcas an "internal mutability" type and store the data payload in anUnsafeCelllike the strong/weak counts are. (Note: this is not needed generally since theRcBox/ArcInneris stored behind a sharedNonNullwhich maintains shared write provenance as a raw pointer.)into_rawto do direct manipulation of the pointer (likefrom_raw) so that it maintains write provenance and doesn't derive the pointer from a reference.This PR implements the third option, as recommended by @RalfJung.
Potential future work: provide
as_rawandclone_rawassociated functions to allow the&T-> (A)Rc<T>pattern to be used soundly without creating (A)Rcfrom references.