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);
-}
