Make compatible as submodule to libstd

[faern]

Preparing to include this repository as a git submodule under libstd over at rust-lang/rust#56410

This is work in progress. Would like feedback, plus get confirmation from the CI that it works everywhere.

[Amanieu]

Overall this looks OK, but I would like to see the actual libstd integration code for a proper review.

[jethrogb]

In SGX, invalid return values can happen and you must check for them. Specifically in the case of synchronization primitives, you must not unwind on an error, see rust-lang/rust#59614. Therefore, you should apply something like the following:

diff --git a/core/src/thread_parker/sgx.rs b/core/src/thread_parker/sgx.rs
index ed511ce..e323801 100644
--- a/core/src/thread_parker/sgx.rs
+++ b/core/src/thread_parker/sgx.rs
@@ -19,6 +19,9 @@ use super::libstd::{
 };
 use core::sync::atomic::{AtomicBool, Ordering};
 
+#[cfg(not(feature = "i-am-libstd"))]
+use std::panic as rtabort;
+
 // Helper type for putting a thread to sleep until some other thread wakes it up
 pub struct ThreadParker {
     parked: AtomicBool,
@@ -55,7 +58,10 @@ impl ThreadParker {
     pub fn park(&self) {
         while self.parked.load(Ordering::Acquire) {
             let result = usercalls::wait(EV_UNPARK, WAIT_INDEFINITE);
-            debug_assert_eq!(result.expect("wait returned error") & EV_UNPARK, EV_UNPARK);
+            match result.map(|eventset| eventset & EV_UNPARK) {
+                Ok(EV_UNPARK) => {}
+                _ => rtabort!("usercall wait returned an invalid value"),
+            }
         }
     }
 
@@ -65,7 +71,7 @@ impl ThreadParker {
     #[inline]
     pub fn park_until(&self, _timeout: Instant) -> bool {
         // FIXME: https://github.com/fortanix/rust-sgx/issues/31
-        panic!("timeout not supported in SGX");
+        rtabort!("timeout not supported in SGX");
     }
 
     // Locks the parker to prevent the target thread from exiting. This is
@@ -90,13 +96,11 @@ impl UnparkHandle {
     #[inline]
     pub fn unpark(self) {
         let result = usercalls::send(EV_UNPARK, Some(self.0));
-        if cfg!(debug_assertions) {
-            if let Err(error) = result {
-                // `InvalidInput` may be returned if the thread we send to has
-                // already been unparked and exited.
-                if error.kind() != io::ErrorKind::InvalidInput {
-                    panic!("send returned an unexpected error: {:?}", error);
-                }
+        if let Err(error) = result {
+            // `InvalidInput` may be returned if the thread we send to has
+            // already been unparked and exited.
+            if error.kind() != io::ErrorKind::InvalidInput {
+                rtabort!("send returned an unexpected error: {:?}", error);
             }
         }
     }

[faern]

@jethrogb parking_lot does not handle very well to crash in the parking code. Which is why all assertions in all ThreadParker implementations are just debug_asserts. So, we already don't unwind on errors, we just ignore them. What is the reason SGX can't tolerate this? Can you explain what bad things can happen with the current implementation?

[jethrogb]

SGX is an security isolation mechanism for running code on untrusted systems. Therefore, the program must detect when the system is trying to manipulate it into an invalid state. If any manipulation is detected, the program must abort and most definitely not try to continue normally. You can read more about these types of attacks in the Iago attacks paper.

The error checks in the thread_parker implementation are different from the checks elsewhere in the code in that they detect a misbehaving system as opposed to a bug in the parking_lot code.

[faern]

@jethrogb Could you evaluate my attempt to implement what you suggested over in the testing branch: https://github.com/faern/parking_lot/blob/as-libstd-submodule-test/core/src/thread_parker/sgx.rs#L23-L30 ?

I applied something very similar to your patch. But I used std::process::abort when not running as part of libstd instead of panicking. std::process::abort and the rtabort! macro end up calling the very same abort implementation, but rtabort! prints the message first.

Not sure it's a good idea to try to unwind or print anything when running outside libstd. Both unwinding and printing to stderr involves locking locks in the background. I suspect doing so if the parking impl is in an inconsistent state might either deadlock or double-panic.

[jethrogb]

LGTM. But note that panic never locks stderr for printing. I agree that unwinding doesn't work well right now but IMO that should just be fixed in the implementation.

[dekellum]

Is there any newer version of this work, or any chance this one could be revived? Or perhaps I just need some helpful advise. I imagine this PR includes making the parking_lot types (like Mutex and Condvar) API compatible with libstd 1.x. Tokio might like to have a non-default feature that uses these types instead of the libstd ones. If it was API compatible, that would be a lot easier!

[Amanieu]

There are no plans to make parking_lot fully API-compatible with libstd. This PR does not do this.

The plan for libstd integration is for the libstd types to become wrappers around RawMutex, etc. But this all happens in libstd, not parking_lot.

[dekellum]

Are there libstd PR's for that I could look at? Thanks for your work in any case! I definitely have found it faster than libstd, for example in the blocking-permit crate I'm working on.

[Amanieu]

You can have a look at rust-lang/rust#56410 but I don't think it will help you much.

[Amanieu]

Closing since this is not going to happen.