diff --git a/integration-tests/src/bindings_tests/task.c b/integration-tests/src/bindings_tests/task.c index 2ef89204c..c011509ca 100644 --- a/integration-tests/src/bindings_tests/task.c +++ b/integration-tests/src/bindings_tests/task.c @@ -315,11 +315,11 @@ static void test_task_get_tags(void) { TCTags tags = tc_task_get_tags(task); int found_pending = false, found_next = false; - for (size_t i = 0; i < tags.num_tags; i++) { - if (strcmp("PENDING", tc_string_content(tags.tags[i])) == 0) { + for (size_t i = 0; i < tags.len; i++) { + if (strcmp("PENDING", tc_string_content(tags.items[i])) == 0) { found_pending = true; } - if (strcmp("next", tc_string_content(tags.tags[i])) == 0) { + if (strcmp("next", tc_string_content(tags.items[i])) == 0) { found_next = true; } } @@ -327,7 +327,7 @@ static void test_task_get_tags(void) { TEST_ASSERT_TRUE(found_next); tc_tags_free(&tags); - TEST_ASSERT_NULL(tags.tags); + TEST_ASSERT_NULL(tags.items); tc_task_free(task); tc_replica_free(rep); diff --git a/lib/src/arrays.rs b/lib/src/arrays.rs index 3c266cb3c..a9d5dde49 100644 --- a/lib/src/arrays.rs +++ b/lib/src/arrays.rs @@ -2,8 +2,6 @@ use crate::string::TCString; use crate::traits::*; use std::ptr::NonNull; -// TODO: generalize to TCStrings? - /// TCTags represents a list of tags associated with a task. /// /// The content of this struct must be treated as read-only. @@ -12,86 +10,74 @@ use std::ptr::NonNull; /// will remain valid even if the task is freed. #[repr(C)] pub struct TCTags { - // TODO: can we use NonNull here? - /// strings representing each tag. these remain owned by the - /// TCTags instance and will be freed by tc_tags_free. - tags: *const NonNull>, - /// number of tags in tags - num_tags: libc::size_t, - /// total size of tags (internal use only) + // WARNING: this struct must match CPointerArray exactly, in size and order + // of fields. Names can differ, as can the pointer type. + /// number of tags in items + len: libc::size_t, + + /// total size of items (internal use only) _capacity: libc::size_t, + + /// strings representing each tag. these remain owned by the TCTags instance and will be freed + /// by tc_tags_free. + items: *const NonNull>, +} + +impl PassByValue for Vec>> { + type CType = TCTags; + + unsafe fn from_ctype(arg: TCTags) -> Self { + // SAFETY: + // - C treats TCTags as read-only, so items, len, and _capacity all came + // from a Vec originally. + unsafe { Vec::from_raw_parts(arg.items as *mut _, arg.len, arg._capacity) } + } + + fn as_ctype(self) -> TCTags { + // emulate Vec::into_raw_parts(): + // - disable dropping the Vec with ManuallyDrop + // - extract ptr, len, and capacity using those methods + let mut vec = std::mem::ManuallyDrop::new(self); + TCTags { + len: vec.len(), + _capacity: vec.capacity(), + items: vec.as_mut_ptr(), + } + } } impl Default for TCTags { fn default() -> Self { Self { - tags: std::ptr::null_mut(), - num_tags: 0, + len: 0, _capacity: 0, + items: std::ptr::null(), } } } -impl TCTags { - /// Create a Vec of TCStrings into a TCTags instance. - pub(crate) fn new(tags: Vec>>) -> Self { - // emulate Vec::into_raw_parts(): - // - disable dropping the Vec with ManuallyDrop - // - extract ptr, len, and capacity using those methods - let mut tags = std::mem::ManuallyDrop::new(tags); - Self { - tags: tags.as_mut_ptr(), - num_tags: tags.len(), - _capacity: tags.capacity(), - } - } - - /// Convert a TCTags to a Vec<_>. - /// - /// # Safety - /// - /// Tags must be _exactly_ as created by [`new`] - unsafe fn into_vec(self) -> Vec>> { - // SAFETY: - // - // * tags.tags needs to have been previously allocated via Vec<*mut TCString> - // * TCString needs to have the same size and alignment as what ptr was allocated with. - // * length needs to be less than or equal to capacity. - // * capacity needs to be the capacity that the pointer was allocated with. - // * vec elements are not NULL - // - // All of this is true for a value returned from `new`, which the caller promised - // not to change. - unsafe { Vec::from_raw_parts(self.tags as *mut _, self.num_tags, self._capacity) } - } -} - -/// Free a TCTags instance. The given pointer must not be NULL. The instance must not be used -/// after this call. +/// Free a TCTags instance. The instance, and all TCStrings it contains, must not be used after +/// this call. #[no_mangle] pub extern "C" fn tc_tags_free<'a>(tctags: *mut TCTags) { debug_assert!(!tctags.is_null()); // SAFETY: - // - tctags is not NULL - // - tctags is valid (caller promises it has not been changed) - // - caller will not use the TCTags after this (promised by caller) - let tctags: &'a mut TCTags = unsafe { &mut *tctags }; + // - *tctags is a valid TCTags (caller promises to treat it as read-only) + let tags = unsafe { Vec::take_from_arg(tctags, TCTags::default()) }; - debug_assert!(!tctags.tags.is_null()); - - // replace the caller's TCTags with one containing a NULL pointer - let tctags: TCTags = std::mem::take(tctags); - - // convert to a regular Vec - // SAFETY: - // - tctags is exactly as returned from TCTags::new (promised by caller) - let mut vec: Vec<_> = unsafe { tctags.into_vec() }; - - // drop each contained string - for tcstring in vec.drain(..) { - // SAFETY: see TCString docstring - drop(unsafe { TCString::take_from_arg(tcstring.as_ptr()) }); - } - - drop(vec); + // tags is a Vec>, so we convert it to a Vec that + // will properly drop those strings when dropped. + let tags: Vec = tags + .iter() + .map(|p| { + // SAFETY: + // *p is a pointer delivered to us from a Vec>, so + // - *p is not NULL + // - *p was generated by Rust + // - *p was not modified (promised by caller) + // - the caller will not use this pointer again (promised by caller) + unsafe { TCString::take_from_arg(p.as_ptr()) } + }) + .collect(); + drop(tags); } diff --git a/lib/src/replica.rs b/lib/src/replica.rs index 7512d1f91..131a8eb91 100644 --- a/lib/src/replica.rs +++ b/lib/src/replica.rs @@ -142,7 +142,8 @@ pub extern "C" fn tc_replica_get_task(rep: *mut TCReplica, tcuuid: TCUuid) -> *m wrap( rep, |rep| { - let uuid = Uuid::from_arg(tcuuid); + // SAFETY: see TCUuid docstring + let uuid = unsafe { Uuid::from_arg(tcuuid) }; if let Some(task) = rep.get_task(uuid)? { Ok(TCTask::from(task).return_val()) } else { @@ -185,7 +186,8 @@ pub extern "C" fn tc_replica_import_task_with_uuid( wrap( rep, |rep| { - let uuid = Uuid::from_arg(tcuuid); + // SAFETY: see TCUuid docstring + let uuid = unsafe { Uuid::from_arg(tcuuid) }; let task = rep.import_task_with_uuid(uuid)?; Ok(TCTask::from(task).return_val()) }, diff --git a/lib/src/task.rs b/lib/src/task.rs index 87dcdd773..75bf12888 100644 --- a/lib/src/task.rs +++ b/lib/src/task.rs @@ -324,13 +324,14 @@ pub extern "C" fn tc_task_get_tags<'a>(task: *mut TCTask) -> TCTags { let tcstrings: Vec>> = task .get_tags() .map(|t| { - // SAFETY: see TCString docstring - let t_ptr = unsafe { TCString::from(t.as_ref()).return_val() }; - // SAFETY: t_ptr was just created and is not NULL - unsafe { NonNull::new_unchecked(t_ptr) } + NonNull::new( + // SAFETY: see TCString docstring + unsafe { TCString::from(t.as_ref()).return_val() }, + ) + .expect("TCString::return_val() returned NULL") }) .collect(); - TCTags::new(tcstrings) + tcstrings.return_val() }) } diff --git a/lib/src/traits.rs b/lib/src/traits.rs new file mode 100644 index 000000000..8011a46e4 --- /dev/null +++ b/lib/src/traits.rs @@ -0,0 +1,143 @@ +/// Support for values passed to Rust by value. These are represented as full structs in C. Such +/// values are implicitly copyable, via C's struct assignment. +/// +/// The Rust and C types may differ, with from_ctype and as_ctype converting between them. +pub(crate) trait PassByValue: Sized { + type CType; + + /// Convert a C value to a Rust value. + /// + /// # Safety + /// + /// `arg` must be a valid CType. + unsafe fn from_ctype(arg: Self::CType) -> Self; + + /// Convert a Rust value to a C value. + fn as_ctype(self) -> Self::CType; + + /// Take a value from C as an argument. + /// + /// # Safety + /// + /// `arg` must be a valid CType. This is typically ensured either by requiring that C + /// code not modify it, or by defining the valid values in C comments. + unsafe fn from_arg(arg: Self::CType) -> Self { + // SAFETY: + // - arg is a valid CType (promised by caller) + unsafe { Self::from_ctype(arg) } + } + + /// Take a value from C as a pointer argument, replacing it with the given value. This is used + /// to invalidate the C value as an additional assurance against subsequent use of the value. + /// + /// # Safety + /// + /// `*arg` must be a valid CType, as with [`from_arg`]. + unsafe fn take_from_arg(arg: *mut Self::CType, mut replacement: Self::CType) -> Self { + // SAFETY: + // - arg is valid (promised by caller) + // - replacement is valid (guaranteed by Rust) + unsafe { std::ptr::swap(arg, &mut replacement) }; + // SAFETY: + // - replacement (formerly *arg) is a valid CType (promised by caller) + unsafe { PassByValue::from_arg(replacement) } + } + + /// Return a value to C + fn return_val(self) -> Self::CType { + self.as_ctype() + } + + /// Return a value to C, via an "output parameter" + /// + /// # Safety + /// + /// `arg_out` must not be NULL and must be properly aligned and pointing to valid memory + /// of the size of CType. + unsafe fn to_arg_out(self, arg_out: *mut Self::CType) { + debug_assert!(!arg_out.is_null()); + // SAFETY: + // - arg_out is not NULL (promised by caller, asserted) + // - arg_out is properly aligned and points to valid memory (promised by caller) + unsafe { *arg_out = self.as_ctype() }; + } +} + +/// Support for values passed to Rust by pointer. These are represented as opaque structs in C, +/// and always handled as pointers. +/// +/// # Safety +/// +/// The functions provided by this trait are used directly in C interface functions, and make the +/// following expectations of the C code: +/// +/// - When passing a value to Rust (via the `…arg…` functions), +/// - the pointer must not be NULL; +/// - the pointer must be one previously returned from Rust; and +/// - the memory addressed by the pointer must never be modified by C code. +/// - For `from_arg_ref`, the value must not be modified during the call to the Rust function +/// - For `from_arg_ref_mut`, the value must not be accessed (read or write) during the call +/// (these last two points are trivially ensured by all TC… types being non-threadsafe) +/// - For `take_from_arg`, the pointer becomes invalid and must not be used in _any way_ after it +/// is passed to the Rust function. +/// - For `return_val` and `to_arg_out`, it is the C caller's responsibility to later free the value. +/// - For `to_arg_out`, `arg_out` must not be NULL and must be properly aligned and pointing to +/// valid memory. +/// +/// These requirements should be expressed in the C documentation for the type implementing this +/// trait. +pub(crate) trait PassByPointer: Sized { + /// Take a value from C as an argument. + /// + /// # Safety + /// + /// See trait documentation. + unsafe fn take_from_arg(arg: *mut Self) -> Self { + debug_assert!(!arg.is_null()); + // SAFETY: see trait documentation + unsafe { *(Box::from_raw(arg)) } + } + + /// Borrow a value from C as an argument. + /// + /// # Safety + /// + /// See trait documentation. + unsafe fn from_arg_ref<'a>(arg: *const Self) -> &'a Self { + debug_assert!(!arg.is_null()); + // SAFETY: see trait documentation + unsafe { &*arg } + } + + /// Mutably borrow a value from C as an argument. + /// + /// # Safety + /// + /// See trait documentation. + unsafe fn from_arg_ref_mut<'a>(arg: *mut Self) -> &'a mut Self { + debug_assert!(!arg.is_null()); + // SAFETY: see trait documentation + unsafe { &mut *arg } + } + + /// Return a value to C, transferring ownership + /// + /// # Safety + /// + /// See trait documentation. + unsafe fn return_val(self) -> *mut Self { + Box::into_raw(Box::new(self)) + } + + /// Return a value to C, transferring ownership, via an "output parameter". + /// + /// # Safety + /// + /// See trait documentation. + unsafe fn to_arg_out(self, arg_out: *mut *mut Self) { + // SAFETY: see trait documentation + unsafe { + *arg_out = self.return_val(); + } + } +} diff --git a/lib/src/uuid.rs b/lib/src/uuid.rs index 8f57c7d1a..dfb084887 100644 --- a/lib/src/uuid.rs +++ b/lib/src/uuid.rs @@ -54,7 +54,9 @@ pub extern "C" fn tc_uuid_to_buf<'a>(tcuuid: TCUuid, buf: *mut libc::c_char) { let buf: &'a mut [u8] = unsafe { std::slice::from_raw_parts_mut(buf as *mut u8, ::uuid::adapter::Hyphenated::LENGTH) }; - let uuid: Uuid = Uuid::from_arg(tcuuid); + // SAFETY: + // - tcuuid is a valid TCUuid (all byte patterns are valid) + let uuid: Uuid = unsafe { Uuid::from_arg(tcuuid) }; uuid.to_hyphenated().encode_lower(buf); } @@ -62,7 +64,9 @@ pub extern "C" fn tc_uuid_to_buf<'a>(tcuuid: TCUuid, buf: *mut libc::c_char) { /// at least TC_UUID_STRING_BYTES long. No NUL terminator is added. #[no_mangle] pub extern "C" fn tc_uuid_to_str(tcuuid: TCUuid) -> *mut TCString<'static> { - let uuid: Uuid = Uuid::from_arg(tcuuid); + // SAFETY: + // - tcuuid is a valid TCUuid (all byte patterns are valid) + let uuid: Uuid = unsafe { Uuid::from_arg(tcuuid) }; let s = uuid.to_string(); // SAFETY: see TCString docstring unsafe { TCString::from(s).return_val() } diff --git a/lib/taskchampion.h b/lib/taskchampion.h index ccb8ff1db..b60658d58 100644 --- a/lib/taskchampion.h +++ b/lib/taskchampion.h @@ -127,18 +127,18 @@ typedef struct TCTask TCTask; */ typedef struct TCTags { /** - * strings representing each tag. these remain owned by the - * TCTags instance and will be freed by tc_tags_free. + * number of tags in items */ - struct TCString *const *tags; + size_t len; /** - * number of tags in tags - */ - size_t num_tags; - /** - * total size of tags (internal use only) + * total size of items (internal use only) */ size_t _capacity; + /** + * strings representing each tag. these remain owned by the TCTags instance and will be freed + * by tc_tags_free. + */ + struct TCString *const *items; } TCTags; /** @@ -155,8 +155,8 @@ extern "C" { #endif // __cplusplus /** - * Free a TCTags instance. The given pointer must not be NULL. The instance must not be used - * after this call. + * Free a TCTags instance. The instance, and all TCStrings it contains, must not be used after + * this call. */ void tc_tags_free(struct TCTags *tctags);