std: Fix segfaulting Weak<!>::new
#49248
Closed
Conversation
This file contains 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
|
r? @sfackler |
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
alexcrichton
force-pushed
the
weak-new-segfault
branch
from
d73508c to
b86bd04
Compare
|
Ping from triage @sfackler! This PR needs your review! |
sfackler
reviewed
Mar 26, 2018
src/liballoc/arc.rs
Outdated
| @@ -991,11 +991,11 @@ impl<T> Weak<T> { | |||
| pub fn new() -> Weak<T> { | |||
| unsafe { | |||
| Weak { | |||
| ptr: Box::into_raw_non_null(box ArcInner { | |||
| ptr: Box::into_raw_non_null(Box::new(ArcInner { | |||
There was a problem hiding this comment.
Can we add a comment here saying why it's important that we don't use box?
sfackler
reviewed
Mar 26, 2018
| // branch. | ||
| unsafe { | ||
| let ptr = self.ptr.as_ref(); | ||
| if mem::size_of_val(ptr) == 0 { |
There was a problem hiding this comment.
This is still inherently bogus since it's creating a reference to an uninhabited type, right?
There was a problem hiding this comment.
I think we're actually safe here in the sense that we're not telling LLVM this is illegal today and this compiles ok, but it's certainly standing on quite the precipice. If you'd prefer though I could probably add enough conditions here to skip this check if T isn't a dynamically sized type and only run this check if it's a DST.
|
This seems like a really hacky fix but if it works it works. Hopefully we can move to a "real" fix in the future. |
This commit is a fix for rust-lang#48493 where calling `Weak::new` where `T` is an uninhabited type would segfault. The cause for this issue was pretty subtle and the fix here is mostly a holdover until rust-lang#47650 is implemented. The `Weak<!>` struct internally contains a `NonNull<RcBox<!>>`. The `RcBox<!>` type is uninhabited, however, as it directly embeds the `!` type. Consequently the size of `RcBox<!>` is zero, which means that `NonNull<RcBox<!>>` always contains a pointer with a value of 1. Currently all boxes of zero-sized-types are actually pointers to the address 1 (as they shouldn't be read anyway). The problem comes about when later on we have a method called `Weak::inner` which previously returned `&RcBox<T>`. This was actually invalid because the instance of `&RcBox<T> ` never existed (only the uninitialized part). This means that when we try to actually modify `&RcBox`'s weak count in the destructor for `Weak::new` we're modifying the address 1! This ends up causing a segfault. This commit takes the strategy of modifying the `Weak::inner` method to return an `Option<&RcBox<T>>` that is `None` whenever the size of `RcBox<T>` is 0 (so it couldn't actually exist). This does unfortunately add more dispatch code to operations like `Weak<Any>::clone`. Eventually the "correct" fix for this is to have `RcBox<T>` store a union of `T` and a ZST like `()`, and that way the size of `RcBox<T>` will never be 0 and we won't need this branch. Until rust-lang#47650 is implemented, though, we can't use `union`s and unsized types together. Closes rust-lang#48493
alexcrichton
force-pushed
the
weak-new-segfault
branch
from
b86bd04 to
06cd4cf
Compare
|
Updated! |
|
|
Alas :( |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This commit is a fix for #48493 where calling
Weak::newwhereTis anuninhabited type would segfault. The cause for this issue was pretty subtle and
the fix here is mostly a holdover until #47650 is implemented.
The
Weak<!>struct internally contains aNonNull<RcBox<!>>. TheRcBox<!>type is uninhabited, however, as it directly embeds the
!type. Consequentlythe size of
RcBox<!>is zero, which means thatNonNull<RcBox<!>>alwayscontains a pointer with a value of 1. Currently all boxes of zero-sized-types
are actually pointers to the address 1 (as they shouldn't be read anyway).
The problem comes about when later on we have a method called
Weak::innerwhich previously returned
&RcBox<T>. This was actually invalid because theinstance of
&RcBox<T>never existed (only the uninitialized part). Thismeans that when we try to actually modify
&RcBox's weak count in thedestructor for
Weak::newwe're modifying the address 1! This ends up causing asegfault.
This commit takes the strategy of modifying the
Weak::innermethod to returnan
Option<&RcBox<T>>that isNonewhenever the size ofRcBox<T>is 0 (soit couldn't actually exist). This does unfortunately add more dispatch code to
operations like
Weak<Any>::clone. Eventually the "correct" fix for this is tohave
RcBox<T>store a union ofTand a ZST like(), and that way the sizeof
RcBox<T>will never be 0 and we won't need this branch. Until #47650 isimplemented, though, we can't use
unions and unsized types together.Closes #48493