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

Use NonNull pointer in the place of references in AtomicRef and AtomicRefMut to avoid noalias related soundness bug. #18

Merged
merged 2 commits into from
Apr 24, 2023

Conversation

Imberflur
Copy link
Contributor

With this test added:

#[test]
fn drop_and_borrow_in_fn_call() {
    fn drop_and_borrow(cell: &AtomicRefCell<Bar>, borrow: AtomicRef<'_, Bar>) {
        drop(borrow);
        *cell.borrow_mut() = Bar::default();
    }

    let a = AtomicRefCell::new(Bar::default());
    let borrow = a.borrow();
    drop_and_borrow(&a, borrow);
}

Running cargo +nightly miri test produces:

test drop_and_borrow_in_fn_call ... error: Undefined Behavior: not granting access to tag <236157> because that would remove [SharedReadOnly for <236092>] which is strongly protected because it is an argument of call 64948
   --> /home/imbris/projects/atomic_refcell/src/lib.rs:144:33
    |
144 |                 value: unsafe { &mut *self.value.get() },
    |                                 ^^^^^^^^^^^^^^^^^^^^^^ not granting access to tag <236157> because that would remove [SharedReadOnly for <236092>] which is strongly protected because it is an argument of call 64948
    |
    = 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: <236157> was created by a SharedReadWrite retag at offsets [0x8..0xc]
   --> /home/imbris/projects/atomic_refcell/src/lib.rs:144:39
    |
144 |                 value: unsafe { &mut *self.value.get() },
    |                                       ^^^^^^^^^^^^^^^^
help: <236092> is this argument
   --> tests/basic.rs:96:51
    |
96  |     fn drop_and_borrow(cell: &AtomicRefCell<Bar>, borrow: AtomicRef<'_, Bar>) {
    |                                                   ^^^^^^
    = note: BACKTRACE (of the first span):
    = note: inside `atomic_refcell::AtomicRefCell::<Bar>::borrow_mut` at /home/imbris/projects/atomic_refcell/src/lib.rs:144:33: 144:55
note: inside `drop_and_borrow_in_fn_call::drop_and_borrow`
   --> tests/basic.rs:98:10
    |
98  |         *cell.borrow_mut() = Bar::default();
    |          ^^^^^^^^^^^^^^^^^
note: inside `drop_and_borrow_in_fn_call`
   --> tests/basic.rs:103:5
    |
103 |     drop_and_borrow(&a, borrow);
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: inside closure
   --> tests/basic.rs:95:33
    |
94  | #[test]
    | ------- in this procedural macro expansion
95  | fn drop_and_borrow_in_fn_call() {
    |                                 ^
    = note: this error originates in the attribute macro `test` (in Nightly builds, run with -Z macro-backtrace for more info)

This PR replaces the reference with NonNull<T> which is the same fix applied for std::cell::RefCell.

function where the `AtomicRef` is dropped and a new borrow is taken from
the cell to catch potential UB in this scenario using Miri.
Imberflur added a commit to Imberflur/shred that referenced this pull request Feb 5, 2023
address stacked borrows UB detected by Miri.

This is similar to bholley/atomic_refcell#18
@Imberflur
Copy link
Contributor Author

Forgot Send/Sync impls! They are added now.

@cuviper
Copy link

cuviper commented Apr 21, 2023

I think you'll want to update Debug too, like I had to fix in rust-lang/rust#97225.

`AtomicRefMut` to avoid `noalias` related soundness bug.
@Imberflur
Copy link
Contributor Author

I updated Debug, thanks for the note!

@bholley bholley merged commit 66e6b8a into bholley:master Apr 24, 2023
@bholley
Copy link
Owner

bholley commented Apr 24, 2023

Thanks. Sorry for the long-delay reviewing this, I had to page a bunch of stuff back in. I've released this as 0.1.10.

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 this pull request may close these issues.

3 participants