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

Topic: Code Examination: slice casting (~20 significant lines)


Lokathor (Jun 21 2019 at 21:55, on Zulip):

Well since it seems like we're going to try posting unsafe code here for review, this is a small and simple test case for this process.

I've got a "slice cast with possible resizing of the slice" function. Normal thing you might write. This is motivated by the fact that gfx-hal had a similar function in 0.1, and it had lurking UB, and then it was reported (including the fixed code) and they still have the exact same function in 0.2. I wanted to make a version of this that's as bulletproof as possible, so here we are.

You can read the code as a PR here or just as a plain file here. The function in question is try_cast_slice. There's also try_cast_slice_mut of course.

RalfJ (Jun 22 2019 at 11:30, on Zulip):

So basically this is transmuting around slices of types where the validity invariant allows any (or any initialized, decision still pending) bit pattern. That seems fine for all the integer and floating point types.

RalfJ (Jun 22 2019 at 11:31, on Zulip):

For Option<NonZero*> you are relying on it being safe to transmute that back and forth to the equally-sized integer type, which I am not sure is a guarantee we are making.

RalfJ (Jun 22 2019 at 11:32, on Zulip):

the same goes for the Zeroed part btw

RalfJ (Jun 22 2019 at 11:33, on Zulip):

it sounds like a fairly reasonable guarantee though -- we do IIRC guarantee the similar thing for Option<&T>, so seems reasonable to also guarantee it for Option<NonZero*>

RalfJ (Jun 22 2019 at 11:33, on Zulip):

btw, I think you could restrict the ZST restriction a bit

RalfJ (Jun 22 2019 at 11:34, on Zulip):

transmuting an empty slice of any type to a slice of ZST is fine (you can pick any length so, so that might be a bit arbitrary)

RalfJ (Jun 22 2019 at 11:34, on Zulip):

and vice versa you could transmute a slice of ZST to an empty slice of any type

RalfJ (Jun 22 2019 at 11:34, on Zulip):

(both assuming that you checked for alignment first, as you do)

nagisa (Jun 22 2019 at 11:50, on Zulip):

NonZeroT is guaranteed to have the same representation as T and at least with the current implementatoin Option<NonZeroT> is also guaranteed to have the same representation as NonZeroT.

nagisa (Jun 22 2019 at 11:51, on Zulip):

Documentation at least specifies that size_of::<Option<NonZeroT> > == size_of::<T>.

centril (Jun 22 2019 at 12:04, on Zulip):

The semi-formal rules are given in https://github.com/rust-lang/rust/pull/60300 which defined the behavior.

gnzlbg (Jun 22 2019 at 13:59, on Zulip):

@RalfJ we could add the NonZero_ types to the layout of scalars, and also consider them when defining the validity of scalars

gnzlbg (Jun 22 2019 at 14:00, on Zulip):

same applies to NonNull<T>, we probably want to add it to the layout of pointers and references section

gnzlbg (Jun 22 2019 at 14:02, on Zulip):

the rest is covered in: https://github.com/rust-lang/unsafe-code-guidelines/blob/master/reference/src/layout/enums.md#discriminant-elision-on-option-like-enums

gnzlbg (Jun 22 2019 at 14:02, on Zulip):

we might want to update that section, e.g., to point to the PR that @centril mentioned, since that kind of RFC'ed part of this behavior

gnzlbg (Jun 22 2019 at 14:27, on Zulip):

so I've sent a PR expanding the discriminant-elision optimization with the PR that @centril mentioned: https://github.com/rust-lang/unsafe-code-guidelines/pull/149

gnzlbg (Jun 22 2019 at 14:28, on Zulip):

We don't need to say anything about the layout of NonZero_ or NonNull in the UCGs, because they are repr(transparent).

Lokathor (Jun 23 2019 at 07:32, on Zulip):

Yeah, ZST is kinda weird in this situation. Just deciding that ZST can only cast to other ZST seemed like the simplest answer that wouldn't cause accidental surprise later on.

RalfJ (Jun 23 2019 at 09:17, on Zulip):

We don't need to say anything about the layout of NonZero_ or NonNull in the UCGs, because they are repr(transparent).

yeah, it's Option<_> of those that I was worried about

RalfJ (Jun 23 2019 at 09:18, on Zulip):

Yeah, ZST is kinda weird in this situation. Just deciding that ZST can only cast to other ZST seemed like the simplest answer that wouldn't cause accidental surprise later on.

that's certainly a safe choice, just wanted to make sure you are aware that you have some more leeway there. also kudos for checking alignment first, many people forget that there can be ZST with non-trivial alignment constraints.

Lokathor (Jun 23 2019 at 13:34, on Zulip):

A lot of this was hammered out with @scottmcm , who suggested for example to split off Zeroable as its own trait which a person might want to utilize separate from Pod.

I settled on ZST and non-ZST not converting between each other because that way, if you get a slice back, you can always round trip it and get the slice you started with. It's not explicitly spelled out as a guarantee, but it really "make sense" as a thing that should generally work. If a ZST is involved then non-ZST -> ZST lets you preserve the length you'd have absolutely no way to know that's actually "correct" for the current memory situation, so when you go from ZST -> non-ZST you'd have to just say that the output length is 0 and you've "lost" some information.

It's 7AM here right now, and I'll leave the PR up until some time around 12 hours from now. People can of course leave a comment here or there after the PR is merged as well.

I think we can call this a moderately successful test run of the unsafe code reviewing. No bugs detected but we spotted at least one place to expand/clarify the guidelines it seems.

Last update: Nov 19 2019 at 17:40UTC