Make Allowed require Copy and remove all the Drop-related complexity.

Originally, I build Allowed so it can work with non-Copy types, which allows Drop types. The semantics of storing non-Copy and non-Drop types in shared memory, however, are somewhat nonobvious. Based on code review feedback, and being particularly cautious because Allowed uses `unsafe`, I decided to add a Copy constraint to Allowed.

Author: Johnathan Van Why

core/platform/src/allows/allowed.rs

@@ -1,9 +1,13 @@
 /// An individual value that has been shared with the kernel using the `allow`
 /// system call.
-// Design note: Allowed's implementation does not directly use the 'b lifetime.
-// Platform uses 'b to prevent the Allowed from accessing the buffer after the
-// buffer becomes invalid.
-pub struct Allowed<'b, T: 'b> {
+// Allowed's implementation does not directly use the 'b lifetime. Platform uses
+// 'b to prevent the Allowed from accessing the buffer after the buffer becomes
+// invalid.
+// Allowed requires T to be Copy due to concerns about the semantics of
+// non-copyable types in shared memory as well as concerns about unexpected
+// behavior with Drop types. See the following PR discussion for more
+// information: https://github.com/tock/libtock-rs/pull/222
+pub struct Allowed<'b, T: Copy + 'b> {
     // Safety properties:
     //   1. `buffer` remains valid and usable for the lifetime of this Allowed
     //      instance.
@@ -37,7 +41,7 @@ pub struct Allowed<'b, T: 'b> {
 // need to be able to deconflict races between the kernel (which will never
 // interrupt an instruction's execution) and this process. Therefore volatile
 // accesses are sufficient to deconflict races.
-impl<'b, T: 'b> Allowed<'b, T> {
+impl<'b, T: Copy + 'b> Allowed<'b, T> {
     // Allowed can only be constructed by the Platform. It is constructed after
     // the `allow` system call, and as such must accept a raw pointer rather
     // than a reference. The caller must make sure the following are true:
@@ -52,33 +56,29 @@ impl<'b, T: 'b> Allowed<'b, T> {
         }
     }
 
-    // Sets the value in the buffer. Note that unlike `core::cell::Cell::set`,
-    // `Allowed::set` will not drop the existing value. To drop the existing
-    // value, use `replace` and drop the return value.
+    // Sets the value in the buffer.
     pub fn set(&self, value: T) {
         unsafe {
             core::ptr::write_volatile(self.buffer.as_ptr(), value);
         }
     }
 }
 
-impl<'b, T: crate::AllowReadable + 'b> Allowed<'b, T> {
+impl<'b, T: crate::AllowReadable + Copy + 'b> Allowed<'b, T> {
     pub fn replace(&self, value: T) -> T {
         let current = unsafe { core::ptr::read_volatile(self.buffer.as_ptr()) };
         unsafe {
             core::ptr::write_volatile(self.buffer.as_ptr(), value);
         }
         current
     }
-}
 
-impl<'b, T: crate::AllowReadable + Copy + 'b> Allowed<'b, T> {
     pub fn get(&self) -> T {
         unsafe { core::ptr::read_volatile(self.buffer.as_ptr()) }
     }
 }
 
-impl<'b, T: crate::AllowReadable + Default + 'b> Allowed<'b, T> {
+impl<'b, T: crate::AllowReadable + Copy + Default + 'b> Allowed<'b, T> {
     pub fn take(&self) -> T {
         self.replace(T::default())
     }

core/platform/src/allows/allowed_tests.rs

@@ -1,4 +1,3 @@
-use crate::allows::test_util::DropCheck;
 use crate::Allowed;
 use core::marker::PhantomData;
 use core::ptr::NonNull;
@@ -32,15 +31,15 @@ use core::ptr::NonNull;
 // interacting with a real Tock kernel.
 //
 // [1] https://plv.mpi-sws.org/rustbelt/stacked-borrows/paper.pdf
-struct KernelPtr<'b, T: 'b> {
+struct KernelPtr<'b, T: Copy + 'b> {
     ptr: NonNull<T>,
 
     // We need to consume the 'b lifetime. This is very similar to Allowed's
     // implementation.
     _phantom: PhantomData<&'b mut T>,
 }
 
-impl<'b, T: 'b> KernelPtr<'b, T> {
+impl<'b, T: Copy + 'b> KernelPtr<'b, T> {
     // The constructor for KernelPtr; simulates allow(). Returns both the
     // Allowed instance the Platform would return and a KernelPtr the test can
     // use to simulate a kernel.
@@ -59,126 +58,67 @@ impl<'b, T: 'b> KernelPtr<'b, T> {
         (allowed, kernel_ptr)
     }
 
-    // Replaces the value in the buffer with a new one. Does not drop the
-    // existing value.
+    // Replaces the value in the buffer with a new one.
     pub fn set(&self, value: T) {
         unsafe {
             core::ptr::write(self.ptr.as_ptr(), value);
         }
     }
-}
 
-impl<'b, T: Copy + 'b> KernelPtr<'b, T> {
     // Copies the contained value out of the buffer.
     pub fn get(&self) -> T {
         unsafe { core::ptr::read(self.ptr.as_ptr()) }
     }
 }
 
-impl<'b, 'f: 'b> KernelPtr<'b, DropCheck<'f>> {
-    // Retrieves the value of the contained DropCheck.
-    pub fn value(&self) -> usize {
-        let drop_check = unsafe { core::ptr::read(self.ptr.as_ptr()) };
-        let value = drop_check.value;
-        core::mem::forget(drop_check);
-        value
-    }
-}
-
 #[test]
 fn set() {
-    let dropped1 = core::cell::Cell::new(false);
-    let dropped2 = core::cell::Cell::new(false);
-    let dropped3 = core::cell::Cell::new(false);
-    let mut buffer = DropCheck {
-        flag: Some(&dropped1),
-        value: 1,
-    };
+    let mut buffer = 1;
     let (allowed, kernel_ptr) = KernelPtr::allow(&mut buffer);
-    assert_eq!(kernel_ptr.value(), 1);
-    assert_eq!(dropped1.get(), false);
+    assert_eq!(kernel_ptr.get(), 1);
 
-    // Simulate the kernel replacing the value in buffer. We don't drop the
-    // existing value.
-    kernel_ptr.set(DropCheck {
-        flag: Some(&dropped2),
-        value: 2,
-    });
-    allowed.set(DropCheck {
-        flag: Some(&dropped3),
-        value: 3,
-    });
-    assert_eq!(kernel_ptr.value(), 3);
-    assert_eq!(dropped1.get(), false);
-    assert_eq!(dropped2.get(), false);
-    assert_eq!(dropped3.get(), false);
+    // Simulate the kernel replacing the value in buffer.
+    kernel_ptr.set(2);
+    allowed.set(3);
+    assert_eq!(kernel_ptr.get(), 3);
 }
 
 #[test]
 fn replace() {
-    let dropped1 = core::cell::Cell::new(false);
-    let dropped2 = core::cell::Cell::new(false);
-    let dropped3 = core::cell::Cell::new(false);
-    let mut buffer = DropCheck {
-        flag: Some(&dropped1),
-        value: 1,
-    };
+    let mut buffer = 1;
     let (allowed, kernel_ptr) = KernelPtr::allow(&mut buffer);
-    assert_eq!(kernel_ptr.value(), 1);
-    assert_eq!(dropped1.get(), false);
+    assert_eq!(kernel_ptr.get(), 1);
 
-    // Simulate the kernel replacing the value in buffer. We don't drop the
-    // existing value.
-    kernel_ptr.set(DropCheck {
-        flag: Some(&dropped2),
-        value: 2,
-    });
-    let returned = allowed.replace(DropCheck {
-        flag: Some(&dropped3),
-        value: 3,
-    });
-    assert_eq!(returned.value, 2);
-    assert_eq!(kernel_ptr.value(), 3);
-    assert_eq!(dropped1.get(), false);
-    assert_eq!(dropped2.get(), false);
-    assert_eq!(dropped3.get(), false);
+    // Simulate the kernel replacing the value in buffer.
+    kernel_ptr.set(2);
+    let returned = allowed.replace(3);
+    assert_eq!(returned, 2);
+    assert_eq!(kernel_ptr.get(), 3);
 }
 
 #[test]
 fn get() {
-    // We can't use DropCheck because Drop and Copy cannot both be implemented
-    // on a single type.
     let mut buffer = 1;
     let (allowed, kernel_ptr) = KernelPtr::allow(&mut buffer);
     assert_eq!(kernel_ptr.get(), 1);
+
+    assert_eq!(allowed.get(), 1);
+    assert_eq!(kernel_ptr.get(), 1);
+
     kernel_ptr.set(2);
     assert_eq!(allowed.get(), 2);
     assert_eq!(kernel_ptr.get(), 2);
 }
 
 #[test]
 fn take() {
-    let dropped1 = core::cell::Cell::new(false);
-    let dropped2 = core::cell::Cell::new(false);
-    let dropped3 = core::cell::Cell::new(false);
-    let mut buffer = DropCheck {
-        flag: Some(&dropped1),
-        value: 1,
-    };
+    let mut buffer = 1;
     let (allowed, kernel_ptr) = KernelPtr::allow(&mut buffer);
-    assert_eq!(kernel_ptr.value(), 1);
-    assert_eq!(dropped1.get(), false);
+    assert_eq!(kernel_ptr.get(), 1);
 
-    // Simulate the kernel replacing the value in buffer. We don't drop the
-    // existing value.
-    kernel_ptr.set(DropCheck {
-        flag: Some(&dropped2),
-        value: 2,
-    });
+    // Simulate the kernel replacing the value in buffer.
+    kernel_ptr.set(2);
     let returned = allowed.take();
-    assert_eq!(returned.value, 2);
-    assert_eq!(kernel_ptr.value(), 0);
-    assert_eq!(dropped1.get(), false);
-    assert_eq!(dropped2.get(), false);
-    assert_eq!(dropped3.get(), false);
+    assert_eq!(returned, 2);
+    assert_eq!(kernel_ptr.get(), 0);
 }

core/platform/src/allows/mod.rs

@@ -6,5 +6,3 @@ pub use allowed::Allowed;
 
 #[cfg(test)]
 mod allowed_tests;
-#[cfg(test)]
-mod test_util;