refactor: Replace MBox with TskBox for "owning" table types (#536)

* Add MIRI checks to CI

This new type handles setup, teardown, and ownership.

.github/workflows/miri.yml

@@ -0,0 +1,27 @@
+on:
+  pull_request:
+
+name: Miri
+
+# NOTE: due to extensive FFI use, all tests
+# run with miri must have miri in the test name.
+# Such tests must make NO calls into tskit-c!
+# (They must make no calls into any FFI for that matter...)
+# The "test miri" args below only run the desired tests.
+
+jobs:
+  test_miri:
+    name: Test (Miri)
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v2
+      - uses: actions-rs/toolchain@v1
+        with:
+          profile: minimal
+          toolchain: nightly
+          override: true
+          components: miri
+      - uses: actions-rs/cargo@v1
+        with:
+          command: miri
+          args: test miri

src/sys/mod.rs

@@ -20,6 +20,8 @@ pub use tables::*;
 pub use tree::LLTree;
 pub use treeseq::LLTreeSeq;
 
+mod tskbox;
+
 #[non_exhaustive]
 #[derive(Error, Debug)]
 pub enum Error {

src/sys/tables.rs

@@ -1,7 +1,5 @@
 use std::ptr::NonNull;
 
-use mbox::MBox;
-
 use super::Error;
 
 macro_rules! basic_lltableref_impl {
@@ -44,37 +42,23 @@ macro_rules! basic_llowningtable_impl {
     ($llowningtable: ident, $tsktable: ident, $init: ident, $free: ident, $clear: ident) => {
         #[repr(transparent)]
         #[derive(Debug)]
-        pub struct $llowningtable(MBox<super::bindings::$tsktable>);
+        pub struct $llowningtable(super::tskbox::TskBox<super::bindings::$tsktable>);
 
         impl $llowningtable {
             pub fn new() -> Self {
-                let temp = unsafe {
-                    libc::malloc(std::mem::size_of::<super::bindings::$tsktable>())
-                        as *mut super::bindings::$tsktable
-                };
-                let nonnull = match std::ptr::NonNull::<super::bindings::$tsktable>::new(temp) {
-                    Some(x) => x,
-                    None => panic!("out of memory"),
-                };
-                let mut table = unsafe { mbox::MBox::from_non_null_raw(nonnull) };
-                let rv = unsafe { super::bindings::$init(&mut (*table), 0) };
-                assert_eq!(rv, 0);
+                let table = super::tskbox::TskBox::new(
+                    |x: *mut super::bindings::$tsktable| unsafe { super::bindings::$init(x, 0) },
+                    super::bindings::$free,
+                );
                 Self(table)
             }
 
             pub fn as_ptr(&self) -> *const super::bindings::$tsktable {
-                MBox::<super::bindings::$tsktable>::as_ptr(&self.0)
+                self.0.as_ptr()
             }
 
             pub fn as_mut_ptr(&mut self) -> *mut super::bindings::$tsktable {
-                MBox::<super::bindings::$tsktable>::as_mut_ptr(&mut self.0)
-            }
-
-            fn free(&mut self) -> Result<(), Error> {
-                match unsafe { super::bindings::$free(self.as_mut_ptr()) } {
-                    code if code < 0 => Err(Error::Code(code)),
-                    _ => Ok(()),
-                }
+                self.0.as_mut_ptr()
             }
 
             pub fn clear(&mut self) -> Result<i32, Error> {
@@ -84,15 +68,6 @@ macro_rules! basic_llowningtable_impl {
                 }
             }
         }
-
-        impl Drop for $llowningtable {
-            fn drop(&mut self) {
-                match self.free() {
-                    Ok(_) => (),
-                    Err(e) => panic!("{}", e),
-                }
-            }
-        }
     };
 }
 

src/sys/tskbox.rs

@@ -0,0 +1,182 @@
+use std::ptr::NonNull;
+
+#[derive(Debug)]
+pub struct TskBox<T> {
+    tsk: NonNull<T>,
+    teardown: Option<unsafe extern "C" fn(*mut T) -> i32>,
+    owning: bool,
+}
+
+// SAFETY: these must be encapsulated in types that work
+// via shared/immutable reference AND/OR use data protection methods.
+unsafe impl<T> Send for TskBox<T> {}
+
+// SAFETY: these must be encapsulated in types that work
+// via shared/immutable reference AND/OR use data protection methods.
+unsafe impl<T> Sync for TskBox<T> {}
+
+impl<T> TskBox<T> {
+    pub fn new<F: Fn(*mut T) -> i32>(
+        init: F,
+        teardown: unsafe extern "C" fn(*mut T) -> i32,
+    ) -> Self {
+        let x = unsafe { libc::malloc(std::mem::size_of::<T>()) as *mut T };
+        let _ = init(x);
+        let tsk = NonNull::new(x).unwrap();
+        Self {
+            tsk,
+            teardown: Some(teardown),
+            owning: true,
+        }
+    }
+
+    /// # Safety
+    ///
+    /// This function clones the NonNull of `owner` and will
+    /// not perform any teardown when dropped.
+    ///
+    /// Cloning the pointer elides the tied lifetimes of owner
+    /// and the new instance.
+    ///
+    /// Therefore, the only sound use of this type involves
+    /// encapsulation in such a way that its lifetime is bound
+    /// to the owner.
+    ///
+    /// For example, instances should only be publicly exposed
+    /// via reference types.
+    #[allow(dead_code)]
+    pub unsafe fn new_borrowed(owner: &Self) -> Self {
+        let tsk = owner.tsk;
+        Self {
+            tsk,
+            teardown: None,
+            owning: false,
+        }
+    }
+
+    /// # Safety
+    ///
+    /// The returned pointer is no longer managed, meaing
+    /// that it is the caller's responsibility to properly
+    /// tear down and free the return value.
+    ///
+    /// Failing to do so will may result in a
+    /// memory leak.
+    #[allow(dead_code)]
+    pub unsafe fn into_raw(self) -> *mut T {
+        let mut s = self;
+        let rv = s.as_mut_ptr();
+        s.teardown = None;
+        s.owning = false;
+        rv
+    }
+
+    pub fn as_ref(&self) -> &T {
+        unsafe { self.tsk.as_ref() }
+    }
+
+    pub fn as_mut(&mut self) -> &mut T {
+        unsafe { self.tsk.as_mut() }
+    }
+
+    pub fn as_ptr(&self) -> *const T {
+        self.as_ref()
+    }
+
+    pub fn as_mut_ptr(&mut self) -> *mut T {
+        self.as_mut()
+    }
+}
+
+impl<T> Drop for TskBox<T> {
+    fn drop(&mut self) {
+        if let Some(teardown) = self.teardown {
+            unsafe {
+                (teardown)(self.tsk.as_mut() as *mut T);
+            }
+        }
+        if self.owning {
+            unsafe { libc::free(self.tsk.as_ptr() as *mut libc::c_void) }
+        }
+    }
+}
+
+#[cfg(test)]
+fn is_send_sync<T: Send + Sync>(_: &T) {}
+
+// NOTE: tests must not make calls into the tskit C API!
+// We need to use miri to check for UB, and miri cannot
+// work accross FFI.
+
+#[test]
+fn test_miri() {
+    struct X {
+        data: i32,
+    }
+
+    unsafe extern "C" fn teardown_x(_: *mut X) -> i32 {
+        0
+    }
+
+    let options = 0_i32;
+
+    let b = TskBox::new(
+        |x: *mut X| unsafe {
+            (*x).data = options;
+            0
+        },
+        teardown_x,
+    );
+
+    let _ = unsafe { TskBox::new_borrowed(&b) };
+
+    is_send_sync(&b)
+}
+
+#[test]
+fn test_into_raw_miri() {
+    struct X {
+        data: i32,
+    }
+
+    unsafe extern "C" fn teardown_x(_: *mut X) -> i32 {
+        0
+    }
+
+    let options = 0_i32;
+
+    let b = TskBox::new(
+        |x: *mut X| unsafe {
+            (*x).data = options;
+            0
+        },
+        teardown_x,
+    );
+
+    let p = unsafe { b.into_raw() };
+
+    unsafe { libc::free(p as *mut libc::c_void) }
+}
+
+#[test]
+fn test_table_collection_tskbox() {
+    let flags: u32 = 0;
+    let _ = TskBox::new(
+        |t: *mut super::bindings::tsk_table_collection_t| unsafe {
+            super::bindings::tsk_table_collection_init(t, flags)
+        },
+        super::bindings::tsk_table_collection_free,
+    );
+}
+
+#[test]
+fn test_table_collection_tskbox_shared_ptr() {
+    let flags: u32 = 0;
+    let tables = TskBox::new(
+        |t: *mut super::bindings::tsk_table_collection_t| unsafe {
+            super::bindings::tsk_table_collection_init(t, flags)
+        },
+        super::bindings::tsk_table_collection_free,
+    );
+    let _ = unsafe { TskBox::new_borrowed(&tables) };
+}