Fix data race in Arc and Weak types - #139
Merged
Merged
Conversation
p-avital
approved these changes
Aug 17, 2026
p-avital
left a comment
p-avital
left a comment
Collaborator
There was a problem hiding this comment.
Thanks! I'll release this soon :)
Contributor
Author
|
@p-avital Thanks! Would you mind if I filed a RustSec advisory to document this? |
meta-codesync Bot
pushed a commit
to facebook/starlark-rust
that referenced
this pull request
Aug 25, 2026
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)
Pull Request resolved: #232
Reviewed By: 8Keep
Differential Revision: D117200708
Pulled By: ndmitchell
fbshipit-source-id: 0ca54c4d87d8a5314af6b58ef631da71907fe72b
meta-codesync Bot
pushed a commit
to facebook/buck2
that referenced
this pull request
Aug 25, 2026
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refcount decrements for
ArcuseRelaxed. 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-2025-08-20 miri test -p stabby-abi --test reproMiri output:
Fix:
Synchronise drops of
std::sync::Arcusing a release ordering to decrement the reference counter and a fence before memory is freed. This mirrors the synchronisation used bystd::sync::Arc