Fix memory leaks when returning strings from C++ to Rust (#104)

src/ffi.cpp

@@ -65,12 +65,14 @@ extern "C" {
         const char* debugMessage;
 
         CXmpError() {
+            debugMessage = NULL;
             reset();
         }
 
         void reset() {
             hadError = 0;
             id = 0;
+            free((void*) debugMessage);
             debugMessage = NULL;
         }
     } CXmpError;
@@ -80,6 +82,7 @@ static void copyErrorForResult(XMP_Error& e, CXmpError* outError) {
     if (outError) {
         outError->hadError = 1;
         outError->id = e.GetID();
+        free((void*) outError->debugMessage);
         outError->debugMessage = copyStringForResult(e.GetErrMsg());
     }
 }
@@ -88,6 +91,8 @@ static void signalUnknownError(CXmpError* outError) {
     if (outError) {
         outError->hadError = 1;
         outError->id = kXMPErr_Unknown;
+        free((void*) outError->debugMessage);
+        outError->debugMessage = NULL;
     }
 }
 
@@ -100,6 +105,7 @@ static bool xmpFileErrorCallback(void* context,
     if (err) {
         err->hadError = 1;
         err->id = cause;
+        free((void*) err->debugMessage);
         err->debugMessage = copyStringForResult(message);
     }
 
@@ -132,6 +138,19 @@ extern "C" {
         #endif
     } CXmpMeta;
 
+    const char* CXmpStringCopy(const char* str) {
+        // This function should be used *only* to test FFI behavior.
+        // It copies a Rust-originated string so that it can subsequently
+        // be used for CXmpStringDrop.
+        return copyStringForResult(str);
+    }
+
+    void CXmpStringDrop(void* str) {
+        // This function must be called from Rust FFI code for every
+        // const char* sent over to Rust (i.e. generated via copyStringForResult).
+        free(str);
+    }
+
     // --- CXmpFile ---
 
     CXmpFile* CXmpFileNew(CXmpError* outError) {

src/ffi.rs

@@ -11,7 +11,42 @@
 // specific language governing permissions and limitations under
 // each license.
 
-use std::os::raw::{c_char, c_int};
+use std::{
+    ffi::{CStr, CString},
+    os::raw::{c_char, c_int},
+};
+
+pub(crate) struct CXmpString {
+    pub(crate) s: *const c_char,
+}
+
+impl CXmpString {
+    pub(crate) fn from_ptr(s: *const c_char) -> Self {
+        Self { s }
+    }
+
+    pub(crate) fn as_string(&self) -> String {
+        unsafe { CStr::from_ptr(self.s).to_string_lossy().into_owned() }
+    }
+
+    pub(crate) fn map<U, F>(&self, f: F) -> Option<U>
+    where
+        F: FnOnce(String) -> U,
+    {
+        if self.s.is_null() {
+            None
+        } else {
+            let s = self.as_string();
+            Some(f(s))
+        }
+    }
+}
+
+impl Drop for CXmpString {
+    fn drop(&mut self) {
+        unsafe { CXmpStringDrop(self.s) };
+    }
+}
 
 #[repr(C)]
 pub(crate) struct CXmpError {
@@ -20,6 +55,29 @@ pub(crate) struct CXmpError {
     pub(crate) debug_message: *const c_char,
 }
 
+impl CXmpError {
+    #[allow(dead_code, clippy::unwrap_used)] // only used in test code
+    pub(crate) fn new(had_error: bool, id: i32, debug_message: Option<&str>) -> Self {
+        // Mimic a debug message coming from C++ code
+        // so that we don't foul up our memory management
+        // when this struct is dropped.
+
+        Self {
+            had_error: if had_error { 1 } else { 0 },
+            id,
+            debug_message: unsafe {
+                match debug_message {
+                    Some(debug_message) => {
+                        let debug_message_as_cstr = CString::new(debug_message).unwrap();
+                        CXmpStringCopy(debug_message_as_cstr.as_ptr())
+                    }
+                    None => std::ptr::null(),
+                }
+            },
+        }
+    }
+}
+
 impl Default for CXmpError {
     fn default() -> Self {
         Self {
@@ -30,6 +88,13 @@ impl Default for CXmpError {
     }
 }
 
+impl Drop for CXmpError {
+    fn drop(&mut self) {
+        unsafe { CXmpStringDrop(self.debug_message) };
+        self.debug_message = std::ptr::null();
+    }
+}
+
 #[derive(Debug, Default, Eq, PartialEq)]
 #[repr(C)]
 pub(crate) struct CXmpDateTime {
@@ -52,6 +117,9 @@ pub(crate) enum CXmpFile {}
 pub(crate) enum CXmpMeta {}
 
 extern "C" {
+    pub(crate) fn CXmpStringCopy(s: *const c_char) -> *const c_char;
+    pub(crate) fn CXmpStringDrop(s: *const c_char);
+
     // --- CXmpFile ---
 
     pub(crate) fn CXmpFileNew(out_error: *mut CXmpError) -> *mut CXmpFile;
@@ -90,15 +158,15 @@ extern "C" {
         out_error: *mut CXmpError,
         namespace_uri: *const c_char,
         suggested_prefix: *const c_char,
-    ) -> *mut c_char;
+    ) -> *const c_char;
 
     pub(crate) fn CXmpMetaGetProperty(
         meta: *mut CXmpMeta,
         out_error: *mut CXmpError,
         schema_ns: *const c_char,
         prop_name: *const c_char,
         out_options: *mut u32,
-    ) -> *mut c_char;
+    ) -> *const c_char;
 
     pub(crate) fn CXmpMetaGetProperty_Bool(
         meta: *mut CXmpMeta,
@@ -212,7 +280,7 @@ extern "C" {
         prop_name: *const c_char,
         index: u32,
         out_options: *mut u32,
-    ) -> *mut c_char;
+    ) -> *const c_char;
 
     pub(crate) fn CXmpMetaGetLocalizedText(
         meta: *mut CXmpMeta,
@@ -223,7 +291,7 @@ extern "C" {
         specific_lang: *const c_char,
         out_actual_lang: *mut *const c_char,
         out_options: *mut u32,
-    ) -> *mut c_char;
+    ) -> *const c_char;
 
     // --- CXmpDateTime ---
 
@@ -232,6 +300,5 @@ extern "C" {
     pub(crate) fn CXmpDateTimeToString(
         dt: *const CXmpDateTime,
         out_error: *mut CXmpError,
-    ) -> *mut c_char;
-
+    ) -> *const c_char;
 }

src/tests/xmp_error.rs

@@ -12,29 +12,19 @@
 // each license.
 
 mod raise_from_c {
-    use std::{ffi::CString, ptr};
-
     use crate::{ffi::CXmpError, XmpError, XmpErrorType};
 
     #[test]
     fn no_error() {
-        let c = CXmpError {
-            had_error: 0,
-            id: 0, // See kXMPErr_* constants in XMPConst.h.
-            debug_message: ptr::null(),
-        };
+        let c = CXmpError::new(false, 0, None);
 
         let result = XmpError::raise_from_c(&c);
         assert!(result.is_ok());
     }
 
     #[test]
     fn unknown_error_without_debug_message() {
-        let c = CXmpError {
-            had_error: 1,
-            id: 0,
-            debug_message: ptr::null(),
-        };
+        let c = CXmpError::new(true, 0, None);
 
         let err = XmpError::raise_from_c(&c).unwrap_err();
         assert_eq!(err.error_type, XmpErrorType::Unknown);
@@ -43,13 +33,7 @@ mod raise_from_c {
 
     #[test]
     fn unknown_error_with_debug_message() {
-        let msg = CString::new("sample message").unwrap();
-
-        let c = CXmpError {
-            had_error: 1,
-            id: 0,
-            debug_message: msg.as_ptr(),
-        };
+        let c = CXmpError::new(true, 0, Some("sample message"));
 
         let err = XmpError::raise_from_c(&c).unwrap_err();
         assert_eq!(err.error_type, XmpErrorType::Unknown);
@@ -58,11 +42,7 @@ mod raise_from_c {
 
     #[test]
     fn user_abort_error() {
-        let c = CXmpError {
-            had_error: 1,
-            id: 12,
-            debug_message: ptr::null(),
-        };
+        let c = CXmpError::new(true, 12, None);
 
         let err = XmpError::raise_from_c(&c).unwrap_err();
         assert_eq!(err.error_type, XmpErrorType::UserAbort);
@@ -71,13 +51,7 @@ mod raise_from_c {
 
     #[test]
     fn bad_id() {
-        let msg = CString::new("bogus XMP error").unwrap();
-
-        let c = CXmpError {
-            had_error: 1,
-            id: 9000,
-            debug_message: msg.as_ptr(),
-        };
+        let c = CXmpError::new(true, 9000, Some("bogus XMP error"));
 
         let err = XmpError::raise_from_c(&c).unwrap_err();
         assert_eq!(err.error_type, XmpErrorType::Unknown);

src/xmp_date_time.rs

@@ -13,7 +13,10 @@
 
 use std::fmt;
 
-use crate::{ffi, XmpError, XmpResult};
+use crate::{
+    ffi::{self, CXmpString},
+    XmpError, XmpResult,
+};
 
 /// The `XmpDateTime` struct allows easy conversion between ISO8601 format
 /// (the "native" representation of dates and times in XMP) and other formats.
@@ -205,16 +208,16 @@ impl fmt::Display for XmpDateTime {
         let mut err = ffi::CXmpError::default();
 
         unsafe {
-            let c_result = ffi::CXmpDateTimeToString(&self.as_ffi(), &mut err);
-
-            if c_result.is_null() {
-                let err = XmpError::raise_from_c(&err);
-                write!(f, "(unable to format date: {:#?})", err)
-            } else {
-                let dt_as_str = std::ffi::CStr::from_ptr(c_result)
-                    .to_string_lossy()
-                    .into_owned();
-                write!(f, "{}", dt_as_str)
+            match CXmpString::from_ptr(ffi::CXmpDateTimeToString(&self.as_ffi(), &mut err))
+                .map(|s| s)
+            {
+                Some(s) => {
+                    write!(f, "{}", s)
+                }
+                None => {
+                    let err = XmpError::raise_from_c(&err);
+                    write!(f, "(unable to format date: {:#?})", err)
+                }
             }
         }
     }