diff --git a/lib/src/annotation.rs b/lib/src/annotation.rs index 167afc498..22a81653d 100644 --- a/lib/src/annotation.rs +++ b/lib/src/annotation.rs @@ -95,12 +95,11 @@ pub unsafe extern "C" fn tc_annotation_free(tcann: *mut TCAnnotation) { /// When this call returns, the `items` pointer will be NULL, signalling an invalid TCAnnotationList. #[no_mangle] pub unsafe extern "C" fn tc_annotation_list_free(tcanns: *mut TCAnnotationList) { - debug_assert!(!tcanns.is_null()); // SAFETY: - // - *tcanns is a valid TCAnnotationList (caller promises to treat it as read-only) - let annotations = - unsafe { TCAnnotationList::take_from_arg(tcanns, TCAnnotationList::null_value()) }; - TCAnnotationList::drop_vector(annotations); + // - tcanns is not NULL and points to a valid TCAnnotationList (caller is not allowed to + // modify the list) + // - caller promises not to use the value after return + unsafe { drop_value_array(tcanns) } } #[cfg(test)] diff --git a/lib/src/string.rs b/lib/src/string.rs index d9fc90507..c02bdb1e6 100644 --- a/lib/src/string.rs +++ b/lib/src/string.rs @@ -321,11 +321,11 @@ pub unsafe extern "C" fn tc_string_free(tcstring: *mut TCString) { /// When this call returns, the `items` pointer will be NULL, signalling an invalid TCStringList. #[no_mangle] pub unsafe extern "C" fn tc_string_list_free(tcstrings: *mut TCStringList) { - debug_assert!(!tcstrings.is_null()); // SAFETY: - // - *tcstrings is a valid TCStringList (caller promises to treat it as read-only) - let strings = unsafe { TCStringList::take_from_arg(tcstrings, TCStringList::null_value()) }; - TCStringList::drop_vector(strings); + // - tcstrings is not NULL and points to a valid TCStringList (caller is not allowed to + // modify the list) + // - caller promises not to use the value after return + unsafe { drop_pointer_array(tcstrings) }; } #[cfg(test)] diff --git a/lib/src/task.rs b/lib/src/task.rs index 03b86d806..87a64493d 100644 --- a/lib/src/task.rs +++ b/lib/src/task.rs @@ -586,11 +586,11 @@ pub unsafe extern "C" fn tc_task_free<'a>(task: *mut TCTask) { /// When this call returns, the `items` pointer will be NULL, signalling an invalid TCTaskList. #[no_mangle] pub unsafe extern "C" fn tc_task_list_free(tctasks: *mut TCTaskList) { - debug_assert!(!tctasks.is_null()); // SAFETY: - // - *tctasks is a valid TCTaskList (caller promises to treat it as read-only) - let tasks = unsafe { TCTaskList::take_from_arg(tctasks, TCTaskList::null_value()) }; - TCTaskList::drop_vector(tasks); + // - tctasks is not NULL and points to a valid TCTaskList (caller is not allowed to + // modify the list) + // - caller promises not to use the value after return + unsafe { drop_pointer_array(tctasks) }; } #[cfg(test)] diff --git a/lib/src/traits.rs b/lib/src/traits.rs index 007d60ac3..fea6ddbca 100644 --- a/lib/src/traits.rs +++ b/lib/src/traits.rs @@ -6,6 +6,8 @@ use std::ptr::NonNull; /// /// The Rust and C types may differ, with from_ctype and as_ctype converting between them. /// Implement this trait for the C type. +/// +/// The RustType must be droppable (not containing raw pointers). pub(crate) trait PassByValue: Sized { type RustType; @@ -146,66 +148,6 @@ pub(crate) trait PassByPointer: Sized { } } -/// *mut P can be passed by value. -/// -/// # Safety -/// -/// The 'static bound means that the pointer is treated as an "owning" pointer, -/// and must not be copied (leading to a double-free) or dropped without dropping -/// its contents (a leak). -impl

PassByValue for *mut P -where - P: PassByPointer + 'static, -{ - type RustType = &'static mut P; - - unsafe fn from_ctype(self) -> Self::RustType { - // SAFETY: self must be a valid *mut P (promised by caller) - unsafe { &mut *self } - } - - /// Convert a Rust value to a C value. - fn as_ctype(arg: Self::RustType) -> Self { - arg - } -} - -/// *const P can be passed by value. See the implementation for *mut P. -impl

PassByValue for *const P -where - P: PassByPointer + 'static, -{ - type RustType = &'static P; - - unsafe fn from_ctype(self) -> Self::RustType { - // SAFETY: self must be a valid *const P (promised by caller) - unsafe { &*self } - } - - /// Convert a Rust value to a C value. - fn as_ctype(arg: Self::RustType) -> Self { - arg - } -} - -/// NonNull

can be passed by value. See the implementation for NonNull

. -impl

PassByValue for NonNull

-where - P: PassByPointer + 'static, -{ - type RustType = &'static mut P; - - unsafe fn from_ctype(mut self) -> Self::RustType { - // SAFETY: self must be a valid NonNull

(promised by caller) - unsafe { self.as_mut() } - } - - /// Convert a Rust value to a C value. - fn as_ctype(arg: Self::RustType) -> Self { - NonNull::new(arg).expect("value must not be NULL") - } -} - /// Support for C arrays of objects referenced by value. /// /// The underlying C type should have three fields, containing items, length, and capacity. The @@ -213,10 +155,10 @@ where /// implemented automatically, converting between the C type and `Vec`. For most cases, /// it is only necessary to implement `tc_.._free` that first calls /// `PassByValue::take_from_arg(arg, CArray::null_value())` to take the existing value and -/// replace it with the null value; then `CArray::drop_value_vector(..)` to drop the resulting +/// replace it with the null value; then one of hte `drop_.._array(..)` functions to drop the resulting /// vector and all of the objects it points to. /// -/// This can be used for objects referenced by pointer, too, with an Element type of `*const T` +/// This can be used for objects referenced by pointer, too, with an Element type of `NonNull` /// /// # Safety /// @@ -225,7 +167,7 @@ where /// /// This class guarantees that the items pointer is non-NULL for any valid array (even when len=0). pub(crate) trait CArray: Sized { - type Element: PassByValue; + type Element; /// Create a new CArray from the given items, len, and capacity. /// @@ -247,20 +189,68 @@ pub(crate) trait CArray: Sized { // - satisfies the first case in from_raw_parts' safety documentation unsafe { Self::from_raw_parts(std::ptr::null(), 0, 0) } } +} - /// Drop a vector of elements. This is a convenience function for implementing - /// tc_.._free functions. - fn drop_vector(mut vec: Vec) { - // first, drop each of the elements in turn - for e in vec.drain(..) { - // SAFETY: - // - e is a valid Element (caller promisd not to change it) - // - Vec::drain has invalidated this entry (value is owned) - drop(unsafe { PassByValue::from_ctype(e) }); - } - // then drop the vector - drop(vec); +/// Given a CArray containing pass-by-value values, drop all of the values and +/// the array. +/// +/// This is a convenience function for `tc_.._list_free` functions. +/// +/// # Safety +/// +/// - Array must be non-NULL and point to a valid CA instance +/// - The caller must not use the value array points to after this function, as +/// it has been freed. It will be replaced with the null value. +pub(crate) unsafe fn drop_value_array(array: *mut CA) +where + CA: CArray, + T: PassByValue, +{ + debug_assert!(!array.is_null()); + + // SAFETY: + // - *array is a valid CA (caller promises to treat it as read-only) + let mut vec = unsafe { CA::take_from_arg(array, CA::null_value()) }; + + // first, drop each of the elements in turn + for e in vec.drain(..) { + // SAFETY: + // - e is a valid Element (caller promisd not to change it) + // - Vec::drain has invalidated this entry (value is owned) + drop(unsafe { PassByValue::from_arg(e) }); } + // then drop the vector + drop(vec); +} + +/// Given a CArray containing NonNull pointers, drop all of the pointed-to values and the array. +/// +/// This is a convenience function for `tc_.._list_free` functions. +/// +/// # Safety +/// +/// - Array must be non-NULL and point to a valid CA instance +/// - The caller must not use the value array points to after this function, as +/// it has been freed. It will be replaced with the null value. +pub(crate) unsafe fn drop_pointer_array(array: *mut CA) +where + CA: CArray>, + T: PassByPointer, +{ + debug_assert!(!array.is_null()); + // SAFETY: + // - *array is a valid CA (caller promises to treat it as read-only) + let mut vec = unsafe { CA::take_from_arg(array, CA::null_value()) }; + + // first, drop each of the elements in turn + for e in vec.drain(..) { + // SAFETY: + // - e is a valid Element (caller promised not to change it) + // - Vec::drain has invalidated this entry (value is owned) + drop(unsafe { PassByPointer::take_from_arg(e.as_ptr()) }); + } + // then drop the vector + drop(vec); } impl PassByValue for A diff --git a/lib/src/uuid.rs b/lib/src/uuid.rs index d140a9eed..56311152b 100644 --- a/lib/src/uuid.rs +++ b/lib/src/uuid.rs @@ -130,11 +130,11 @@ pub unsafe extern "C" fn tc_uuid_from_str<'a>(s: *mut TCString, uuid_out: *mut T /// When this call returns, the `items` pointer will be NULL, signalling an invalid TCUuidList. #[no_mangle] pub unsafe extern "C" fn tc_uuid_list_free(tcuuids: *mut TCUuidList) { - debug_assert!(!tcuuids.is_null()); // SAFETY: - // - *tcuuids is a valid TCUuidList (caller promises to treat it as read-only) - let uuids = unsafe { TCUuidList::take_from_arg(tcuuids, TCUuidList::null_value()) }; - TCUuidList::drop_vector(uuids); + // - tcuuids is not NULL and points to a valid TCUuidList (caller is not allowed to + // modify the list) + // - caller promises not to use the value after return + unsafe { drop_value_array(tcuuids) }; } #[cfg(test)]