diff --git a/src/ffi.cpp b/src/ffi.cpp index 8ec52d6..8bec03b 100644 --- a/src/ffi.cpp +++ b/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) { diff --git a/src/ffi.rs b/src/ffi.rs index 6f584e0..ff938fa 100644 --- a/src/ffi.rs +++ b/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(&self, f: F) -> Option + 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,7 +158,7 @@ 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, @@ -98,7 +166,7 @@ extern "C" { 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; } diff --git a/src/tests/xmp_error.rs b/src/tests/xmp_error.rs index 319077d..7dc3f67 100644 --- a/src/tests/xmp_error.rs +++ b/src/tests/xmp_error.rs @@ -12,17 +12,11 @@ // 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()); @@ -30,11 +24,7 @@ mod raise_from_c { #[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); diff --git a/src/xmp_date_time.rs b/src/xmp_date_time.rs index 80dc71c..db0d170 100644 --- a/src/xmp_date_time.rs +++ b/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) + } } } } diff --git a/src/xmp_meta.rs b/src/xmp_meta.rs index 0950199..07c8564 100644 --- a/src/xmp_meta.rs +++ b/src/xmp_meta.rs @@ -11,14 +11,11 @@ // specific language governing permissions and limitations under // each license. -use std::{ - ffi::{CStr, CString}, - path::Path, - str::FromStr, -}; +use std::{ffi::CString, path::Path, str::FromStr}; use crate::{ - ffi, OpenFileOptions, XmpDateTime, XmpError, XmpErrorType, XmpFile, XmpResult, XmpValue, + ffi::{self, CXmpString}, + OpenFileOptions, XmpDateTime, XmpError, XmpErrorType, XmpFile, XmpResult, XmpValue, }; /// An `XmpMeta` struct allows you to inspect and modify the data model @@ -114,11 +111,15 @@ impl XmpMeta { unsafe { let mut err = ffi::CXmpError::default(); - let c_result = ffi::CXmpMetaRegisterNamespace(&mut err, c_ns.as_ptr(), c_sp.as_ptr()); + let result = CXmpString::from_ptr(ffi::CXmpMetaRegisterNamespace( + &mut err, + c_ns.as_ptr(), + c_sp.as_ptr(), + )); XmpError::raise_from_c(&err)?; - Ok(CStr::from_ptr(c_result).to_string_lossy().into_owned()) + Ok(result.as_string()) } } @@ -160,22 +161,14 @@ impl XmpMeta { let mut err = ffi::CXmpError::default(); unsafe { - let c_result = ffi::CXmpMetaGetProperty( + CXmpString::from_ptr(ffi::CXmpMetaGetProperty( self.m, &mut err, c_ns.as_ptr(), c_name.as_ptr(), &mut options, - ); - - if c_result.is_null() { - None - } else { - Some(XmpValue { - value: CStr::from_ptr(c_result).to_string_lossy().into_owned(), - options, - }) - } + )) + .map(|value| XmpValue { value, options }) } } @@ -190,7 +183,7 @@ impl XmpMeta { meta: self, ns: CString::new(namespace).unwrap_or_default(), name: CString::new(path).unwrap_or_default(), - index: 1, + index: 0, } } @@ -687,7 +680,7 @@ impl XmpMeta { unsafe { let mut c_actual_lang: *const i8 = std::ptr::null_mut(); - let c_result = ffi::CXmpMetaGetLocalizedText( + CXmpString::from_ptr(ffi::CXmpMetaGetLocalizedText( self.m, &mut err, c_ns.as_ptr(), @@ -699,19 +692,13 @@ impl XmpMeta { c_specific_lang.as_ptr(), &mut c_actual_lang, &mut options, - ); - - if c_result.is_null() { - None - } else { - Some(( - XmpValue { - value: CStr::from_ptr(c_result).to_string_lossy().into_owned(), - options, - }, - CStr::from_ptr(c_actual_lang).to_string_lossy().into_owned(), - )) - } + )) + .map(|value| { + ( + XmpValue { value, options }, + CXmpString::from_ptr(c_actual_lang).as_string(), + ) + }) } } } @@ -755,25 +742,17 @@ impl<'a> Iterator for ArrayProperty<'a> { let mut options: u32 = 0; let mut err = ffi::CXmpError::default(); - let c_result = ffi::CXmpMetaGetArrayItem( + self.index += 1; + + CXmpString::from_ptr(ffi::CXmpMetaGetArrayItem( self.meta.m, &mut err, self.ns.as_ptr(), self.name.as_ptr(), self.index, &mut options, - ); - - self.index += 1; - - if c_result.is_null() { - None - } else { - Some(XmpValue { - value: CStr::from_ptr(c_result).to_string_lossy().into_owned(), - options, - }) - } + )) + .map(|value| XmpValue { value, options }) } } }