Fix unsoundness in ImmediateIO and TransactionalIO, aka RUSTSEC-2020-…

…0152 aka

#1.

Unsafe impls of Sync for both of these structs are removed, and Send bounds are
introduced where necessary to allow Sync to be properly deduced instead.

The unsafe impls were introduced due to an incorrect analysis of why Sync was
not being auto-deduced as expected for these structs. The removed comment
explains the original flawed analysis, which attributed it to the presence of a
struct member of type PhantomData<EI>, under the assumption that the only
"real" value of type EI in the struct is ultimately inside an IOMutex that
should bless it with Sync-ness regardless if EI is itself Sync.

In fact, Sync was not being deduced due to the absence of a *Send* bound on the
type EI, which is required for Expander<EI> to be Send, which is required for
IOMutex<Expander<EI>> to be *Sync*. The presence of the PhantomData<EI> member
turns out to be entirely irrelevant.

The bug could theoretically result in a data race if either type is used with
an EI that is not safe to send between threads.

This is a breaking change due to the addition of a trait bound in the public
API.

src/config.rs

@@ -130,13 +130,13 @@ impl From<ExpanderConfig> for u8 {
 ///     .unwrap();
 /// ```
 #[must_use = "Configuration changes are not applied unless committed"]
-pub struct Configurator<'e, EI: ExpanderInterface> {
+pub struct Configurator<'e, EI: ExpanderInterface + Send> {
     expander: &'e mut Expander<EI>,
     expander_config_dirty: bool,
     banks: [BankConfig; 7],
 }
 
-impl<'e, EI: ExpanderInterface> Configurator<'e, EI> {
+impl<'e, EI: ExpanderInterface + Send> Configurator<'e, EI> {
     pub(crate) fn new(expander: &'e mut Expander<EI>) -> Self {
         Self {
             expander,

src/expander/immediate.rs

@@ -14,23 +14,12 @@ use registers::valid_port;
 pub struct ImmediateIO<M, EI>(M, PhantomData<EI>)
 where
     M: IOMutex<Expander<EI>>,
-    EI: ExpanderInterface;
-
-// Unsafety: This is only needed because the presence of PhantomData<EI> causes the struct to no
-// longer be Sync, because EI is often not Sync since it owns a global resource (e.g. SPI device).
-// However, the EI is actually owned by the Expander which is in the mutex which normally
-// re-instates Sync-ness. PhantomData is there to shut up the unused type parameter error.
-unsafe impl<M, EI> Sync for ImmediateIO<M, EI>
-where
-    M: IOMutex<Expander<EI>>,
-    EI: ExpanderInterface,
-{
-}
+    EI: ExpanderInterface + Send;
 
 impl<M, EI> ImmediateIO<M, EI>
 where
     M: IOMutex<Expander<EI>>,
-    EI: ExpanderInterface,
+    EI: ExpanderInterface + Send,
 {
     pub(crate) fn new(expander: Expander<EI>) -> Self {
         ImmediateIO(M::new(expander), PhantomData)
@@ -54,7 +43,7 @@ where
 impl<M, EI> ExpanderIO for ImmediateIO<M, EI>
 where
     M: IOMutex<Expander<EI>>,
-    EI: ExpanderInterface,
+    EI: ExpanderInterface + Send,
 {
     fn write_port(&self, port: u8, bit: bool) {
         self.0.lock(|ex| ex.write_port(port, bit).unwrap())

src/expander/mod.rs

@@ -18,7 +18,7 @@ pub struct Expander<EI: ExpanderInterface> {
     pub(crate) config: ExpanderConfig,
 }
 
-impl<EI: ExpanderInterface> Expander<EI> {
+impl<EI: ExpanderInterface + Send> Expander<EI> {
     /// Create a new `Expander`.
     ///
     /// Takes ownership of the `ExpanderInterface` which it should use to communicate with the

src/expander/transactional.rs

@@ -48,7 +48,7 @@ pub enum Strategy {
 pub struct TransactionalIO<M, EI>
 where
     M: IOMutex<Expander<EI>>,
-    EI: ExpanderInterface,
+    EI: ExpanderInterface + Send,
 {
     expander: M,
     issued: AtomicUsize,
@@ -58,21 +58,10 @@ where
     _ei: PhantomData<EI>,
 }
 
-// NOTE(unsafe): This is only needed because the presence of PhantomData<EI> causes the struct to
-// no longer be Sync, because EI is often not Sync since it owns a global resource (e.g. SPI
-// device).  However, the EI is actually owned by the Expander which is in the mutex which normally
-// re-instates Sync-ness. PhantomData is there to shut up the unused type parameter error.
-unsafe impl<M, EI> Sync for TransactionalIO<M, EI>
-where
-    M: IOMutex<Expander<EI>>,
-    EI: ExpanderInterface,
-{
-}
-
 impl<M, EI> TransactionalIO<M, EI>
 where
     M: IOMutex<Expander<EI>>,
-    EI: ExpanderInterface,
+    EI: ExpanderInterface + Send,
 {
     pub(crate) fn new(expander: Expander<EI>) -> Self {
         TransactionalIO {
@@ -160,7 +149,7 @@ where
 impl<M, EI> ExpanderIO for TransactionalIO<M, EI>
 where
     M: IOMutex<Expander<EI>>,
-    EI: ExpanderInterface,
+    EI: ExpanderInterface + Send,
 {
     fn write_port(&self, port: u8, bit: bool) {
         let or_bit = 1 << port;

src/interface.rs

@@ -121,9 +121,8 @@ pub(crate) mod test_spy {
 
     use super::ExpanderInterface;
     use registers::RegisterAddress;
-    use std::cell::RefCell;
     use std::fmt;
-    use std::rc::Rc;
+    use std::sync::{Arc, Mutex};
 
     #[derive(Clone, Copy, Debug, PartialEq)]
     pub enum TestRegister {
@@ -134,15 +133,15 @@ pub(crate) mod test_spy {
     }
 
     pub struct TestSpyInterface {
-        registers: Rc<RefCell<Vec<TestRegister>>>,
-        reads: Rc<RefCell<Vec<u8>>>,
+        registers: Arc<Mutex<Vec<TestRegister>>>,
+        reads: Arc<Mutex<Vec<u8>>>,
     }
 
     impl TestSpyInterface {
         pub fn new() -> Self {
             let mut new = Self {
-                registers: Rc::new(RefCell::new(Vec::new())),
-                reads: Rc::new(RefCell::new(Vec::new())),
+                registers: Arc::new(Mutex::new(Vec::new())),
+                reads: Arc::new(Mutex::new(Vec::new())),
             };
             new.reset();
             new
@@ -151,8 +150,8 @@ pub(crate) mod test_spy {
         pub fn reset(&mut self) {
             use self::TestRegister::*;
 
-            self.reads.borrow_mut().clear();
-            let mut regs = self.registers.borrow_mut();
+            self.reads.lock().unwrap().clear();
+            let mut regs = self.registers.lock().unwrap();
             regs.clear();
             regs.resize(0x60, Forbidden);
 
@@ -179,21 +178,21 @@ pub(crate) mod test_spy {
         }
 
         pub fn get(&self, addr: u8) -> TestRegister {
-            self.registers.borrow()[addr as usize]
+            self.registers.lock().unwrap()[addr as usize]
         }
 
         pub fn set(&mut self, addr: u8, val: TestRegister) {
-            self.registers.borrow_mut()[addr as usize] = val;
+            self.registers.lock().unwrap()[addr as usize] = val;
         }
 
         pub fn reads(&self) -> Vec<u8> {
-            self.reads.borrow().clone()
+            self.reads.lock().unwrap().clone()
         }
     }
 
     impl ExpanderInterface for TestSpyInterface {
         fn write_register(&mut self, addr: RegisterAddress, value: u8) -> Result<(), ()> {
-            let mut regs = self.registers.borrow_mut();
+            let mut regs = self.registers.lock().unwrap();
             let enc_addr = u8::from(addr);
             assert!(enc_addr <= 0x5F);
             match regs[enc_addr as usize] {
@@ -204,8 +203,8 @@ pub(crate) mod test_spy {
             Ok(())
         }
         fn read_register(&mut self, addr: RegisterAddress) -> Result<u8, ()> {
-            self.reads.borrow_mut().push(addr.into());
-            let regs = self.registers.borrow();
+            self.reads.lock().unwrap().push(addr.into());
+            let regs = self.registers.lock().unwrap();
             let enc_addr = u8::from(addr);
             assert!(enc_addr <= 0x5F);
             match regs[enc_addr as usize] {
@@ -240,14 +239,14 @@ pub(crate) mod test_spy {
     }
 
     pub struct SemanticTestSpyInterface {
-        ports: Rc<RefCell<Vec<TestPort>>>,
+        ports: Arc<Mutex<Vec<TestPort>>>,
     }
 
     impl SemanticTestSpyInterface {
         pub fn new(init: Vec<bool>) -> Self {
             assert!(init.len() == 32 - 4);
             Self {
-                ports: Rc::new(RefCell::new(
+                ports: Arc::new(Mutex::new(
                     init.into_iter()
                         .map(|b| TestPort::Reset(b))
                         .collect::<Vec<_>>(),
@@ -264,7 +263,8 @@ pub(crate) mod test_spy {
         pub fn peek_all(&self) -> Vec<TestPort> {
             use self::TestPort::*;
             self.ports
-                .borrow()
+                .lock()
+                .unwrap()
                 .iter()
                 .cloned()
                 .map(|v| match v {
@@ -277,7 +277,8 @@ pub(crate) mod test_spy {
         pub fn peek_bits(&self) -> Vec<bool> {
             use self::TestPort::*;
             self.ports
-                .borrow()
+                .lock()
+                .unwrap()
                 .iter()
                 .cloned()
                 .map(|v| match v {
@@ -289,7 +290,7 @@ pub(crate) mod test_spy {
         fn write_port(&self, port: u8, bit: bool) {
             use self::TestPort::*;
             let idx = port as usize - 4;
-            let slot_ref = &mut self.ports.borrow_mut()[idx];
+            let slot_ref = &mut self.ports.lock().unwrap()[idx];
             *slot_ref = match slot_ref {
                 Reset(_) | BlindWrite(_) => BlindWrite(bit),
                 Read(_) | ReadWrite(_) => ReadWrite(bit),
@@ -299,7 +300,7 @@ pub(crate) mod test_spy {
         fn read_port(&self, port: u8) -> bool {
             use self::TestPort::*;
             let idx = port as usize - 4;
-            let slot_ref = &mut self.ports.borrow_mut()[idx];
+            let slot_ref = &mut self.ports.lock().unwrap()[idx];
             let (upd, ret) = match slot_ref {
                 Reset(b) | Read(b) => (Read(*b), *b),
                 ReadWrite(b) => (ReadWrite(*b), *b),
@@ -316,7 +317,8 @@ pub(crate) mod test_spy {
                 f,
                 "[ {} ]",
                 self.ports
-                    .borrow()
+                    .lock()
+                    .unwrap()
                     .iter()
                     .map(|&v| format!("{:?}", v))
                     .collect::<Vec<_>>()