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.
diff --git a/core/platform/src/allows/allowed.rs b/core/platform/src/allows/allowed.rs index db8ba26..4d66c2b 100644 --- a/core/platform/src/allows/allowed.rs +++ b/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 @@ // 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,9 +56,7 @@ } } - // 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); @@ -62,7 +64,7 @@ } } -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 { @@ -70,15 +72,13 @@ } 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()) }
diff --git a/core/platform/src/allows/allowed_tests.rs b/core/platform/src/allows/allowed_tests.rs index ec6a358..f1f0c08 100644 --- a/core/platform/src/allows/allowed_tests.rs +++ b/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,7 +31,7 @@ // 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 @@ -40,7 +39,7 @@ _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,98 +58,53 @@ (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); @@ -158,27 +112,13 @@ #[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); }
diff --git a/core/platform/src/allows/mod.rs b/core/platform/src/allows/mod.rs index 410d158..8cb2158 100644 --- a/core/platform/src/allows/mod.rs +++ b/core/platform/src/allows/mod.rs
@@ -6,5 +6,3 @@ #[cfg(test)] mod allowed_tests; -#[cfg(test)] -mod test_util;
diff --git a/core/platform/src/allows/test_util.rs b/core/platform/src/allows/test_util.rs deleted file mode 100644 index 0f63d9c..0000000 --- a/core/platform/src/allows/test_util.rs +++ /dev/null
@@ -1,37 +0,0 @@ -//! Contains testing utilities for libtock_platform::allowed. - -// A value that can be placed in an Allowed that checks whether it has been -// dropped. -#[derive(Default)] -pub(crate) struct DropCheck<'f> { - pub flag: Option<&'f core::cell::Cell<bool>>, - pub value: usize, -} - -impl<'f> Drop for DropCheck<'f> { - fn drop(&mut self) { - if let Some(flag) = self.flag { - flag.set(true); - } - } -} - -// Note: DropCheck cannot be safely used in a non-test context, as DropCheck -// does not satisfy AllowReadable's "every bit pattern is valid" requirement. -// However, in the tests we aren't sharing the buffer with a real Tock kernel, -// and instead we guarantee that only valid instances of DropCheck are loaded -// into the buffer. -unsafe impl<'f> crate::AllowReadable for DropCheck<'f> {} - -// Verify that DropCheck works, as none of the tests require it to actually set -// the drop flag. -#[test] -fn drop_check() { - let flag = core::cell::Cell::new(false); - let drop_check = DropCheck { - flag: Some(&flag), - value: 0, - }; - drop(drop_check); - assert_eq!(flag.get(), true); -}