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

Unaligned error with async generator field #1919

Closed
cuviper opened this issue Nov 21, 2021 · 6 comments · Fixed by rust-lang/rust#91303
Closed

Unaligned error with async generator field #1919

cuviper opened this issue Nov 21, 2021 · 6 comments · Fixed by rust-lang/rust#91303

Comments

@cuviper
Copy link
Member

cuviper commented Nov 21, 2021

The following code in the playground reports a Miri error:

use futures::executor::block_on;
use futures::AsyncReadExt;

async fn hello_world() {
    let data = [0u8; 1];
    let mut reader = &data[..];

    let mut marker = [0u8; 1];
    reader.read_exact(&mut marker).await.unwrap();
}

fn main() {
    let future = hello_world();
    block_on(future);
}
error: Undefined Behavior: accessing memory with alignment 1, but alignment 2 is required
  --> src/main.rs:8:22
   |
8  |     let mut marker = [0u8; 1];
   |                      ^^^^^^^^ accessing memory with alignment 1, but alignment 2 is required
   |
   = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
   = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
error notes

   = note: inside closure at src/main.rs:8:22
   = note: inside `<std::future::from_generator::GenFuture<[static generator@src/main.rs:4:24: 10:2]> as futures::Future>::poll` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/future/mod.rs:80:19
   = note: inside closure at /playground/.cargo/registry/src/github.com-1ecc6299db9ec823/futures-executor-0.3.17/src/local_pool.rs:315:23
   = note: inside closure at /playground/.cargo/registry/src/github.com-1ecc6299db9ec823/futures-executor-0.3.17/src/local_pool.rs:90:37
   = note: inside `std::thread::LocalKey::<std::sync::Arc<futures::futures_executor::local_pool::ThreadNotify>>::try_with::<[closure@futures::futures_executor::local_pool::run_executor<(), [closure@futures::futures_executor::block_on<std::future::from_generator::GenFuture<[static generator@src/main.rs:4:24: 10:2]>>::{closure#0}]>::{closure#0}], ()>` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/thread/local.rs:399:16
   = note: inside `std::thread::LocalKey::<std::sync::Arc<futures::futures_executor::local_pool::ThreadNotify>>::with::<[closure@futures::futures_executor::local_pool::run_executor<(), [closure@futures::futures_executor::block_on<std::future::from_generator::GenFuture<[static generator@src/main.rs:4:24: 10:2]>>::{closure#0}]>::{closure#0}], ()>` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/thread/local.rs:375:9
   = note: inside `futures::futures_executor::local_pool::run_executor::<(), [closure@futures::futures_executor::block_on<std::future::from_generator::GenFuture<[static generator@src/main.rs:4:24: 10:2]>>::{closure#0}]>` at /playground/.cargo/registry/src/github.com-1ecc6299db9ec823/futures-executor-0.3.17/src/local_pool.rs:86:5
   = note: inside `futures::futures_executor::block_on::<std::future::from_generator::GenFuture<[static generator@src/main.rs:4:24: 10:2]>>` at /playground/.cargo/registry/src/github.com-1ecc6299db9ec823/futures-executor-0.3.17/src/local_pool.rs:315:5
note: inside `main` at src/main.rs:14:5
  --> src/main.rs:14:5
   |
14 |     block_on(future);
   |     ^^^^^^^^^^^^^^^^
   = note: inside `<fn() as std::ops::FnOnce<()>>::call_once - shim(fn())` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:227:5
   = note: inside `std::sys_common::backtrace::__rust_begin_short_backtrace::<fn(), ()>` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/sys_common/backtrace.rs:123:18
   = note: inside closure at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/rt.rs:145:18
   = note: inside `std::ops::function::impls::<impl std::ops::FnOnce<()> for &dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe>::call_once` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:259:13
   = note: inside `std::panicking::r#try::do_call::<&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe, i32>` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:406:40
   = note: inside `std::panicking::r#try::<i32, &dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe>` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:370:19
   = note: inside `std::panic::catch_unwind::<&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe, i32>` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panic.rs:133:14
   = note: inside closure at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/rt.rs:128:48
   = note: inside `std::panicking::r#try::do_call::<[closure@std::rt::lang_start_internal::{closure#2}], isize>` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:406:40
   = note: inside `std::panicking::r#try::<isize, [closure@std::rt::lang_start_internal::{closure#2}]>` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:370:19
   = note: inside `std::panic::catch_unwind::<[closure@std::rt::lang_start_internal::{closure#2}], isize>` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panic.rs:133:14
   = note: inside `std::rt::lang_start_internal` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/rt.rs:128:20
   = note: inside `std::rt::lang_start::<()>` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/rt.rs:144:17

I believe Miri is wrong here, but I don't really know how it represents async code.

When I look at the LLVM IR, I see a memset(i8* align 2 ...) for this marker within the generator Suspend0 state. That alignment comes from PlaceRef::project_field's effective_field_align, because the struct has alignment 8 and the field is at offset 42, so the field will always be at alignment 2.

You can also bump this around in the example by changing the data length. At 2, marker moves to offset 43 and alignment 1, and miri is happy. At 3 we get offset 44 and alignment 4, and at 7 we get offset 48 and alignment 8, both miri errors.

So it seems Miri is seeing the projection alignment, but from the error span it looks like it may be applying that requirement to the initialization value?

@cuviper
Copy link
Member Author

cuviper commented Nov 21, 2021

Thanks to @jorgecarleitao for reporting the UB error to the security response team! We decided that this didn't need to be kept under embargo.

@jorgecarleitao
Copy link

Originally appeared when running MIRI checks over this PR: jorgecarleitao/arrow2#620.

@RalfJung
Copy link
Member

RalfJung commented Nov 22, 2021

Thanks for the report!

The span looks indeed strange, but that might be cause by the underlying MIR (produced by the async transform) having not-entirely-correct span information... those spans are not used for diagnostics (AFAIK) so they are less well-tested than spans in the MIR of regular functions.

When I look at the LLVM IR, I see a memset(i8* align 2 ...) for this marker within the generator Suspend0 state. That alignment comes from PlaceRef::project_field's effective_field_align, because the struct has alignment 8 and the field is at offset 42, so the field will always be at alignment 2.

That agrees with what Miri sees -- the alignment must be 2. But Miri says the actual alignment of the pointer used here is just 1, and hence it raises an error.

The first thing to figure out would be which exact MIR operation is causing the failure -- from the span it is not clear which data is insufficiently aligned and where the pointer to that data comes from. This will require running Miri locally with MIRI_LOG=info.

@RalfJung
Copy link
Member

@RalfJung
Copy link
Member

This is the MIR operation that causes the failure:

(((*(_1.0: &mut [static generator@async-read.rs:7:24: 13:2])) as variant#3).2: [u8; 1]) = [const 0_u8; 1]

So it looks like the field 2 of variant#3 demands alignment 2 but <something> does not actually provide that much alignment -- or Miri is not interpreting the alignment information in that type correctly.

I am not sure what is the best way to figure out the generator type that rustc created here and check if its alignment information makes sense. Cc @tmandry @wesleywiser

@RalfJung
Copy link
Member

#1925 found the same bug in non-async code, which finally put me on the right track to figure out what is going on. It is indeed a Miri bug, sorry for that! rust-lang/rust#91303 should fix it.

bors added a commit that referenced this issue Nov 28, 2021
add tests for alignment on array initialization

This adds regression tests for #1925, #1919.
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