Miri reported undefined behavior when using ThreadLocal::iter concurrently from multiple threads.

[james7132]

Miri seems to report undefined behavior when using ThreadLocal::iter from multiple threads concurrently.

The code that generated this: https://github.com/james7132/async-executor/blob/master/src/lib.rs#L805. I'll try to make a minimal repro sometime soon.

Full miri log trace.

error: Undefined Behavior: Data race detected between (1) non-atomic read on thread `<unnamed>` and (2) atomic load on thread `<unnamed>` at alloc3225. (2) just happened here
   --> /home/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/thread_local-1.1.7/src/lib.rs:391:24
    |
391 |                     if entry.present.load(Ordering::Acquire) {
    |                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Data race detected between (1) non-atomic read on thread `<unnamed>` and (2) atomic load on thread `<unnamed>` at alloc3225. (2) just happened here
    |
help: and (1) occurred earlier here
   --> src/lib.rs:74:21
    |
15  |     .each(0..4, |_| future::block_on(ex.run(shutdown.recv())))
    |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    = help: overlapping atomic and non-atomic accesses must be synchronized, even if both are read-only
    = help: see https://doc.rust-lang.org/nightly/std/sync/atomic/index.html#memory-model-for-atomic-accesses for more information about the Rust memory model
    = 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
    = note: BACKTRACE (of the first span):
    = note: inside `thread_local::RawIter::next::<async_executor::LocalQueue>` at /home/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/thread_local-1.1.7/src/lib.rs:391:24: 391:61
    = note: inside `<thread_local::Iter<'_, async_executor::LocalQueue> as std::iter::Iterator>::next` at /home/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/thread_local-1.1.7/src/lib.rs:459:9: 459:41
    = note: inside `<thread_local::Iter<'_, async_executor::LocalQueue> as std::iter::Iterator>::fold::<usize, {closure@<thread_local::Iter<'_, async_executor::LocalQueue> as std::iter::Iterator>::count::{closure#0}}>` at /home/runner/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/iter/traits/iterator.rs:2617:29: 2617:40
    = note: inside `<thread_local::Iter<'_, async_executor::LocalQueue> as std::iter::Iterator>::count` at /home/runner/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/iter/traits/iterator.rs:264:9: 268:10
    = note: inside closure at /home/runner/work/async-executor/async-executor/src/lib.rs:798:25: 798:52
    = note: inside closure at /home/runner/work/async-executor/async-executor/src/lib.rs:705:23: 705:31
    = note: inside `<futures_lite::future::PollFn<{closure@async_executor::Ticker<'_>::runnable_with<{closure@async_executor::Runner<'_>::runnable::{closure#0}::{closure#0}}>::{closure#0}::{closure#0}}> as futures_lite::Future>::poll` at /home/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/futures-lite-2.2.0/src/future.rs:200:9: 200:21
    = note: inside closure at /home/runner/work/async-executor/async-executor/src/lib.rs:726:10: 726:15
    = note: inside closure at /home/runner/work/async-executor/async-executor/src/lib.rs:819:14: 819:19
    = note: inside closure at /home/runner/work/async-executor/async-executor/src/lib.rs:255:62: 255:67
    = note: inside `<futures_lite::future::Or<async_channel::Recv<'_, ()>, {async block@async_executor::Executor<'_>::run<std::result::Result<(), async_channel::RecvError>, async_channel::Recv<'_, ()>>::{closure#0}::{closure#0}}> as futures_lite::Future>::poll` at /home/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/futures-lite-2.2.0/src/future.rs:449:33: 449:54
    = note: inside closure at /home/runner/work/async-executor/async-executor/src/lib.rs:263:32: 263:37
    = note: inside closure at /home/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/futures-lite-2.2.0/src/future.rs:99:19: 99:43
error: doctest failed, to rerun pass `--doc`
    = note: inside `std::thread::LocalKey::<std::cell::RefCell<(parking::Parker, std::task::Waker)>>::try_with::<{closure@futures_lite::future::block_on<std::result::Result<(), async_channel::RecvError>, {async fn body@async_executor::Executor<'_>::run<std::result::Result<(), async_channel::RecvError>, async_channel::Recv<'_, ()>>::{closure#0}}>::{closure#0}}, std::result::Result<(), async_channel::RecvError>>` at /home/runner/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/thread/local.rs:286:16: 286:31
    = note: inside `std::thread::LocalKey::<std::cell::RefCell<(parking::Parker, std::task::Waker)>>::with::<{closure@futures_lite::future::block_on<std::result::Result<(), async_channel::RecvError>, {async fn body@async_executor::Executor<'_>::run<std::result::Result<(), async_channel::RecvError>, async_channel::Recv<'_, ()>>::{closure#0}}>::{closure#0}}, std::result::Result<(), async_channel::RecvError>>` at /home/runner/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/thread/local.rs:262:9: 262:25
    = note: inside `futures_lite::future::block_on::<std::result::Result<(), async_channel::RecvError>, {async fn body@async_executor::Executor<'_>::run<std::result::Result<(), async_channel::RecvError>, async_channel::Recv<'_, ()>>::{closure#0}}>` at /home/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/futures-lite-2.2.0/src/future.rs:78:5: 104:7
note: inside closure
   --> src/lib.rs:74:21
    |
15  |     .each(0..4, |_| future::block_on(ex.run(shutdown.recv())))
    |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

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

error: aborting due to 1 previous error

[james7132]

Looks like this might be the cause: https://github.com/Amanieu/thread_local-rs/blob/master/src/lib.rs#L220

[james7132]

Yep, that's 100% it. According to https://doc.rust-lang.org/nightly/std/sync/atomic/index.html#memory-model-for-atomic-accesses, all unsynchronized operations involving a piece of memory needs to be atomic or non-atomic for behavior to be defined:

Since C++ does not support mixing atomic and non-atomic accesses, or non-synchronized different-sized accesses to the same data, Rust does not support those operations either. Note that both of those restrictions only apply if the accesses are non-synchronized.

[james7132]

This looks like it was (fairly recently) defined as undefined behavior, and a data race here: rust-lang/rust#115719.

[hymm]

Would it fix this if the atomic load is changed to as_ptr https://doc.rust-lang.org/std/sync/atomic/struct.AtomicPtr.html#method.as_ptr

[james7132]

Doing non-atomic reads and writes on the resulting integer can be a data race.

I don't think so. The C++20 memory model for atomics, and thus Rust's, do not allow for any form of mixing atomic and non-atomic operations, even if both are reads. Even if in practice, this is probably OK on current platforms, there's zero guarantee this is universally true unless the compiler explicitly defines this behavior.

[SabrinaJewson]

That’s not true in C++20. If you read the definition of a data race it requires the actions to be “conflicting”, and actions only conflict if one of them modifies a memory location.

Edit: Right, the UB in C++20 comes from TBAA, not data races, which we don’t have in Rust. So in a way we’re introducing UB, but not restricting capabilities.