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

Remove use of uninitialized() in Weak::new() #51851

Closed
wants to merge 2 commits into from
Closed

Conversation

SimonSapin
Copy link
Contributor

@SimonSapin SimonSapin commented Jun 27, 2018

This avoids the question of whether uninitialized::<T>() is necessarily UB when T = !

Fixes #48493

@rust-highfive
Copy link
Collaborator

r? @bluss

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 27, 2018
.unwrap_or_else(|_| handle_alloc_error(layout))
.cast::<ArcInner<T>>();
ptr.as_mut().strong = atomic::AtomicUsize::new(0);
ptr.as_mut().weak = atomic::AtomicUsize::new(1);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using = rather than ptr::write would be incorrect in a generic context, but I believe it is fine here since AtomicUsize: !Drop.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO: a cute trick that unnecessarily hinges on a technicality. Depending on the exact definitions this line might be doing a (no-op) drop and that (no-op) drop might technically involve a read. I find it hard to imagine that would ever actually lead to a miscompile, but personally I'd stick with ptr::write for the sake of avoiding that headache and for consistency.

Copy link
Contributor Author

@SimonSapin SimonSapin Jun 27, 2018

Choose a reason for hiding this comment

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

Fair point. Replaced with ptr::write.

@sfackler
Copy link
Member

This still constructs an instance of a type which contains an uninhabited value - that is intrinsically undefined behavior. IIRC the actual cause of the crash with Weak<!> is that mem::size_of::<ArcInner<!>>() == 0, so the weak and strong fields don't actually exist.

@SimonSapin
Copy link
Contributor Author

Ah interesting, I didn’t realize that ArcInner<!> would also be zero-size.

Should we have a code path for size_of::<T>() == 0 that allocates ArcInner<()> instead?

@sfackler
Copy link
Member

We'd need to make sure all of the code that interacts with the inner value realizes that it needs to use ArcInner<()> rather than ArcInner<!>.

@SimonSapin
Copy link
Contributor Author

ArcInner<!> is not zero-size, though. This prints 16 with 1.28.0-nightly (84804c3 2018-06-26):

#![feature(never_type)]

use std::sync::atomic;

struct ArcInner<T: ?Sized> {
    strong: atomic::AtomicUsize,

    // the value usize::MAX acts as a sentinel for temporarily "locking" the
    // ability to upgrade weak pointers or downgrade strong ones; this is used
    // to avoid races in `make_mut` and `get_mut`.
    weak: atomic::AtomicUsize,

    data: T,
}

fn main() {
    println!("{}", std::mem::size_of::<ArcInner<!>>());
}

@sfackler
Copy link
Member

That was not the case in March, and I wouldn't want to depend on that being the case in the future: #48493 (comment).

@hanna-kruppe
Copy link
Contributor

hanna-kruppe commented Jun 27, 2018

(usize, usize, !) being a ZST was actually a bug (see #49298) and even without that soundness hole we'd want to avoid it so we can do place forwarding on pre-monomorphization MIR. So this approach seems basically fine.

It would be better to not leave size_of::<T>() bytes uninitialized without a union to insulate it, but (1) that can't be implemented currently, and (2) we don't even have the definite statement from the Unsafe Code Guidelines about in what ways that unintialized memory would be bad, much less an actual miscompilation caused by it.

This avoids the question of whether `uninitialized::<T>()` is necessarily UB when `T = !`
@SimonSapin
Copy link
Contributor Author

Given #49298 and #50622 and the fact that I can’t reproduce the segfault in #48493 anymore, I think that this PR can be marked as closing #48493.

@SimonSapin
Copy link
Contributor Author

leave size_of::() bytes uninitialized without a union to insulate it

This was already the case before this PR, Vec::with_capacity also does the same, and size_of::<T>() is zero for empty enums and the never type.

@hanna-kruppe
Copy link
Contributor

This was already the case before this PR

Yes. I even said so myself but it appears that got lost in editing.

Vec::with_capacity also does the same

Yes, lots of other unsafe code may also have to be updated if we'll end up being very strict about uninitialized memory.

(minor point: I think it's more likely for Rc's code fulfills or will fulfill whatever preconditions the unsafe code guidelines specify for when you need valid memory than for Vec's code)

and size_of::() is zero for empty enums and the never type.

Sure, those are fine, it's only an issue for non-ZSTs.

@SimonSapin
Copy link
Contributor Author

I can’t reproduce the segfault in #48493 anymore

Oops never mind, I can when enabling optimizations. This PR fixes it though :)

@hanna-kruppe
Copy link
Contributor

cc @RalfJung @eddyb

@bors
Copy link
Contributor

bors commented Jun 29, 2018

☔ The latest upstream changes (presumably #50357) made this pull request unmergeable. Please resolve the merge conflicts.

@RalfJung
Copy link
Member

This seems to be superseded by #50357 for Arc, and the same approach should also work for Rc?

@SimonSapin
Copy link
Contributor Author

This is indeed superseded, closing.

I’ll make another PR later for applying to change to Rc and adding the non-regression test.

@SimonSapin SimonSapin closed this Jun 29, 2018
@SimonSapin SimonSapin deleted the weak-unboxing branch June 29, 2018 11:43
@SimonSapin
Copy link
Contributor Author

I’ll make another PR later

(Unless someone else beats me to it ;)

@SimonSapin
Copy link
Contributor Author

#51901

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants