Fix data race in PinnedPagableArc type - #232
Conversation
|
Hi @Ollie-Pearce! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
@ndmitchell has imported this pull request. If you are a Meta employee, you can view this in D117200708. (Because this pull request was imported automatically, there will not be any future comments.) |
8Keep
left a comment
There was a problem hiding this comment.
Review automatically exported from Phabricator review in Meta.
|
@ndmitchell merged this pull request in ab8400a. |
Summary:
When `PinnedPagableArc` is dropped the pin counter is decremented with a `Relaxed` ordering. This can result in a data race when two threads drop an owned instance of a type duplicated via `Clone`.
## Reproduction:
### Test Case:
```rust
#[test]
fn racy_test() {
let backend = InMemoryPagableStorage::new();
let foo = PinnedPagableArc::new(false, PagableStorageHandle::new(backend.handle()));
let bar = foo.clone();
std::thread::scope(|s| {
s.spawn(move || drop(bar));
let _read = *foo;
drop(foo);
});
}
```
### Running with Miri:
`MIRIFLAGS="-Zmiri-many-seeds=0..16" cargo +nightly miri test -p pagable --test repro`
### Miri output:
```
error: Undefined Behavior: Data race detected between (1) non-atomic read on thread `racy_test`
and (2) retag write of type `pagable::pagable_arc::PagableArcInnerData<bool>` on thread `unnamed-3`
at alloc768762+0x20
--> pagable/src/pagable_arc.rs:877:44
|
877 | let data: &mut _ = unsafe { &mut *ptr.data.get() };
| ^^^^^^^^^^^^^^^^^^^^ (2) just happened here
help: and (1) occurred earlier here
--> pagable/src/pagable_arc.rs:716:14
```
## Fix:
Synchronise the `PinnedPagableArc` pin counter with `Release`/`Acquire` memory orderings and use a fence before data is unpinned.
This mirrors the synchronisation used by [`std::sync::Arc`](https://doc.rust-lang.org/src/alloc/sync.rs.html#2827).
If you'd like to see another example [I found a similar bug in the `stabby` crate](ZettaScaleLabs/stabby#139)
X-link: facebook/starlark-rust#232
Reviewed By: 8Keep
Differential Revision: D117200708
Pulled By: ndmitchell
fbshipit-source-id: 0ca54c4d87d8a5314af6b58ef631da71907fe72b
When
PinnedPagableArcis dropped the pin counter is decremented with aRelaxedordering. This can result in a data race when two threads drop an owned instance of a type duplicated viaClone.Reproduction:
Test Case:
Running with Miri:
MIRIFLAGS="-Zmiri-many-seeds=0..16" cargo +nightly miri test -p pagable --test reproMiri output:
Fix:
Synchronise the
PinnedPagableArcpin counter withRelease/Acquirememory orderings and use a fence before data is unpinned.This mirrors the synchronisation used by
std::sync::Arc.If you'd like to see another example I found a similar bug in the
stabbycrate