From 696cfd36eb8e7d9815f48d6591385247f27b6419 Mon Sep 17 00:00:00 2001 From: Andrew Lilley Brinker Date: Thu, 30 Jul 2020 13:38:19 -0700 Subject: [PATCH 1/4] Added metadata key/value iteration --- rust/pact_matching_ffi/src/models/message.rs | 141 +++++++++++++++++-- rust/pact_matching_ffi/src/util/ptr.rs | 4 +- 2 files changed, 130 insertions(+), 15 deletions(-) diff --git a/rust/pact_matching_ffi/src/models/message.rs b/rust/pact_matching_ffi/src/models/message.rs index b93d747ab..baa7a9575 100644 --- a/rust/pact_matching_ffi/src/models/message.rs +++ b/rust/pact_matching_ffi/src/models/message.rs @@ -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::into_leaked_cstring(value)?) }, fail: { ptr::null_to::() @@ -276,7 +275,6 @@ 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. @@ -294,23 +292,140 @@ 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] +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_mut_to::() + } + } +} + +/// Free the metadata iterator when you're done using it. +#[no_mangle] +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] +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: { - ptr::null_to::() + 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) -> Result { + Ok(MetadataPair { + key: string::into_leaked_cstring(key)?, + value: string::into_leaked_cstring(value)? + }) + } +} /*=============================================================================================== * # 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")))? From cb895a6ddd65e59bee7ab0756b844ffc5bfd70a8 Mon Sep 17 00:00:00 2001 From: Andrew Lilley Brinker Date: Thu, 30 Jul 2020 13:57:59 -0700 Subject: [PATCH 2/4] Correct bad documentation --- rust/pact_matching_ffi/src/models/message.rs | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/rust/pact_matching_ffi/src/models/message.rs b/rust/pact_matching_ffi/src/models/message.rs index baa7a9575..fcac33435 100644 --- a/rust/pact_matching_ffi/src/models/message.rs +++ b/rust/pact_matching_ffi/src/models/message.rs @@ -275,13 +275,14 @@ 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 /// From 47d97bf0eaa9e6b8d82b63ea619054d80c139729 Mon Sep 17 00:00:00 2001 From: Andrew Lilley Brinker Date: Mon, 3 Aug 2020 10:57:37 -0700 Subject: [PATCH 3/4] Corrected safety of strings passed to C. This commit corrects the leaking of strings to C, to use into_raw() to make sure ownership is transferred. It also adds a `Drop` implementation for the `MetadataPair` type to ensure the inner strings are freed. --- rust/pact_matching_ffi/src/models/message.rs | 35 +++++++++++++++----- rust/pact_matching_ffi/src/util/string.rs | 12 ++----- 2 files changed, 28 insertions(+), 19 deletions(-) diff --git a/rust/pact_matching_ffi/src/models/message.rs b/rust/pact_matching_ffi/src/models/message.rs index fcac33435..185b9dc5d 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 @@ -104,7 +105,7 @@ pub unsafe extern "C" fn message_delete(message: *mut Message) -> c_int { #[allow(clippy::or_fun_call)] pub unsafe extern "C" fn message_get_description( message: *const Message, -) -> *const c_char { +) -> *mut c_char { ffi! { name: "message_get_description", params: [message], @@ -114,7 +115,7 @@ pub unsafe extern "C" fn message_get_description( Ok(description) }, fail: { - ptr::null_to::() + ptr::null_mut_to::() } } } @@ -232,7 +233,7 @@ pub unsafe extern "C" fn message_find_metadata( Ok(string::into_leaked_cstring(value)?) }, fail: { - ptr::null_to::() + ptr::null_mut_to::() } } } @@ -275,7 +276,6 @@ pub unsafe extern "C" fn message_insert_metadata( } } - /// Get an iterator over the metadata of a message. /// /// This iterator carries a pointer to the message, and must @@ -323,6 +323,8 @@ pub unsafe extern "C" fn message_get_metadata_iter( /// /// 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 { @@ -356,6 +358,7 @@ pub unsafe extern "C" fn metadata_iter_next( /// 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 { @@ -374,7 +377,10 @@ pub unsafe extern "C" fn metadata_iter_delete( /// Free a pair of key and value returned from `message_next_metadata_iter`. #[no_mangle] -pub unsafe extern "C" fn metadata_pair_delete(pair: *mut MetadataPair) -> c_int { +#[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], @@ -415,19 +421,30 @@ impl MetadataIterator { #[repr(C)] #[allow(missing_copy_implementations)] pub struct MetadataPair { - key: *const c_char, - value: *const c_char, + key: *mut c_char, + value: *mut c_char, } impl MetadataPair { - fn new(key: &str, value: &str) -> Result { + fn new( + key: &str, + value: &str, + ) -> Result { Ok(MetadataPair { key: string::into_leaked_cstring(key)?, - value: string::into_leaked_cstring(value)? + value: string::into_leaked_cstring(value)?, }) } } +// Ensure that the owned strings are freed when the pair is dropped. +impl Drop for MetadataPair { + fn drop(&mut self) { + string::string_delete(self.key); + string::string_delete(self.value); + } +} + /*=============================================================================================== * # Status Types *---------------------------------------------------------------------------------------------*/ diff --git a/rust/pact_matching_ffi/src/util/string.rs b/rust/pact_matching_ffi/src/util/string.rs index a61fc93e7..16fa5340e 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 @@ -10,15 +9,8 @@ use std::mem; /// 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) +) -> anyhow::Result<*mut c_char> { + Ok(CString::new(t)?.into_raw()) } /// Delete a string previously returned by this FFI. From e5868d7cd992fc2a5b27c6fd8c88fc04e2d41a21 Mon Sep 17 00:00:00 2001 From: Andrew Lilley Brinker Date: Mon, 3 Aug 2020 12:15:58 -0700 Subject: [PATCH 4/4] Expose `*const` where possible. This converts some `*mut c_char` into `*const c_char`, modifying `MetadataPair::drop` to cast back to `*mut c_char` at deletion, with a comment explaining why this cast is safe. --- rust/pact_matching_ffi/src/models/message.rs | 39 ++++++++++++-------- rust/pact_matching_ffi/src/util/string.rs | 4 +- 2 files changed, 24 insertions(+), 19 deletions(-) diff --git a/rust/pact_matching_ffi/src/models/message.rs b/rust/pact_matching_ffi/src/models/message.rs index 185b9dc5d..98f0b2d63 100644 --- a/rust/pact_matching_ffi/src/models/message.rs +++ b/rust/pact_matching_ffi/src/models/message.rs @@ -105,17 +105,16 @@ pub unsafe extern "C" fn message_delete(message: *mut Message) -> c_int { #[allow(clippy::or_fun_call)] pub unsafe extern "C" fn message_get_description( message: *const Message, -) -> *mut c_char { +) -> *const c_char { ffi! { name: "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_mut_to::() + ptr::null_to::() } } } @@ -230,10 +229,10 @@ pub unsafe extern "C" fn message_find_metadata( // 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::into_leaked_cstring(value)?) + Ok(string::to_c(value)? as *const c_char) }, fail: { - ptr::null_mut_to::() + ptr::null_to::() } } } @@ -421,27 +420,35 @@ impl MetadataIterator { #[repr(C)] #[allow(missing_copy_implementations)] pub struct MetadataPair { - key: *mut c_char, - value: *mut c_char, + key: *const c_char, + value: *const c_char, } impl MetadataPair { - fn new( - key: &str, - value: &str, - ) -> Result { + fn new(key: &str, value: &str) -> anyhow::Result { Ok(MetadataPair { - key: string::into_leaked_cstring(key)?, - value: string::into_leaked_cstring(value)?, + 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); - string::string_delete(self.value); + string::string_delete(self.key as *mut c_char); + string::string_delete(self.value as *mut c_char); } } diff --git a/rust/pact_matching_ffi/src/util/string.rs b/rust/pact_matching_ffi/src/util/string.rs index 16fa5340e..dfc18b0fe 100644 --- a/rust/pact_matching_ffi/src/util/string.rs +++ b/rust/pact_matching_ffi/src/util/string.rs @@ -7,9 +7,7 @@ use std::ffi::CString; /// /// The returned pointer must be passed to CString::from_raw to /// prevent leaking memory. -pub(crate) fn into_leaked_cstring( - t: &str, -) -> anyhow::Result<*mut c_char> { +pub(crate) fn to_c(t: &str) -> anyhow::Result<*mut c_char> { Ok(CString::new(t)?.into_raw()) }