Fix unsoundness issues in std::sys::pal::sgx::waitqueue::unsafe_list - #160641
Fix unsoundness issues in std::sys::pal::sgx::waitqueue::unsafe_list#160641jethrogb wants to merge 1 commit into
Conversation
rustbot
added
O-SGX
|
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.
This comment was marked as resolved.
This comment was marked as resolved.
129fea0 to
599d150
Compare
| /// The caller must make sure to synchronize ending the borrow of the | ||
| /// return value and deallocation of the containing entry. | ||
| pub unsafe fn pop<'a>(&mut self) -> Option<&'a T> { | ||
| unsafe { self.init() }; |
There was a problem hiding this comment.
This wasn't needed in the original code either - this function does nothing if is_empty and is_empty doesn't require init to have run.
| /// currently in `self`. That should be the case for any pointer that is | ||
| /// currently stored in the list. | ||
| unsafe fn launder(&mut self, ptr: EntryPtr<T>) -> DerefPtr<'_, T> { | ||
| let ptr = if ptr.addr() == self.head_tail { EntryPtr::from(self.head_tail()) } else { ptr }; |
There was a problem hiding this comment.
This code is rather unfortunate. It should optimize to a no-op, but it doesn't.
This is what Claude says about it:
The LLVM IR shows why it can't go to zero: each launder becomes
select i1 (icmp eq ptr %loaded, %derived), ptr %derived, ptr %loadedwhere%derivedis a GEP off the&mut selfargument. Both arms have the same address under the condition, but they are distinct pointer values with different provenance, and LLVM deliberately refuses to substitute one pointer for another based onicmp eq ptr(doing so is exactly the class of provenance-breaking miscompile LLVM has been tightening up). So the select survives as acmov, or, where only the address ends up mattering, folds but strands its deadcmp. This cost is inherent to any conditional re-derivation approach — the compare must exist in the IR, and the optimizer is not allowed to prove the arms equal.
I don't think that public API accepting mutable references is correct.... the caller would need to use I don't have time right now to look closer at the code. |
Thank you for your honesty! However, under our very newly adopted LLM policy, you need to pre-arrange reviews for LLM-assisted PRs before opening a PR – and soundness-critical fixes (I think this one qualifies) are entirely disallowed. I'll close this PR until you have found such a reviewer (try asking in #t-libs on Zulip) or rewritten the PR manually.
I highly doubt that claim as miri cannot currently test the SGX target. Please review the test setup manually. In general, I think you could avoid a lot of the complication here by just using an |
rustbot
removed
the
S-waiting-on-review
Good luck with that. I don't think I can improve upon this code.
Can you provide any miri reference that I can look at?
Why not? All aliasing is done through interior mutability. Please review the (pre-existing) safety comments on
It's possible to write this code with less safe abstractions and more use of |
|
For advice to how to proceed, please open a new thread in the #llm-mentoring channel on zulip. Thank you. Edit: Oh, you already posted at #t-libs > Code review |
hanna-kruppe
added
the
llm-assisted
9 participants
Replace invalid uses of references in
std::sys::pal::sgx::waitqueue::unsafe_listinternals with raw pointers. I tried to keep the code structure the same as much as possible, this required adding interior mutability to be able to keep the ”public” API the same, since it passes mutable references to things that may be accessed concurrently in other threads (the linked list entry).In addition to the use of references flagged in the original issue, it turns out the head/tail (raw) pointer stored in the linked list caused provenance issues in miri. Added a mechanism to always use the current receiver's provenance for that pointer.
This PR was developed with Claude Fable 5 through extensive interactive use, where I directed a detailed plan for making the changes needed for this fix. My input includes keeping the structure the same and the new internal abstraction for dealing with raw pointers. I'm not familiar with miri, I used Claude to test the changes with miri and didn't verify the test setup. It said the test suite was failing before the changes (both stacked borrows and tree borrows) but passing after. The head/tail pointer provenance issue was found with Claude. I thoroughly reviewed all the code, including comments, and made manual changes/deletions where necessary/appropriate. The PR description was written by me.
Fixes #114581 (maybe?)
Fixes #160603