Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Oct 22, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3159de4
Fix memory leaks in CXmpError
scouten-adobe Oct 22, 2022
63c818c
Fix memory leaks in remaining APIs that return string values
scouten-adobe Oct 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions src/ffi.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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());
}
}
Expand All @@ -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;
}
}

Expand All @@ -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);
}

Expand Down Expand Up @@ -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) {
Expand Down
81 changes: 74 additions & 7 deletions src/ffi.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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 {
Expand All @@ -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 {
Expand All @@ -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;
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand All @@ -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 ---

Expand All @@ -232,6 +300,5 @@ extern "C" {
pub(crate) fn CXmpDateTimeToString(
dt: *const CXmpDateTime,
out_error: *mut CXmpError,
) -> *mut c_char;

) -> *const c_char;
}
36 changes: 5 additions & 31 deletions src/tests/xmp_error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand Down
25 changes: 14 additions & 11 deletions src/xmp_date_time.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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)
}
}
}
}
Expand Down
Loading