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.

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

Weak::new() segfaults on uninhabited types #48493

Closed
jleedev opened this issue Feb 24, 2018 · 25 comments

Comments

@jleedev
Copy link

commented Feb 24, 2018

This gives a segmentation fault:

fn main() {
    enum Void {}
    std::rc::Weak::<Void>::new();
}

There's no rust backtrace to show; I could reproduce this on various platforms and compiler versions.

@vitalyd

This comment has been minimized.

Copy link

commented Feb 24, 2018

std::mem::uninitialized::<T>() that gets called in Weak::new() is UB. The x86 assembly that gets generated, however, doesn’t have the ud illegal instruction but rather it does a bogus movq $0, 1

This is a nice one :)

@sfackler

This comment has been minimized.

Copy link
Member

commented Feb 24, 2018

Seems like we should be able to fix it by replacing the T in the data with a

union MaybeInitialized<T> {
    initialized: T,
    uninitialized: (),
}

right?

@vitalyd

This comment has been minimized.

Copy link

commented Feb 24, 2018

@sfackler, yeah seems like that might work (admittedly, I’ve not thought about it too deeply). I’m assuming the unstable T: Copy requirement for the union wouldn’t be an issue for std types right?

@vitalyd

This comment has been minimized.

Copy link

commented Feb 24, 2018

Or rather, std would allow T: !Copy (the T: Copy is required for stable).

@sfackler

This comment has been minimized.

Copy link
Member

commented Feb 24, 2018

@sfackler

This comment has been minimized.

Copy link
Member

commented Mar 9, 2018

There is a bit of a complication unfortunately - Unsize isn't implemented for unions the same way it's implemented for structs currently which breaks the CoerceUnsized impls :(

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Mar 12, 2018

@sfackler in the meantime while we wait for #47650 I wonder, would it be possible to fix this otherwise? Could we perhaps, for example, use MaybeInitialized to create a value and place it in memory?

@sfackler

This comment has been minimized.

Copy link
Member

commented Mar 12, 2018

Not sure - if the inner struct contains a bare T, I think it's undefined behavior for an instance of it to exist at all if T is uninhabited. We could use Option<T> and an accessor that does match value { Some(ref v) => v, None => intrinsics::unreachable() } I think?

@sfackler

This comment has been minimized.

Copy link
Member

commented Mar 12, 2018

The Option<T> approach adds an extra tag byte in some cases though unfortunately.

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Mar 12, 2018

Hm yeah ideally we'd avoid changing the actual memory layout. Do you have a minimal example of where the segfault is coming from? The playground indicates that zero is being stored at the address 1, but I'm not actually sure where that comes from...

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Mar 12, 2018

Hm so actually this reduction indicates that the problem is actually with the destructor here, not with the constructor. Specifically Box<RcBox<Void>> is always represented as a pointer to 1 because sizeof(RcBox<Void>) == 0 (AFAIK).

That means when we attempt to access the pointer in the destructor it segfaults as we're storing the decremented refcount to address 0.

I wonder if this fix doesn't need to hold off until we get unsized unions? (or rather it'd still be present even if we had unsized unions)

@rkruppe

This comment has been minimized.

Copy link
Member

commented Mar 12, 2018

RcBox contains the refcounts so its size should never be zero. The reason RcBox<Void> has size zero is because it's uninhabited (due to containing the uninhabited Void), if Void was for example a unit struct then sizeof(RcBox<Void>) would be 2 * sizeof(Cell<usize>) + sizeof(Void).

@sfackler

This comment has been minimized.

Copy link
Member

commented Mar 12, 2018

Is size_of a const fn yet? We could maybe replace the T with

struct MaybeInitialized<T> {
    _align: [T; 0],
    data: [u8; size_of::<T>()],
}
@rkruppe

This comment has been minimized.

Copy link
Member

commented Mar 12, 2018

@sfackler That doesn't work for DSTs either (since size_of requires T: Sized).

@sfackler

This comment has been minimized.

Copy link
Member

commented Mar 12, 2018

Oh right :(

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Mar 12, 2018

@rkruppe oh bug or not mem::size_of::<RcBox<Void>>() is currently 0

@rkruppe

This comment has been minimized.

Copy link
Member

commented Mar 12, 2018

@alexcrichton I was trying to say that the size being zero (and hence the dangling pointer being dereferenced in the destructor) is just is a consequence of the uninhabitedness.

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Mar 12, 2018

Ah yeah definitely, but I also see now how MaybeInitialized would actually solve this issue because the size of that union would indeed be 0 and would force the outer struct to not necessarily be 0 as well.

alexcrichton added a commit to alexcrichton/rust that referenced this issue Mar 21, 2018

std: Fix segfaulting `Weak<!>::new`
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

This comment has been minimized.

Copy link
Member

commented Mar 21, 2018

I've sent a PR for this at #49248

@vitalyd

This comment has been minimized.

Copy link

commented Mar 21, 2018

@alexcrichton, not sure it really matters here but the constructor also fails:

fn main() {
    enum Void {}
    std::mem::forget(std::rc::Weak::<Void>::new());
}

The above still generates a bogus mov instruction.

alexcrichton added a commit to alexcrichton/rust that referenced this issue Mar 22, 2018

std: Fix segfaulting `Weak<!>::new`
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

This comment has been minimized.

Copy link
Member

commented Mar 22, 2018

@vitalyd interesting! Apparently that's reduced down to:

#![feature(box_syntax)]

use std::mem;

enum Void {}

struct RcBox<T> {
    _a: usize,
    _b: T,
}

pub unsafe fn bar() {
    mem::forget(box RcBox {
        _a: 1,
        _b: mem::uninitialized::<Void>(),
    });
}

The box keyword is necessary there to reproduce the issue (interestingly enough). I've fixed that in #49248 I believe

@alexcrichton alexcrichton referenced this issue Mar 22, 2018
0 of 4 tasks complete

alexcrichton added a commit to alexcrichton/rust that referenced this issue Mar 26, 2018

std: Fix segfaulting `Weak<!>::new`
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

This comment has been minimized.

Copy link
Member

commented Apr 19, 2018

De-nominating as this is blocked on #47650

@SimonSapin

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2018

I cannot reproduce this segfault anymore, neither with the original test case nor the one in #48493 (comment). I believe this is because the size of RcBox<!> was changed from zero to that of RcBox<()> (#49298, #50622), so we’re no longer not-allocating zero bytes and then writing to a dangling pointer.

With the issue fixed in practice as far as I can tell and #51851 removing the use of mem::uninitialized (and the question of whether that is UB for uninhabited types even if we end up never touching that value), I think that #51851 can be marked as closing this bug.

@SimonSapin

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2018

(Tested in 1.28.0-nightly (84804c3 2018-06-26).)

@SimonSapin

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2018

Sorry, the above with only without optimization. With optimizations, the second test case fails with "illegal instruction" in Nightly and the first prints memory allocation of 16 bytes failed (which I can only explain as: undefined behavior, whatever).

With #51851 however, the first test case Weak::<Void>::new() appears to be fixed even with optimizations.

bors added a commit that referenced this issue Jul 6, 2018

Auto merge of #51901 - rust-lang:weak-unboxing, r=alexcrichton
Rc: remove unused allocation and fix segfault in Weak::new()

Same as #50357 did for `Arc`.

Fixes #48493

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue Jul 7, 2018

Rollup merge of rust-lang#51901 - rust-lang:weak-unboxing, r=alexcric…
…hton

Rc: remove unused allocation and fix segfault in Weak::new()

Same as rust-lang#50357 did for `Arc`.

Fixes rust-lang#48493

@bors bors closed this in #51901 Jul 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.