fix(ffi): multiple memory leaks are present in different areas of the ffi bridge

Author: maxjoehnk

crates/ui/ffi/src/apis/connections.rs

@@ -9,9 +9,10 @@ use mizer_api::Message;
 use mizer_devices::DeviceManager;
 use mizer_gamepads::{Axis, Button, GamepadRef, GamepadState};
 
-use crate::types::{Array, FFIFromPointer};
+use crate::types::{drop_pointer, Array, FFIFromPointer};
 
 pub struct GamepadConnectionRef(pub GamepadRef);
+// No drop method for this pointer as it is kept open for the whole runtime of the ui
 pub struct ConnectionsRef(pub DeviceManager);
 
 #[no_mangle]
@@ -116,3 +117,8 @@ bitflags! {
         const LEFT = 0b1000;
     }
 }
+
+#[no_mangle]
+pub extern "C" fn drop_gamepad_pointer(pointer: *const GamepadConnectionRef) {
+    drop_pointer(pointer)
+}

crates/ui/ffi/src/apis/fixture.rs

@@ -1,5 +1,5 @@
 use crate::apis::programmer::{FFIColorValue, FFIFixtureId};
-use crate::types::{drop_pointer, Array, FFIFromPointer};
+use crate::types::{drop_array, drop_pointer, Array, FFIFromPointer};
 use mizer_fixtures::{FixtureId, FixtureState, FixtureStates};
 use std::sync::Arc;
 
@@ -61,6 +61,11 @@ pub extern "C" fn drop_fixture_pointer(ptr: *const FixturesRef) {
     drop_pointer(ptr);
 }
 
+#[no_mangle]
+pub extern "C" fn drop_fixture_states(fixture_states: FFIFixtureStates) {
+    drop_array(fixture_states.fixture_values);
+}
+
 #[derive(Default)]
 #[repr(C)]
 pub struct FFIFixtureState {

crates/ui/ffi/src/apis/layout.rs

@@ -1,5 +1,5 @@
 use crate::apis::transport::Timecode;
-use crate::types::{drop_pointer, Array, FFIFromPointer};
+use crate::types::{drop_array, drop_pointer, Array, FFIFromPointer};
 use mizer_node::NodePath;
 use mizer_runtime::LayoutsView;
 use parking_lot::Mutex;
@@ -171,6 +171,11 @@ pub extern "C" fn drop_layout_pointer(ptr: *const LayoutRef) {
     drop_pointer(ptr);
 }
 
+#[no_mangle]
+pub extern "C" fn drop_step_sequencer_value(value: FFIStepSequencerValue) {
+    drop_array(value.value);
+}
+
 #[derive(Default)]
 #[repr(C)]
 pub struct FFIControlColor {

crates/ui/ffi/src/apis/mod.rs

@@ -1,3 +1,5 @@
+use crate::types::{drop_array, Array};
+
 pub mod connections;
 pub mod fixture;
 pub mod layout;
@@ -8,3 +10,13 @@ pub mod sequencer;
 pub mod status;
 pub mod timecode;
 pub mod transport;
+
+#[no_mangle]
+pub extern "C" fn drop_byte_array(array: Array<u8>) {
+    drop_array(array)
+}
+
+#[no_mangle]
+pub extern "C" fn drop_double_array(array: Array<f64>) {
+    drop_array(array)
+}

crates/ui/ffi/src/apis/node_history.rs

@@ -1,60 +1,46 @@
-use std::ffi::c_char;
+use std::ffi::{c_char, CString};
 use std::sync::Arc;
 
-use parking_lot::Mutex;
-
 use mizer_runtime::NodePreviewRef;
 use mizer_util::StructuredData;
 
 use crate::apis::programmer::FFIColorValue;
 use crate::apis::transport::Timecode;
-use crate::pointer_inventory::PointerInventory;
 use crate::types::{drop_pointer, Array, FFIFromPointer};
 
 pub struct NodeHistory {
     preview_ref: NodePreviewRef,
-    pointer_inventory: Mutex<PointerInventory>,
 }
 
 impl NodeHistory {
     pub fn new(preview_ref: NodePreviewRef) -> Self {
         Self {
             preview_ref,
-            pointer_inventory: Default::default(),
         }
     }
 
-    // TODO: This should probably drop old inventory after 2 generations
     fn convert(&self, data: StructuredData) -> FFIStructuredData {
-        let mut inventory = self.pointer_inventory.lock();
-        convert_with_inventory(data, &mut inventory)
-    }
-}
-
-fn convert_with_inventory(
-    data: StructuredData,
-    inventory: &mut PointerInventory,
-) -> FFIStructuredData {
-    match data {
-        StructuredData::Boolean(bool) => FFIStructuredData::boolean(bool),
-        StructuredData::Float(float) => FFIStructuredData::float(float),
-        StructuredData::Int(int) => FFIStructuredData::int(int),
-        StructuredData::Text(text) => FFIStructuredData::text(inventory.allocate_string(text)),
-        StructuredData::Array(array) => FFIStructuredData::array(
-            array
-                .into_iter()
-                .map(|item| convert_with_inventory(item, inventory))
-                .collect(),
-        ),
-        StructuredData::Object(map) => FFIStructuredData::object(
-            map.into_iter()
-                .map(|(key, value)| FFIStructuredDataObjectEntry {
-                    key: inventory.allocate_string(key),
-                    value: convert_with_inventory(value, inventory),
-                })
-                .collect(),
-        ),
-        StructuredData::Null => FFIStructuredData::null(),
+        match data {
+            StructuredData::Boolean(bool) => FFIStructuredData::boolean(bool),
+            StructuredData::Float(float) => FFIStructuredData::float(float),
+            StructuredData::Int(int) => FFIStructuredData::int(int),
+            StructuredData::Text(text) => FFIStructuredData::text(CString::new(text).unwrap_or_default().into_raw()),
+            StructuredData::Array(array) => FFIStructuredData::array(
+                array
+                    .into_iter()
+                    .map(|item| self.convert(item))
+                    .collect(),
+            ),
+            StructuredData::Object(map) => FFIStructuredData::object(
+                map.into_iter()
+                    .map(|(key, value)| FFIStructuredDataObjectEntry {
+                        key: CString::new(key).unwrap_or_default().into_raw(),
+                        value: self.convert(value),
+                    })
+                    .collect(),
+            ),
+            StructuredData::Null => FFIStructuredData::null(),
+        }
     }
 }
 
@@ -66,7 +52,7 @@ pub struct FFIStructuredData {
 }
 
 impl FFIStructuredData {
-    fn text(text: *const c_char) -> Self {
+    fn text(text: *mut c_char) -> Self {
         Self {
             r#type: FFIStructuredDataType::Text,
             value: FFIStructuredDataValue { text },
@@ -118,6 +104,30 @@ impl FFIStructuredData {
             value: FFIStructuredDataValue { null: () },
         }
     }
+
+    pub(crate) fn as_array(&self) -> Option<Array<FFIStructuredData>> {
+        if let FFIStructuredDataType::Array = self.r#type {
+            Some(unsafe { self.value.array })
+        } else {
+            None
+        }
+    }
+
+    pub(crate) fn as_object(&self) -> Option<Array<FFIStructuredDataObjectEntry>> {
+        if let FFIStructuredDataType::Object = self.r#type {
+            Some(unsafe { self.value.object })
+        } else {
+            None
+        }
+    }
+
+    pub(crate) fn as_text(&self) -> Option<CString> {
+        if let FFIStructuredDataType::Text = self.r#type {
+            Some(unsafe { CString::from_raw(self.value.text) })
+        } else {
+            None
+        }
+    }
 }
 
 #[derive(Clone, Copy)]
@@ -135,7 +145,7 @@ pub enum FFIStructuredDataType {
 #[derive(Clone, Copy)]
 #[repr(C)]
 pub union FFIStructuredDataValue {
-    pub text: *const c_char,
+    pub text: *mut c_char,
     pub floating_point: f64,
     pub integer: i64,
     pub boolean: u8,
@@ -147,7 +157,7 @@ pub union FFIStructuredDataValue {
 #[derive(Clone, Copy)]
 #[repr(C)]
 pub struct FFIStructuredDataObjectEntry {
-    pub key: *const c_char,
+    pub key: *mut c_char,
     pub value: FFIStructuredData,
 }
 
@@ -215,3 +225,22 @@ pub extern "C" fn read_node_multi_preview(ptr: *const NodeHistory) -> Array<f64>
 
     values.into()
 }
+
+#[no_mangle]
+pub extern "C" fn drop_structured_data(data: FFIStructuredData) {
+    if let Some(array) = data.as_array() {
+        let vec = array.into_vec();
+        for item in vec {
+            drop_structured_data(item);
+        }
+    }else if let Some(object) = data.as_object() {
+        let vec = object.into_vec();
+        for item in vec {
+            let key = unsafe { CString::from_raw(item.key) };
+            drop(key);
+            drop_structured_data(item.value);
+        }
+    }else if let Some(text) = data.as_text() {
+        drop(text)
+    }
+}

crates/ui/ffi/src/apis/nodes.rs

@@ -1,31 +1,26 @@
-use crate::pointer_inventory::PointerInventory;
 use crate::types::{drop_pointer, Array, FFIFromPointer};
 use mizer_runtime::NodeMetadataRef;
-use parking_lot::Mutex;
-use std::ffi::c_char;
+use std::ffi::{c_char, CString};
 use std::sync::Arc;
 
 pub struct NodesRef {
     metadata_ref: NodeMetadataRef,
-    pointer_inventory: Mutex<PointerInventory>,
 }
 
 impl NodesRef {
     pub fn new(metadata_ref: NodeMetadataRef) -> Self {
         Self {
             metadata_ref,
-            pointer_inventory: Default::default(),
         }
     }
 
     fn get_port_metadata<B: FromIterator<FFINodePortMetadata>>(&self) -> B {
-        let mut pointer_inventory = self.pointer_inventory.lock();
         let data = self.metadata_ref.get_all_port_metadata();
 
         data.into_iter()
             .map(|(path, port, metadata)| FFINodePortMetadata {
-                node_path: pointer_inventory.allocate_string(path.to_string()),
-                port_id: pointer_inventory.allocate_string(port.to_string()),
+                node_path: CString::new(path.to_string()).unwrap_or_default().into_raw(),
+                port_id: CString::new(port.to_string()).unwrap_or_default().into_raw(),
                 pushed_value: metadata.pushed_value.into(),
             })
             .collect()
@@ -48,9 +43,20 @@ pub extern "C" fn drop_nodes_pointer(ptr: *const NodesRef) {
     drop_pointer(ptr);
 }
 
+#[no_mangle]
+pub extern "C" fn drop_node_port_metadata(metadata: Array<FFINodePortMetadata>) {
+    let vec = metadata.into_vec();
+    for metadata in vec {
+        unsafe {
+            let _ = CString::from_raw(metadata.node_path);
+            let _ = CString::from_raw(metadata.port_id);
+        }
+    }
+}
+
 #[repr(C)]
 pub struct FFINodePortMetadata {
-    pub node_path: *const c_char,
-    pub port_id: *const c_char,
+    pub node_path: *mut c_char,
+    pub port_id: *mut c_char,
     pub pushed_value: u8,
 }