diff --git a/.github/workflows/miri.yml b/.github/workflows/miri.yml new file mode 100644 index 00000000..9aa5dd4b --- /dev/null +++ b/.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 diff --git a/src/sys/mod.rs b/src/sys/mod.rs index f10bf195..c0ed9c7b 100644 --- a/src/sys/mod.rs +++ b/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 { diff --git a/src/sys/tables.rs b/src/sys/tables.rs index d6976449..db8e1b19 100644 --- a/src/sys/tables.rs +++ b/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); + pub struct $llowningtable(super::tskbox::TskBox); impl $llowningtable { pub fn new() -> Self { - let temp = unsafe { - libc::malloc(std::mem::size_of::()) - as *mut super::bindings::$tsktable - }; - let nonnull = match std::ptr::NonNull::::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::::as_ptr(&self.0) + self.0.as_ptr() } pub fn as_mut_ptr(&mut self) -> *mut super::bindings::$tsktable { - MBox::::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 { @@ -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), - } - } - } }; } diff --git a/src/sys/tskbox.rs b/src/sys/tskbox.rs new file mode 100644 index 00000000..4e23f7fb --- /dev/null +++ b/src/sys/tskbox.rs @@ -0,0 +1,182 @@ +use std::ptr::NonNull; + +#[derive(Debug)] +pub struct TskBox { + tsk: NonNull, + teardown: Option i32>, + owning: bool, +} + +// SAFETY: these must be encapsulated in types that work +// via shared/immutable reference AND/OR use data protection methods. +unsafe impl Send for TskBox {} + +// SAFETY: these must be encapsulated in types that work +// via shared/immutable reference AND/OR use data protection methods. +unsafe impl Sync for TskBox {} + +impl TskBox { + pub fn new i32>( + init: F, + teardown: unsafe extern "C" fn(*mut T) -> i32, + ) -> Self { + let x = unsafe { libc::malloc(std::mem::size_of::()) 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 Drop for TskBox { + 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) {} + +// 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) }; +}