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 possible UB in Body #3015

Closed
jjant opened this issue Oct 17, 2022 · 10 comments
Closed

Miri reports possible UB in Body #3015

jjant opened this issue Oct 17, 2022 · 10 comments
Labels
C-bug Category: bug. Something is wrong. This is bad!

Comments

@jjant
Copy link

jjant commented Oct 17, 2022

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.

Related:

Testing
I ported our failing test into the hyper codebase here: #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/gitpro.ttaallkk.top-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/gitpro.ttaallkk.top-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/gitpro.ttaallkk.top-1ecc6299db9ec823/futures-util-0.3.24/src/future/poll_fn.rs:56:9
    = note: inside closure at /Users/jjantdev/.cargo/registry/src/gitpro.ttaallkk.top-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/gitpro.ttaallkk.top-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/gitpro.ttaallkk.top-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/gitpro.ttaallkk.top-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/gitpro.ttaallkk.top-1ecc6299db9ec823/tokio-1.21.2/src/runtime/task/core.rs:174:13
    = note: inside closure at /Users/jjantdev/.cargo/registry/src/gitpro.ttaallkk.top-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/gitpro.ttaallkk.top-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/gitpro.ttaallkk.top-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/gitpro.ttaallkk.top-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/gitpro.ttaallkk.top-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/gitpro.ttaallkk.top-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/gitpro.ttaallkk.top-1ecc6299db9ec823/tokio-1.21.2/src/runtime/task/mod.rs:385:9
    = note: inside closure at /Users/jjantdev/.cargo/registry/src/gitpro.ttaallkk.top-1ecc6299db9ec823/tokio-1.21.2/src/runtime/scheduler/current_thread.rs:564:25
    = note: inside closure at /Users/jjantdev/.cargo/registry/src/gitpro.ttaallkk.top-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/gitpro.ttaallkk.top-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/gitpro.ttaallkk.top-1ecc6299db9ec823/tokio-1.21.2/src/coop.rs:72:5
    = note: inside closure at /Users/jjantdev/.cargo/registry/src/gitpro.ttaallkk.top-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/gitpro.ttaallkk.top-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/gitpro.ttaallkk.top-1ecc6299db9ec823/tokio-1.21.2/src/runtime/scheduler/current_thread.rs:284:9
    = note: inside closure at /Users/jjantdev/.cargo/registry/src/gitpro.ttaallkk.top-1ecc6299db9ec823/tokio-1.21.2/src/runtime/scheduler/current_thread.rs:563:34
    = note: inside closure at /Users/jjantdev/.cargo/registry/src/gitpro.ttaallkk.top-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/gitpro.ttaallkk.top-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/gitpro.ttaallkk.top-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)

@jjant jjant added the C-bug Category: bug. Something is wrong. This is bad! label Oct 17, 2022
@Nemo157
Copy link

Nemo157 commented Oct 18, 2022

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).

@Nemo157
Copy link

Nemo157 commented Oct 18, 2022

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.

@LucioFranco
Copy link
Member

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?

@Darksonn
Copy link
Contributor

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.

@seanmonstar
Copy link
Member

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/gitpro.ttaallkk.top-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/gitpro.ttaallkk.top-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/gitpro.ttaallkk.top-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/gitpro.ttaallkk.top-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() {
    |                                  ^

@seanmonstar
Copy link
Member

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

@RalfJung
Copy link
Contributor

RalfJung commented Dec 5, 2022

If that started recently, it might be a different problem -- see rust-lang/unsafe-code-guidelines#381. You'll be able to tell once rust-lang/rust#105301 lands, which takes back some recent Miri changes to work around rust-lang/unsafe-code-guidelines#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).

@RalfJung
Copy link
Contributor

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.

@seanmonstar
Copy link
Member

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

@seanmonstar
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: bug. Something is wrong. This is bad!
Projects
None yet
Development

No branches or pull requests

6 participants