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

Topic: Can't tell if it's a miri false positive


Lokathor (Nov 04 2019 at 06:54, on Zulip):

Hello all, Ralf particularly for this one, I have a crate called bytemuck for casting slices and such. https://github.com/Lokathor/bytemuck

I pulled it out of a large lib a month or so ago and just remembered to move the tests as well this weekend. I figured i'd run it through miri too, but then I got a test failure!

D:\dev\bytemuck>cargo miri test
   Compiling bytemuck v0.1.1-alpha.0 (D:\dev\bytemuck)

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

    Finished test [unoptimized + debuginfo] target(s) in 0.88s
   Compiling bytemuck v0.1.1-alpha.0 (D:\dev\bytemuck)

running 3 tests
test test_try_cast_slice ... ok
error[E0080]: Miri evaluation error: trying to reborrow for Unique, but parent tag <untagged> does
not have an appropriate item in the borrow stack
    --> C:\Users\Daniel\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\src\libcore\slice\mod.rs:5257:5
     |
5257 |     &mut *ptr::slice_from_raw_parts_mut(data, len)
     |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Miri evaluation error: trying to reborrow for Unique, but parent tag <untagged> does not have an appropriate item in the borrow stack
     |
     = note: inside call to `std::slice::from_raw_parts_mut::<u8>` at D:\dev\bytemuck\src\lib.rs:232:17
note: inside call to `bytemuck::try_cast_slice_mut::<u32, u8>` at tests\cast_slice_tests.rs:47:30
    --> tests\cast_slice_tests.rs:47:30

Honestly I can't really tell if this is actually a problem or if miri is being weird or what. I get the same error even when I to use identically sized type (eg: &mut [u32] to &mut [f32]), but I don't get the error with a straight up core::mem::transmute.

gnzlbg (Nov 04 2019 at 09:47, on Zulip):

@Lokathor a minimal working example in the playground that reproduces the issue would help

Lokathor (Nov 04 2019 at 15:34, on Zulip):

uh, sure, https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=b28a15e3d99616b03caafdd794550946

gnzlbg (Nov 04 2019 at 20:33, on Zulip):

@Lokathor the problem is that you are using .as_ptr instead of .as_mut_ptr

Lokathor (Nov 04 2019 at 20:33, on Zulip):

oh my gosh i should have seen that one

Lokathor (Nov 04 2019 at 20:34, on Zulip):

and if I'd used the new cast method i would have had the compiler detect it too

gnzlbg (Nov 04 2019 at 20:34, on Zulip):

_I think_ that ends up creating &mut [B] -> &[B] -> *const B -> *mut A -> .. -> &mut [A] chain, which creates a mut reference from a shared one

Lokathor (Nov 04 2019 at 20:34, on Zulip):

yeah

gnzlbg (Nov 04 2019 at 20:35, on Zulip):

I don't know if this is a false positive or not

gnzlbg (Nov 04 2019 at 20:36, on Zulip):

but at least with as_mut_ptr it works
i've seen quite a lot of code that does this, and that code isn't wrong, in the sense that doing that isn't really "wrong"

gnzlbg (Nov 04 2019 at 20:36, on Zulip):

but in miri that &self invalidates the Unique in the borrow stack

Lokathor (Nov 04 2019 at 22:50, on Zulip):

I'll fix it later today and yank the old version, better safe than sorry I suppose

Lokathor (Nov 05 2019 at 02:14, on Zulip):

Fixed and released!

RalfJ (Nov 05 2019 at 09:22, on Zulip):

@Lokathor if you dont @mention me I don't get pinged ;)

Lokathor (Nov 05 2019 at 09:23, on Zulip):

oh I figured you just sense the smell of fresh UB every morning

RalfJ (Nov 05 2019 at 09:23, on Zulip):

_I think_ that ends up creating &mut [B] -> &[B] -> *const B -> *mut A -> .. -> &mut [A] chain, which creates a mut reference from a shared one

if that is indeed the chain then its definitely a true positive. this is considered mutation of shared data, a big no-go.

RalfJ (Nov 05 2019 at 09:24, on Zulip):

oh I figured you just sense the smell of fresh UB every morning

sure, but I don't know where the smell comes from so I don't know where to go to fix it. I got used to ignoring it. ;)

Lokathor (Nov 05 2019 at 09:24, on Zulip):

i switched to 1.38 and the cast method to avoid this possibly in the future

RalfJ (Nov 05 2019 at 09:24, on Zulip):

cast method?

Lokathor (Nov 05 2019 at 09:24, on Zulip):

pointers have a cast method now

RalfJ (Nov 05 2019 at 09:24, on Zulip):

ah, that keeps mutability the same but changes the type?

Lokathor (Nov 05 2019 at 09:24, on Zulip):

yes

RalfJ (Nov 05 2019 at 09:25, on Zulip):

maybe we should have a clippy lint against mutability-changing raw ptr casts... @Shnatsel has done a lot of safety clippy work recently I hear, does this seem like an interesting case?

RalfJ (Nov 05 2019 at 09:25, on Zulip):

I have seen at least 2 or 3 cases over the last few weeks for a const-to-mut raw ptr cast being the give-away for mutation of shared data

Lokathor (Nov 05 2019 at 09:26, on Zulip):

https://github.com/rust-lang/rust-clippy/issues/4771

gnzlbg (Nov 05 2019 at 09:54, on Zulip):

this is considered mutation of shared data, a big no-go.

In a &mut T -> &T -> &mut T chain, as long as the &T is not "duplicated", it is still unique, but the current model does not account for that.

gnzlbg (Nov 05 2019 at 09:56, on Zulip):

(which is a fair thing to do, and that means that this code is wrong according to that model)

RalfJ (Nov 05 2019 at 09:56, on Zulip):

this is considered mutation of shared data, a big no-go.

In a &mut T -> &T -> &mut T chain, as long as the &T is not "duplicated", it is still unique, but the current model does not account for that.

it's not the &mut that is causing UB here, it's the &T

RalfJ (Nov 05 2019 at 09:56, on Zulip):

and &T comes with a promise of immutability

RalfJ (Nov 05 2019 at 09:56, on Zulip):

so, the program is UB because of a violation of immutability.
there is indeed no violation of uniqueness here, but that's besides the point.

gnzlbg (Nov 05 2019 at 09:57, on Zulip):

I thought the cause of the UB was the &T -> &mut T "cast"

RalfJ (Nov 05 2019 at 10:22, on Zulip):

it is. creating a mutable ref to some memory is considered mutating said memory. &T and pointers derived from it dont allow mutation.

RalfJ (Nov 05 2019 at 10:23, on Zulip):

(and here someone might actually write to that memory later, so even if creating a mutable ref wouldnt be the issue, doing the write later certainly would)

gnzlbg (Nov 05 2019 at 10:54, on Zulip):

to allow this the model would need to allow, e.g., writing through a &mut T derived from a &T as long as it is unique and the memory behind it is writeable

RalfJ (Nov 05 2019 at 12:47, on Zulip):

uh, no, the model doesnt want to allow this

RalfJ (Nov 05 2019 at 12:47, on Zulip):

allowing this would kill all the useful optimizations we have for &T

RalfJ (Nov 05 2019 at 12:48, on Zulip):

you must be able to pass an &T to unknown code and know for sure that it will not be mutated

RalfJ (Nov 05 2019 at 12:48, on Zulip):

this is not a model bug. it is an explicit design goal of the rust memory model to make this code UB. and that has been the case from the start, hence all this talking about shared references being read-only and UnsafeCell to make the exception explicit to the compiler.

gnzlbg (Nov 05 2019 at 15:26, on Zulip):

in this case no unknown code is involved though

Shnatsel (Nov 05 2019 at 19:31, on Zulip):

@RalfJ yeah, mutability-changing raw ptr casts sound like a thing we'd like to lint against.
Actually, why not have rustc itself lint against that, as well as other obviously gross misuse such as the eternally beautiful MaybeUninit::uninit().assume_init() one-liner?

Shnatsel (Nov 05 2019 at 19:32, on Zulip):

Clippy is great, but kind of optional, and I want some tool to yell at people unconditionally whenever they try anything like that.

RalfJ (Nov 05 2019 at 19:47, on Zulip):

in this case no unknown code is involved though

I dont see what that has to do with anything. the R-AM has no notion of "unknown code" that would affect execution behavior.

RalfJ (Nov 05 2019 at 19:47, on Zulip):

@Shnatsel such lints could still start in clippy and then be moved if they prove useful

RalfJ (Nov 05 2019 at 19:47, on Zulip):

I dont know if we have a strategy for lints aimed at unsafe code in rustc itself, but having a pool of useful ones in clippy would help ;)

RalfJ (Nov 05 2019 at 19:48, on Zulip):

also MaybeUninit::uninit().assume_init() isnt always wrong...

RalfJ (Nov 05 2019 at 19:48, on Zulip):

we do these days lint against it if it is used at a "bad" type... well, we will once https://github.com/rust-lang/rust/pull/66044 lands

Shnatsel (Nov 05 2019 at 20:17, on Zulip):

Opened Clippy lint request: https://github.com/rust-lang/rust-clippy/issues/4774
And a compiler warning request: https://github.com/rust-lang/rust/issues/66136

RalfJ (Nov 05 2019 at 20:39, on Zulip):

maybe in the future dont open two issues with the same text at the same time though^^

RalfJ (Nov 05 2019 at 20:39, on Zulip):

that leads to lots of duplicate discussion

Shnatsel (Nov 05 2019 at 20:45, on Zulip):

Yeah good point. Sorry about that.

gnzlbg (Nov 06 2019 at 12:48, on Zulip):

I dont see what that has to do with anything. the R-AM has no notion of "unknown code" that would affect execution behavior.

The memory the &T refers to is mutable, and if the &T is unique, then casting that &T to an &mut T and accessing the memory via it is safe.

You said that this is not safe because " this would kill all the useful optimizations we have for &T" and "you must be able to pass an &T to unknown code and know for sure that it will not be mutated" and I mentioned that in my particular example, there is no unknown code anywhere, so I don't see what unknown code has to do with this example either.

gnzlbg (Nov 06 2019 at 12:50, on Zulip):
fn foo() {
    let mut x : i32= 0;
    let y: &mut i32 = unsafe { transmute(&x) };
    *y = 42;
}
gnzlbg (Nov 06 2019 at 12:53, on Zulip):

So I don't know why this example needs to be UB "because of unknown code".

gnzlbg (Nov 06 2019 at 12:54, on Zulip):

AFAICT it is not doing anything that cannot be made defined behavior. The only valid argument I see to make this undefined is that it simplifies the design.

rkruppe (Nov 06 2019 at 15:41, on Zulip):

The rules of the R-AM need to work well for all programs. The rule change you proposed makes them not work well for all programs (greatly diminishes optimization potential when unknown code is involved, which is the norm). In theory you could try to fix this by making a different proposal which somehow makes your specific program not-UB but leaves programs where "unknown code is involved" also UB, but that is not what you did so far and there are several reasons to expect such attempts to fail (e.g. as Ralf said, there's no notion of "unknown code" in R-AM terms).

gnzlbg (Nov 06 2019 at 18:25, on Zulip):

I'm not proposing any rules. I said that I see _a lot_ of code like the one @lokathor wrote, both in the wild and in the standard library, and that a lot of people expect &mut T as &T as &mut T to be a nop, instead of instant UB. That UB does not buy us anything in such code optimization wise, and at least with the current backends and optimizations, bugs due to it only show up on miri.

gnzlbg (Nov 06 2019 at 18:31, on Zulip):

Its hard to explain to a user that wrote such code why it is UB, what can go wrong, and why they need to fix it, when in these particular situations absolutely nothing can go wrong.

gnzlbg (Nov 06 2019 at 18:33, on Zulip):

I agree that the current rules buy us optimization in some other code, but they do not buy us any optimizations in examples like the above, which apparently are quiet common.

rkruppe (Nov 06 2019 at 19:14, on Zulip):

to allow this the model would need to allow, e.g., writing through a &mut T derived from a &T as long as it is unique and the memory behind it is writeable

this is what I was referring to

rkruppe (Nov 06 2019 at 19:25, on Zulip):

In any case, the informed opinion that there's most likely no decent (e.g., comprehensible, implementable) rule set that simultaneously (1) does not have the UB you're complaining about in those specific programs, and (2) does not lose optimization power on other programs, stands and will almost certainly continue to stand unless you (or someone else, hypothetically) convincingly challenges it. It's all good and well to point out a downside of the plan of record but you can't expect that downside to be avoided if nobody can offer a better alternative.

gnzlbg (Nov 07 2019 at 16:12, on Zulip):

I don't have an alternative that satisfies all other constraints

RalfJ (Nov 16 2019 at 09:57, on Zulip):

That UB does not buy us anything in such code optimization wise

you mean UB for that specific snippet of code, right? because what makes this UB is the general rule not to write through shared references and their descendants. and that UB certainly buys us a lot in terms of optimizations.

this is like pointing at *(0usize as *mut i32) = 0 and saying "well that UB doesn't buy us anything". that's true in a very literal interpretation of the statement, but that's also failing to take into account why this is UB in the first place.

RalfJ (Nov 16 2019 at 09:58, on Zulip):

a lot of people expect &mut T as &T as &mut T to be a nop

a lot of people also expect uninit memory to be "just some random bytes", or they expect inttoptr(ptrtoint(ptr)) to be a NOP. turns out a lot of people are just wrong about some basic facts of how Rust, and C and C++, work. we should try to educate them as best we can.

Last update: Nov 20 2019 at 12:20UTC