fix memory leak, remove blanket pointer-by-value impls

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)
+    //  - tcanns is not NULL and points to a valid TCAnnotationList (caller is not allowed to
-    let annotations =
+    //    modify the list)
-        unsafe { TCAnnotationList::take_from_arg(tcanns, TCAnnotationList::null_value()) };
+    //  - caller promises not to use the value after return
-    TCAnnotationList::drop_vector(annotations);
+    unsafe { drop_value_array(tcanns) }
 }
 
 #[cfg(test)]

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)
+    //  - tcstrings is not NULL and points to a valid TCStringList (caller is not allowed to
-    let strings = unsafe { TCStringList::take_from_arg(tcstrings, TCStringList::null_value()) };
+    //    modify the list)
-    TCStringList::drop_vector(strings);
+    //  - caller promises not to use the value after return
+    unsafe { drop_pointer_array(tcstrings) };
 }
 
 #[cfg(test)]

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)
+    //  - tctasks is not NULL and points to a valid TCTaskList (caller is not allowed to
-    let tasks = unsafe { TCTaskList::take_from_arg(tctasks, TCTaskList::null_value()) };
+    //    modify the list)
-    TCTaskList::drop_vector(tasks);
+    //  - caller promises not to use the value after return
+    unsafe { drop_pointer_array(tctasks) };
 }
 
 #[cfg(test)]

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<P> 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<P> 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<P> can be passed by value.  See the implementation for NonNull<P>.
-impl<P> PassByValue for NonNull<P>
-where
-    P: PassByPointer + 'static,
-{
-    type RustType = &'static mut P;
-
-    unsafe fn from_ctype(mut self) -> Self::RustType {
-        // SAFETY: self must be a valid NonNull<P> (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<Element>`.  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<T>`
 ///
 /// # 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
+/// Given a CArray containing pass-by-value values, drop all of the values and
-    /// tc_.._free functions.
+/// the array.
-    fn drop_vector(mut vec: Vec<Self::Element>) {
+///
-        // first, drop each of the elements in turn
+/// This is a convenience function for `tc_.._list_free` functions.
-        for e in vec.drain(..) {
+///
-            // SAFETY:
+/// # Safety
-            //  - e is a valid Element (caller promisd not to change it)
+///
-            //  - Vec::drain has invalidated this entry (value is owned)
+/// - Array must be non-NULL and point to a valid CA instance
-            drop(unsafe { PassByValue::from_ctype(e) });
+/// - 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.
-        // then drop the vector
+pub(crate) unsafe fn drop_value_array<CA, T>(array: *mut CA)
-        drop(vec);
+where
+    CA: CArray<Element = T>,
+    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<CA, T>(array: *mut CA)
+where
+    CA: CArray<Element = NonNull<T>>,
+    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<A> PassByValue for A

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)
+    //  - tcuuids is not NULL and points to a valid TCUuidList (caller is not allowed to
-    let uuids = unsafe { TCUuidList::take_from_arg(tcuuids, TCUuidList::null_value()) };
+    //    modify the list)
-    TCUuidList::drop_vector(uuids);
+    //  - caller promises not to use the value after return
+    unsafe { drop_value_array(tcuuids) };
 }
 
 #[cfg(test)]