Make WrappedObject Send + Sync using RwLock

Cargo.toml

@@ -21,6 +21,7 @@ pci_types = { version = "0.10.0", public = true }
 byteorder = { version = "1.5.0", default-features = false }
 
 [dev-dependencies]
+static_assertions = "1.1.0"
 aml_test_tools = { path = "tools/aml_test_tools" }
 pretty_env_logger = "0.5.0"
 

src/aml/namespace.rs

@@ -69,10 +69,11 @@ impl Namespace {
                     if args.len() != 1 {
                         return Err(AmlError::MethodArgCountIncorrect);
                     }
-                    let Object::String(ref feature) = *args[0] else {
+                    let args0_read = args[0].read();
+                    let Object::String(ref feature) = *args0_read else {
                         return Err(AmlError::ObjectNotOfExpectedType {
                             expected: ObjectType::String,
-                            got: args[0].typ(),
+                            got: args0_read.typ(),
                         });
                     };
 
@@ -383,7 +384,7 @@ impl fmt::Display for Namespace {
                     if end { END } else { BRANCH },
                     name.as_str(),
                     if flags.is_alias() { "[A] " } else { "" },
-                    **object
+                    *object.read()
                 )?;
 
                 // If the object has a corresponding scope, print it here

src/aml/object.rs

@@ -6,9 +6,10 @@ use alloc::{
     vec::Vec,
 };
 use bit_field::BitField;
-use core::{cell::UnsafeCell, cmp::Ordering, fmt, ops, sync::atomic::AtomicU64};
+use core::{cmp::Ordering, fmt, sync::atomic::AtomicU64};
+use spinning_top::{RwSpinlock, guard::{RwSpinlockReadGuard, RwSpinlockWriteGuard}};
 
-type NativeMethod = dyn Fn(&[WrappedObject]) -> Result<WrappedObject, AmlError>;
+type NativeMethod = dyn Fn(&[WrappedObject]) -> Result<WrappedObject, AmlError> + Send + Sync;
 
 #[derive(Clone)]
 pub enum Object {
@@ -36,7 +37,7 @@ pub enum Object {
 impl Object {
     pub fn native_method<F>(num_args: u8, f: F) -> Object
     where
-        F: Fn(&[WrappedObject]) -> Result<WrappedObject, AmlError> + 'static,
+        F: Fn(&[WrappedObject]) -> Result<WrappedObject, AmlError> + Send + Sync + 'static,
     {
         let mut flags = 0;
         flags.set_bits(0..3, num_args);
@@ -61,15 +62,15 @@ impl fmt::Display for Object {
             Object::Method { .. } => write!(f, "Method"),
             Object::NativeMethod { .. } => write!(f, "NativeMethod"),
             Object::Mutex { .. } => write!(f, "Mutex"),
-            Object::Reference { kind, inner } => write!(f, "Reference({:?} -> {})", kind, **inner),
+            Object::Reference { kind, inner } => write!(f, "Reference({:?} -> {})", kind, *inner.read()),
             Object::OpRegion(region) => write!(f, "{region:?}"),
             Object::Package(elements) => {
                 write!(f, "Package {{ ")?;
                 for (i, element) in elements.iter().enumerate() {
                     if i == elements.len() - 1 {
-                        write!(f, "{}", **element)?;
+                        write!(f, "{}", *element.read())?;
                     } else {
-                        write!(f, "{}, ", **element)?;
+                        write!(f, "{}, ", *element.read())?;
                     }
                 }
                 write!(f, " }}")?;
@@ -87,53 +88,38 @@ impl fmt::Display for Object {
     }
 }
 
-/// `ObjectToken` is used to mediate mutable access to objects from a [`WrappedObject`]. It must be
-/// acquired by locking the single token provided by [`super::Interpreter`].
-#[non_exhaustive]
-pub struct ObjectToken {
-    _dont_construct_me: (),
-}
+#[derive(Clone)]
+pub struct WrappedObject(Arc<RwSpinlock<Object>>);
 
-impl ObjectToken {
-    /// Create an [`ObjectToken`]. This should **only** be done **once** by the main interpreter,
-    /// as contructing your own token allows invalid mutable access to objects.
-    pub(super) unsafe fn create_interpreter_token() -> ObjectToken {
-        ObjectToken { _dont_construct_me: () }
+impl fmt::Debug for WrappedObject {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        write!(f, "WrappedObject({})", *self.0.read())
     }
 }
 
-#[derive(Clone, Debug)]
-pub struct WrappedObject(Arc<UnsafeCell<Object>>);
-
 impl WrappedObject {
     pub fn new(object: Object) -> WrappedObject {
-        #[allow(clippy::arc_with_non_send_sync)]
-        WrappedObject(Arc::new(UnsafeCell::new(object)))
+        WrappedObject(Arc::new(RwSpinlock::new(object)))
     }
 
-    /// Gain a mutable reference to an [`Object`] from this [`WrappedObject`].
-    ///
-    /// # Safety
-    /// This requires an [`ObjectToken`] which is protected by a lock on [`super::Interpreter`],
-    /// which prevents mutable access to objects from multiple contexts. It does not, however,
-    /// prevent the same object, referenced from multiple [`WrappedObject`]s, having multiple
-    /// mutable (and therefore aliasing) references being made to it, and therefore care must be
-    /// taken in the interpreter to prevent this.
-    pub unsafe fn gain_mut<'r, 'a, 't>(&'a self, _token: &'t ObjectToken) -> &'r mut Object
-    where
-        't: 'r,
-        'a: 'r,
-    {
-        unsafe { &mut *(self.0.get()) }
+    pub fn read(&self) -> RwSpinlockReadGuard<'_, Object> {
+        self.0.read()
+    }
+
+    pub fn write(&self) -> RwSpinlockWriteGuard<'_, Object> {
+        self.0.write()
     }
 
     pub fn unwrap_reference(self) -> WrappedObject {
         let mut object = self;
         loop {
-            if let Object::Reference { ref inner, .. } = *object {
-                object = inner.clone();
-            } else {
-                return object.clone();
+            let inner = {
+                let read = object.read();
+                if let Object::Reference { inner, .. } = &*read { Some(inner.clone()) } else { None }
+            };
+            match inner {
+                Some(inner) => object = inner,
+                None => return object,
             }
         }
     }
@@ -143,34 +129,27 @@ impl WrappedObject {
     pub fn unwrap_transparent_reference(self) -> WrappedObject {
         let mut object = self;
         loop {
-            if let Object::Reference { kind, ref inner } = *object
-                && (kind == ReferenceKind::Local || kind == ReferenceKind::Arg || kind == ReferenceKind::Named)
-            {
-                object = inner.clone();
-            } else {
-                return object.clone();
+            let inner = {
+                let read = object.read();
+                if let Object::Reference { kind, inner } = &*read
+                    && (*kind == ReferenceKind::Local || *kind == ReferenceKind::Arg || *kind == ReferenceKind::Named)
+                {
+                    Some(inner.clone())
+                } else {
+                    None
+                }
+            };
+            match inner {
+                Some(inner) => object = inner,
+                None => return object,
             }
         }
     }
 }
 
-impl ops::Deref for WrappedObject {
-    type Target = Object;
-
-    fn deref(&self) -> &Self::Target {
-        /*
-         * SAFETY: elided lifetime ensures reference cannot outlive at least one reference-counted
-         * instance of the object. `WrappedObject::gain_mut` is unsafe, and so it is the user's
-         * responsibility to ensure shared references from `Deref` do not co-exist with an
-         * exclusive reference.
-         */
-        unsafe { &*self.0.get() }
-    }
-}
-
 impl fmt::Display for WrappedObject {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        write!(f, "Wrapped({})", unsafe { &*self.0.get() })
+        write!(f, "Wrapped({})", *self.0.read())
     }
 }
 
@@ -261,9 +240,10 @@ impl Object {
 
     pub fn read_buffer_field(&self, integer_size: IntegerSize) -> Result<Object, AmlError> {
         if let Self::BufferField { buffer, offset, length } = self {
-            let buffer = match **buffer {
-                Object::Buffer(ref buffer) => buffer.as_slice(),
-                Object::String(ref string) => string.as_bytes(),
+            let buffer_read = buffer.read();
+            let buffer = match &*buffer_read {
+                Object::Buffer(buffer) => buffer.as_slice(),
+                Object::String(string) => string.as_bytes(),
                 _ => panic!(),
             };
             if *length <= integer_size as usize {
@@ -280,10 +260,11 @@ impl Object {
         }
     }
 
-    pub fn write_buffer_field(&mut self, value: &[u8], token: &ObjectToken) -> Result<(), AmlError> {
+    pub fn write_buffer_field(&mut self, value: &[u8]) -> Result<(), AmlError> {
         // TODO: bounds check the buffer first to avoid panicking
         if let Self::BufferField { buffer, offset, length } = self {
-            let buffer = match unsafe { buffer.gain_mut(token) } {
+            let mut buffer_write = buffer.write();
+            let buffer = match &mut *buffer_write {
                 Object::Buffer(buffer) => buffer.as_mut_slice(),
                 // XXX: this unfortunately requires us to trust AML to keep the string as valid
                 // UTF8... maybe there is a better way?

src/aml/pci_routing.rs

@@ -66,9 +66,11 @@ impl PciRoutingTable {
 
         let prt = interpreter.evaluate(prt_path.clone(), vec![])?;
 
-        if let Object::Package(ref inner_values) = *prt {
+        let prt_read = prt.read();
+        if let Object::Package(inner_values) = &*prt_read {
             for value in inner_values {
-                if let Object::Package(ref pin_package) = **value {
+                let value_read = value.read();
+                if let Object::Package(pin_package) = &*value_read {
                     /*
                      * Each inner package has the following structure:
                      *   | Field      | Type      | Description                                               |
@@ -92,27 +94,30 @@ impl PciRoutingTable {
                      *   |            |           | pin is connected.                                         |
                      *   | -----------|-----------|-----------------------------------------------------------|
                      */
-                    let Object::Integer(address) = *pin_package[0] else {
-                        return Err(AmlError::PrtInvalidAddress);
+                    let address = match &*pin_package[0].read() {
+                        Object::Integer(addr) => *addr,
+                        _ => return Err(AmlError::PrtInvalidAddress),
                     };
                     let device = address.get_bits(16..32).try_into().map_err(|_| AmlError::PrtInvalidAddress)?;
                     let function = address.get_bits(0..16).try_into().map_err(|_| AmlError::PrtInvalidAddress)?;
-                    let pin = match *pin_package[1] {
+                    let pin = match &*pin_package[1].read() {
                         Object::Integer(0) => Pin::IntA,
                         Object::Integer(1) => Pin::IntB,
                         Object::Integer(2) => Pin::IntC,
                         Object::Integer(3) => Pin::IntD,
                         _ => return Err(AmlError::PrtInvalidPin),
                     };
 
-                    match *pin_package[2] {
+                    let p2_read = pin_package[2].read();
+                    match &*p2_read {
                         Object::Integer(0) => {
                             /*
                              * The Source Index field contains the GSI number that this interrupt is attached
                              * to.
                              */
-                            let Object::Integer(gsi) = *pin_package[3] else {
-                                return Err(AmlError::PrtInvalidGsi);
+                            let gsi = match &*pin_package[3].read() {
+                                Object::Integer(gsi) => *gsi,
+                                _ => return Err(AmlError::PrtInvalidGsi),
                             };
                             entries.push(PciRoute {
                                 device,
@@ -121,11 +126,13 @@ impl PciRoutingTable {
                                 route_type: PciRouteType::Gsi(gsi as u32),
                             });
                         }
-                        Object::String(ref name) => {
+                        Object::String(name) => {
+                            let name = name.clone();
+                            drop(p2_read);
                             let link_object_name = interpreter
                                 .namespace
                                 .lock()
-                                .search_for_level(&AmlName::from_str(name)?, &prt_path)?;
+                                .search_for_level(&AmlName::from_str(&name)?, &prt_path)?;
                             entries.push(PciRoute {
                                 device,
                                 function,
@@ -136,13 +143,16 @@ impl PciRoutingTable {
                         _ => return Err(AmlError::PrtInvalidSource),
                     }
                 } else {
-                    return Err(AmlError::InvalidOperationOnObject { op: Operation::DecodePrt, typ: value.typ() });
+                    return Err(AmlError::InvalidOperationOnObject {
+                        op: Operation::DecodePrt,
+                        typ: value_read.typ(),
+                    });
                 }
             }
 
             Ok(PciRoutingTable { entries })
         } else {
-            Err(AmlError::InvalidOperationOnObject { op: Operation::DecodePrt, typ: prt.typ() })
+            Err(AmlError::InvalidOperationOnObject { op: Operation::DecodePrt, typ: prt_read.typ() })
         }
     }
 

src/aml/resource.rs

@@ -16,7 +16,8 @@ pub enum Resource {
 
 /// Parse a `ResourceDescriptor` buffer into a list of resources.
 pub fn resource_descriptor_list(descriptor: WrappedObject) -> Result<Vec<Resource>, AmlError> {
-    if let Object::Buffer(ref bytes) = *descriptor {
+    let desc_read = descriptor.read();
+    if let Object::Buffer(ref bytes) = *desc_read {
         let mut descriptors = Vec::new();
         let mut bytes = bytes.as_slice();
 
@@ -33,7 +34,7 @@ pub fn resource_descriptor_list(descriptor: WrappedObject) -> Result<Vec<Resourc
 
         Ok(descriptors)
     } else {
-        Err(AmlError::InvalidOperationOnObject { op: Operation::ParseResource, typ: descriptor.typ() })
+        Err(AmlError::InvalidOperationOnObject { op: Operation::ParseResource, typ: desc_read.typ() })
     }
 }
 

tests/static_assertions.rs

@@ -0,0 +1,6 @@
+use aml_test_tools::handlers::null_handler::NullHandler;
+use acpi::aml::{Interpreter, object::WrappedObject};
+use static_assertions::assert_impl_all;
+
+assert_impl_all!(Interpreter<NullHandler>: Send, Sync);
+assert_impl_all!(WrappedObject: Send, Sync);

tools/aml_test_tools/src/lib.rs

@@ -448,10 +448,11 @@ where
 
         if let Some(result) = interpreter.evaluate_if_present(AmlName::from_str("\\MAIN").unwrap(), vec![])? {
             let expected_result = expected_result.as_ref().unwrap_or(&ExpectedResult::Integer(0));
-            if result_matches(expected_result, &result) {
+            let result_read = result.read();
+            if result_matches(expected_result, &*result_read) {
                 Ok(())
             } else {
-                let e = format!("Unexpected MAIN result: {}, expected: {:?}", *result, expected_result);
+                let e = format!("Unexpected MAIN result: {}, expected: {:?}", *result_read, expected_result);
                 error!("{}", e);
                 Err(AmlError::HostError(e))
             }