Weak::new() segfaults on uninhabited types

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.

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 :)

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?

@sfackler

@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?

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

Yeah, std doesn’t need to worry about that.

…

On Sat, Feb 24, 2018 at 7:32 AM vitalyd ***@***.***> wrote: Or rather, std would allow T: !Copy (the T: Copy is required for stable). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#48493 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABY2UQ-hbEs8ffqB5pkAhNiLjPvIGRkrks5tYCt0gaJpZM4SRtVH> .

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 :(

@sfackler

@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?

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?

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

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...

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)

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).

I've sent a PR for this at #49248

@alexcrichton

@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.

@vitalyd

@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

std: Fix segfaulting `Weak<!>::new`

De-nominating as this is blocked on #47650

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.

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

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.

Auto merge of #51901 - rust-lang:weak-unboxing, r=alexcrichton

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

Navigation Menu

Repository navigation

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

6 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

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

Description

Activity

vitalyd commented on Feb 24, 2018

sfackler commented on Feb 24, 2018

vitalyd commented on Feb 24, 2018

vitalyd commented on Feb 24, 2018

sfackler commented on Feb 24, 2018

sfackler commented on Mar 9, 2018

alexcrichton commented on Mar 12, 2018

sfackler commented on Mar 12, 2018

sfackler commented on Mar 12, 2018

alexcrichton commented on Mar 12, 2018

alexcrichton commented on Mar 12, 2018

hanna-kruppe commented on Mar 12, 2018

6 remaining items

alexcrichton commented on Mar 21, 2018

vitalyd commented on Mar 21, 2018

alexcrichton commented on Mar 22, 2018

alexcrichton commented on Apr 19, 2018

SimonSapin commented on Jun 27, 2018

SimonSapin commented on Jun 27, 2018

SimonSapin commented on Jun 27, 2018

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions