Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants