Stream: t-lang/wg-unsafe-code-guidelines

Topic: updating uninitialized swapping


Jake Goulding (Feb 24 2020 at 16:58, on Zulip):

Does this seem like the correct replacement / upgrade guide?

fn switch(&mut self) {
    // This is safe since we will overwrite it without ever reading it.
    let tmp = mem::replace(self, unsafe { mem::uninitialized() });
    // We absolutely must **never** panic while the uninitialized value is around!

    let new = match tmp {
        Foo::Bar(val) => Foo::Baz(val),
        Foo::Baz(val) => Foo::Bar(val),
    };

    let uninitialized = mem::replace(self, new);
    mem::forget(uninitialized);
}

New

fn switch(&mut self) {
    unsafe {
        let this: &mut MaybeUninit<Self> = mem::transmute(self);

        // After this line, `self` and `this` are effectively undefined and
        // must not be read until we re-establish a value. This also means
        // that the code must **not** panic!
        let tmp = ptr::read(this.as_mut_ptr());

        let new = match tmp {
            Foo::Bar(val) => Foo::Baz(val),
            Foo::Baz(val) => Foo::Bar(val),
        };

        ptr::write(this.as_mut_ptr(), new)
    }
}
Jake Goulding (Feb 24 2020 at 17:11, on Zulip):

The read and write seem reasonable, but that transmute feels heavy-handed.

Steven Fackler (Feb 24 2020 at 17:25, on Zulip):

You can go through raw pointer casts if you want - &mut *(self as *mut Self as *mut MaybeUninit<Self>)

Steven Fackler (Feb 24 2020 at 17:25, on Zulip):

might also be worth adding an abort-on-unwind guard type as a bit of extra insurance

centril (Feb 24 2020 at 17:26, on Zulip):

a transmute seems preferable and less of a footgun, cc @RalfJ

centril (Feb 24 2020 at 17:28, on Zulip):

This is https://docs.rs/take_mut/0.2.2/take_mut/fn.take.html tho so I wouldn't roll my own inline version of that

RalfJ (Feb 24 2020 at 17:41, on Zulip):

Jake Goulding said:

The read and write seem reasonable, but that transmute feels heavy-handed.

uninitialized is in some sense a much bigger hammer than transmute though, I'd say

Jake Goulding (Feb 24 2020 at 18:34, on Zulip):

The take_mut crate doesn't use MaybeUninit; does that worry anyone?

Amanieu (Feb 24 2020 at 18:43, on Zulip):

It should be fine, it never uses undefined bytes.

Jake Goulding (Feb 24 2020 at 20:14, on Zulip):

it never uses undefined bytes.

I'm not sure I follow. Seems like if I take that argument to the end, there was never a need for MaybeUninit in the first place, so long as people never used undefined bytes.

Lokathor (Feb 25 2020 at 08:27, on Zulip):

The comment is definitely incorrect

Lokathor (Feb 25 2020 at 08:29, on Zulip):

the panic danger is not that self and this are both uninitialized. The panic danger is that tmp is a byte duplicate of a non-Copy type so you're in danger of a double drop.

Lokathor (Feb 25 2020 at 08:33, on Zulip):

In fact, because of the way read and write are used in the new version, there's absolutely no need for uninit at all.

Elichai Turkel (Feb 25 2020 at 09:43, on Zulip):

I agree with @Lokathor There's no need in your code for MaybeUninit. there's only a need to prevent panicking and dropping. because if it has a non trivial Drop impl it can then use after free or read uninit bytes etc.

RalfJ (Feb 26 2020 at 10:06, on Zulip):

Jake Goulding said:

it never uses undefined bytes.

I'm not sure I follow. Seems like if I take that argument to the end, there was never a need for MaybeUninit in the first place, so long as people never used undefined bytes.

ah, and again we have confusion about the word "use"

RalfJ (Feb 26 2020 at 10:07, on Zulip):

the term we use in the reference now is "you must not produce an invalid value".
MaybeUninit::<bool>::uninit().assume_init() produces a bool that is invalid, hence it is UB.
take_mut, however, never does anything like that.

RalfJ (Feb 26 2020 at 10:08, on Zulip):

doing ptr::read on an &mut T does not produce an invalid value anywhere -- the only value it produces at all is what gets returned, which is a valid T (assuming there wasn't already something wrong with the reference)

Elichai Turkel (Feb 26 2020 at 21:52, on Zulip):

What happens if you read from &mut &mut T? Can you end up with 2 unique pointers pointing to the same location?

Lokathor (Feb 26 2020 at 22:13, on Zulip):

yes actually.

ptr::read doesn't in any way affect what you read from, so there's no "de-initialization" or anything like that (despite what many people seem to believe).

HeroicKatora (Feb 26 2020 at 22:54, on Zulip):

Does that mean that the take_mut crate is unsound? It does pretty much this.
https://docs.rs/take_mut/0.2.2/src/take_mut/lib.rs.html#31-41
And if it is unsound then is there a way to make it sound?

Lokathor (Feb 27 2020 at 04:28, on Zulip):

So I got up from a nap, and happened to check Zullip first, and you really made my heart sink with this one. :(

You'd have to run Miri on it to be sure. Maybe I'm wrong and this particular code flow is somehow a freakish edgecase that is allowed. I don't think so though. I think it really is unsound if T is a unique reference (or something that contains a unique reference).

If there is a problem, I don't think there's any possible fix in the general case.

You could add a bound of where T: 'static to the function, but that can be evaded if you have &'static mut T. That sounds silly for a moment maybe, until you remember that Box::leak is officially stable and safe and gives you &'a mit T when you leak the box and it goes out of its way to say you can have 'a be 'static if your referenced type is 'static.

bjorn3 (Feb 27 2020 at 06:45, on Zulip):

take_mut uses catch_unwind, but that won't catch foreign exceptions.

RalfJ (Feb 27 2020 at 08:16, on Zulip):

Elichai Turkel said:

What happens if you read from &mut &mut T? Can you end up with 2 unique pointers pointing to the same location?

this is where the details of the aliasing model really become relevant -- e.g., Stacked Borrows. A more concrete example would help me answer your question.

RalfJ (Feb 27 2020 at 08:17, on Zulip):

but take_mut should be fine, I think -- the uniqueness is not about which references exist but about which get used

RalfJ (Feb 27 2020 at 08:18, on Zulip):

(where "use" is defined rather loosely, and a reborrow or assignment/move/copy is already a use)

RalfJ (Feb 27 2020 at 08:19, on Zulip):

bjorn3 said:

take_mut uses catch_unwind, but that won't catch foreign exceptions.

I dont think there are non-UB foreign exceptions in Rust as of today?

bjorn3 (Feb 27 2020 at 08:30, on Zulip):

Indeed, but there will be in the future.

Lokathor (Feb 27 2020 at 09:02, on Zulip):

@RalfJ So... a person can have two unique references to the same location as long as they don't actually use both of them?

That seems quite different from the previous position of "any aliasing is immediately UB (even before you do anything with it)"

RalfJ (Feb 27 2020 at 09:16, on Zulip):

bjorn3 said:

Indeed, but there will be in the future.

this seems pretty relevant for the unwinding discussion then -- adding kinds of uwninding that catch_unwind does not catch could make take_mut, rayon and other libraries unsound

RalfJ (Feb 27 2020 at 09:17, on Zulip):

is that already on the radar of the FFI-unwind group? Where would be a good point to raise this?

RalfJ (Feb 27 2020 at 09:17, on Zulip):

Cc @nikomatsakis @BatmanAoD (Kyle Strand)

RalfJ (Feb 27 2020 at 09:18, on Zulip):

@Lokathor I mean, safe code can have two mutable references to the same location as long as it doesnt use both of them...

let x = &mut 0;
let y = &mut *x;
// x and y are aliasing
RalfJ (Feb 27 2020 at 09:19, on Zulip):

I dont think I ever said "any aliasing is UB", I said "just creating the reference counts as access and causes UB if there is a conflict"

RalfJ (Feb 27 2020 at 09:19, on Zulip):

but if all uses/reborrows/reference creations are well-nested following the stack, there's no conflict

Lokathor (Feb 27 2020 at 09:19, on Zulip):

but the take_mut case doesn't follow the stack really

Lokathor (Feb 27 2020 at 09:20, on Zulip):

you'd have like... two elements at the same stack depth, uh, sorta

RalfJ (Feb 27 2020 at 09:24, on Zulip):

that's not really possible. I guess I need to see some concrete code to understand what you are concerned about.

Amanieu (Feb 27 2020 at 11:53, on Zulip):

@RalfJ regarding unwinding the best place to ask is #project-ffi-unwind

Amanieu (Feb 27 2020 at 11:53, on Zulip):

But yes, catch_unwind can only catch Rust panics because it has to return a Box<Any>. This is not possible for foreign exceptions.

Amanieu (Feb 27 2020 at 12:01, on Zulip):

I guess we could return a Box<ForeignException>, but then if you try to rethrow that with resume_unwind you get a Rust panic, not the original foreign exception.

Lokathor (Feb 27 2020 at 16:24, on Zulip):

probably catch_foreign_exception as a separate utility would be best

Thom Chiovoloni (Feb 28 2020 at 02:59, on Zulip):

probably catch_foreign_exception as a separate utility would be best

I can't imagine any realistic case where you'd want to catch only foreign exceptions and not rust panics

Lokathor (Feb 28 2020 at 07:03, on Zulip):

immediately around an ffi call of course

RalfJ (Feb 28 2020 at 09:20, on Zulip):

@Amanieu you are right, sorry for the noise, I'll move the unwinding thing there

Thales Fragoso (Feb 28 2020 at 20:31, on Zulip):

Is there a way to create a safe abstractions over DMA transactions without UB ? Let's say I have a [MaybeUninit<u8>; 1024 ] and tell the DMA to fill it, and I would wish to transform it in a &[u8] after it's done, would it be UB to use slice::from_raw_parts ?

Thales Fragoso (Feb 28 2020 at 20:32, on Zulip):

How would I tell the compiler that the thing is in fact initialized, since it has no idea of a DMA peripheral ?
Sorry if this isn't the right place for this

Last update: Jun 05 2020 at 22:20UTC