UB in the batch_semaphore linked list #3399
Comments
Darksonn
added
C-bug
|
yeah, i think you're right that the waiter should be in an |
Tried following your guide, and still fails. What am I missing? diff --git a/tokio/src/sync/batch_semaphore.rs b/tokio/src/sync/batch_semaphore.rs
index 803f2a18..49d562bc 100644
--- a/tokio/src/sync/batch_semaphore.rs
+++ b/tokio/src/sync/batch_semaphore.rs
@@ -20,6 +20,7 @@ use crate::loom::sync::atomic::AtomicUsize;
use crate::loom::sync::{Mutex, MutexGuard};
use crate::util::linked_list::{self, LinkedList};
+use std::cell::UnsafeCell as UnsafeCellSTD;
use std::future::Future;
use std::marker::PhantomPinned;
use std::pin::Pin;
@@ -65,7 +66,7 @@ pub enum TryAcquireError {
pub struct AcquireError(());
pub(crate) struct Acquire<'a> {
- node: Waiter,
+ node: UnsafeCellSTD<Waiter>,
semaphore: &'a Semaphore,
num_permits: u32,
queued: bool,
@@ -458,7 +459,7 @@ impl Future for Acquire<'_> {
impl<'a> Acquire<'a> {
fn new(semaphore: &'a Semaphore, num_permits: u32) -> Self {
Self {
- node: Waiter::new(num_permits),
+ node: UnsafeCellSTD::new(Waiter::new(num_permits)),
semaphore,
num_permits,
queued: false,
@@ -476,7 +477,7 @@ impl<'a> Acquire<'a> {
let this = self.get_unchecked_mut();
(
- Pin::new_unchecked(&mut this.node),
+ Pin::new_unchecked(this.node.get_mut()),
&this.semaphore,
this.num_permits,
&mut this.queued,
@@ -499,11 +500,11 @@ impl Drop for Acquire<'_> {
let mut waiters = self.semaphore.waiters.lock();
// remove the entry from the list
- let node = NonNull::from(&mut self.node);
+ let node = NonNull::from(self.node.get_mut());
// Safety: we have locked the wait list.
unsafe { waiters.queue.remove(node) };
- let acquired_permits = self.num_permits as usize - self.node.state.load(Acquire);
+ let acquired_permits = self.num_permits as usize - self.node.get_mut().state.load(Acquire);
if acquired_permits > 0 {
self.semaphore.add_permits_locked(acquired_permits, waiters);
} |
|
Further analysis has showed that it is not so simple. |
|
@blasrodri The usage of Worst case (just speculation) this might be more fundamental of a change. With the existence of Fwiw, this pattern also appears in sync::Notify iirc so it may not be only this Future type to address. One way to handle this is by heap allocating the shared state in order to keep the "only shared references" property in the face of |
|
Relevant thread: Are sound self-referential structs possible without boxing? The answer appears to be "No, it's not possible". |
|
Miri should stop complaining about this once #4397 is merged. |
|
FWIW, It's possible to fix this soundness issue while still using intrusive Futures. You just need to store the intrusive Node outside the Future that's being polled, then have said Future reference it. This avoids the Pin<&mut Self> transitively creating a mutable reference to the intrusive Node. In reality however, the Node would still be stored in the async fn compiler's generated Future which would still have this issue of transitive pin mut ref when polled but at least its outside user written code and now a fundamental issue with Rust async. |
|
In general, we are waiting for a resolution to the problem where async Rust itself is unsound. I don't think what you suggested changes anything regarding what miri will accept. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
The following code has UB:
To see this, run it with:
The problem was discovered by @kprotty on the discord server.
The solution probably looks something like putting the
WaiterinAcquireinside anUnsafeCelland only using raw pointers to access it, instead of mutable references.Click to see miri failure
The text was updated successfully, but these errors were encountered: