Merge pull request andrewcsmith#11 from l0calh05t/safety

Redesign of create_auxiliary_task for improved safety

Cargo.toml

@@ -4,8 +4,6 @@ version = "0.1.0"
 authors = ["Andrew C. Smith <[email protected]>"]
 
 [dependencies]
-libc = "0.2"
-nix = "0.21"
 
 [dev-dependencies]
 sample = { package = "dasp", version = "0.11.0", features = [ "signal", "slice" ] }

examples/auxiliary_task.rs

@@ -8,66 +8,38 @@ extern crate sample;
 
 use bela::*;
 
-#[derive(Clone)]
-struct PrintTask<F> {
-    callback: F,
-    args: String,
-}
-
-impl<F> Auxiliary for PrintTask<F>
-where
-    F: FnMut(&mut String),
-    for<'r> F: FnMut(&'r mut String),
-{
-    type Args = String;
-
-    fn destructure(&mut self) -> (&mut dyn FnMut(&mut String), &mut Self::Args) {
-        let PrintTask { callback, args } = self;
-
-        (callback, args)
-    }
-}
-
-struct MyData<'a> {
+struct MyData {
     frame_index: usize,
-    tasks: Vec<CreatedTask<'a>>,
+    tasks: Vec<CreatedTask>,
 }
 
-type BelaApp<'a> = Bela<AppData<'a, MyData<'a>>>;
+type BelaApp<'a> = Bela<AppData<'a, MyData>>;
 
 fn main() {
     go().unwrap();
 }
 
 fn go() -> Result<(), error::Error> {
-    let what_to_print = "this is a string".to_string();
-    let print_task = PrintTask {
-        callback: |args: &mut String| {
-            args.push_str("LOL");
-            println!("{}", args);
-        },
-        args: what_to_print,
-    };
-
-    let more_to_print = "this is another string".to_string();
-    let mut another_print_task = PrintTask {
-        callback: |args: &mut String| {
-            args.push_str("LOL");
-            println!("{}", args);
-        },
-        args: more_to_print,
-    };
-
-    let mut boxed = Box::new(print_task);
-
     let mut setup = |_context: &mut Context, user_data: &mut MyData| -> Result<(), error::Error> {
         println!("Setting up");
-        user_data
-            .tasks
-            .push(unsafe { BelaApp::create_auxiliary_task(&mut boxed, 10, "printing_stuff") });
-        user_data.tasks.push(unsafe {
-            BelaApp::create_auxiliary_task(&mut another_print_task, 10, "printing_more_stuff")
+        let print_task = Box::new(|| {
+            println!("this is a string");
         });
+
+        let another_print_task = Box::new(|| {
+            println!("this is another string");
+        });
+
+        user_data.tasks.push(BelaApp::create_auxiliary_task(
+            print_task,
+            10,
+            &std::ffi::CString::new("printing_stuff").unwrap(),
+        ));
+        user_data.tasks.push(BelaApp::create_auxiliary_task(
+            another_print_task,
+            10,
+            &std::ffi::CStr::from_bytes_with_nul(b"printing_more_stuff\0").unwrap(),
+        ));
         Ok(())
     };
 

src/lib.rs

@@ -1,10 +1,7 @@
 extern crate bela_sys;
-extern crate libc;
-extern crate nix;
 
 use bela_sys::{BelaContext, BelaInitSettings};
 use std::convert::TryInto;
-use std::marker::PhantomData;
 use std::{mem, slice};
 use std::{thread, time};
 
@@ -74,77 +71,41 @@ pub struct Bela<T> {
     user_data: T,
 }
 
-unsafe extern "C" fn render_trampoline<'a, T>(
+extern "C" fn render_trampoline<'a, T>(
     context: *mut BelaContext,
     user_data: *mut std::os::raw::c_void,
 ) where
     T: UserData<'a> + 'a,
 {
     let mut context = Context::new(context);
-    let user_data = &mut *(user_data as *mut T);
+    let user_data = unsafe { &mut *(user_data as *mut T) };
     user_data.render_fn(&mut context);
 }
 
-unsafe extern "C" fn setup_trampoline<'a, T>(
+extern "C" fn setup_trampoline<'a, T>(
     context: *mut BelaContext,
     user_data: *mut std::os::raw::c_void,
 ) -> bool
 where
     T: UserData<'a> + 'a,
 {
     let mut context = Context::new(context);
-    let user_data = &mut *(user_data as *mut T);
+    let user_data = unsafe { &mut *(user_data as *mut T) };
     user_data.setup_fn(&mut context).is_ok()
 }
 
-unsafe extern "C" fn cleanup_trampoline<'a, T>(
+extern "C" fn cleanup_trampoline<'a, T>(
     context: *mut BelaContext,
     user_data: *mut std::os::raw::c_void,
 ) where
     T: UserData<'a> + 'a,
 {
     let mut context = Context::new(context);
-    let user_data = &mut *(user_data as *mut T);
+    let user_data = unsafe { &mut *(user_data as *mut T) };
     user_data.cleanup_fn(&mut context);
 }
 
-/// The "args" here must include the actual auxiliary task callback!
-unsafe extern "C" fn auxiliary_task_trampoline<T>(aux_ptr: *mut std::os::raw::c_void)
-where
-    T: Auxiliary,
-{
-    let auxiliary = &mut *(aux_ptr as *mut T);
-    let (callback, args) = auxiliary.destructure();
-    callback(args);
-}
-
-/// Trait for `AuxiliaryTask`s, which run at a lower priority than the audio
-/// thread.
-///
-/// An `AuxiliaryTask` must contain both its callback closure and its arguments;
-/// this is so that we can capture outer variables in the closure, and also
-/// mutate state if we need to in a type-safe way.
-pub trait Auxiliary {
-    type Args: ?Sized;
-
-    /// `destructure` should split the Auxiliary into the closure and its
-    /// arguments. This is called by the `unsafe extern` trampoline function to
-    /// actually run the task at the proper Xenomai priority.
-    fn destructure(&mut self) -> (&mut dyn FnMut(&mut Self::Args), &mut Self::Args);
-}
-
-impl<T> Auxiliary for Box<T>
-where
-    T: Auxiliary + ?Sized,
-{
-    type Args = T::Args;
-
-    fn destructure(&mut self) -> (&mut dyn FnMut(&mut Self::Args), &mut Self::Args) {
-        T::destructure(self)
-    }
-}
-
-pub struct CreatedTask<'a>(bela_sys::AuxiliaryTask, PhantomData<&'a mut ()>);
+pub struct CreatedTask(bela_sys::AuxiliaryTask);
 
 impl<'a, T: UserData<'a> + 'a> Bela<T> {
     pub fn new(user_data: T) -> Self {
@@ -196,8 +157,10 @@ impl<'a, T: UserData<'a> + 'a> Bela<T> {
         settings.settings.render = Some(render_trampoline::<T>);
         settings.settings.cleanup = Some(cleanup_trampoline::<T>);
         let out = unsafe {
-            let ptr: *mut std::os::raw::c_void = mem::transmute(&mut self.user_data);
-            bela_sys::Bela_initAudio(settings.settings_ptr(), ptr)
+            bela_sys::Bela_initAudio(
+                settings.settings_ptr(),
+                &mut self.user_data as *mut _ as *mut _,
+            )
         };
 
         match out {
@@ -226,34 +189,40 @@ impl<'a, T: UserData<'a> + 'a> Bela<T> {
         unsafe { bela_sys::Bela_stopRequested() != 0 }
     }
 
-    /// Takes a _mutable reference_ to the task, because we must be ensured that
-    /// the task is unique and that it does not move.
-    ///
-    /// # Safety
-    /// I highly recommend ONLY USING STACK-ALLOCATED CLOSURES AS TASKS. This
-    /// particular implementation is wildly unsafe, but if you use a stack
-    /// closure it _should_ be possible to avoid a segfault. See the
-    /// auxiliary_task example for a demo.
-    pub unsafe fn create_auxiliary_task<'b, 'c, A: 'b>(
-        task: &'c mut A,
+    /// Create an auxiliary task that runs on a lower-priority thread
+    /// `name` must be globally unique across all Xenomai processes!
+    pub fn create_auxiliary_task<Auxiliary>(
+        task: Box<Auxiliary>,
         priority: i32,
-        name: &'static str,
-    ) -> CreatedTask<'b>
+        name: &std::ffi::CStr,
+    ) -> CreatedTask
     where
-        A: Auxiliary,
+        Auxiliary: FnMut() + Send + 'static,
     {
-        let task_ptr = task as *const _ as *mut std::os::raw::c_void;
+        // TODO: Bela API does not currently offer an API to stop and unregister a task,
+        // so we can only leak the task. Otherwise, we could `Box::into_raw` here, store the
+        // raw pointer in `CreatedTask` and drop it after unregistering & joining the thread
+        // using `Box::from_raw`.
+        let task_ptr = Box::leak(task) as *mut _ as *mut _;
+
+        extern "C" fn auxiliary_task_trampoline<Auxiliary>(aux_ptr: *mut std::os::raw::c_void)
+        where
+            Auxiliary: FnMut() + Send + 'static,
+        {
+            let task_ptr = unsafe { &mut *(aux_ptr as *mut Auxiliary) };
+            task_ptr();
+        }
 
-        let aux_task = {
+        let aux_task = unsafe {
             bela_sys::Bela_createAuxiliaryTask(
-                Some(auxiliary_task_trampoline::<A>),
+                Some(auxiliary_task_trampoline::<Auxiliary>),
                 priority,
-                name.as_bytes().as_ptr(),
+                name.as_ptr(),
                 task_ptr,
             )
         };
 
-        CreatedTask(aux_task, PhantomData)
+        CreatedTask(aux_task)
     }
 
     pub fn schedule_auxiliary_task(task: &CreatedTask) -> Result<(), error::Error> {