hyper Miri reports possible UB in `Body`

Version Hyper 0.14.20

Platform Darwin bcd07430b6c3.ant.amazon.com 21.6.0 Darwin Kernel Version 21.6.0: Mon Aug 22 20:19:52 PDT 2022; root:xnu-8020.140.49~2/RELEASE_ARM64_T6000 arm64

Description Recently, I was trying to run miri in our crates in smithy-rs and ran into a Miri failure (output below) for this test.

I talked with @LucioFranco and he mentioned that the culprit is probably this poll_fn bug: https://internals.rust-lang.org/t/surprising-soundness-trouble-around-pollfn/17484/. In the context of hyper, it looks like it's specifically this line.

https://github.com/rust-lang/rust/pull/102737
https://github.com/rust-lang/miri/issues/2580#issue-1395561629

Testing I ported our failing test into the hyper codebase here: https://github.com/hyperium/hyper/pull/3014 in case it helps in diagnosing the problem.

Full miri report

test byte_stream::tests::read_from_channel_body ... error: Undefined Behavior: trying to retag from <462357> for SharedReadWrite permission at alloc190436[0x40], but that tag does not exist in the borrow stack for this location
   --> /Users/jjantdev/.cargo/registry/src/github.com-1ecc6299db9ec823/hyper-0.14.20/src/body/body.rs:566:44
    |
566 |         futures_util::future::poll_fn(|cx| self.poll_ready(cx)).await
    |                                            ^^^^^^^^^^^^^^^^^^^
    |                                            |
    |                                            trying to retag from <462357> for SharedReadWrite permission at alloc190436[0x40], but that tag does not exist in the borrow stack for this location
    |                                            this error occurs as part of two-phase retag at alloc190436[0x28..0x50]
    |
    = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
    = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
help: <462357> was created by a Unique retag at offsets [0x28..0x50]
   --> aws-smithy-http/src/byte_stream.rs:615:52
    |
615 |             sender.send_data(Bytes::from("data 2")).await.unwrap();
    |                                                    ^^^^^^
help: <462357> was later invalidated at offsets [0x40..0x41] by a read access
   --> aws-smithy-http/src/byte_stream.rs:618:9
    |
618 | /         assert_eq!(
619 | |             byte_stream.collect().await.expect("no errors").into_bytes(),
620 | |             Bytes::from("data 1data 2data 3")
621 | |         );
    | |__________^
    = note: BACKTRACE:
    = note: inside closure at /Users/jjantdev/.cargo/registry/src/github.com-1ecc6299db9ec823/hyper-0.14.20/src/body/body.rs:566:44
    = note: inside `<futures_util::future::poll_fn::PollFn<[closure@hyper::body::Sender::ready::{closure#0}::{closure#0}]> as futures_core::Future>::poll` at /Users/jjantdev/.cargo/registry/src/github.com-1ecc6299db9ec823/futures-util-0.3.24/src/future/poll_fn.rs:56:9
    = note: inside closure at /Users/jjantdev/.cargo/registry/src/github.com-1ecc6299db9ec823/hyper-0.14.20/src/body/body.rs:566:64
    = note: inside `<std::future::from_generator::GenFuture<[static generator@hyper::body::Sender::ready::{closure#0}]> as futures_core::Future>::poll` at /Users/jjantdev/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/future/mod.rs:91:19
    = note: inside closure at /Users/jjantdev/.cargo/registry/src/github.com-1ecc6299db9ec823/hyper-0.14.20/src/body/body.rs:571:21
    = note: inside `<std::future::from_generator::GenFuture<[static generator@hyper::body::Sender::send_data::{closure#0}]> as futures_core::Future>::poll` at /Users/jjantdev/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/future/mod.rs:91:19
note: inside closure at aws-smithy-http/src/byte_stream.rs:615:52
   --> aws-smithy-http/src/byte_stream.rs:615:52
    |
615 |             sender.send_data(Bytes::from("data 2")).await.unwrap();
    |                                                    ^^^^^^
    = note: inside `<std::future::from_generator::GenFuture<[static generator@aws-smithy-http/src/byte_stream.rs:613:33: 617:10]> as futures_core::Future>::poll` at /Users/jjantdev/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/future/mod.rs:91:19
    = note: inside closure at /Users/jjantdev/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.21.2/src/runtime/task/core.rs:184:17
    = note: inside `tokio::loom::std::unsafe_cell::UnsafeCell::<tokio::runtime::task::core::Stage<std::future::from_generator::GenFuture<[static generator@aws-smithy-http/src/byte_stream.rs:613:33: 617:10]>>>::with_mut::<std::task::Poll<()>, [closure@tokio::runtime::task::core::CoreStage<std::future::from_generator::GenFuture<[static generator@aws-smithy-http/src/byte_stream.rs:613:33: 617:10]>>::poll::{closure#0}]>` at /Users/jjantdev/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.21.2/src/loom/std/unsafe_cell.rs:14:9
    = note: inside `tokio::runtime::task::core::CoreStage::<std::future::from_generator::GenFuture<[static generator@aws-smithy-http/src/byte_stream.rs:613:33: 617:10]>>::poll` at /Users/jjantdev/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.21.2/src/runtime/task/core.rs:174:13
    = note: inside closure at /Users/jjantdev/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.21.2/src/runtime/task/harness.rs:480:19
    = note: inside `<std::panic::AssertUnwindSafe<[closure@tokio::runtime::task::harness::poll_future<std::future::from_generator::GenFuture<[static generator@aws-smithy-http/src/byte_stream.rs:613:33: 617:10]>, std::sync::Arc<tokio::runtime::scheduler::current_thread::Shared>>::{closure#0}]> as std::ops::FnOnce<()>>::call_once` at /Users/jjantdev/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/panic/unwind_safe.rs:271:9
    = note: inside `std::panicking::r#try::do_call::<std::panic::AssertUnwindSafe<[closure@tokio::runtime::task::harness::poll_future<std::future::from_generator::GenFuture<[static generator@aws-smithy-http/src/byte_stream.rs:613:33: 617:10]>, std::sync::Arc<tokio::runtime::scheduler::current_thread::Shared>>::{closure#0}]>, std::task::Poll<()>>` at /Users/jjantdev/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/std/src/panicking.rs:483:40
    = note: inside `std::panicking::r#try::<std::task::Poll<()>, std::panic::AssertUnwindSafe<[closure@tokio::runtime::task::harness::poll_future<std::future::from_generator::GenFuture<[static generator@aws-smithy-http/src/byte_stream.rs:613:33: 617:10]>, std::sync::Arc<tokio::runtime::scheduler::current_thread::Shared>>::{closure#0}]>>` at /Users/jjantdev/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/std/src/panicking.rs:447:19
    = note: inside `std::panic::catch_unwind::<std::panic::AssertUnwindSafe<[closure@tokio::runtime::task::harness::poll_future<std::future::from_generator::GenFuture<[static generator@aws-smithy-http/src/byte_stream.rs:613:33: 617:10]>, std::sync::Arc<tokio::runtime::scheduler::current_thread::Shared>>::{closure#0}]>, std::task::Poll<()>>` at /Users/jjantdev/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/std/src/panic.rs:137:14
    = note: inside `tokio::runtime::task::harness::poll_future::<std::future::from_generator::GenFuture<[static generator@aws-smithy-http/src/byte_stream.rs:613:33: 617:10]>, std::sync::Arc<tokio::runtime::scheduler::current_thread::Shared>>` at /Users/jjantdev/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.21.2/src/runtime/task/harness.rs:468:18
    = note: inside `tokio::runtime::task::harness::Harness::<std::future::from_generator::GenFuture<[static generator@aws-smithy-http/src/byte_stream.rs:613:33: 617:10]>, std::sync::Arc<tokio::runtime::scheduler::current_thread::Shared>>::poll_inner` at /Users/jjantdev/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.21.2/src/runtime/task/harness.rs:104:27
    = note: inside `tokio::runtime::task::harness::Harness::<std::future::from_generator::GenFuture<[static generator@aws-smithy-http/src/byte_stream.rs:613:33: 617:10]>, std::sync::Arc<tokio::runtime::scheduler::current_thread::Shared>>::poll` at /Users/jjantdev/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.21.2/src/runtime/task/harness.rs:57:15
    = note: inside `tokio::runtime::task::raw::poll::<std::future::from_generator::GenFuture<[static generator@aws-smithy-http/src/byte_stream.rs:613:33: 617:10]>, std::sync::Arc<tokio::runtime::scheduler::current_thread::Shared>>` at /Users/jjantdev/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.21.2/src/runtime/task/raw.rs:194:5
    = note: inside `tokio::runtime::task::raw::RawTask::poll` at /Users/jjantdev/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.21.2/src/runtime/task/raw.rs:134:18
    = note: inside `tokio::runtime::task::LocalNotified::<std::sync::Arc<tokio::runtime::scheduler::current_thread::Shared>>::run` at /Users/jjantdev/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.21.2/src/runtime/task/mod.rs:385:9
    = note: inside closure at /Users/jjantdev/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.21.2/src/runtime/scheduler/current_thread.rs:564:25
    = note: inside closure at /Users/jjantdev/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.21.2/src/coop.rs:102:9
    = note: inside `std::thread::LocalKey::<std::cell::Cell<tokio::coop::Budget>>::try_with::<[closure@tokio::coop::with_budget<(), [closure@tokio::runtime::scheduler::current_thread::CoreGuard<'_>::block_on<std::pin::Pin<&mut std::future::from_generator::GenFuture<[static generator@aws-smithy-http/src/byte_stream.rs:610:39: 622:6]>>>::{closure#0}::{closure#3}]>::{closure#0}], ()>` at /Users/jjantdev/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/std/src/thread/local.rs:446:16
    = note: inside `std::thread::LocalKey::<std::cell::Cell<tokio::coop::Budget>>::with::<[closure@tokio::coop::with_budget<(), [closure@tokio::runtime::scheduler::current_thread::CoreGuard<'_>::block_on<std::pin::Pin<&mut std::future::from_generator::GenFuture<[static generator@aws-smithy-http/src/byte_stream.rs:610:39: 622:6]>>>::{closure#0}::{closure#3}]>::{closure#0}], ()>` at /Users/jjantdev/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/std/src/thread/local.rs:422:9
    = note: inside `tokio::coop::with_budget::<(), [closure@tokio::runtime::scheduler::current_thread::CoreGuard<'_>::block_on<std::pin::Pin<&mut std::future::from_generator::GenFuture<[static generator@aws-smithy-http/src/byte_stream.rs:610:39: 622:6]>>>::{closure#0}::{closure#3}]>` at /Users/jjantdev/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.21.2/src/coop.rs:95:5
    = note: inside `tokio::coop::budget::<(), [closure@tokio::runtime::scheduler::current_thread::CoreGuard<'_>::block_on<std::pin::Pin<&mut std::future::from_generator::GenFuture<[static generator@aws-smithy-http/src/byte_stream.rs:610:39: 622:6]>>>::{closure#0}::{closure#3}]>` at /Users/jjantdev/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.21.2/src/coop.rs:72:5
    = note: inside closure at /Users/jjantdev/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.21.2/src/runtime/scheduler/current_thread.rs:284:29
    = note: inside `tokio::runtime::scheduler::current_thread::Context::enter::<(), [closure@tokio::runtime::scheduler::current_thread::Context::run_task<(), [closure@tokio::runtime::scheduler::current_thread::CoreGuard<'_>::block_on<std::pin::Pin<&mut std::future::from_generator::GenFuture<[static generator@aws-smithy-http/src/byte_stream.rs:610:39: 622:6]>>>::{closure#0}::{closure#3}]>::{closure#0}]>` at /Users/jjantdev/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.21.2/src/runtime/scheduler/current_thread.rs:349:19
    = note: inside `tokio::runtime::scheduler::current_thread::Context::run_task::<(), [closure@tokio::runtime::scheduler::current_thread::CoreGuard<'_>::block_on<std::pin::Pin<&mut std::future::from_generator::GenFuture<[static generator@aws-smithy-http/src/byte_stream.rs:610:39: 622:6]>>>::{closure#0}::{closure#3}]>` at /Users/jjantdev/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.21.2/src/runtime/scheduler/current_thread.rs:284:9
    = note: inside closure at /Users/jjantdev/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.21.2/src/runtime/scheduler/current_thread.rs:563:34
    = note: inside closure at /Users/jjantdev/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.21.2/src/runtime/scheduler/current_thread.rs:595:57
    = note: inside `tokio::macros::scoped_tls::ScopedKey::<tokio::runtime::scheduler::current_thread::Context>::set::<[closure@tokio::runtime::scheduler::current_thread::CoreGuard<'_>::enter<[closure@tokio::runtime::scheduler::current_thread::CoreGuard<'_>::block_on<std::pin::Pin<&mut std::future::from_generator::GenFuture<[static generator@aws-smithy-http/src/byte_stream.rs:610:39: 622:6]>>>::{closure#0}], std::option::Option<()>>::{closure#0}], (std::boxed::Box<tokio::runtime::scheduler::current_thread::Core>, std::option::Option<()>)>` at /Users/jjantdev/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.21.2/src/macros/scoped_tls.rs:61:9
    = note: inside `tokio::runtime::scheduler::current_thread::CoreGuard::<'_>::enter::<[closure@tokio::runtime::scheduler::current_thread::CoreGuard<'_>::block_on<std::pin::Pin<&mut std::future::from_generator::GenFuture<[static generator@aws-smithy-http/src/byte_stream.rs:610:39: 622:6]>>>::{closure#0}], std::option::Option<()>>` at /Users/jjantdev/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.21.2/src/runtime/scheduler/current_thread.rs:595:27
note: inside `byte_stream::tests::read_from_channel_body` at aws-smithy-http/src/byte_stream.rs:618:9
   --> aws-smithy-http/src/byte_stream.rs:618:9
    |
618 | /         assert_eq!(
619 | |             byte_stream.collect().await.expect("no errors").into_bytes(),
620 | |             Bytes::from("data 1data 2data 3")
621 | |         );
    | |__________^
note: inside closure at aws-smithy-http/src/byte_stream.rs:610:11
   --> aws-smithy-http/src/byte_stream.rs:610:11
    |
609 |       #[tokio::test]
    |       -------------- in this procedural macro expansion
610 |       async fn read_from_channel_body() {
    |  ___________^
611 | |         let (mut sender, body) = hyper::Body::channel();
612 | |         let byte_stream = Inner::new(body);
613 | |         tokio::spawn(async move {
...   |
621 | |         );
622 | |     }
    | |_____^
    = note: this error originates in the attribute macro `::core::prelude::v1::test` which comes from the expansion of the attribute macro `tokio::test` (in Nightly builds, run with -Z macro-backtrace for more info)

Oct 17 '22 15:10 jjant

This looks more serious than the issue talked about on irlo because afaict with a quick skim there is no unsafe involved here, poll_fn on its own is a sound API, it can just cause surprising issues with other unsafe code (and specifically futures::join! used it unsoundly).

Oct 18 '22 16:10 Nemo157

I tested with poll_fn patched to be Unpin and can confirm it doesn't fix the miri issue. Looking at the error a bit closer it appears to be saying only 1 byte within the allocation was invalidated by a read access, and pointing at bytes related code. It seems more likely to be miri not liking something that bytes is doing.

Oct 18 '22 17:10 Nemo157

Yeah, I also got to the same conclusion w/ Unpin. Maybe its something to do with https://docs.rs/aws-smithy-http/latest/src/aws_smithy_http/byte_stream.rs.html#501 copy_to_bytes?

Oct 18 '22 17:10 LucioFranco

I tested with poll_fn patched to be Unpin and can confirm it doesn't fix the miri issue.

The poll_fn doesn't take ownership of any futures in the first place, so it cannot possibly be the poll_fn issue.

Oct 18 '22 21:10 Darksonn

Looks like its trigger in hyper's CI, an example appears in this job. The relevant output:

 test body::incoming::tests::channel_buffers_one ... ok
error: Undefined Behavior: attempting a read access using <265384> at alloc119302[0x0], but that tag does not exist in the borrow stack for this location
   --> src/body/incoming.rs:299:44
    |
299 |         futures_util::future::poll_fn(|cx| self.poll_ready(cx)).await
    |                                            ^^^^^^^^^^^^^^^^^^^
    |                                            |
    |                                            attempting a read access using <265384> at alloc119302[0x0], but that tag does not exist in the borrow stack for this location
    |                                            this error occurs as part of an access at alloc119302[0x0..0x8]
    |
    = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
    = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
help: <265384> was created by a Unique retag at offsets [0x0..0x8]
   --> src/body/incoming.rs:299:64
    |
299 |         futures_util::future::poll_fn(|cx| self.poll_ready(cx)).await
    |                                                                ^^^^^^
help: <265384> was later invalidated at offsets [0x0..0x20] by a SharedReadWrite retag
   --> src/body/incoming.rs:527:15
    |
527 |         match tx_ready.poll() {
    |               ^^^^^^^^^^^^^^^
    = note: BACKTRACE:
    = note: inside closure at src/body/incoming.rs:299:44
    = note: inside `<futures_util::future::PollFn<[closure@src/body/incoming.rs:299:39: 299:43]> as futures_util::Future>::poll` at /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/futures-util-0.3.25/src/future/poll_fn.rs:56:9
note: inside closure at src/body/incoming.rs:299:64
   --> src/body/incoming.rs:299:64
    |
299 |         futures_util::future::poll_fn(|cx| self.poll_ready(cx)).await
    |                                                                ^^^^^^
    = note: inside closure at /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-test-0.4.2/src/task.rs:97:30
    = note: inside `tokio_test::task::MockTask::enter::<[closure@tokio_test::task::Spawn<[async fn body@src/body/incoming.rs:298:52: 300:6]>::poll::{closure#0}], std::task::Poll<std::result::Result<(), error::Error>>>` at /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-test-0.4.2/src/task.rs:145:9
    = note: inside `tokio_test::task::Spawn::<[async fn body@src/body/incoming.rs:298:52: 300:6]>::poll` at /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-test-0.4.2/src/task.rs:97:9
note: inside `body::incoming::tests::channel_notices_closure` at src/body/incoming.rs:527:15
   --> src/body/incoming.rs:527:15
    |
527 |         match tx_ready.poll() {
    |               ^^^^^^^^^^^^^^^
note: inside closure at src/body/incoming.rs:514:34
   --> src/body/incoming.rs:514:34
    |
513 |     #[test]
    |     ------- in this procedural macro expansion
514 |     fn channel_notices_closure() {
    |                                  ^

Dec 05 '22 20:12 seanmonstar

It's not clear this is actually a problem with poll_fn, we're not doing anything interesting with it. It could be the mpsc channel, or Bytes inside it (or actually Body I suppose), but I'm not able to understand the error messages enough to act on it.

I'll probably tag that test to be ignore in miri until it's clear there even is a problem, and how we can fix it (since otherwise, it's holding up CI for PRs that don't touch that code).

cc @RalfJung

Dec 05 '22 20:12 seanmonstar

If that started recently, it might be a different problem -- see https://github.com/rust-lang/unsafe-code-guidelines/issues/381. You'll be able to tell once https://github.com/rust-lang/rust/pull/105301 lands, which takes back some recent Miri changes to work around https://github.com/rust-lang/unsafe-code-guidelines/issues/381 (this makes Miri unsound, but the UB Miri shows is not really actionable for crate authors anyway, that is something we need to first fix in the core language -- either by removing dereferenceable from LLVM codegen or by finding a different model for dereferenceable that avoids making this code UB).

Dec 05 '22 22:12 RalfJung

Does latest Miri still flag this code as UB? As mentioned in my previous comment there have been a bunch of changes in Miri around Unpin at that time.

Nov 28 '23 08:11 RalfJung

It seems like it passes in #3432 (some other spurious failure that seems unrelated...).

Jan 19 '24 20:01 seanmonstar

Well, the tests that triggered this issue are re-enabled and passing now. So I'll close this :)

Jan 22 '24 14:01 seanmonstar