Fix UB from &mut-derived pointer in Thread::new - #160817
Conversation
rustbot
added
O-unix
|
r? @nia-e rustbot has assigned @nia-e. Use Why was this reviewer chosen?The reviewer was selected based on:
|
This comment has been minimized.
This comment has been minimized.
| assert_eq!(r, 0); | ||
| let r = libc::pthread_cond_init(self.raw(), attr.0.as_ptr()); | ||
| let r = | ||
| libc::pthread_cond_init(ptr::addr_of_mut!((*this).inner).cast(), attr.0.as_ptr()); |
There was a problem hiding this comment.
uses of addr_of_mut! should be replaced with &raw mut
rustbot
added
S-waiting-on-author
|
Reminder, once the PR becomes ready for a review, use |
|
We have some more concerns about the |
|
To follow up from the other thread, it turns out that the Most likely, the fix will involve wrapping the field that gets aliased by the OS in |
|
If you want, you can check out the discussion over at #160815 and rewrite the fix using |
Thanks that would be Great. I will follow-up on this soon... |
The pthread implementations of `Mutex` and `Condvar` may store the address of the object during `init` (e.g. AIX keeps condition variables in an intrusive list) and write through it later. Wrapping the object in `UnsafeCell` is not sufficient for that: creating an exclusive reference to the `UnsafeCell` (as `Drop::drop` does) invalidates pointers that were derived from it earlier, so a write performed by the implementation while `drop` runs violates the aliasing rules. `UnsafePinned` is designed exactly for this situation: it opts out of the uniqueness guarantee of `&mut`, so the state aliased by the OS may be written at any time. Fixes rust-lang#160815 Co-Authored-By: ʟᴜɴᴇx <thisissamir04@gmail.com>
5c7748b to
7bc2187
Compare
|
|
hi - it seems like you're using an LLM to write part of this PR. that's not explicitly disallowed, but please read our AI policy - this particular PR touches on sensitive soundness-relevant code, so all of those parts should be human-authored in full. feel free to head over to #llm-mentoring on Zulip if you'd like to arrange mentoring for an LLM-assisted PR. thanks! |
| let attr = AttrGuard(&mut attr); | ||
| let r = libc::pthread_condattr_setclock(attr.0.as_mut_ptr(), Self::CLOCK); | ||
| assert_eq!(r, 0); | ||
| let r = libc::pthread_cond_init(self.raw(), attr.0.as_ptr()); |
There was a problem hiding this comment.
The soundness of thls line is much more subtle than it might appear simply by looking at the code, so it isn't reasonable to have it in an unsafe block with no safety comment.
In particular, the raw pointer self.raw() outlives the reference it was created from. There are two ways this can be unsound (aliasing model violations, and the memory mistakenly being used for other purposes while the pointer to it is still in use), and these likely both need a detailed explanation of why they are not a problem in this case.
6 participants
fix_approach: Take the pointer from the raw allocation
(Arc::as_ptr)instead of from a&mut,and pass libc the address straight from the allocation
(addr_of_mut). It staysvalid no matter how the thread is used later.