Fix a dangling reference in rustc_thread_pool
#146799
Conversation
rustbot
added
S-waiting-on-review
| // SAFETY: Once we call `set` on the internal `latch`, | ||
| // the target may proceed and invalidate `this`! | ||
| match unsafe { &(*this).kind } { |
There was a problem hiding this comment.
A safety comment here should explain why it is safe to create a reference out of (*this).kind. That it does not seem to do that, may be the reason it is a NOTE.
There was a problem hiding this comment.
Well, we added the NOTE in rayon before there was much consensus on proper SAFETY comments. It's definitely a safety-relevant comment, even if it's not directly justifying this unsafe.
(Personally, I would have used a single unsafe block around all of this, but this was also added after the port from rayon. Originally, it's still using the unsafe fn context for inner unsafe too.)
| CountLatchKind::Stealing { latch, registry, worker_index } => { | ||
| let registry = Arc::clone(registry); | ||
| let worker_index = *worker_index; | ||
| if unsafe { CoreLatch::set(latch) } { |
There was a problem hiding this comment.
This seems to be the "call [to] set on the internal latch", but it does not have a safety comment. It could say that this is only safe if none of the following code in this match arm "access[es] any part of this anymore".
But it is not obvious that worker_index should be considered part of this, because it is obscured by the match creating three new names that are really parts of this. Is it possible to shadow or drop all names that should not be reused?
There was a problem hiding this comment.
My change does shadow worker_thread, and registry was already. The only thing left is latch that we're using in this call, which I guess we could shadow to *const CoreLatch if you want, since that's what we pass to set.
This diverged from
rayonin #142384, where a cleanup commit turned the matchedworker_indexinto a reference, which is read after thesetthat may kill it. I've moved that read beforehand, and I hope the new comments will emphasize the subtlety of this unsafe code.Hopefully fixes #146677.