diff --git a/rust/pact_matching_ffi/src/models/message.rs b/rust/pact_matching_ffi/src/models/message.rs index b93d747ab..98f0b2d63 100644 --- a/rust/pact_matching_ffi/src/models/message.rs +++ b/rust/pact_matching_ffi/src/models/message.rs @@ -9,6 +9,7 @@ use crate::util::*; use crate::{as_mut, as_ref, cstr, ffi, safe_str}; use anyhow::{anyhow, Context}; use libc::{c_char, c_int, c_uint, EXIT_FAILURE, EXIT_SUCCESS}; +use std::ops::Drop; /*=============================================================================================== * # Re-Exports @@ -110,8 +111,7 @@ pub unsafe extern "C" fn message_get_description( params: [message], op: { let message = as_ref!(message); - let description = string::into_leaked_cstring(&message.description)?; - Ok(description) + Ok(string::to_c(&message.description)? as *const c_char) }, fail: { ptr::null_to::() @@ -222,15 +222,14 @@ pub unsafe extern "C" fn message_find_metadata( name: "message_find_metadata", params: [message, key], op: { + // Reconstitute the message. let message = as_ref!(message); + // Safely get a Rust String out of the key. let key = safe_str!(key); - - match message.metadata.get(key) { - None => Ok(ptr::null_to::()), - Some(value) => { - Ok(string::into_leaked_cstring(value)?) - }, - } + // Get the value, if present, for that key. + let value = message.metadata.get(key).ok_or(anyhow::anyhow!("invalid metadata key"))?; + // Leak the string to the C-side. + Ok(string::to_c(value)? as *const c_char) }, fail: { ptr::null_to::() @@ -276,14 +275,13 @@ pub unsafe extern "C" fn message_insert_metadata( } } -/* -/// Get a copy of the metadata list from this message. -/// It is in the form of a list of (key, value) pairs, -/// in an unspecified order. -/// The returned structure must be deleted with `metadata_list_delete`. +/// Get an iterator over the metadata of a message. /// -/// Since it is a copy, the returned structure may safely outlive -/// the `Message`. +/// This iterator carries a pointer to the message, and must +/// not outlive the message. +/// +/// The message metadata also must not be modified during iteration. If it is, +/// the old iterator must be deleted and a new iterator created. /// /// # Errors /// @@ -294,23 +292,165 @@ pub unsafe extern "C" fn message_insert_metadata( #[no_mangle] #[allow(clippy::missing_safety_doc)] #[allow(clippy::or_fun_call)] -pub unsafe extern "C" fn message_get_metadata_list( +pub unsafe extern "C" fn message_get_metadata_iter( message: *mut Message, ) -> *mut MetadataIterator { ffi! { - name: "message_get_metadata_list", + name: "message_get_metadata_iter", params: [message], op: { let message = as_mut!(message); - todo!() + let iter = MetadataIterator { + keys: message.metadata.keys().cloned().collect(), + current: 0, + message: message as *const Message, + }; + + Ok(ptr::raw_to(iter)) + }, + fail: { + ptr::null_mut_to::() + } + } +} + +/// Get the next key and value out of the iterator, if possible +/// +/// Returns a pointer to a heap allocated array of 2 elements, the pointer to the +/// key string on the heap, and the pointer to the value string on the heap. +/// +/// The user needs to free both the contained strings and the array. +#[no_mangle] +#[allow(clippy::missing_safety_doc)] +#[allow(clippy::or_fun_call)] +pub unsafe extern "C" fn metadata_iter_next( + iter: *mut MetadataIterator, +) -> *mut MetadataPair { + ffi! { + name: "metadata_iter_next", + params: [iter], + op: { + // Reconstitute the iterator. + let iter = as_mut!(iter); + + // Reconstitute the message. + let message = as_ref!(iter.message); + + // Get the current key from the iterator. + let key = iter.next().ok_or(anyhow::anyhow!("iter past the end of metadata"))?; + + // Get the value for the current key. + let (key, value) = message.metadata.get_key_value(key).ok_or(anyhow::anyhow!("iter provided invalid metadata key"))?; + + // Package up for return. + let pair = MetadataPair::new(key, value)?; + + // Leak the value out to the C-side. + Ok(ptr::raw_to(pair)) }, fail: { - ptr::null_to::() + ptr::null_mut_to::() } } } -*/ + +/// Free the metadata iterator when you're done using it. +#[no_mangle] +#[allow(clippy::missing_safety_doc)] +pub unsafe extern "C" fn metadata_iter_delete( + iter: *mut MetadataIterator, +) -> c_int { + ffi! { + name: "metadata_iter_delete", + params: [iter], + op: { + ptr::drop_raw(iter); + Ok(EXIT_SUCCESS) + }, + fail: { + EXIT_FAILURE + } + } +} + +/// Free a pair of key and value returned from `message_next_metadata_iter`. +#[no_mangle] +#[allow(clippy::missing_safety_doc)] +pub unsafe extern "C" fn metadata_pair_delete( + pair: *mut MetadataPair, +) -> c_int { + ffi! { + name: "metadata_pair_delete", + params: [pair], + op: { + ptr::drop_raw(pair); + Ok(EXIT_SUCCESS) + }, + fail: { + EXIT_FAILURE + } + } +} + +/// An iterator that enables FFI iteration over metadata by putting all the keys on the heap +/// and tracking which one we're currently at. +/// +/// This assumes no mutation of the underlying metadata happens while the iterator is live. +#[derive(Debug)] +pub struct MetadataIterator { + /// The metadata keys + keys: Vec, + /// The current key + current: usize, + /// Pointer to the message. + message: *const Message, +} + +impl MetadataIterator { + fn next(&mut self) -> Option<&String> { + let idx = self.current; + self.current += 1; + self.keys.get(idx) + } +} + +/// A single key-value pair exported to the C-side. +#[derive(Debug)] +#[repr(C)] +#[allow(missing_copy_implementations)] +pub struct MetadataPair { + key: *const c_char, + value: *const c_char, +} + +impl MetadataPair { + fn new(key: &str, value: &str) -> anyhow::Result { + Ok(MetadataPair { + key: string::to_c(key)? as *const c_char, + value: string::to_c(value)? as *const c_char, + }) + } +} + +// Ensure that the owned strings are freed when the pair is dropped. +// +// Notice that we're casting from a `*const c_char` to a `*mut c_char`. +// This may seem wrong, but is safe so long as it doesn't violate Rust's +// guarantees around immutable references, which this doesn't. In this case, +// the underlying data came from `CString::into_raw` which takes ownership +// of the `CString` and hands it off via a `*mut pointer`. We cast that pointer +// back to `*const` to limit the C-side from doing any shenanigans, since the +// pointed-to values live inside of the `Message` metadata `HashMap`, but +// cast back to `*mut` here so we can free the memory. +// +// The discussion here helps explain: https://github.com/rust-lang/rust-clippy/issues/4774 +impl Drop for MetadataPair { + fn drop(&mut self) { + string::string_delete(self.key as *mut c_char); + string::string_delete(self.value as *mut c_char); + } +} /*=============================================================================================== * # Status Types diff --git a/rust/pact_matching_ffi/src/util/ptr.rs b/rust/pact_matching_ffi/src/util/ptr.rs index b02851271..a850b2e6b 100644 --- a/rust/pact_matching_ffi/src/util/ptr.rs +++ b/rust/pact_matching_ffi/src/util/ptr.rs @@ -36,7 +36,7 @@ pub(crate) fn null_mut_to() -> *mut T { /// Get an immutable reference from a raw pointer #[macro_export] macro_rules! as_ref { - ( $name:ident ) => {{ + ( $name:expr ) => {{ $name .as_ref() .ok_or(anyhow!(concat!(stringify!($name), " is null")))? @@ -46,7 +46,7 @@ macro_rules! as_ref { /// Get a mutable reference from a raw pointer #[macro_export] macro_rules! as_mut { - ( $name:ident ) => {{ + ( $name:expr ) => {{ $name .as_mut() .ok_or(anyhow!(concat!(stringify!($name), " is null")))? diff --git a/rust/pact_matching_ffi/src/util/string.rs b/rust/pact_matching_ffi/src/util/string.rs index a61fc93e7..dfc18b0fe 100644 --- a/rust/pact_matching_ffi/src/util/string.rs +++ b/rust/pact_matching_ffi/src/util/string.rs @@ -1,6 +1,5 @@ use libc::c_char; use std::ffi::CString; -use std::mem; /// Converts the string into a C-compatible null terminated string, /// then forgets the container while returning a pointer to the @@ -8,17 +7,8 @@ use std::mem; /// /// The returned pointer must be passed to CString::from_raw to /// prevent leaking memory. -pub(crate) fn into_leaked_cstring( - t: &str, -) -> anyhow::Result<*const c_char> { - let copy = CString::new(t)?; - let ptr = copy.as_ptr(); - - // Intentionally leak this memory so that it stays - // valid while C is using it. - mem::forget(copy); - - Ok(ptr) +pub(crate) fn to_c(t: &str) -> anyhow::Result<*mut c_char> { + Ok(CString::new(t)?.into_raw()) } /// Delete a string previously returned by this FFI.