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

Miri reports "unsupported operation: unable to turn pointer into raw bytes" in futures-lite-1.12.0 #2068

Closed
saethlin opened this issue Apr 17, 2022 · 13 comments · Fixed by rust-lang/rust#96162

Comments

@saethlin
Copy link
Member

failures:

---- src/stream.rs - stream::StreamExt::last (line 1325) stdout ----
Test executable failed (exit code 1).

stderr:
error: unsupported operation: unable to turn pointer into raw bytes
    --> /home/ben/.rustup/toolchains/miri/lib/rustlib/src/rust/library/core/src/ptr/mod.rs:1096:9
     |
1096 |         copy_nonoverlapping(src, tmp.as_mut_ptr(), 1);
     |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ unable to turn pointer into raw bytes
     |
     = help: this is likely not a bug in the program; it indicates that the program performed an operation that the interpreter does not support
             
     = note: inside `std::ptr::read::<std::option::Option<i32>>` at /home/ben/.rustup/toolchains/miri/lib/rustlib/src/rust/library/core/src/ptr/mod.rs:1096:9
     = note: inside `std::mem::replace::<std::option::Option<i32>>` at /home/ben/.rustup/toolchains/miri/lib/rustlib/src/rust/library/core/src/mem/mod.rs:904:22
     = note: inside `std::option::Option::<i32>::take` at /home/ben/.rustup/toolchains/miri/lib/rustlib/src/rust/library/core/src/option.rs:1557:9
note: inside `<futures_lite::stream::LastFuture<futures_lite::stream::Empty<i32>> as futures_lite::Future>::poll` at /tmp/futures-lite-1.12.0/src/stream.rs:2735:44
    --> /tmp/futures-lite-1.12.0/src/stream.rs:2735:44
     |
2735 |                 None => return Poll::Ready(this.last.take()),
     |                                            ^^^^^^^^^^^^^^^^
note: inside closure at src/stream.rs:11:20
    --> src/stream.rs:1333:20
     |
11   | assert_eq!(s.last().await, None);
     |                    ^^^^^^
     = note: inside `<std::future::from_generator::GenFuture<[static generator@src/stream.rs:6:24: 12:2]> as futures_lite::Future>::poll` at /home/ben/.rustup/toolchains/miri/lib/rustlib/src/rust/library/core/src/future/mod.rs:91:19
     = note: inside `spin_on::spin_on::<std::future::from_generator::GenFuture<[static generator@src/stream.rs:6:24: 12:2]>>` at /home/ben/.cargo/registry/src/github.com-1ecc6299db9ec823/spin_on-0.1.1/src/lib.rs:78:38
note: inside `main::_doctest_main_src_stream_rs_1325_0` at src/stream.rs:6:1
    --> src/stream.rs:1328:1
     |
6    | / spin_on::spin_on(async {
7    | | let s = stream::iter(vec![1, 2, 3, 4]);
8    | | assert_eq!(s.last().await, Some(4));
9    | |
10   | | let s = stream::empty::<i32>();
11   | | assert_eq!(s.last().await, None);
12   | | });
     | |__^
note: inside `main` at src/stream.rs:13:3
    --> src/stream.rs:1335:3
     |
13   | } _doctest_main_src_stream_rs_1325_0() }
     |   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace

error: aborting due to previous error

I'm very lost here. The only hypothesis I can come up with is that a dependency has written pointer bytes into that Option<i32>, but even if that is the case it doesn't make sense that Miri would report an unsupported operation. This should be a type validation failure, right?

@5225225
Copy link
Contributor

5225225 commented Apr 17, 2022

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=d70742104058788577a5ec1a412c31c0 is a minimal reproduction of this with only futures_core and no unsafe code. (I'll see if I can get it failing using AsyncIterator since apparently we have that in std)

This seems very cursed because the order the tests in main happen is important. If you swap them, miri passes. If you do the vec![4] one twice, and then the empty one, miri complains on the empty one. If you do it in 2 different fuzzing_block_on's, miri passes. If you use the array's into_iter, miri passes.

Edit: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=7e3c1c0b3d66d398b3b28676cc9c1286 is a reproduction using AsyncIterator and no external crates. Still fails, same error.

Edit: Not an issue with AsyncIterator, it's an issue with.... I don't even know. But I got a reproduction that errors depending on the type of an unused field: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=8472d9fc6cd72bf8f3dd7ab2514b2139

Edit: Okay, the unused field is a red herring, it's just that if you have two different types with the same size but different generic parameters, they're confused with each other. See this example with only one field: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=6881cba8cb2e0e70f9f9217f05eb5802

@saethlin
Copy link
Member Author

I don't pretend to understand this, but we've tried a few things. Option<u32>::Some(0) does not trip the bug. Result<u32, ()>::Err(()) trips it, but any Ok doesn't. Option<NonZeroU32> and Option<NonZeroU64> and Option<(u16,u16,u16)> don't. I'm at my wits end.

@RalfJung
Copy link
Member

This might be an instance of rust-lang/rust#87184? It would be the first instance of that Miri limitation in the real world.

@RalfJung
Copy link
Member

RalfJung commented Apr 17, 2022

Specifically it seems like a read of type Option<i32> is used to read data that has provenance. Miri only supports reading data with provenance when reading an entire pointer-sized chunk of data.

Arguably, under strict provenance, that read is UB since it constitutes a ptr-to-int transmutation.

@RalfJung
Copy link
Member

I suspect this is something about the two local variables in

async {
        LastFuture { last: &0u32 }.await;
        LastFuture { last: Option::<u32>::None }.await;
    }

being stored in overlapping parts of the generator, and somehow the initialization with Option::<u32>::None not overwriting the old data properly. But setting the discriminant to None should overwrite parts of that pointer which in Miri should clear all the provenance (that's the bug I linked above) so the story doesn't entirely work out.

@RalfJung
Copy link
Member

This example shows what I would expect Miri to do in that case. That's still surprising, but note that you only get an uninit error when looking at the bytes of Option<i32> that are padding because the active variant is None -- so this is dubious at best.

In the async example, (a) the error is a different one (there still is provenance there!) and (b) it happens already when accessing the data as Option<i32>, not just when looking at the padding.

@RalfJung
Copy link
Member

Hm, no, a partial overwrite of a pointer is not happening in this example. Odd.

@RalfJung
Copy link
Member

Okay I think what actually happens is that the fields storing the LastFuture<&i32> and LastFuture<Option<i32>> are not at the same offset, they just overlap. LastFuture<&i32> is at offset 8 into the generator, LastFuture<Option<i32>> at offset 4. (They have different alignment, that's why layout computation ends up differing.)

It also looks like those things are initialized in-place (rather than constructed elsewhere and copied into the generator), but this is the part I am least sure about.

Now the following happens: when the first future is created, the underlying allocation looks like this:

alloc605 (stack variable, size: 16, align: 8) {
    00 __ __ __ __ __ __ __ ╾0x23534[a655]<untagged> (8 ptr bytes)╼
}

The first byte is the discriminant, then padding, then the pointer of type &i32.

Now we in-place initialize a None::<i32> at offset 4 in here. That writes just the discriminant, meaning the allocation is now

alloc605 (stack variable, size: 16, align: 8) {
    00 __ __ __ 00 00 00 00 ╾0x23534[a655]<untagged> (8 ptr bytes)╼ │ .░░░....╾──────╼
}

Finally, we read an Option<i32> at offset 4. That type has a ScalarPair layout, consisting of two 4-byte-sized pieces. We read the first scalar, and all is fine. We read the second scalar, and those are just 4 bytes of an 8-byte pointer, which is not representable in a Miri Scalar and hence we get an "unsupported" error. (That's [part of] what rust-lang/rust#87184 is about.)

What I am not sure about is why the Deinit that @JakobDegen recently added doesn't remove the pointer. I also haven't seen the in-place initialization of the None::<i32> in the MIR trace. But still, all evidence indicates that this is what happens.

@RalfJung
Copy link
Member

Resolving rust-lang/rust#96158 will fix this problem, since then Miri will no longer (incorrectly) try to use the more efficient representation for types like Option<u32>.

@RalfJung
Copy link
Member

Ah, I figured out the puzzle with the in-place init as well: it's not in-place init, it's that writing an 'uninit' Scalar forgets to clear the provenance.

If it had cleared the provenance, the unsupported error wouldn't have happened, and it would have just treated that memory as uninitialized. Here's the stand-alone reproducer for that (needs -Zmiri-disable-validity):

use std::mem::MaybeUninit;

fn main() { unsafe {
    let mut x = [MaybeUninit::<i32>::zeroed(); 3];
    // Put in a ptr into the last 8 bytes.
    x.as_mut_ptr().offset(1).cast::<&i32>().write_unaligned(&0);
    // Overwrite parts of that pointer with 'uninit' through a Scalar.
    let ptr = x.as_mut_ptr().offset(1).cast::<i32>();
    *ptr = MaybeUninit::uninit().assume_init();
    // Reading this back should hence work fine.
    let _c = *ptr;
} }

@RalfJung
Copy link
Member

RalfJung commented Apr 17, 2022

This is a nice find -- it uncovered two different rather subtle bugs, each one of which would have masked the other if it had been fixed at some point.

Also, sorry for the many notifications in this thread.^^ I am done here for now. ;)

@5225225
Copy link
Contributor

5225225 commented Apr 17, 2022

This is a nice find

you should have seen me absolutely losing it in the rust lang community discord for a good 2 hours trying to minimize this and work out what was going on

it was great fun, if being absolutely bewildered is your idea of fun, which mine is :D

@RalfJung
Copy link
Member

@5225225 your minimized example was extremely helpful for me to further narrow this down, so thanks a ton for doing that legwork! I can only imagine how crazy this behavior must have seemed without a clear mental model of what happens inside Miri.

(Also I am not sure if my explanation of the problem above made much sense. I'm happy to explain more of you are curious, but we should probably take that to Zulip.)

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Apr 19, 2022
interpret: Fix writing uninit to an allocation

When calling `mark_init`, we need to also be mindful of what happens with the relocations! Specifically, when we de-init memory, we need to clear relocations in that range as well or else strange things will happen (and printing will not show the de-init, since relocations take precedence there).

Fixes rust-lang/miri#2068.

Here's the Miri testcase that this fixes (requires `-Zmiri-disable-validation`):
```rust
use std::mem::MaybeUninit;

fn main() { unsafe {
    let mut x = MaybeUninit::<i64>::uninit();
    // Put in a ptr.
    x.as_mut_ptr().cast::<&i32>().write_unaligned(&0);
    // Overwrite parts of that pointer with 'uninit' through a Scalar.
    let ptr = x.as_mut_ptr().cast::<i32>();
    *ptr = MaybeUninit::uninit().assume_init();
    // Reading this back should hence work fine.
    let _c = *ptr;
} }
```
Previously this failed with
```
error: unsupported operation: unable to turn pointer into raw bytes
  --> ../miri/uninit.rs:11:14
   |
11 |     let _c = *ptr;
   |              ^^^^ unable to turn pointer into raw bytes
   |
   = help: this is likely not a bug in the program; it indicates that the program performed an operation that the interpreter does not support

   = note: inside `main` at ../miri/uninit.rs:11:14
```
RalfJung added a commit to RalfJung/miri that referenced this issue Apr 20, 2022
RalfJung added a commit to RalfJung/miri that referenced this issue Apr 20, 2022
bors added a commit that referenced this issue Apr 20, 2022
bors added a commit that referenced this issue Apr 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants