diff --git a/lib/src/replica.rs b/lib/src/replica.rs index 1d8014424..47220ba7f 100644 --- a/lib/src/replica.rs +++ b/lib/src/replica.rs @@ -11,10 +11,18 @@ pub struct TCReplica { error: Option>, } -/// Utility function to safely convert *mut TCReplica into &mut TCReplica -fn rep_ref(rep: *mut TCReplica) -> &'static mut TCReplica { - debug_assert!(!rep.is_null()); - unsafe { &mut *rep } +impl TCReplica { + /// Borrow a TCReplica from C as an argument. + /// + /// # Safety + /// + /// The pointer must not be NULL. It is the caller's responsibility to ensure that the + /// lifetime assigned to the reference and the lifetime of the TCReplica itself do not outlive + /// the lifetime promised by C. + pub(crate) unsafe fn from_arg_ref<'a>(tcstring: *mut TCReplica) -> &'a mut Self { + debug_assert!(!tcstring.is_null()); + &mut *tcstring + } } fn err_to_tcstring(e: impl std::string::ToString) -> TCString<'static> { @@ -26,7 +34,10 @@ fn wrap<'a, T, F>(rep: *mut TCReplica, f: F, err_value: T) -> T where F: FnOnce(&mut Replica) -> anyhow::Result, { - let rep: &'a mut TCReplica = rep_ref(rep); + // SAFETY: + // - rep is not null (promised by caller) + // - rep outlives 'a (promised by caller) + let rep: &'a mut TCReplica = unsafe { TCReplica::from_arg_ref(rep) }; rep.error = None; match f(&mut rep.inner) { Ok(v) => v, @@ -100,7 +111,7 @@ pub extern "C" fn tc_replica_get_task(rep: *mut TCReplica, uuid: TCUuid) -> *mut |rep| { let uuid: Uuid = uuid.into(); if let Some(task) = rep.get_task(uuid)? { - Ok(TCTask::as_ptr(task)) + Ok(TCTask::return_val(task)) } else { Ok(std::ptr::null_mut()) } @@ -128,7 +139,7 @@ pub extern "C" fn tc_replica_new_task( rep, |rep| { let task = rep.new_task(status.into(), description.as_str()?.to_string())?; - Ok(TCTask::as_ptr(task)) + Ok(TCTask::return_val(task)) }, std::ptr::null_mut(), ) @@ -147,7 +158,7 @@ pub extern "C" fn tc_replica_import_task_with_uuid( |rep| { let uuid: Uuid = uuid.into(); let task = rep.import_task_with_uuid(uuid)?; - Ok(TCTask::as_ptr(task)) + Ok(TCTask::return_val(task)) }, std::ptr::null_mut(), ) @@ -174,12 +185,15 @@ pub extern "C" fn tc_replica_undo<'a>(rep: *mut TCReplica) -> TCResult { ) } -/// Get the latest error for a replica, or NULL if the last operation succeeded. -/// Subsequent calls to this function will return NULL. The caller must free the +/// Get the latest error for a replica, or NULL if the last operation succeeded. Subsequent calls +/// to this function will return NULL. The rep pointer must not be NULL. The caller must free the /// returned string. #[no_mangle] pub extern "C" fn tc_replica_error<'a>(rep: *mut TCReplica) -> *mut TCString<'static> { - let rep: &'a mut TCReplica = rep_ref(rep); + // SAFETY: + // - rep is not null (promised by caller) + // - rep outlives 'a (promised by caller) + let rep: &'a mut TCReplica = unsafe { TCReplica::from_arg_ref(rep) }; if let Some(tcstring) = rep.error.take() { tcstring.return_val() } else { diff --git a/lib/src/task.rs b/lib/src/task.rs index ba129caa4..21b9d177a 100644 --- a/lib/src/task.rs +++ b/lib/src/task.rs @@ -10,21 +10,31 @@ pub struct TCTask { } impl TCTask { - pub(crate) fn as_ptr(task: Task) -> *mut TCTask { + /// Borrow a TCTask from C as an argument. + /// + /// # Safety + /// + /// The pointer must not be NULL. It is the caller's responsibility to ensure that the + /// lifetime assigned to the reference and the lifetime of the TCTask itself do not outlive + /// the lifetime promised by C. + pub(crate) unsafe fn from_arg_ref<'a>(tcstring: *const TCTask) -> &'a Self { + debug_assert!(!tcstring.is_null()); + &*tcstring + } + + /// Convert this to a return value for handing off to C. + pub(crate) fn return_val(task: Task) -> *mut TCTask { Box::into_raw(Box::new(TCTask { inner: task })) } } -/// Utility function to safely convert *const TCTask into &Task -fn task_ref(task: *const TCTask) -> &'static Task { - debug_assert!(!task.is_null()); - unsafe { &(*task).inner } -} - /// Get a task's UUID. #[no_mangle] pub extern "C" fn tc_task_get_uuid<'a>(task: *const TCTask) -> TCUuid { - let task: &'a Task = task_ref(task); + // SAFETY: + // - task is not NULL (promised by caller) + // - task's lifetime exceeds this function (promised by caller) + let task: &'a Task = &unsafe { TCTask::from_arg_ref(task) }.inner; let uuid = task.get_uuid(); uuid.into() } @@ -32,7 +42,10 @@ pub extern "C" fn tc_task_get_uuid<'a>(task: *const TCTask) -> TCUuid { /// Get a task's status. #[no_mangle] pub extern "C" fn tc_task_get_status<'a>(task: *const TCTask) -> TCStatus { - let task: &'a Task = task_ref(task); + // SAFETY: + // - task is not NULL (promised by caller) + // - task's lifetime exceeds this function (promised by caller) + let task: &'a Task = &unsafe { TCTask::from_arg_ref(task) }.inner; task.get_status().into() } @@ -45,7 +58,10 @@ pub extern "C" fn tc_task_get_status<'a>(task: *const TCTask) -> TCStatus { /// contains embedded NUL characters). #[no_mangle] pub extern "C" fn tc_task_get_description<'a>(task: *const TCTask) -> *mut TCString<'static> { - let task = task_ref(task); + // SAFETY: + // - task is not NULL (promised by caller) + // - task's lifetime exceeds this function (promised by caller) + let task: &'a Task = &unsafe { TCTask::from_arg_ref(task) }.inner; let descr: TCString = task.get_description().into(); descr.return_val() } @@ -64,9 +80,14 @@ pub extern "C" fn tc_task_get_description<'a>(task: *const TCTask) -> *mut TCStr * get_modified */ -/// Free a task. +/// Free a task. The given task must not be NULL. The task must not be used after this function +/// returns, and must not be freed more than once. #[no_mangle] pub extern "C" fn tc_task_free<'a>(task: *mut TCTask) { debug_assert!(!task.is_null()); + // SAFETY: + // - task is not NULL (promised by caller) + // - task's lifetime exceeds the drop (promised by caller) + // - task does not outlive this function (promised by caller) drop(unsafe { Box::from_raw(task) }); } diff --git a/lib/src/uuid.rs b/lib/src/uuid.rs index 535c03086..43fffe97b 100644 --- a/lib/src/uuid.rs +++ b/lib/src/uuid.rs @@ -43,6 +43,12 @@ pub static TC_UUID_STRING_BYTES: usize = ::uuid::adapter::Hyphenated::LENGTH; #[no_mangle] pub extern "C" fn tc_uuid_to_buf<'a>(uuid: TCUuid, buf: *mut libc::c_char) { debug_assert!(!buf.is_null()); + // SAFETY: + // - buf is valid for len bytes (by C convention) + // - (no alignment requirements for a byte slice) + // - content of buf will not be mutated during the lifetime of this slice (lifetime + // does not outlive this function call) + // - the length of the buffer is less than isize::MAX (promised by caller) let buf: &'a mut [u8] = unsafe { std::slice::from_raw_parts_mut(buf as *mut u8, ::uuid::adapter::Hyphenated::LENGTH) }; @@ -70,6 +76,9 @@ pub extern "C" fn tc_uuid_from_str<'a>(s: *mut TCString, uuid_out: *mut TCUuid) let s = unsafe { TCString::from_arg(s) }; if let Ok(s) = s.as_str() { if let Ok(u) = Uuid::parse_str(s) { + // SAFETY: + // - uuid_out is not NULL (promised by caller) + // - alignment is not required unsafe { *uuid_out = u.into() }; return true; } diff --git a/lib/taskchampion.h b/lib/taskchampion.h index 070b366e0..a42060ac6 100644 --- a/lib/taskchampion.h +++ b/lib/taskchampion.h @@ -124,8 +124,8 @@ struct TCTask *tc_replica_import_task_with_uuid(struct TCReplica *rep, struct TC enum TCResult tc_replica_undo(struct TCReplica *rep); /** - * Get the latest error for a replica, or NULL if the last operation succeeded. - * Subsequent calls to this function will return NULL. The caller must free the + * Get the latest error for a replica, or NULL if the last operation succeeded. Subsequent calls + * to this function will return NULL. The rep pointer must not be NULL. The caller must free the * returned string. */ struct TCString *tc_replica_error(struct TCReplica *rep); @@ -212,7 +212,8 @@ enum TCStatus tc_task_get_status(const struct TCTask *task); struct TCString *tc_task_get_description(const struct TCTask *task); /** - * Free a task. + * Free a task. The given task must not be NULL. The task must not be used after this function + * returns, and must not be freed more than once. */ void tc_task_free(struct TCTask *task);