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
Fix a use-after-free race in SpinLatch::set, and release 1.9.3 #934
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -189,19 +189,21 @@ impl<'r> Latch for SpinLatch<'r> { | |
fn set(&self) { | ||
let cross_registry; | ||
|
||
let registry = if self.cross { | ||
let registry: &Registry = if self.cross { | ||
// Ensure the registry stays alive while we notify it. | ||
// Otherwise, it would be possible that we set the spin | ||
// latch and the other thread sees it and exits, causing | ||
// the registry to be deallocated, all before we get a | ||
// chance to invoke `registry.notify_worker_latch_is_set`. | ||
cross_registry = Arc::clone(self.registry); | ||
&cross_registry | ||
&*cross_registry | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is amazing but also terrible as a single character seems to decide if there is a race or not. Also this isn't criticism on the patch at all but could be a question for the folks at rust-lang if things like that could ever be made more explicit. CC @joshtriplett There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To be fair, we're also playing with fire to have a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, the problem was really on the other branch, borrowing |
||
} else { | ||
// If this is not a "cross-registry" spin-latch, then the | ||
// thread which is performing `set` is itself ensuring | ||
// that the registry stays alive. | ||
self.registry | ||
// that the registry stays alive. However, that doesn't | ||
// include this *particular* `Arc` handle if the waiting | ||
// thread then exits, so we must completely dereference it. | ||
&**self.registry | ||
}; | ||
let target_worker_index = self.target_worker_index; | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've set the release date for this Friday, so affected folks have a little time to test it.