From d62037c753f0d0a36c2bc577c246d72f527635c8 Mon Sep 17 00:00:00 2001 From: AP Ljungquist Date: Mon, 16 Dec 2024 13:51:44 +0100 Subject: [PATCH] fix(axevent)!: Take ownership of returned value in `get_string` (#149) `crates/axevent/src/flex.rs`: - Use custom type instead of `GString` or `GStringPtr` because `axevent` supports both keys and values that are not valid UTF-8 and, though it seems unlikely that users will want to consume or produce such events, supporting it is easy. The biggest drawback is reduced ergonomics since users have to convert to normal strings themselves. - Don't use `mbox` to avoid adding another crate to the public API. Furthermore `mbox::MString` requires the string to be valid UTF-8, just like `glib::GString` so we would have to use one of the other types, and it is not clear to me if or how that would lead to a better API. - Add `debug_assert!(!ptr.is_null())` to make it more likely that violations of the preconditions are caught without incurring a cost at runtime in production. Since this API is internal to the crate, I think the value is low, but so is the cost so might as well have it. --- apps-aarch64.checksum | 2 +- apps-aarch64.filesize | 2 +- apps/send_event/src/main.rs | 18 ++++++++------ crates/axevent/src/flex.rs | 49 ++++++++++++++++++++++++++++++++++--- 4 files changed, 58 insertions(+), 13 deletions(-) diff --git a/apps-aarch64.checksum b/apps-aarch64.checksum index 3d206ba3..d19829d8 100644 --- a/apps-aarch64.checksum +++ b/apps-aarch64.checksum @@ -7,6 +7,6 @@ f47872232e0f2cd4ace9e2e77b07cafb431cbe73 target-aarch64/acap/hello_world_0_0_0_ 75818664787ff2ff7dbdd32fadfe20c23974c867 target-aarch64/acap/inspect_env_0_0_0_aarch64.eap 9d205dbde1b7144d30fb4c5a45fcaef93cb12810 target-aarch64/acap/licensekey_handler_0_0_0_aarch64.eap ed84b4b9ae09256dcef1a7287cd802e39c8b6a92 target-aarch64/acap/reverse_proxy_0_0_0_aarch64.eap -aee472d1e21f50694fc495c3483ba3dc7bae21e7 target-aarch64/acap/send_event_1_0_0_aarch64.eap +1451c62ae3306ba77db206db137836561f1069b4 target-aarch64/acap/send_event_1_0_0_aarch64.eap e8bfb22e1b1416bff9d33980839ccbc8ce4b9d16 target-aarch64/acap/using_a_build_script_0_0_0_aarch64.eap 5a533b71d01edb65d15751dd93fa153f5ef343ea target-aarch64/acap/vapix_access_0_0_0_aarch64.eap diff --git a/apps-aarch64.filesize b/apps-aarch64.filesize index 6052b445..323a4531 100644 --- a/apps-aarch64.filesize +++ b/apps-aarch64.filesize @@ -7,6 +7,6 @@ 2136 target-aarch64/acap/inspect_env_0_0_0_aarch64.eap 2060 target-aarch64/acap/licensekey_handler_0_0_0_aarch64.eap 10425 target-aarch64/acap/reverse_proxy_0_0_0_aarch64.eap -4087 target-aarch64/acap/send_event_1_0_0_aarch64.eap +4088 target-aarch64/acap/send_event_1_0_0_aarch64.eap 859 target-aarch64/acap/using_a_build_script_0_0_0_aarch64.eap 11361 target-aarch64/acap/vapix_access_0_0_0_aarch64.eap diff --git a/apps/send_event/src/main.rs b/apps/send_event/src/main.rs index 1467ec2d..bd465eb6 100644 --- a/apps/send_event/src/main.rs +++ b/apps/send_event/src/main.rs @@ -78,16 +78,12 @@ fn main() { #[cfg(test)] mod tests { - use std::{ - ffi::{CStr, CString}, - time, - time::Duration, - }; + use std::{ffi::CStr, time, time::Duration}; use anyhow::bail; use axevent::{ ergo::{date_time, system_time, Declaration, MainLoop, Subscription}, - flex::{Event, Handler, KeyValueSet}, + flex::{CStringPtr, Event, Handler, KeyValueSet}, }; use log::debug; @@ -102,7 +98,7 @@ mod tests { Ok(kvs) } - fn send_and_receive_event(sent: &CStr) -> anyhow::Result { + fn send_and_receive_event(sent: &CStr) -> anyhow::Result { let main_loop = MainLoop::new(); let handler = Handler::new(); @@ -153,7 +149,13 @@ mod tests { #[test] fn can_send_and_receive_event() { - let expected = c"Hello"; + // axevent supports keys and values that are not valid UTF-8; + // using an invalid UTF-8 string is a low-effort way of ensuring that support is propagated. + let expected = c"Hello\xc3\x28"; + + // Sanity check + assert!(expected.to_str().is_err()); + let actual = send_and_receive_event(expected).unwrap(); assert_eq!(actual.as_c_str(), expected); } diff --git a/crates/axevent/src/flex.rs b/crates/axevent/src/flex.rs index e3316f37..06530028 100644 --- a/crates/axevent/src/flex.rs +++ b/crates/axevent/src/flex.rs @@ -13,6 +13,7 @@ use std::{ ffi::{c_char, c_double, c_int, c_uint, c_void, CStr, CString}, fmt::Debug, process, ptr, + ptr::NonNull, sync::Mutex, }; @@ -36,7 +37,7 @@ use glib::{ translate::{from_glib_full, from_glib_none, IntoGlibPtr}, DateTime, }; -use glib_sys::{gboolean, gpointer, GError}; +use glib_sys::{g_free, gboolean, gpointer, GError}; use log::debug; macro_rules! abort_unwind { @@ -63,6 +64,40 @@ macro_rules! try_func { }} } +#[repr(transparent)] +pub struct CStringPtr(NonNull); + +impl CStringPtr { + /// Create an owned string from a foreign allocation + /// + /// # Safety + /// + /// In addition to the safety preconditions for [`CStr::from_ptr`] the memory must have been + /// allocated in a manner compatible with [`glib_sys::g_free`] and there must be no other + /// users of this memory. + unsafe fn from_ptr(ptr: *mut c_char) -> Self { + debug_assert!(!ptr.is_null()); + Self(NonNull::new_unchecked(ptr)) + } + + pub fn as_c_str(&self) -> &CStr { + // SAFETY: The preconditions for instantiating this type include all preconditions + // for `CStr::from_ptr`. + unsafe { CStr::from_ptr(self.0.as_ptr() as *const c_char) } + } +} + +impl Drop for CStringPtr { + fn drop(&mut self) { + // SAFETY: The preconditions for instantiating this type include: + // - having full ownership of the memory. + // - having allocated the memory in a manner that is compatible with `g_free`. + unsafe { + g_free(self.0.as_ptr() as *mut c_void); + } + } +} + struct Deferred(Option>); impl Drop for Deferred { @@ -583,7 +618,7 @@ impl KeyValueSet { } } - pub fn get_string(&self, key: &CStr, namespace: Option<&CStr>) -> Result { + pub fn get_string(&self, key: &CStr, namespace: Option<&CStr>) -> Result { unsafe { let mut value: *mut c_char = ptr::null_mut(); try_func!( @@ -596,7 +631,15 @@ impl KeyValueSet { }, &mut value, )?; - Ok(CString::from(CStr::from_ptr(value))) + // SAFETY: This is safe because: + // - The foreign function sets the error if the value is null in which case we return + // early above. + // - The foreign function creates the value with `g_strdup` so it will be nul + // terminated, reads to up to and including the nul terminator are valid, and it may + // be freed using `g_free`. + // - This function owns the memory and does not mutate it. + // - Values will never be longer than `isize::MAX` in practice. + Ok(CStringPtr::from_ptr(value)) } }