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

#[once] fixtures are unsound for types that are not Sync #235

Closed
narpfel opened this issue Mar 31, 2024 · 4 comments · Fixed by #237
Closed

#[once] fixtures are unsound for types that are not Sync #235

narpfel opened this issue Mar 31, 2024 · 4 comments · Fixed by #237

Comments

@narpfel
Copy link
Contributor

narpfel commented Mar 31, 2024

A #[once] fixture lets multiple threads access the same mutable static variable concurrently via shared references. This is unsound when the value is !Sync.

Small reproducer:

// src/lib.rs

#[cfg(test)]
mod tests {
    use std::cell::Cell;

    #[rstest::fixture]
    #[once]
    fn cell() -> Cell<u32> {
        Cell::new(1)
    }

    #[rstest::rstest]
    fn a(cell: &Cell<u32>) {
        loop {
            cell.set(1);
            assert_eq!(cell.get(), 1);
        }
    }

    #[rstest::rstest]
    fn b(cell: &Cell<u32>) {
        loop {
            assert_eq!(cell.get(), 1);
            cell.set(1);
        }
    }
}

Running this with MIRIFLAGS="-Zmiri-num-cpus=2" cargo miri test will show the UB:

$ MIRIFLAGS="-Zmiri-num-cpus=2" cargo miri test
Preparing a sysroot for Miri (target: x86_64-unknown-linux-gnu)... done
   Compiling t v0.1.0 (/tmp/t)
    Finished `test` profile [unoptimized + debuginfo] target(s) in 0.07s
     Running unittests src/lib.rs (target/miri/x86_64-unknown-linux-gnu/debug/deps/t-f6e346030fdf0db9)

running 2 tests
error: Undefined Behavior: Data race detected between (1) non-atomic write on thread `tests::a` and (2) non-atomic read on thread `tests::b` at alloc1+0x4. (2) just happened here
   --> ~/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/cell.rs:521:18
    |
521 |         unsafe { *self.value.get() }
    |                  ^^^^^^^^^^^^^^^^^ Data race detected between (1) non-atomic write on thread `tests::a` and (2) non-atomic read on thread `tests::b` at alloc1+0x4. (2) just happened here
    |
help: and (1) occurred earlier here
   --> src/lib.rs:16:13
    |
16  |             cell.set(1);
    |             ^^^^^^^^^^^
    = 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
[...]
@narpfel
Copy link
Contributor Author

narpfel commented Mar 31, 2024

If bumping the MSRV to 1.70 (2023-06-01) is acceptable, the simple fix for this would be to use std::sync::OnceLock. If a bump to 1.60 (2022-04-07) would be acceptable, the once_cell crate could be used. This would also make it possible to remove both unsafe blocks.

@la10736
Copy link
Owner

la10736 commented Apr 2, 2024

THX for reporting it!!!
I'm trying to find some time to fix it... I do not have much time if you can provide a PR I'll really appreciate it

@narpfel
Copy link
Contributor Author

narpfel commented Apr 2, 2024

if you can provide a PR I'll really appreciate it

Sure, no problem: #237.

@la10736
Copy link
Owner

la10736 commented Apr 3, 2024

Anyway, I run cargo msrv and it suggest to set msrv to 1.67.1. So I prefer to use std::sync::OnceLock instead

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.

2 participants