From 9a5443fe21bfaba77a4c872ec76b8e828bb265c0 Mon Sep 17 00:00:00 2001 From: Raoul Strackx Date: Mon, 15 May 2023 15:10:25 +0200 Subject: [PATCH 1/2] `waitqueue` clarifications for SGX platform --- library/std/src/sys/sgx/waitqueue/mod.rs | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/library/std/src/sys/sgx/waitqueue/mod.rs b/library/std/src/sys/sgx/waitqueue/mod.rs index 5e1d859ee99c3..32328123f730e 100644 --- a/library/std/src/sys/sgx/waitqueue/mod.rs +++ b/library/std/src/sys/sgx/waitqueue/mod.rs @@ -148,6 +148,8 @@ impl WaitQueue { /// until a wakeup event. /// /// This function does not return until this thread has been awoken. + /// + /// Safety: `before_wait` must not panic pub fn wait(mut guard: SpinMutexGuard<'_, WaitVariable>, before_wait: F) { // very unsafe: check requirements of UnsafeList::push unsafe { @@ -159,6 +161,9 @@ impl WaitQueue { drop(guard); before_wait(); while !entry.lock().wake { + // `entry.wake` is only set in `notify_one` and `notify_all` functions. Both ensure + // the entry is removed from the queue _before_ setting this bool. There are no + // other references to `entry`. // don't panic, this would invalidate `entry` during unwinding let eventset = rtunwrap!(Ok, usercalls::wait(EV_UNPARK, WAIT_INDEFINITE)); rtassert!(eventset & EV_UNPARK == EV_UNPARK); @@ -169,6 +174,7 @@ impl WaitQueue { /// Adds the calling thread to the `WaitVariable`'s wait queue, then wait /// until a wakeup event or timeout. If event was observed, returns true. /// If not, it will remove the calling thread from the wait queue. + /// Safety: `before_wait` must not panic pub fn wait_timeout( lock: &SpinMutex>, timeout: Duration, @@ -183,7 +189,9 @@ impl WaitQueue { let entry_lock = lock.lock().queue.inner.push(&mut entry); before_wait(); usercalls::wait_timeout(EV_UNPARK, timeout, || entry_lock.lock().wake); - // acquire the wait queue's lock first to avoid deadlock. + // acquire the wait queue's lock first to avoid deadlock + // and ensure no other function can simultaneously access the list + // (e.g., `notify_one` or `notify_all`) let mut guard = lock.lock(); let success = entry_lock.lock().wake; if !success { @@ -204,8 +212,8 @@ impl WaitQueue { ) -> Result, SpinMutexGuard<'_, WaitVariable>> { // SAFETY: lifetime of the pop() return value is limited to the map // closure (The closure return value is 'static). The underlying - // stack frame won't be freed until after the WaitGuard created below - // is dropped. + // stack frame won't be freed until after the lock on the queue is released + // (i.e., `guard` is dropped). unsafe { let tcs = guard.queue.inner.pop().map(|entry| -> Tcs { let mut entry_guard = entry.lock(); @@ -231,7 +239,7 @@ impl WaitQueue { ) -> Result, SpinMutexGuard<'_, WaitVariable>> { // SAFETY: lifetime of the pop() return values are limited to the // while loop body. The underlying stack frames won't be freed until - // after the WaitGuard created below is dropped. + // after the lock on the queue is released (i.e., `guard` is dropped). unsafe { let mut count = 0; while let Some(entry) = guard.queue.inner.pop() { From 9baab45e2f2bc3218737613bda074e5b04d2034b Mon Sep 17 00:00:00 2001 From: Raoul Strackx Date: Mon, 31 Jul 2023 13:28:44 +0200 Subject: [PATCH 2/2] Aborting when `before_wait` function panics --- library/std/src/sys/sgx/waitqueue/mod.rs | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/library/std/src/sys/sgx/waitqueue/mod.rs b/library/std/src/sys/sgx/waitqueue/mod.rs index 32328123f730e..25eca61d67b66 100644 --- a/library/std/src/sys/sgx/waitqueue/mod.rs +++ b/library/std/src/sys/sgx/waitqueue/mod.rs @@ -18,6 +18,7 @@ mod unsafe_list; use crate::num::NonZeroUsize; use crate::ops::{Deref, DerefMut}; +use crate::panic::{self, AssertUnwindSafe}; use crate::time::Duration; use super::abi::thread; @@ -147,9 +148,8 @@ impl WaitQueue { /// Adds the calling thread to the `WaitVariable`'s wait queue, then wait /// until a wakeup event. /// - /// This function does not return until this thread has been awoken. - /// - /// Safety: `before_wait` must not panic + /// This function does not return until this thread has been awoken. When `before_wait` panics, + /// this function will abort. pub fn wait(mut guard: SpinMutexGuard<'_, WaitVariable>, before_wait: F) { // very unsafe: check requirements of UnsafeList::push unsafe { @@ -159,7 +159,9 @@ impl WaitQueue { })); let entry = guard.queue.inner.push(&mut entry); drop(guard); - before_wait(); + if let Err(_e) = panic::catch_unwind(AssertUnwindSafe(|| before_wait())) { + rtabort!("Panic before wait on wakeup event") + } while !entry.lock().wake { // `entry.wake` is only set in `notify_one` and `notify_all` functions. Both ensure // the entry is removed from the queue _before_ setting this bool. There are no @@ -174,7 +176,7 @@ impl WaitQueue { /// Adds the calling thread to the `WaitVariable`'s wait queue, then wait /// until a wakeup event or timeout. If event was observed, returns true. /// If not, it will remove the calling thread from the wait queue. - /// Safety: `before_wait` must not panic + /// When `before_wait` panics, this function will abort. pub fn wait_timeout( lock: &SpinMutex>, timeout: Duration, @@ -187,7 +189,9 @@ impl WaitQueue { wake: false, })); let entry_lock = lock.lock().queue.inner.push(&mut entry); - before_wait(); + if let Err(_e) = panic::catch_unwind(AssertUnwindSafe(|| before_wait())) { + rtabort!("Panic before wait on wakeup event or timeout") + } usercalls::wait_timeout(EV_UNPARK, timeout, || entry_lock.lock().wake); // acquire the wait queue's lock first to avoid deadlock // and ensure no other function can simultaneously access the list