Fix race condition in Arc's get_mut and make_unqiue #26610
Conversation
|
Could this also move the methods to |
| let need_clone = self.inner().strong.load(SeqCst) != 1; | ||
| self.inner().weak.store(1, Relaxed); // release lock immediately; | ||
| // we don't need to hold it while cloning | ||
| if need_clone { *self = Arc::new((**self).clone()); } | ||
| } |
There was a problem hiding this comment.
We should only clone if there are other live strong references; the correct implementation is something like the following (untested, possibly buggy):
if self.inner().strong.compare_and_swap(1, 0, Release) != 1 {
// Another strong pointer exists; clone.
*self = Arc::new((**self).clone());
} else if self.inner().weak.load(SeqCst) != 1 {
// There are no strong pointers left, but there are remaining weak
// pointers; steal the contents.
// FIXME: Is there a better way to write this?
let weak = Weak { _ptr: self._ptr };
self._ptr = 0;
let stolen_data = ptr::read(&(*weak._ptr).data);
*self = Arc::new(stolen_data);
} else {
// No other strong or weak pointers; just revive this Arc.
self.inner().strong = 1;
}
No spinlock necessary.
There was a problem hiding this comment.
This would not work for get_mut though, so the spinlock would still need to exist, just not for make_unique?
There was a problem hiding this comment.
Yes. Granted, we could change the semantics of get_mut so it kills weak pointers rather than failing, which would allow removing the spinlock, but that's probably not a decision we should make based on implementation details.
|
Clone/Drop for Weak and Arc::weak_count also need to be CAS loops. I would also suggest using weak.CAS(acquire); strong.load(Relaxed); weak.store(Release);, as I believe that is sufficient and will remove an unnecessary fence on x86 (and maybe pull it out into a private is_unique() to reduce code duplication?) |
|
An argument could be made that get_mut() on an Arc with no other strong references should kill all weak pointers and memcpy the data if necessary instead of failing. Not sure how get_mut() is used in practice. Either way, it needs documentation which explicitly states what happens in the presence of weak pointers. IIRC, we need to move from atomic increment to CAS anyway to avoid unsafety from overflowing the counter, so any overhead involved in that is irrelevant. |
|
|
||
| // NOTE: this code currently ignores the possibility of overflow | ||
| // into usize::MAX; in general both Rc and Arc need to be adjusted | ||
| // to deal with overflow. |
There was a problem hiding this comment.
Since references take up memory, I don't think usize can be overflowed by them... except we now have safe mem::forget which makes it trivial. RefCell is also affected.
There was a problem hiding this comment.
There's a loong discussion about it (internals)
There was a problem hiding this comment.
Actually, scratch that, RefCell overflows into a "deadlock" state - mutably locked by too many immutable borrows, which denies any further borrows.
aturon
force-pushed
the
fix_make_unique
branch
2 times, most recently
from
0d56386 to
d264b0e
Compare
|
Thanks everyone for the feedback, especially @eefriedman for the insight about the semantics of |
| /// ``` | ||
| #[inline] | ||
| #[unstable(feature = "arc_unique")] | ||
| pub fn get_mut(this: &mut Arc<T>) -> Option<&mut T> { |
There was a problem hiding this comment.
That could interfere with any get_mut() on T. (The doc comment still thinks it's a free function, though.)
There was a problem hiding this comment.
I don’t see how that’s a problem given we have UFCS.
There was a problem hiding this comment.
It follows the new convention for smart pointer methods. I guess it can be written as this: &mut Self though.
aturon
force-pushed
the
fix_make_unique
branch
4 times, most recently
from
9726a81 to
d733e2b
Compare
|
Additional nits addressed. I switched to a swap + mem::forget to avoid manual zeroing. |
| // Note that we hold both a strong reference and a weak reference. | ||
| // Thus, releasing our strong reference only will not, by itself, cause | ||
| // the memory to be deallocated. | ||
| if this.inner().strong.compare_and_swap(1, 0, Release) != 1 { |
There was a problem hiding this comment.
Can you add some comments here to the effect of the memory orderings? It's not immediately clear to me why this is Release and the one below is SeqCst. Also, can't the one at the bottom be relaxed b/c this is the only pointer in existence? (e.g. single threaded)
|
A few minor nits here and there, but otherwise looks good to me. Tricky stuff! r=me |
aturon
force-pushed
the
fix_make_unique
branch
2 times, most recently
from
8566f31 to
71b496c
Compare
|
Any news on this? It would be awesome to get rid of those Servo warnings :) |
|
@boghison Sorry, got tied up with other things after sorting out the memory orderings with @alexcrichton. Just running tests locally, and then it's ready for bors. |
This commit resolves the race condition in the `get_mut` and `make_unique` functions, which arose through interaction with weak pointers. The basic strategy is to "lock" the weak pointer count when trying to establish uniqueness, by reusing the field as a simple spinlock. The overhead for normal use of `Arc` is expected to be minimal -- it will be *none* when only strong pointers are used, and only requires a move from atomic increment to CAS for usage of weak pointers. The commit also removes the `unsafe` and deprecated status of these functions. Along the way, the commit also improves several memory orderings, and adds commentary about why various orderings suffice.
|
|
||
| // the value usize::MAX acts as a sentinel for temporarily "locking" the | ||
| // ability to upgrade weak pointers or downgrade strong ones; this is used | ||
| // to avoid races in `make_unique` and `get_mut`. |
There was a problem hiding this comment.
I think this comment is slightly out of date now, I believe the only race is between get_mut and downgrade, right?
This commit resolves the race condition in the `get_mut` and `make_unique` functions, which arose through interaction with weak pointers. The basic strategy is to "lock" the weak pointer count when trying to establish uniqueness, by reusing the field as a simple spinlock. The overhead for normal use of `Arc` is expected to be minimal -- it will be *none* when only strong pointers are used, and only requires a move from atomic increment to CAS for usage of weak pointers. The commit also removes the `unsafe` and deprecated status of these functions. Closes #24880 r? @alexcrichton cc @metajack @SimonSapin @Ms2ger
This commit resolves the race condition in the
get_mutandmake_uniquefunctions, which arose through interaction with weakpointers. The basic strategy is to "lock" the weak pointer count when
trying to establish uniqueness, by reusing the field as a simple
spinlock. The overhead for normal use of
Arcis expected to be minimal-- it will be none when only strong pointers are used, and only
requires a move from atomic increment to CAS for usage of weak pointers.
The commit also removes the
unsafeand deprecated status of these functions.Closes #24880
r? @alexcrichton
cc @metajack @SimonSapin @Ms2ger