Fix memory managment around the FFI-barrier (#1174)

commit-id:e557eb16

---

**Stack**:
- #1166
- #1165
- #1161
- #1159
- #1174 ⬅


⚠️ *Part of a stack created by [spr](https://github.com/ejoffe/spr). Do
not merge manually using the UI - doing so may have unexpected results.*

plugins/cairo-lang-macro-attributes/src/lib.rs

@@ -17,10 +17,20 @@ pub fn attribute_macro(_args: TokenStream, input: TokenStream) -> TokenStream {
 
         #[no_mangle]
         pub unsafe extern "C" fn expand(token_stream: cairo_lang_macro_stable::StableTokenStream) -> cairo_lang_macro_stable::StableProcMacroResult {
-            let token_stream = TokenStream::from_stable(token_stream);
+            let token_stream = cairo_lang_macro::TokenStream::from_stable(token_stream);
             let result = #item_name(token_stream);
             result.into_stable()
         }
     };
     TokenStream::from(expanded)
 }
+
+#[proc_macro]
+pub fn macro_commons(_input: TokenStream) -> TokenStream {
+    TokenStream::from(quote! {
+        #[no_mangle]
+        pub unsafe extern "C" fn free_result(result: cairo_lang_macro_stable::StableProcMacroResult) {
+            cairo_lang_macro::ProcMacroResult::from_owned_stable(result);
+        }
+    })
+}

plugins/cairo-lang-macro-stable/src/lib.rs

@@ -1,15 +1,15 @@
-use std::ffi::CString;
+use std::ffi::{CStr, CString};
 use std::os::raw::c_char;
 
 /// Token stream.
 ///
 /// This struct implements FFI-safe stable ABI.
 #[repr(C)]
-#[derive(Debug)]
+#[derive(Debug, Clone)]
 pub struct StableTokenStream(*mut c_char);
 
 #[repr(C)]
-#[derive(Debug)]
+#[derive(Debug, Clone)]
 pub enum StableAuxData {
     None,
     Some(*mut c_char),
@@ -19,7 +19,7 @@ pub enum StableAuxData {
 ///
 /// This struct implements FFI-safe stable ABI.
 #[repr(C)]
-#[derive(Debug)]
+#[derive(Debug, Clone)]
 pub enum StableProcMacroResult {
     /// Plugin has not taken any action.
     Leave,
@@ -41,7 +41,13 @@ impl StableTokenStream {
     ///
     /// # Safety
     pub unsafe fn to_string(&self) -> String {
-        raw_to_string(self.0)
+        // Note that this does not deallocate the c-string.
+        // The memory must still be freed with `CString::from_raw`.
+        CStr::from_ptr(self.0).to_string_lossy().to_string()
+    }
+
+    pub fn into_owned_string(self) -> String {
+        unsafe { raw_to_string(self.0) }
     }
 }
 

plugins/cairo-lang-macro/src/lib.rs

@@ -1,4 +1,4 @@
-use std::ffi::{c_char, CString};
+use std::ffi::{c_char, CStr, CString};
 use std::fmt::Display;
 
 pub use cairo_lang_macro_attributes::*;
@@ -72,7 +72,9 @@ impl ProcMacroResult {
         }
     }
 
-    /// Convert to native Rust representation.
+    /// Convert to native Rust representation, without taking the ownership of the string.
+    ///
+    /// Note that you still need to free the memory by calling `from_owned_stable`.
     ///
     /// # Safety
     #[doc(hidden)]
@@ -85,7 +87,28 @@ impl ProcMacroResult {
                 aux_data,
             } => ProcMacroResult::Replace {
                 token_stream: TokenStream::from_stable(token_stream),
-                aux_data: AuxData::from_stable(aux_data).unwrap(),
+                aux_data: AuxData::from_stable(aux_data),
+            },
+        }
+    }
+
+    /// Convert to native Rust representation, with taking the ownership of the string.
+    ///
+    /// Useful when you need to free the allocated memory.
+    /// Only use on the same side of FFI-barrier, where the memory has been allocated.
+    ///
+    /// # Safety
+    #[doc(hidden)]
+    pub unsafe fn from_owned_stable(result: StableProcMacroResult) -> Self {
+        match result {
+            StableProcMacroResult::Leave => ProcMacroResult::Leave,
+            StableProcMacroResult::Remove => ProcMacroResult::Remove,
+            StableProcMacroResult::Replace {
+                token_stream,
+                aux_data,
+            } => ProcMacroResult::Replace {
+                token_stream: TokenStream::from_owned_stable(token_stream),
+                aux_data: AuxData::from_owned_stable(aux_data),
             },
         }
     }
@@ -101,13 +124,26 @@ impl TokenStream {
         StableTokenStream::new(cstr.into_raw())
     }
 
-    /// Convert to native Rust representation.
+    /// Convert to native Rust representation, without taking the ownership of the string.
+    ///
+    /// Note that you still need to free the memory by calling `from_owned_stable`.
     ///
     /// # Safety
     #[doc(hidden)]
     pub unsafe fn from_stable(token_stream: StableTokenStream) -> Self {
         Self::new(token_stream.to_string())
     }
+
+    /// Convert to native Rust representation, with taking the ownership of the string.
+    ///
+    /// Useful when you need to free the allocated memory.
+    /// Only use on the same side of FFI-barrier, where the memory has been allocated.
+    ///
+    /// # Safety
+    #[doc(hidden)]
+    pub unsafe fn from_owned_stable(token_stream: StableTokenStream) -> Self {
+        Self::new(token_stream.into_owned_string())
+    }
 }
 
 impl AuxData {
@@ -131,23 +167,52 @@ impl AuxData {
         StableAuxData::Some(cstr.into_raw())
     }
 
-    /// Convert to native Rust representation.
+    /// Convert to native Rust representation, without taking the ownership of the string.
+    ///
+    /// Note that you still need to free the memory by calling `from_owned_stable`.
     ///
     /// # Safety
     #[doc(hidden)]
-    pub unsafe fn from_stable(aux_data: StableAuxData) -> Result<Option<Self>, serde_json::Error> {
+    pub unsafe fn from_stable(aux_data: StableAuxData) -> Option<Self> {
         match aux_data {
-            StableAuxData::None => Ok(None),
-            StableAuxData::Some(raw) => Some(Self::try_new(raw_to_string(raw))).transpose(),
+            StableAuxData::None => None,
+            StableAuxData::Some(raw) => Some(Self::new(from_raw_cstr(raw))),
+        }
+    }
+
+    /// Convert to native Rust representation, with taking the ownership of the string.
+    ///
+    /// Useful when you need to free the allocated memory.
+    /// Only use on the same side of FFI-barrier, where the memory has been allocated.
+    ///
+    /// # Safety
+    #[doc(hidden)]
+    pub unsafe fn from_owned_stable(aux_data: StableAuxData) -> Option<Self> {
+        match aux_data {
+            StableAuxData::None => None,
+            StableAuxData::Some(raw) => Some(Self::new(from_raw_cstring(raw))),
         }
     }
 }
 
-unsafe fn raw_to_string(raw: *mut c_char) -> String {
+// Create a string from a raw pointer to a c_char.
+// Note that this will free the underlying memory.
+unsafe fn from_raw_cstring(raw: *mut c_char) -> String {
     if raw.is_null() {
         String::default()
     } else {
         let cstr = CString::from_raw(raw);
         cstr.to_string_lossy().to_string()
     }
 }
+
+// Note that this will not free the underlying memory.
+// You still need to free the memory by calling `CString::from_raw`.
+unsafe fn from_raw_cstr(raw: *mut c_char) -> String {
+    if raw.is_null() {
+        String::default()
+    } else {
+        let cstr = CStr::from_ptr(raw);
+        cstr.to_string_lossy().to_string()
+    }
+}

scarb/src/compiler/plugin/proc_macro/ffi.rs

@@ -65,17 +65,40 @@ impl ProcMacroInstance {
     }
 
     /// Apply expansion to token stream.
+    ///
+    /// This function implements the actual calls to functions from the dynamic library.
+    ///
+    /// All values passing the FFI-barrier must implement a stable ABI.
+    ///
+    /// Please be aware that the memory management of values passing the FFI-barrier is tricky.
+    /// The memory must be freed on the same side of the barrier, where the allocation was made.
     pub(crate) fn generate_code(&self, token_stream: TokenStream) -> ProcMacroResult {
-        let ffi_token_stream = token_stream.into_stable();
-        let result = (self.plugin.vtable.expand)(ffi_token_stream);
-        unsafe { ProcMacroResult::from_stable(result) }
+        // This must be manually freed with call to from_owned_stable.
+        let stable_token_stream = token_stream.into_stable();
+        // Call FFI interface for code expansion.
+        // Note that `stable_result` has been allocated by the dynamic library.
+        let stable_result = (self.plugin.vtable.expand)(stable_token_stream.clone());
+        // Free the memory allocated by the `stable_token_stream`.
+        // This will call `CString::from_raw` under the hood, to take ownership.
+        unsafe {
+            TokenStream::from_owned_stable(stable_token_stream);
+        };
+        // Create Rust representation of the result.
+        // Note, that the memory still needs to be freed on the allocator side!
+        let result = unsafe { ProcMacroResult::from_stable(stable_result.clone()) };
+        // Call FFI interface to free the `stable_result` that has been allocated by previous call.
+        (self.plugin.vtable.free_result)(stable_result);
+        // Return obtained result.
+        result
     }
 }
 
 type ExpandCode = extern "C" fn(StableTokenStream) -> StableProcMacroResult;
+type FreeResult = extern "C" fn(StableProcMacroResult);
 
 struct VTableV0 {
     expand: RawSymbol<ExpandCode>,
+    free_result: RawSymbol<FreeResult>,
 }
 
 impl VTableV0 {
@@ -84,7 +107,14 @@ impl VTableV0 {
             .get(b"expand\0")
             .context("failed to load expand function for procedural macro")?;
         let expand = expand.into_raw();
-        Ok(VTableV0 { expand })
+        let free_result: Symbol<'_, FreeResult> = library
+            .get(b"free_result\0")
+            .context("failed to load free_result function for procedural macro")?;
+        let free_result = free_result.into_raw();
+        Ok(VTableV0 {
+            expand,
+            free_result,
+        })
     }
 }
 

scarb/tests/build_cairo_plugin.rs

@@ -69,8 +69,10 @@ fn simple_project(t: &impl PathChild) {
                 .manifest_extra(r#"[cairo-plugin]"#)
         })
         .lib_rs(indoc! {r#"
-        use cairo_lang_macro::{ProcMacroResult, TokenStream, attribute_macro};
+        use cairo_lang_macro::{ProcMacroResult, TokenStream, attribute_macro, macro_commons};
 
+        macro_commons!();
+        
         #[attribute_macro]
         pub fn some_macro(token_stream: TokenStream) -> ProcMacroResult {
             let _code = token_stream.to_string();
@@ -110,8 +112,9 @@ fn compile_cairo_plugin() {
         .unwrap();
     assert!(
         output.status.success(),
-        "{}",
-        String::from_utf8_lossy(&output.stderr)
+        "stdout={}\n stderr={}",
+        String::from_utf8_lossy(&output.stdout),
+        String::from_utf8_lossy(&output.stderr),
     );
     let stdout = String::from_utf8_lossy(&output.stdout).to_string();
     assert!(stdout.contains("Compiling hello v1.0.0"));