microsoft / microsoft/litebox

Miri data race in futex test between volatile write in `wait` and `futex_word.store(1, Ordering::SeqCst)`

Open
#1,009 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Rust
Stars
2.7k
Forks
144
Avg merge
12h 21m
Merged PRs (30d)
146

Description

Miri has detected the following data race:

```
$ MIRIFLAGS='-Zmiri-permissive-provenance -Zmiri-disable-isolation -Zmiri-ignore-leaks'
rustup run nightly cargo miri test -p litebox test_futex_multiple_waiters_with_timeout
Finished `test` profile [unoptimized + debuginfo] target(s) in 0.18s
Running unittests src/lib.rs (target/miri/x86_64-unknown-linux-gnu/debug/deps/litebox-656da94cab53557b)

running 1 test
test sync::futex::tests::test_futex_multiple_waiters_with_timeout ... error: Undefined Behavior: Data race detected between (1) non-atomic read on thread `unnamed-2` and (2) atomic store on thread `sync::futex::te` at alloc117191+0x10
--> litebox/src/sync/futex.rs:336:9
|
336 | futex_word.store(1, Ordering::SeqCst);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ (2) just happened here
|
help: and (1) occurred earlier here
--> litebox/src/platform/trivial_providers.rs:146:39
|
146 | 1 | 2 | 4 | 8 => unsafe { ptr.offset(count).read_volatile() },
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
= 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: this is on thread `sync::futex::te`
= note: stack backtrace:
0: sync::futex::tests::test_futex_multiple_waiters_with_timeout
at litebox/src/sync/futex.rs:336:9: 336:46
1: sync::futex::tests::test_futex_multiple_waiters_with_timeout::{closure#0}
at litebox/src/sync/futex.rs:296:50: 296:50
```

From a cursory read of the issue, this probably wants "volatile atomic" semantics (https://github.com/rust-lang/unsafe-code-guidelines/issues/615): read should have volatile semantics (only once, non-tearing, ...), but it also synchronizes with an atomic in userspace.

Now, this is somewhat moot: most real targets do use `RawConstPtr`, which _does_ use assembly, and thus effectively is a stand-in for the currently unsupported volatile atomics and does deliver the correct semantics. However, the (Miri) test should probably use a stand-in with real atomic accesses instead.

Looking at this infrastructure, there might be another actual synchronization bug between the `entry.done.store(true, Ordering::Relaxed)` and `entry.get().done.load(Ordering::Acquire)`, but I'd need to look into this further, and it's a separate issue.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Run the reported Miri command for test_futex_multiple_waiters_with_timeout, then read litebox/src/sync/futex.rs around the test and litebox/src/platform/trivial_providers.rs around the volatile read. Determine how the test can use real atomic accesses while preserving the intended futex behavior. Done means the Miri test no longer reports this data race; the separate entry.done synchronization concern is out of scope.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
operating-systems, testing-qa
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.