From 471119dbdf4d6a3c0870c5309e12df13517f8e06 Mon Sep 17 00:00:00 2001 From: "Dustin J. Mitchell" Date: Thu, 17 Feb 2022 04:11:06 +0000 Subject: [PATCH] TCString as PassByValue --- Cargo.lock | 2 +- .../src/bindings_tests/replica.c | 76 +-- integration-tests/src/bindings_tests/string.c | 67 +-- integration-tests/src/bindings_tests/task.c | 140 ++--- integration-tests/src/bindings_tests/uuid.c | 6 +- lib/src/annotation.rs | 22 +- lib/src/kv.rs | 18 +- lib/src/lib.rs | 2 +- lib/src/replica.rs | 65 ++- lib/src/server.rs | 41 +- lib/src/string.rs | 498 +++++++++++++----- lib/src/task.rs | 219 ++++---- lib/src/traits.rs | 8 +- lib/src/uda.rs | 38 +- lib/src/util.rs | 15 +- lib/src/uuid.rs | 23 +- lib/taskchampion.h | 182 ++++--- 17 files changed, 853 insertions(+), 569 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 256a68b3e..fed2a869a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3798,7 +3798,7 @@ dependencies = [ [[package]] name = "xtask" -version = "0.1.0" +version = "0.4.1" dependencies = [ "anyhow", "cbindgen", diff --git a/integration-tests/src/bindings_tests/replica.c b/integration-tests/src/bindings_tests/replica.c index 282d2948b..cd02d3a30 100644 --- a/integration-tests/src/bindings_tests/replica.c +++ b/integration-tests/src/bindings_tests/replica.c @@ -8,7 +8,7 @@ static void test_replica_creation(void) { TCReplica *rep = tc_replica_new_in_memory(); TEST_ASSERT_NOT_NULL(rep); - TEST_ASSERT_NULL(tc_replica_error(rep)); + TEST_ASSERT_NULL(tc_replica_error(rep).ptr); tc_replica_free(rep); } @@ -16,28 +16,28 @@ static void test_replica_creation(void) { static void test_replica_creation_disk(void) { TCReplica *rep = tc_replica_new_on_disk(tc_string_borrow("test-db"), NULL); TEST_ASSERT_NOT_NULL(rep); - TEST_ASSERT_NULL(tc_replica_error(rep)); + TEST_ASSERT_NULL(tc_replica_error(rep).ptr); tc_replica_free(rep); } // undo on an empty in-memory TCReplica does nothing static void test_replica_undo_empty(void) { TCReplica *rep = tc_replica_new_in_memory(); - TEST_ASSERT_NULL(tc_replica_error(rep)); + TEST_ASSERT_NULL(tc_replica_error(rep).ptr); int undone; int rv = tc_replica_undo(rep, &undone); TEST_ASSERT_EQUAL(TC_RESULT_OK, rv); TEST_ASSERT_EQUAL(0, undone); - TEST_ASSERT_NULL(tc_replica_error(rep)); + TEST_ASSERT_NULL(tc_replica_error(rep).ptr); tc_replica_free(rep); } // adding an undo point succeeds static void test_replica_add_undo_point(void) { TCReplica *rep = tc_replica_new_in_memory(); - TEST_ASSERT_NULL(tc_replica_error(rep)); + TEST_ASSERT_NULL(tc_replica_error(rep).ptr); TEST_ASSERT_EQUAL(TC_RESULT_OK, tc_replica_add_undo_point(rep, true)); - TEST_ASSERT_NULL(tc_replica_error(rep)); + TEST_ASSERT_NULL(tc_replica_error(rep).ptr); tc_replica_free(rep); } @@ -48,10 +48,10 @@ static void test_replica_working_set(void) { TCUuid uuid, uuid1, uuid2, uuid3; TCReplica *rep = tc_replica_new_in_memory(); - TEST_ASSERT_NULL(tc_replica_error(rep)); + TEST_ASSERT_NULL(tc_replica_error(rep).ptr); TEST_ASSERT_EQUAL(TC_RESULT_OK, tc_replica_rebuild_working_set(rep, true)); - TEST_ASSERT_NULL(tc_replica_error(rep)); + TEST_ASSERT_NULL(tc_replica_error(rep).ptr); ws = tc_replica_working_set(rep); TEST_ASSERT_EQUAL(0, tc_working_set_len(ws)); @@ -62,7 +62,7 @@ static void test_replica_working_set(void) { uuid1 = tc_task_get_uuid(task1); TEST_ASSERT_EQUAL(TC_RESULT_OK, tc_replica_rebuild_working_set(rep, true)); - TEST_ASSERT_NULL(tc_replica_error(rep)); + TEST_ASSERT_NULL(tc_replica_error(rep).ptr); task2 = tc_replica_new_task(rep, TC_STATUS_PENDING, tc_string_borrow("task2")); TEST_ASSERT_NOT_NULL(task2); @@ -73,7 +73,7 @@ static void test_replica_working_set(void) { uuid3 = tc_task_get_uuid(task3); TEST_ASSERT_EQUAL(TC_RESULT_OK, tc_replica_rebuild_working_set(rep, false)); - TEST_ASSERT_NULL(tc_replica_error(rep)); + TEST_ASSERT_NULL(tc_replica_error(rep).ptr); // finish task2 to leave a "hole" tc_task_to_mut(task2, rep); @@ -81,7 +81,7 @@ static void test_replica_working_set(void) { tc_task_to_immut(task2); TEST_ASSERT_EQUAL(TC_RESULT_OK, tc_replica_rebuild_working_set(rep, false)); - TEST_ASSERT_NULL(tc_replica_error(rep)); + TEST_ASSERT_NULL(tc_replica_error(rep).ptr); tc_task_free(task1); tc_task_free(task2); @@ -115,17 +115,17 @@ static void test_replica_working_set(void) { // When tc_replica_undo is passed NULL for undone_out, it still succeeds static void test_replica_undo_empty_null_undone_out(void) { TCReplica *rep = tc_replica_new_in_memory(); - TEST_ASSERT_NULL(tc_replica_error(rep)); + TEST_ASSERT_NULL(tc_replica_error(rep).ptr); int rv = tc_replica_undo(rep, NULL); TEST_ASSERT_EQUAL(TC_RESULT_OK, rv); - TEST_ASSERT_NULL(tc_replica_error(rep)); + TEST_ASSERT_NULL(tc_replica_error(rep).ptr); tc_replica_free(rep); } // creating a task succeeds and the resulting task looks good static void test_replica_task_creation(void) { TCReplica *rep = tc_replica_new_in_memory(); - TEST_ASSERT_NULL(tc_replica_error(rep)); + TEST_ASSERT_NULL(tc_replica_error(rep).ptr); TCTask *task = tc_replica_new_task( rep, @@ -136,10 +136,10 @@ static void test_replica_task_creation(void) { TCUuid uuid = tc_task_get_uuid(task); TEST_ASSERT_EQUAL(TC_STATUS_PENDING, tc_task_get_status(task)); - TCString *desc = tc_task_get_description(task); - TEST_ASSERT_NOT_NULL(desc); - TEST_ASSERT_EQUAL_STRING("my task", tc_string_content(desc)); - tc_string_free(desc); + TCString desc = tc_task_get_description(task); + TEST_ASSERT_NOT_NULL(desc.ptr); + TEST_ASSERT_EQUAL_STRING("my task", tc_string_content(&desc)); + tc_string_free(&desc); tc_task_free(task); @@ -157,18 +157,18 @@ static void test_replica_task_creation(void) { // When tc_replica_undo is passed NULL for undone_out, it still succeeds static void test_replica_sync_local(void) { TCReplica *rep = tc_replica_new_in_memory(); - TEST_ASSERT_NULL(tc_replica_error(rep)); + TEST_ASSERT_NULL(tc_replica_error(rep).ptr); mkdir("test-sync-server", 0755); // ignore error, if dir already exists - TCString *err = NULL; + TCString err; TCServer *server = tc_server_new_local(tc_string_borrow("test-sync-server"), &err); TEST_ASSERT_NOT_NULL(server); - TEST_ASSERT_NULL(err); + TEST_ASSERT_NULL(err.ptr); int rv = tc_replica_sync(rep, server, false); TEST_ASSERT_EQUAL(TC_RESULT_OK, rv); - TEST_ASSERT_NULL(tc_replica_error(rep)); + TEST_ASSERT_NULL(tc_replica_error(rep).ptr); tc_server_free(server); tc_replica_free(rep); @@ -176,20 +176,20 @@ static void test_replica_sync_local(void) { // test error handling server = tc_server_new_local(tc_string_borrow("/no/such/directory"), &err); TEST_ASSERT_NULL(server); - TEST_ASSERT_NOT_NULL(err); - tc_string_free(err); + TEST_ASSERT_NOT_NULL(err.ptr); + tc_string_free(&err); } // When tc_replica_undo is passed NULL for undone_out, it still succeeds static void test_replica_remote_server(void) { - TCString *err = NULL; + TCString err; TCServer *server = tc_server_new_remote( tc_string_borrow("tc.freecinc.com"), tc_uuid_new_v4(), tc_string_borrow("\xf0\x28\x8c\x28"), // NOTE: not utf-8 &err); TEST_ASSERT_NOT_NULL(server); - TEST_ASSERT_NULL(err); + TEST_ASSERT_NULL(err.ptr); // can't actually do anything with this server! @@ -199,7 +199,7 @@ static void test_replica_remote_server(void) { // a replica with tasks in it returns an appropriate list of tasks and list of uuids static void test_replica_all_tasks(void) { TCReplica *rep = tc_replica_new_in_memory(); - TEST_ASSERT_NULL(tc_replica_error(rep)); + TEST_ASSERT_NULL(tc_replica_error(rep).ptr); TCTask *task1 = tc_replica_new_task( rep, @@ -225,13 +225,13 @@ static void test_replica_all_tasks(void) { bool seen1, seen2 = false; for (size_t i = 0; i < tasks.len; i++) { TCTask *task = tasks.items[i]; - TCString *descr = tc_task_get_description(task); - if (0 == strcmp(tc_string_content(descr), "task1")) { + TCString descr = tc_task_get_description(task); + if (0 == strcmp(tc_string_content(&descr), "task1")) { seen1 = true; - } else if (0 == strcmp(tc_string_content(descr), "task2")) { + } else if (0 == strcmp(tc_string_content(&descr), "task2")) { seen2 = true; } - tc_string_free(descr); + tc_string_free(&descr); } TEST_ASSERT_TRUE(seen1); TEST_ASSERT_TRUE(seen2); @@ -267,7 +267,7 @@ static void test_replica_all_tasks(void) { // importing a task succeeds and the resulting task looks good static void test_replica_task_import(void) { TCReplica *rep = tc_replica_new_in_memory(); - TEST_ASSERT_NULL(tc_replica_error(rep)); + TEST_ASSERT_NULL(tc_replica_error(rep).ptr); TCUuid uuid; TEST_ASSERT_EQUAL(TC_RESULT_OK, tc_uuid_from_str(tc_string_borrow("23cb25e0-5d1a-4932-8131-594ac6d3a843"), &uuid)); @@ -277,10 +277,10 @@ static void test_replica_task_import(void) { TEST_ASSERT_EQUAL_MEMORY(uuid.bytes, tc_task_get_uuid(task).bytes, sizeof(uuid.bytes)); TEST_ASSERT_EQUAL(TC_STATUS_PENDING, tc_task_get_status(task)); - TCString *desc = tc_task_get_description(task); - TEST_ASSERT_NOT_NULL(desc); - TEST_ASSERT_EQUAL_STRING("", tc_string_content(desc)); // default value - tc_string_free(desc); + TCString desc = tc_task_get_description(task); + TEST_ASSERT_NOT_NULL(desc.ptr); + TEST_ASSERT_EQUAL_STRING("", tc_string_content(&desc)); // default value + tc_string_free(&desc); tc_task_free(task); @@ -298,13 +298,13 @@ static void test_replica_task_import(void) { // importing a task succeeds and the resulting task looks good static void test_replica_get_task_not_found(void) { TCReplica *rep = tc_replica_new_in_memory(); - TEST_ASSERT_NULL(tc_replica_error(rep)); + TEST_ASSERT_NULL(tc_replica_error(rep).ptr); TCUuid uuid; TEST_ASSERT_EQUAL(TC_RESULT_OK, tc_uuid_from_str(tc_string_borrow("23cb25e0-5d1a-4932-8131-594ac6d3a843"), &uuid)); TCTask *task = tc_replica_get_task(rep, uuid); TEST_ASSERT_NULL(task); - TEST_ASSERT_NULL(tc_replica_error(rep)); + TEST_ASSERT_NULL(tc_replica_error(rep).ptr); } int replica_tests(void) { diff --git a/integration-tests/src/bindings_tests/string.c b/integration-tests/src/bindings_tests/string.c index 0eebec8f7..2bd2749c0 100644 --- a/integration-tests/src/bindings_tests/string.c +++ b/integration-tests/src/bindings_tests/string.c @@ -5,103 +5,110 @@ // creating strings does not crash static void test_string_creation(void) { - TCString *s = tc_string_borrow("abcdef"); - tc_string_free(s); + TCString s = tc_string_borrow("abcdef"); + tc_string_free(&s); + TEST_ASSERT_NULL(s.ptr); } // creating cloned strings does not crash static void test_string_cloning(void) { char *abcdef = strdup("abcdef"); - TCString *s = tc_string_clone(abcdef); - TEST_ASSERT_NOT_NULL(s); + TCString s = tc_string_clone(abcdef); + TEST_ASSERT_NOT_NULL(s.ptr); free(abcdef); - TEST_ASSERT_EQUAL_STRING("abcdef", tc_string_content(s)); - tc_string_free(s); + TEST_ASSERT_EQUAL_STRING("abcdef", tc_string_content(&s)); + tc_string_free(&s); + TEST_ASSERT_NULL(s.ptr); } // creating cloned strings with invalid utf-8 does not crash // ..but content is NULL and content_and_len returns the value static void test_string_cloning_invalid_utf8(void) { - TCString *s = tc_string_clone("\xf0\x28\x8c\x28"); - TEST_ASSERT_NOT_NULL(s); + TCString s = tc_string_clone("\xf0\x28\x8c\x28"); + TEST_ASSERT_NOT_NULL(s.ptr); // NOTE: this is not one of the cases where invalid UTF-8 results in NULL, // but that may change. size_t len; - const char *buf = tc_string_content_with_len(s, &len); + const char *buf = tc_string_content_with_len(&s, &len); TEST_ASSERT_NOT_NULL(buf); TEST_ASSERT_EQUAL(4, len); TEST_ASSERT_EQUAL_MEMORY("\xf0\x28\x8c\x28", buf, len); - tc_string_free(s); + tc_string_free(&s); + TEST_ASSERT_NULL(s.ptr); } // borrowed strings echo back their content static void test_string_borrowed_strings_echo(void) { - TCString *s = tc_string_borrow("abcdef"); - TEST_ASSERT_NOT_NULL(s); + TCString s = tc_string_borrow("abcdef"); + TEST_ASSERT_NOT_NULL(s.ptr); - TEST_ASSERT_EQUAL_STRING("abcdef", tc_string_content(s)); + TEST_ASSERT_EQUAL_STRING("abcdef", tc_string_content(&s)); size_t len; - const char *buf = tc_string_content_with_len(s, &len); + const char *buf = tc_string_content_with_len(&s, &len); TEST_ASSERT_NOT_NULL(buf); TEST_ASSERT_EQUAL(6, len); TEST_ASSERT_EQUAL_MEMORY("abcdef", buf, len); - tc_string_free(s); + tc_string_free(&s); + TEST_ASSERT_NULL(s.ptr); } // cloned strings echo back their content static void test_string_cloned_strings_echo(void) { char *orig = strdup("abcdef"); - TCString *s = tc_string_clone(orig); - TEST_ASSERT_NOT_NULL(s); + TCString s = tc_string_clone(orig); + TEST_ASSERT_NOT_NULL(s.ptr); free(orig); - TEST_ASSERT_EQUAL_STRING("abcdef", tc_string_content(s)); + TEST_ASSERT_EQUAL_STRING("abcdef", tc_string_content(&s)); size_t len; - const char *buf = tc_string_content_with_len(s, &len); + const char *buf = tc_string_content_with_len(&s, &len); TEST_ASSERT_NOT_NULL(buf); TEST_ASSERT_EQUAL(6, len); TEST_ASSERT_EQUAL_MEMORY("abcdef", buf, len); - tc_string_free(s); + tc_string_free(&s); + TEST_ASSERT_NULL(s.ptr); } // tc_clone_with_len can have NULs, and tc_string_content returns NULL for // strings containing embedded NULs static void test_string_content_null_for_embedded_nuls(void) { - TCString *s = tc_string_clone_with_len("ab\0de", 5); - TEST_ASSERT_NOT_NULL(s); + TCString s = tc_string_clone_with_len("ab\0de", 5); + TEST_ASSERT_NOT_NULL(s.ptr); - TEST_ASSERT_NULL(tc_string_content(s)); + TEST_ASSERT_NULL(tc_string_content(&s)); size_t len; - const char *buf = tc_string_content_with_len(s, &len); + const char *buf = tc_string_content_with_len(&s, &len); TEST_ASSERT_NOT_NULL(buf); TEST_ASSERT_EQUAL(5, len); TEST_ASSERT_EQUAL_MEMORY("ab\0de", buf, len); - tc_string_free(s); + tc_string_free(&s); + TEST_ASSERT_NULL(s.ptr); } // tc_string_clone_with_len will accept invalid utf-8, but then tc_string_content // returns NULL. static void test_string_clone_with_len_invalid_utf8(void) { - TCString *s = tc_string_clone_with_len("\xf0\x28\x8c\x28", 4); - TEST_ASSERT_NOT_NULL(s); + TCString s = tc_string_clone_with_len("\xf0\x28\x8c\x28", 4); + TEST_ASSERT_NOT_NULL(s.ptr); - TEST_ASSERT_NULL(tc_string_content(s)); + TEST_ASSERT_NULL(tc_string_content(&s)); size_t len; - const char *buf = tc_string_content_with_len(s, &len); + const char *buf = tc_string_content_with_len(&s, &len); TEST_ASSERT_NOT_NULL(buf); TEST_ASSERT_EQUAL(4, len); TEST_ASSERT_EQUAL_MEMORY("\xf0\x28\x8c\x28", buf, len); - tc_string_free(s); + tc_string_free(&s); + TEST_ASSERT_NULL(s.ptr); } int string_tests(void) { diff --git a/integration-tests/src/bindings_tests/task.c b/integration-tests/src/bindings_tests/task.c index b2412f3c3..f3ff29ae0 100644 --- a/integration-tests/src/bindings_tests/task.c +++ b/integration-tests/src/bindings_tests/task.c @@ -6,7 +6,7 @@ // creating a task succeeds and the resulting task looks good static void test_task_creation(void) { TCReplica *rep = tc_replica_new_in_memory(); - TEST_ASSERT_NULL(tc_replica_error(rep)); + TEST_ASSERT_NULL(tc_replica_error(rep).ptr); TCTask *task = tc_replica_new_task( rep, @@ -16,10 +16,10 @@ static void test_task_creation(void) { TEST_ASSERT_EQUAL(TC_STATUS_PENDING, tc_task_get_status(task)); - TCString *desc = tc_task_get_description(task); - TEST_ASSERT_NOT_NULL(desc); - TEST_ASSERT_EQUAL_STRING("my task", tc_string_content(desc)); - tc_string_free(desc); + TCString desc = tc_task_get_description(task); + TEST_ASSERT_NOT_NULL(desc.ptr); + TEST_ASSERT_EQUAL_STRING("my task", tc_string_content(&desc)); + tc_string_free(&desc); tc_task_free(task); @@ -29,7 +29,7 @@ static void test_task_creation(void) { // freeing a mutable task works, marking it immutable static void test_task_free_mutable_task(void) { TCReplica *rep = tc_replica_new_in_memory(); - TEST_ASSERT_NULL(tc_replica_error(rep)); + TEST_ASSERT_NULL(tc_replica_error(rep).ptr); TCTask *task = tc_replica_new_task( rep, @@ -57,7 +57,7 @@ static void test_task_free_mutable_task(void) { // updating status on a task works static void test_task_get_set_status(void) { TCReplica *rep = tc_replica_new_in_memory(); - TEST_ASSERT_NULL(tc_replica_error(rep)); + TEST_ASSERT_NULL(tc_replica_error(rep).ptr); TCTask *task = tc_replica_new_task( rep, @@ -81,7 +81,7 @@ static void test_task_get_set_status(void) { // updating description on a task works static void test_task_get_set_description(void) { TCReplica *rep = tc_replica_new_in_memory(); - TEST_ASSERT_NULL(tc_replica_error(rep)); + TEST_ASSERT_NULL(tc_replica_error(rep).ptr); TCTask *task = tc_replica_new_task( rep, @@ -89,22 +89,22 @@ static void test_task_get_set_description(void) { tc_string_borrow("my task")); TEST_ASSERT_NOT_NULL(task); - TCString *desc; + TCString desc; tc_task_to_mut(task, rep); TEST_ASSERT_EQUAL(TC_RESULT_OK, tc_task_set_description(task, tc_string_borrow("updated"))); - TEST_ASSERT_TRUE(desc = tc_task_get_description(task)); - TEST_ASSERT_NOT_NULL(desc); - TEST_ASSERT_EQUAL_STRING("updated", tc_string_content(desc)); - tc_string_free(desc); + desc = tc_task_get_description(task); + TEST_ASSERT_NOT_NULL(desc.ptr); + TEST_ASSERT_EQUAL_STRING("updated", tc_string_content(&desc)); + tc_string_free(&desc); tc_task_to_immut(task); desc = tc_task_get_description(task); - TEST_ASSERT_NOT_NULL(desc); - TEST_ASSERT_EQUAL_STRING("updated", tc_string_content(desc)); - tc_string_free(desc); + TEST_ASSERT_NOT_NULL(desc.ptr); + TEST_ASSERT_EQUAL_STRING("updated", tc_string_content(&desc)); + tc_string_free(&desc); tc_task_free(task); @@ -114,7 +114,7 @@ static void test_task_get_set_description(void) { // updating entry on a task works static void test_task_get_set_entry(void) { TCReplica *rep = tc_replica_new_in_memory(); - TEST_ASSERT_NULL(tc_replica_error(rep)); + TEST_ASSERT_NULL(tc_replica_error(rep).ptr); TCTask *task = tc_replica_new_task( rep, @@ -141,7 +141,7 @@ static void test_task_get_set_entry(void) { // updating wait on a task works static void test_task_get_set_wait_and_is_waiting(void) { TCReplica *rep = tc_replica_new_in_memory(); - TEST_ASSERT_NULL(tc_replica_error(rep)); + TEST_ASSERT_NULL(tc_replica_error(rep).ptr); TCTask *task = tc_replica_new_task( rep, @@ -175,7 +175,7 @@ static void test_task_get_set_wait_and_is_waiting(void) { // updating modified on a task works static void test_task_get_set_modified(void) { TCReplica *rep = tc_replica_new_in_memory(); - TEST_ASSERT_NULL(tc_replica_error(rep)); + TEST_ASSERT_NULL(tc_replica_error(rep).ptr); TCTask *task = tc_replica_new_task( rep, @@ -202,7 +202,7 @@ static void test_task_get_set_modified(void) { // starting and stopping a task works, as seen by tc_task_is_active static void test_task_start_stop_is_active(void) { TCReplica *rep = tc_replica_new_in_memory(); - TEST_ASSERT_NULL(tc_replica_error(rep)); + TEST_ASSERT_NULL(tc_replica_error(rep).ptr); TCTask *task = tc_replica_new_task( rep, @@ -227,7 +227,7 @@ static void test_task_start_stop_is_active(void) { // tc_task_done and delete work and set the status static void test_task_done_and_delete(void) { TCReplica *rep = tc_replica_new_in_memory(); - TEST_ASSERT_NULL(tc_replica_error(rep)); + TEST_ASSERT_NULL(tc_replica_error(rep).ptr); TCTask *task = tc_replica_new_task( rep, @@ -250,7 +250,7 @@ static void test_task_done_and_delete(void) { // adding and removing tags to a task works, and invalid tags are rejected static void test_task_add_remove_has_tag(void) { TCReplica *rep = tc_replica_new_in_memory(); - TEST_ASSERT_NULL(tc_replica_error(rep)); + TEST_ASSERT_NULL(tc_replica_error(rep).ptr); TCTask *task = tc_replica_new_task( rep, @@ -263,33 +263,33 @@ static void test_task_add_remove_has_tag(void) { TEST_ASSERT_FALSE(tc_task_has_tag(task, tc_string_borrow("next"))); TEST_ASSERT_EQUAL(TC_RESULT_OK, tc_task_add_tag(task, tc_string_borrow("next"))); - TEST_ASSERT_NULL(tc_task_error(task)); + TEST_ASSERT_NULL(tc_task_error(task).ptr); TEST_ASSERT_TRUE(tc_task_has_tag(task, tc_string_borrow("next"))); // invalid - synthetic tag TEST_ASSERT_EQUAL(TC_RESULT_ERROR, tc_task_add_tag(task, tc_string_borrow("PENDING"))); - TCString *err = tc_task_error(task); - TEST_ASSERT_NOT_NULL(err); - tc_string_free(err); + TCString err = tc_task_error(task); + TEST_ASSERT_NOT_NULL(err.ptr); + tc_string_free(&err); // invald - not a valid tag string TEST_ASSERT_EQUAL(TC_RESULT_ERROR, tc_task_add_tag(task, tc_string_borrow("my tag"))); err = tc_task_error(task); - TEST_ASSERT_NOT_NULL(err); - tc_string_free(err); + TEST_ASSERT_NOT_NULL(err.ptr); + tc_string_free(&err); // invald - not utf-8 TEST_ASSERT_EQUAL(TC_RESULT_ERROR, tc_task_add_tag(task, tc_string_borrow("\xf0\x28\x8c\x28"))); err = tc_task_error(task); - TEST_ASSERT_NOT_NULL(err); - tc_string_free(err); + TEST_ASSERT_NOT_NULL(err.ptr); + tc_string_free(&err); TEST_ASSERT_TRUE(tc_task_has_tag(task, tc_string_borrow("next"))); // remove the tag TEST_ASSERT_EQUAL(TC_RESULT_OK, tc_task_remove_tag(task, tc_string_borrow("next"))); - TEST_ASSERT_NULL(tc_task_error(task)); + TEST_ASSERT_NULL(tc_task_error(task).ptr); TEST_ASSERT_FALSE(tc_task_has_tag(task, tc_string_borrow("next"))); @@ -300,7 +300,7 @@ static void test_task_add_remove_has_tag(void) { // get_tags returns the list of tags static void test_task_get_tags(void) { TCReplica *rep = tc_replica_new_in_memory(); - TEST_ASSERT_NULL(tc_replica_error(rep)); + TEST_ASSERT_NULL(tc_replica_error(rep).ptr); TCTask *task = tc_replica_new_task( rep, @@ -316,10 +316,10 @@ static void test_task_get_tags(void) { int found_pending = false, found_next = false; for (size_t i = 0; i < tags.len; i++) { - if (strcmp("PENDING", tc_string_content(tags.items[i])) == 0) { + if (strcmp("PENDING", tc_string_content(&tags.items[i])) == 0) { found_pending = true; } - if (strcmp("next", tc_string_content(tags.items[i])) == 0) { + if (strcmp("next", tc_string_content(&tags.items[i])) == 0) { found_next = true; } } @@ -336,7 +336,7 @@ static void test_task_get_tags(void) { // annotation manipulation (add, remove, list, free) static void test_task_annotations(void) { TCReplica *rep = tc_replica_new_in_memory(); - TEST_ASSERT_NULL(tc_replica_error(rep)); + TEST_ASSERT_NULL(tc_replica_error(rep).ptr); TCTask *task = tc_replica_new_task( rep, @@ -356,22 +356,22 @@ static void test_task_annotations(void) { ann.entry = 1644623411; ann.description = tc_string_borrow("ann1"); TEST_ASSERT_EQUAL(TC_RESULT_OK, tc_task_add_annotation(task, &ann)); - TEST_ASSERT_NULL(ann.description); + TEST_ASSERT_NULL(ann.description.ptr); ann.entry = 1644623422; ann.description = tc_string_borrow("ann2"); TEST_ASSERT_EQUAL(TC_RESULT_OK, tc_task_add_annotation(task, &ann)); - TEST_ASSERT_NULL(ann.description); + TEST_ASSERT_NULL(ann.description.ptr); anns = tc_task_get_annotations(task); int found1 = false, found2 = false; for (size_t i = 0; i < anns.len; i++) { - if (0 == strcmp("ann1", tc_string_content(anns.items[i].description))) { + if (0 == strcmp("ann1", tc_string_content(&anns.items[i].description))) { TEST_ASSERT_EQUAL(anns.items[i].entry, 1644623411); found1 = true; } - if (0 == strcmp("ann2", tc_string_content(anns.items[i].description))) { + if (0 == strcmp("ann2", tc_string_content(&anns.items[i].description))) { TEST_ASSERT_EQUAL(anns.items[i].entry, 1644623422); found2 = true; } @@ -389,7 +389,7 @@ static void test_task_annotations(void) { // UDA manipulation (add, remove, list, free) static void test_task_udas(void) { TCReplica *rep = tc_replica_new_in_memory(); - TEST_ASSERT_NULL(tc_replica_error(rep)); + TEST_ASSERT_NULL(tc_replica_error(rep).ptr); TCTask *task = tc_replica_new_task( rep, @@ -399,11 +399,11 @@ static void test_task_udas(void) { tc_task_to_mut(task, rep); - TCString *value; + TCString value; TCUdaList udas; - TEST_ASSERT_NULL(tc_task_get_uda(task, tc_string_borrow("ns"), tc_string_borrow("u1"))); - TEST_ASSERT_NULL(tc_task_get_legacy_uda(task, tc_string_borrow("leg1"))); + TEST_ASSERT_NULL(tc_task_get_uda(task, tc_string_borrow("ns"), tc_string_borrow("u1")).ptr); + TEST_ASSERT_NULL(tc_task_get_legacy_uda(task, tc_string_borrow("leg1")).ptr); udas = tc_task_get_udas(task); TEST_ASSERT_NOT_NULL(udas.items); @@ -422,25 +422,25 @@ static void test_task_udas(void) { tc_string_borrow("vvv"))); value = tc_task_get_uda(task, tc_string_borrow("ns"), tc_string_borrow("u1")); - TEST_ASSERT_NOT_NULL(value); - TEST_ASSERT_EQUAL_STRING("vvv", tc_string_content(value)); - tc_string_free(value); - TEST_ASSERT_NULL(tc_task_get_legacy_uda(task, tc_string_borrow("leg1"))); + TEST_ASSERT_NOT_NULL(value.ptr); + TEST_ASSERT_EQUAL_STRING("vvv", tc_string_content(&value)); + tc_string_free(&value); + TEST_ASSERT_NULL(tc_task_get_legacy_uda(task, tc_string_borrow("leg1")).ptr); udas = tc_task_get_udas(task); TEST_ASSERT_NOT_NULL(udas.items); TEST_ASSERT_EQUAL(1, udas.len); - TEST_ASSERT_EQUAL_STRING("ns", tc_string_content(udas.items[0].ns)); - TEST_ASSERT_EQUAL_STRING("u1", tc_string_content(udas.items[0].key)); - TEST_ASSERT_EQUAL_STRING("vvv", tc_string_content(udas.items[0].value)); + TEST_ASSERT_EQUAL_STRING("ns", tc_string_content(&udas.items[0].ns)); + TEST_ASSERT_EQUAL_STRING("u1", tc_string_content(&udas.items[0].key)); + TEST_ASSERT_EQUAL_STRING("vvv", tc_string_content(&udas.items[0].value)); tc_uda_list_free(&udas); udas = tc_task_get_legacy_udas(task); TEST_ASSERT_NOT_NULL(udas.items); TEST_ASSERT_EQUAL(1, udas.len); - TEST_ASSERT_NULL(udas.items[0].ns); - TEST_ASSERT_EQUAL_STRING("ns.u1", tc_string_content(udas.items[0].key)); - TEST_ASSERT_EQUAL_STRING("vvv", tc_string_content(udas.items[0].value)); + TEST_ASSERT_NULL(udas.items[0].ns.ptr); + TEST_ASSERT_EQUAL_STRING("ns.u1", tc_string_content(&udas.items[0].key)); + TEST_ASSERT_EQUAL_STRING("vvv", tc_string_content(&udas.items[0].value)); tc_uda_list_free(&udas); TEST_ASSERT_EQUAL(TC_RESULT_OK, @@ -449,14 +449,14 @@ static void test_task_udas(void) { tc_string_borrow("legv"))); value = tc_task_get_uda(task, tc_string_borrow("ns"), tc_string_borrow("u1")); - TEST_ASSERT_NOT_NULL(value); - TEST_ASSERT_EQUAL_STRING("vvv", tc_string_content(value)); - tc_string_free(value); + TEST_ASSERT_NOT_NULL(value.ptr); + TEST_ASSERT_EQUAL_STRING("vvv", tc_string_content(&value)); + tc_string_free(&value); value = tc_task_get_legacy_uda(task, tc_string_borrow("leg1")); - TEST_ASSERT_NOT_NULL(value); - TEST_ASSERT_EQUAL_STRING("legv", tc_string_content(value)); - tc_string_free(value); + TEST_ASSERT_NOT_NULL(value.ptr); + TEST_ASSERT_EQUAL_STRING("legv", tc_string_content(&value)); + tc_string_free(&value); udas = tc_task_get_udas(task); TEST_ASSERT_NOT_NULL(udas.items); @@ -473,7 +473,7 @@ static void test_task_udas(void) { tc_string_borrow("ns"), tc_string_borrow("u1"))); - TEST_ASSERT_NULL(tc_task_get_uda(task, tc_string_borrow("ns"), tc_string_borrow("u1"))); + TEST_ASSERT_NULL(tc_task_get_uda(task, tc_string_borrow("ns"), tc_string_borrow("u1")).ptr); TEST_ASSERT_EQUAL(TC_RESULT_OK, tc_task_remove_uda(task, @@ -483,24 +483,24 @@ static void test_task_udas(void) { udas = tc_task_get_udas(task); TEST_ASSERT_NOT_NULL(udas.items); TEST_ASSERT_EQUAL(1, udas.len); - TEST_ASSERT_EQUAL_STRING("", tc_string_content(udas.items[0].ns)); - TEST_ASSERT_EQUAL_STRING("leg1", tc_string_content(udas.items[0].key)); - TEST_ASSERT_EQUAL_STRING("legv", tc_string_content(udas.items[0].value)); + TEST_ASSERT_EQUAL_STRING("", tc_string_content(&udas.items[0].ns)); + TEST_ASSERT_EQUAL_STRING("leg1", tc_string_content(&udas.items[0].key)); + TEST_ASSERT_EQUAL_STRING("legv", tc_string_content(&udas.items[0].value)); tc_uda_list_free(&udas); udas = tc_task_get_legacy_udas(task); TEST_ASSERT_NOT_NULL(udas.items); TEST_ASSERT_EQUAL(1, udas.len); - TEST_ASSERT_NULL(udas.items[0].ns); - TEST_ASSERT_EQUAL_STRING("leg1", tc_string_content(udas.items[0].key)); - TEST_ASSERT_EQUAL_STRING("legv", tc_string_content(udas.items[0].value)); + TEST_ASSERT_NULL(udas.items[0].ns.ptr); + TEST_ASSERT_EQUAL_STRING("leg1", tc_string_content(&udas.items[0].key)); + TEST_ASSERT_EQUAL_STRING("legv", tc_string_content(&udas.items[0].value)); tc_uda_list_free(&udas); TEST_ASSERT_EQUAL(TC_RESULT_OK, tc_task_remove_legacy_uda(task, tc_string_borrow("leg1"))); - TEST_ASSERT_NULL(tc_task_get_legacy_uda(task, tc_string_borrow("leg1"))); + TEST_ASSERT_NULL(tc_task_get_legacy_uda(task, tc_string_borrow("leg1")).ptr); TEST_ASSERT_EQUAL(TC_RESULT_OK, tc_task_remove_legacy_uda(task, @@ -523,8 +523,8 @@ static void test_task_udas(void) { static void tckvlist_assert_key(TCKVList *list, char *key, char *value) { TEST_ASSERT_NOT_NULL(list); for (size_t i = 0; i < list->len; i++) { - if (0 == strcmp(tc_string_content(list->items[i].key), key)) { - TEST_ASSERT_EQUAL_STRING(value, tc_string_content(list->items[i].value)); + if (0 == strcmp(tc_string_content(&list->items[i].key), key)) { + TEST_ASSERT_EQUAL_STRING(value, tc_string_content(&list->items[i].value)); return; } } @@ -534,7 +534,7 @@ static void tckvlist_assert_key(TCKVList *list, char *key, char *value) { // get_tags returns the list of tags static void test_task_taskmap(void) { TCReplica *rep = tc_replica_new_in_memory(); - TEST_ASSERT_NULL(tc_replica_error(rep)); + TEST_ASSERT_NULL(tc_replica_error(rep).ptr); TCTask *task = tc_replica_new_task(rep, TC_STATUS_PENDING, tc_string_borrow("my task")); TEST_ASSERT_NOT_NULL(task); diff --git a/integration-tests/src/bindings_tests/uuid.c b/integration-tests/src/bindings_tests/uuid.c index 0c9b72be1..83b02482d 100644 --- a/integration-tests/src/bindings_tests/uuid.c +++ b/integration-tests/src/bindings_tests/uuid.c @@ -22,11 +22,11 @@ static void test_uuid_to_buf(void) { // converting UUIDs to a buf works static void test_uuid_to_str(void) { TCUuid u = tc_uuid_nil(); - TCString *s = tc_uuid_to_str(u); + TCString s = tc_uuid_to_str(u); TEST_ASSERT_EQUAL_STRING( "00000000-0000-0000-0000-000000000000", - tc_string_content(s)); - tc_string_free(s); + tc_string_content(&s)); + tc_string_free(&s); } // converting valid UUIDs from string works diff --git a/lib/src/annotation.rs b/lib/src/annotation.rs index 7142eff62..774f94478 100644 --- a/lib/src/annotation.rs +++ b/lib/src/annotation.rs @@ -22,24 +22,24 @@ pub struct TCAnnotation { /// Time the annotation was made. Must be nonzero. pub entry: libc::time_t, /// Content of the annotation. Must not be NULL. - pub description: *mut TCString<'static>, + pub description: TCString, } impl PassByValue for TCAnnotation { // NOTE: we cannot use `RustType = Annotation` here because conversion of the - // TCString to a String can fail. - type RustType = (DateTime, TCString<'static>); + // Rust to a String can fail. + type RustType = (DateTime, RustString<'static>); - unsafe fn from_ctype(self) -> Self::RustType { + unsafe fn from_ctype(mut self) -> Self::RustType { // SAFETY: // - any time_t value is valid // - time_t is copy, so ownership is not important let entry = unsafe { libc::time_t::val_from_arg(self.entry) }.unwrap(); // SAFETY: - // - self.description is not NULL (field docstring) - // - self.description came from return_ptr in as_ctype + // - self.description is valid (came from return_val in as_ctype) // - self is owned, so we can take ownership of this TCString - let description = unsafe { TCString::take_from_ptr_arg(self.description) }; + let description = + unsafe { TCString::take_val_from_arg(&mut self.description, TCString::default()) }; (entry, description) } @@ -48,7 +48,7 @@ impl PassByValue for TCAnnotation { entry: libc::time_t::as_ctype(Some(entry)), // SAFETY: // - ownership of the TCString tied to ownership of Self - description: unsafe { description.return_ptr() }, + description: unsafe { TCString::return_val(description) }, } } } @@ -57,7 +57,7 @@ impl Default for TCAnnotation { fn default() -> Self { TCAnnotation { entry: 0 as libc::time_t, - description: std::ptr::null_mut(), + description: TCString::default(), } } } @@ -125,7 +125,7 @@ mod test { #[test] fn empty_list_has_non_null_pointer() { - let tcanns = TCAnnotationList::return_val(Vec::new()); + let tcanns = unsafe { TCAnnotationList::return_val(Vec::new()) }; assert!(!tcanns.items.is_null()); assert_eq!(tcanns.len, 0); assert_eq!(tcanns._capacity, 0); @@ -133,7 +133,7 @@ mod test { #[test] fn free_sets_null_pointer() { - let mut tcanns = TCAnnotationList::return_val(Vec::new()); + let mut tcanns = unsafe { TCAnnotationList::return_val(Vec::new()) }; // SAFETY: testing expected behavior unsafe { tc_annotation_list_free(&mut tcanns) }; assert!(tcanns.items.is_null()); diff --git a/lib/src/kv.rs b/lib/src/kv.rs index 1a6f61c00..716883c75 100644 --- a/lib/src/kv.rs +++ b/lib/src/kv.rs @@ -7,21 +7,21 @@ use crate::types::*; /// will be freed when it is freed with tc_kv_list_free. #[repr(C)] pub struct TCKV { - pub key: *mut TCString<'static>, - pub value: *mut TCString<'static>, + pub key: TCString, + pub value: TCString, } impl PassByValue for TCKV { - type RustType = (TCString<'static>, TCString<'static>); + type RustType = (RustString<'static>, RustString<'static>); unsafe fn from_ctype(self) -> Self::RustType { // SAFETY: // - self.key is not NULL (field docstring) // - self.key came from return_ptr in as_ctype // - self is owned, so we can take ownership of this TCString - let key = unsafe { TCString::take_from_ptr_arg(self.key) }; + let key = unsafe { TCString::val_from_arg(self.key) }; // SAFETY: (same) - let value = unsafe { TCString::take_from_ptr_arg(self.value) }; + let value = unsafe { TCString::val_from_arg(self.value) }; (key, value) } @@ -29,10 +29,10 @@ impl PassByValue for TCKV { TCKV { // SAFETY: // - ownership of the TCString tied to ownership of Self - key: unsafe { key.return_ptr() }, + key: unsafe { TCString::return_val(key) }, // SAFETY: // - ownership of the TCString tied to ownership of Self - value: unsafe { value.return_ptr() }, + value: unsafe { TCString::return_val(value) }, } } } @@ -88,7 +88,7 @@ mod test { #[test] fn empty_list_has_non_null_pointer() { - let tckvs = TCKVList::return_val(Vec::new()); + let tckvs = unsafe { TCKVList::return_val(Vec::new()) }; assert!(!tckvs.items.is_null()); assert_eq!(tckvs.len, 0); assert_eq!(tckvs._capacity, 0); @@ -96,7 +96,7 @@ mod test { #[test] fn free_sets_null_pointer() { - let mut tckvs = TCKVList::return_val(Vec::new()); + let mut tckvs = unsafe { TCKVList::return_val(Vec::new()) }; // SAFETY: testing expected behavior unsafe { tc_kv_list_free(&mut tckvs) }; assert!(tckvs.items.is_null()); diff --git a/lib/src/lib.rs b/lib/src/lib.rs index 614d57f7b..946342fb3 100644 --- a/lib/src/lib.rs +++ b/lib/src/lib.rs @@ -29,7 +29,7 @@ pub(crate) mod types { pub(crate) use crate::result::TCResult; pub(crate) use crate::server::TCServer; pub(crate) use crate::status::TCStatus; - pub(crate) use crate::string::{TCString, TCStringList}; + pub(crate) use crate::string::{RustString, TCString, TCStringList}; pub(crate) use crate::task::{TCTask, TCTaskList}; pub(crate) use crate::uda::{TCUda, TCUdaList, Uda}; pub(crate) use crate::uuid::{TCUuid, TCUuidList}; diff --git a/lib/src/replica.rs b/lib/src/replica.rs index a35260b51..a1d488ede 100644 --- a/lib/src/replica.rs +++ b/lib/src/replica.rs @@ -1,6 +1,6 @@ use crate::traits::*; use crate::types::*; -use crate::util::err_to_tcstring; +use crate::util::err_to_ruststring; use std::ptr::NonNull; use taskchampion::{Replica, StorageConfig}; @@ -34,7 +34,7 @@ pub struct TCReplica { mut_borrowed: bool, /// The error from the most recent operation, if any - error: Option>, + error: Option>, } impl PassByPointer for TCReplica {} @@ -88,7 +88,7 @@ where match f(&mut rep.inner) { Ok(v) => v, Err(e) => { - rep.error = Some(err_to_tcstring(e)); + rep.error = Some(err_to_ruststring(e)); err_value } } @@ -111,14 +111,20 @@ pub unsafe extern "C" fn tc_replica_new_in_memory() -> *mut TCReplica { /// must free this string. #[no_mangle] pub unsafe extern "C" fn tc_replica_new_on_disk( - path: *mut TCString, - error_out: *mut *mut TCString, + path: TCString, + error_out: *mut TCString, ) -> *mut TCReplica { + if !error_out.is_null() { + // SAFETY: + // - error_out is not NULL (just checked) + // - properly aligned and valid (promised by caller) + unsafe { *error_out = TCString::default() }; + } + // SAFETY: - // - path is not NULL (promised by caller) - // - path is return from a tc_string_.. so is valid + // - path is valid (promised by caller) // - caller will not use path after this call (convention) - let path = unsafe { TCString::take_from_ptr_arg(path) }; + let path = unsafe { TCString::val_from_arg(path) }; let storage_res = StorageConfig::OnDisk { taskdb_dir: path.to_path_buf(), } @@ -130,11 +136,9 @@ pub unsafe extern "C" fn tc_replica_new_on_disk( if !error_out.is_null() { unsafe { // SAFETY: - // - return_ptr: caller promises to free this string - // - *error_out: - // - error_out is not NULL (checked) - // - error_out points to a valid place for a pointer (caller promises) - *error_out = err_to_tcstring(e).return_ptr(); + // - error_out is not NULL (just checked) + // - properly aligned and valid (promised by caller) + TCString::val_to_arg_out(err_to_ruststring(e), error_out); } } return std::ptr::null_mut(); @@ -169,7 +173,9 @@ pub unsafe extern "C" fn tc_replica_all_tasks(rep: *mut TCReplica) -> TCTaskList .expect("TCTask::return_ptr returned NULL") }) .collect(); - Ok(TCTaskList::return_val(tasks)) + // SAFETY: + // - value is not allocated and need not be freed + Ok(unsafe { TCTaskList::return_val(tasks) }) }, TCTaskList::null_value(), ) @@ -178,6 +184,8 @@ pub unsafe extern "C" fn tc_replica_all_tasks(rep: *mut TCReplica) -> TCTaskList /// Get a list of all uuids for tasks in the replica. /// /// Returns a TCUuidList with a NULL items field on error. +/// +/// The caller must free the UUID list with `tc_uuid_list_free`. #[no_mangle] pub unsafe extern "C" fn tc_replica_all_task_uuids(rep: *mut TCReplica) -> TCUuidList { wrap( @@ -186,9 +194,13 @@ pub unsafe extern "C" fn tc_replica_all_task_uuids(rep: *mut TCReplica) -> TCUui let uuids: Vec<_> = rep .all_task_uuids()? .drain(..) - .map(TCUuid::return_val) + // SAFETY: + // - value is not allocated and need not be freed + .map(|uuid| unsafe { TCUuid::return_val(uuid) }) .collect(); - Ok(TCUuidList::return_val(uuids)) + // SAFETY: + // - value will be freed (promised by caller) + Ok(unsafe { TCUuidList::return_val(uuids) }) }, TCUuidList::null_value(), ) @@ -244,13 +256,12 @@ pub unsafe extern "C" fn tc_replica_get_task(rep: *mut TCReplica, tcuuid: TCUuid pub unsafe extern "C" fn tc_replica_new_task( rep: *mut TCReplica, status: TCStatus, - description: *mut TCString, + description: TCString, ) -> *mut TCTask { // SAFETY: - // - description is not NULL (promised by caller) - // - description is return from a tc_string_.. so is valid + // - description is valid (promised by caller) // - caller will not use description after this call (convention) - let description = unsafe { TCString::take_from_ptr_arg(description) }; + let mut description = unsafe { TCString::val_from_arg(description) }; wrap( rep, |rep| { @@ -369,23 +380,23 @@ pub unsafe extern "C" fn tc_replica_rebuild_working_set( ) } -/// 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. +/// Get the latest error for a replica, or a string with NULL ptr if no error exists. 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 unsafe extern "C" fn tc_replica_error(rep: *mut TCReplica) -> *mut TCString<'static> { +pub unsafe extern "C" fn tc_replica_error(rep: *mut TCReplica) -> TCString { // SAFETY: // - rep is not NULL (promised by caller) // - *rep is a valid TCReplica (promised by caller) // - rep is valid for the duration of this function // - rep is not modified by anything else (not threadsafe) let rep: &mut TCReplica = unsafe { TCReplica::from_ptr_arg_ref_mut(rep) }; - if let Some(tcstring) = rep.error.take() { + if let Some(rstring) = rep.error.take() { // SAFETY: // - caller promises to free this string - unsafe { tcstring.return_ptr() } + unsafe { TCString::return_val(rstring) } } else { - std::ptr::null_mut() + TCString::default() } } diff --git a/lib/src/server.rs b/lib/src/server.rs index 74feff34f..711fcacf5 100644 --- a/lib/src/server.rs +++ b/lib/src/server.rs @@ -1,6 +1,6 @@ use crate::traits::*; use crate::types::*; -use crate::util::err_to_tcstring; +use crate::util::err_to_ruststring; use taskchampion::{Server, ServerConfig}; /// TCServer represents an interface to a sync server. Aside from new and free, a server @@ -26,20 +26,26 @@ impl AsMut> for TCServer { } /// Utility function to allow using `?` notation to return an error value. -fn wrap(f: F, error_out: *mut *mut TCString, err_value: T) -> T +fn wrap(f: F, error_out: *mut TCString, err_value: T) -> T where F: FnOnce() -> anyhow::Result, { + if !error_out.is_null() { + // SAFETY: + // - error_out is not NULL (just checked) + // - properly aligned and valid (promised by caller) + unsafe { *error_out = TCString::default() }; + } + match f() { Ok(v) => v, Err(e) => { if !error_out.is_null() { // SAFETY: - // - error_out is not NULL (checked) - // - ..and points to a valid pointer (promised by caller) - // - caller will free this string (promised by caller) + // - error_out is not NULL (just checked) + // - properly aligned and valid (promised by caller) unsafe { - *error_out = err_to_tcstring(e).return_ptr(); + TCString::val_to_arg_out(err_to_ruststring(e), error_out); } } err_value @@ -56,16 +62,15 @@ where /// The server must be freed after it is used - tc_replica_sync does not automatically free it. #[no_mangle] pub unsafe extern "C" fn tc_server_new_local( - server_dir: *mut TCString, - error_out: *mut *mut TCString, + server_dir: TCString, + error_out: *mut TCString, ) -> *mut TCServer { wrap( || { // SAFETY: - // - server_dir is not NULL (promised by caller) - // - server_dir is return from a tc_string_.. so is valid + // - server_dir is valid (promised by caller) // - caller will not use server_dir after this call (convention) - let server_dir = unsafe { TCString::take_from_ptr_arg(server_dir) }; + let server_dir = unsafe { TCString::val_from_arg(server_dir) }; let server_config = ServerConfig::Local { server_dir: server_dir.to_path_buf(), }; @@ -87,30 +92,26 @@ pub unsafe extern "C" fn tc_server_new_local( /// The server must be freed after it is used - tc_replica_sync does not automatically free it. #[no_mangle] pub unsafe extern "C" fn tc_server_new_remote( - origin: *mut TCString, + origin: TCString, client_key: TCUuid, - encryption_secret: *mut TCString, - error_out: *mut *mut TCString, + encryption_secret: TCString, + error_out: *mut TCString, ) -> *mut TCServer { wrap( || { - debug_assert!(!origin.is_null()); - debug_assert!(!encryption_secret.is_null()); // SAFETY: - // - origin is not NULL // - origin is valid (promised by caller) // - origin ownership is transferred to this function - let origin = unsafe { TCString::take_from_ptr_arg(origin) }.into_string()?; + let origin = unsafe { TCString::val_from_arg(origin) }.into_string()?; // SAFETY: // - client_key is a valid Uuid (any 8-byte sequence counts) let client_key = unsafe { TCUuid::val_from_arg(client_key) }; // SAFETY: - // - encryption_secret is not NULL // - encryption_secret is valid (promised by caller) // - encryption_secret ownership is transferred to this function - let encryption_secret = unsafe { TCString::take_from_ptr_arg(encryption_secret) } + let encryption_secret = unsafe { TCString::val_from_arg(encryption_secret) } .as_bytes() .to_vec(); diff --git a/lib/src/string.rs b/lib/src/string.rs index 70e4795ee..3767f7bb2 100644 --- a/lib/src/string.rs +++ b/lib/src/string.rs @@ -1,9 +1,9 @@ use crate::traits::*; +use crate::util::{string_into_raw_parts, vec_into_raw_parts}; use std::ffi::{CStr, CString, OsStr}; +use std::os::raw::c_char; use std::os::unix::ffi::OsStrExt; use std::path::PathBuf; -use std::ptr::NonNull; -use std::str::Utf8Error; /// TCString supports passing strings into and out of the TaskChampion API. /// @@ -22,98 +22,273 @@ use std::str::Utf8Error; /// /// # Safety /// -/// When a `*TCString` appears as a return value or output argument, ownership is passed to the +/// The `ptr` field may be checked for NULL, where documentation indicates this is possible. All +/// other fields in a TCString are private and must not be used from C. They exist in the struct +/// to ensure proper allocation and alignment. +/// +/// When a `TCString` appears as a return value or output argument, ownership is passed to the /// caller. The caller must pass that ownership back to another function or free the string. /// -/// Any function taking a `*TCString` requires: +/// Any function taking a `TCString` requires: /// - the pointer must not be NUL; /// - the pointer must be one previously returned from a tc_… function; and /// - the memory referenced by the pointer must never be modified by C code. /// -/// Unless specified otherwise, TaskChampion functions take ownership of a `*TCString` when it is -/// given as a function argument, and the pointer is invalid when the function returns. Callers -/// must not use or free TCStrings after passing them to such API functions. +/// Unless specified otherwise, TaskChampion functions take ownership of a `TCString` when it is +/// given as a function argument, and the caller must not use or free TCStrings after passing them +/// to such API functions. +/// +/// A TCString with a NULL `ptr` field need not be freed, although tc_free_string will not fail +/// for such a value. /// /// TCString is not threadsafe. +/// cbindgen:field-names=[ptr, _u1, _u2, _u3] +#[repr(C)] +pub struct TCString { + // defined based on the type + ptr: *mut libc::c_void, + len: usize, + cap: usize, + + // type of TCString this represents + ty: u8, +} + +// TODO: figure out how to ignore this but still use it in TCString +/// A discriminator for TCString +#[repr(u8)] +enum TCStringType { + /// Null. Nothing is contained in this string. + /// + /// * `ptr` is NULL. + /// * `len` and `cap` are zero. + Null = 0, + + /// A CString. + /// + /// * `ptr` is the result of CString::into_raw, containing a terminating NUL. It may not be + /// valid UTF-8. + /// * `len` and `cap` are zero. + CString, + + /// A CStr, referencing memory borrowed from C + /// + /// * `ptr` points to the string, containing a terminating NUL. It may not be valid UTF-8. + /// * `len` and `cap` are zero. + CStr, + + /// A String. + /// + /// * `ptr`, `len`, and `cap` are as would be returned from String::into_raw_parts. + String, + + /// A byte sequence. + /// + /// * `ptr`, `len`, and `cap` are as would be returned from Vec::into_raw_parts. + Bytes, +} + +impl Default for TCString { + fn default() -> Self { + TCString { + ptr: std::ptr::null_mut(), + len: 0, + cap: 0, + ty: TCStringType::Null as u8, + } + } +} + +impl TCString { + pub(crate) fn is_null(&self) -> bool { + self.ptr.is_null() + } +} + #[derive(PartialEq, Debug)] -pub enum TCString<'a> { +pub enum RustString<'a> { + Null, CString(CString), CStr(&'a CStr), String(String), - - /// This variant denotes an input string that was not valid UTF-8. This allows reporting this - /// error when the string is read, with the constructor remaining infallible. - InvalidUtf8(Utf8Error, Vec), - - /// None is the default value for TCString, but this variant is never seen by C code or by Rust - /// code outside of this module. - None, + Bytes(Vec), } -impl<'a> Default for TCString<'a> { +impl<'a> Default for RustString<'a> { fn default() -> Self { - TCString::None + RustString::Null } } -impl<'a> PassByPointer for TCString<'a> {} +impl PassByValue for TCString { + type RustType = RustString<'static>; -impl<'a> TCString<'a> { - /// Get a regular Rust &str for this value. - pub(crate) fn as_str(&self) -> Result<&str, std::str::Utf8Error> { - match self { - TCString::CString(cstring) => cstring.as_c_str().to_str(), - TCString::CStr(cstr) => cstr.to_str(), - TCString::String(string) => Ok(string.as_ref()), - TCString::InvalidUtf8(e, _) => Err(*e), - TCString::None => unreachable!(), + unsafe fn from_ctype(self) -> Self::RustType { + match self.ty { + ty if ty == TCStringType::CString as u8 => { + // SAFETY: + // - ptr was derived from CString::into_raw + // - data was not modified since that time (caller promises) + RustString::CString(unsafe { CString::from_raw(self.ptr as *mut c_char) }) + } + ty if ty == TCStringType::CStr as u8 => { + // SAFETY: + // - ptr was created by CStr::as_ptr + // - data was not modified since that time (caller promises) + RustString::CStr(unsafe { CStr::from_ptr(self.ptr as *mut c_char) }) + } + ty if ty == TCStringType::String as u8 => { + // SAFETY: + // - ptr was created by string_into_raw_parts + // - data was not modified since that time (caller promises) + RustString::String(unsafe { + String::from_raw_parts(self.ptr as *mut u8, self.len, self.cap) + }) + } + ty if ty == TCStringType::Bytes as u8 => { + // SAFETY: + // - ptr was created by vec_into_raw_parts + // - data was not modified since that time (caller promises) + RustString::Bytes(unsafe { + Vec::from_raw_parts(self.ptr as *mut u8, self.len, self.cap) + }) + } + _ => RustString::Null, } } - /// Consume this TCString and return an equivalent String, or an error if not - /// valid UTF-8. In the error condition, the original data is lost. - pub(crate) fn into_string(self) -> Result { + fn as_ctype(arg: Self::RustType) -> Self { + match arg { + RustString::Null => Self { + ty: TCStringType::Null as u8, + ..Default::default() + }, + RustString::CString(cstring) => Self { + ty: TCStringType::CString as u8, + ptr: cstring.into_raw() as *mut libc::c_void, + ..Default::default() + }, + RustString::CStr(cstr) => Self { + ty: TCStringType::CStr as u8, + ptr: cstr.as_ptr() as *mut libc::c_void, + ..Default::default() + }, + RustString::String(string) => { + let (ptr, len, cap) = string_into_raw_parts(string); + Self { + ty: TCStringType::String as u8, + ptr: ptr as *mut libc::c_void, + len, + cap, + } + } + RustString::Bytes(bytes) => { + let (ptr, len, cap) = vec_into_raw_parts(bytes); + Self { + ty: TCStringType::Bytes as u8, + ptr: ptr as *mut libc::c_void, + len, + cap, + } + } + } + } +} + +impl<'a> RustString<'a> { + /// Get a regular Rust &str for this value. + pub(crate) fn as_str(&mut self) -> Result<&str, std::str::Utf8Error> { match self { - TCString::CString(cstring) => cstring.into_string().map_err(|e| e.utf8_error()), - TCString::CStr(cstr) => cstr.to_str().map(|s| s.to_string()), - TCString::String(string) => Ok(string), - TCString::InvalidUtf8(e, _) => Err(e), - TCString::None => unreachable!(), + RustString::CString(cstring) => cstring.as_c_str().to_str(), + RustString::CStr(cstr) => cstr.to_str(), + RustString::String(ref string) => Ok(string.as_ref()), + RustString::Bytes(_) => { + self.bytes_to_string()?; + self.as_str() // now the String variant, so won't recurse + } + RustString::Null => unreachable!(), + } + } + + /// Consume this RustString and return an equivalent String, or an error if not + /// valid UTF-8. In the error condition, the original data is lost. + pub(crate) fn into_string(mut self) -> Result { + match self { + RustString::CString(cstring) => cstring.into_string().map_err(|e| e.utf8_error()), + RustString::CStr(cstr) => cstr.to_str().map(|s| s.to_string()), + RustString::String(string) => Ok(string), + RustString::Bytes(_) => { + self.bytes_to_string()?; + self.into_string() // now the String variant, so won't recurse + } + RustString::Null => unreachable!(), } } pub(crate) fn as_bytes(&self) -> &[u8] { match self { - TCString::CString(cstring) => cstring.as_bytes(), - TCString::CStr(cstr) => cstr.to_bytes(), - TCString::String(string) => string.as_bytes(), - TCString::InvalidUtf8(_, data) => data.as_ref(), - TCString::None => unreachable!(), + RustString::CString(cstring) => cstring.as_bytes(), + RustString::CStr(cstr) => cstr.to_bytes(), + RustString::String(string) => string.as_bytes(), + RustString::Bytes(bytes) => bytes.as_ref(), + RustString::Null => unreachable!(), } } - /// Convert the TCString, in place, into one of the C variants. If this is not + /// Convert the RustString, in place, from the Bytes to String variant. On successful return, + /// the RustString has variant RustString::String. + fn bytes_to_string(&mut self) -> Result<(), std::str::Utf8Error> { + let mut owned = RustString::Null; + // temporarily swap a Null value into self; we'll swap that back + // shortly. + std::mem::swap(self, &mut owned); + match owned { + RustString::Bytes(bytes) => match String::from_utf8(bytes) { + Ok(string) => { + *self = RustString::String(string); + Ok(()) + } + Err(e) => { + let (e, bytes) = (e.utf8_error(), e.into_bytes()); + // put self back as we found it + *self = RustString::Bytes(bytes); + Err(e) + } + }, + _ => { + // not bytes, so just swap back + std::mem::swap(self, &mut owned); + Ok(()) + } + } + } + + /// Convert the RustString, in place, into one of the C variants. If this is not /// possible, such as if the string contains an embedded NUL, then the string /// remains unchanged. - fn to_c_string_mut(&mut self) { - if matches!(self, TCString::String(_)) { - // we must take ownership of the String in order to try converting it, - // leaving the underlying TCString as its default (None) - if let TCString::String(string) = std::mem::take(self) { + fn string_to_cstring(&mut self) { + let mut owned = RustString::Null; + // temporarily swap a Null value into self; we'll swap that back shortly + std::mem::swap(self, &mut owned); + match owned { + RustString::String(string) => { match CString::new(string) { - Ok(cstring) => *self = TCString::CString(cstring), + Ok(cstring) => { + *self = RustString::CString(cstring); + } Err(nul_err) => { // recover the underlying String from the NulError and restore - // the TCString + // the RustString let original_bytes = nul_err.into_vec(); // SAFETY: original_bytes came from a String moments ago, so still valid utf8 let string = unsafe { String::from_utf8_unchecked(original_bytes) }; - *self = TCString::String(string); + *self = RustString::String(string); } } - } else { - // the `matches!` above verified self was a TCString::String - unreachable!() + } + _ => { + // not a CString, so just swap back + std::mem::swap(self, &mut owned); } } } @@ -125,18 +300,55 @@ impl<'a> TCString<'a> { } } -impl<'a> From for TCString<'a> { - fn from(string: String) -> TCString<'a> { - TCString::String(string) +impl<'a> From for RustString<'a> { + fn from(string: String) -> RustString<'a> { + RustString::String(string) } } -impl<'a> From<&str> for TCString<'static> { - fn from(string: &str) -> TCString<'static> { - TCString::String(string.to_string()) +impl<'a> From<&str> for RustString<'static> { + fn from(string: &str) -> RustString<'static> { + RustString::String(string.to_string()) } } +/// Utility function to borrow a TCString from a pointer arg, modify it, +/// and restore it. +/// +/// This implements a kind of "interior mutability", relying on the +/// single-threaded use of all TC* types. +/// +/// # SAFETY +/// +/// - tcstring must not be NULL +/// - *tcstring must be a valid TCString +/// - *tcstring must not be accessed by anything else, despite the *const +unsafe fn wrap(tcstring: *const TCString, f: F) -> T +where + F: FnOnce(&mut RustString) -> T, +{ + debug_assert!(!tcstring.is_null()); + + // SAFETY: + // - we have exclusive to *tcstring (promised by caller) + let tcstring = tcstring as *mut TCString; + + // SAFETY: + // - tcstring is not NULL + // - *tcstring is a valid string (promised by caller) + let mut rstring = unsafe { TCString::take_val_from_arg(tcstring, TCString::default()) }; + + let rv = f(&mut rstring); + + // update the caller's TCString with the updated RustString + // SAFETY: + // - tcstring is not NULL (we just took from it) + // - tcstring points to valid memory (we just took from it) + unsafe { TCString::val_to_arg_out(rstring, tcstring) }; + + rv +} + /// TCStringList represents a list of strings. /// /// The content of this struct must be treated as read-only. @@ -151,11 +363,11 @@ pub struct TCStringList { /// TCStringList representing each string. these remain owned by the TCStringList instance and will /// be freed by tc_string_list_free. This pointer is never NULL for a valid TCStringList, and the /// *TCStringList at indexes 0..len-1 are not NULL. - items: *const NonNull>, + items: *const TCString, } impl CList for TCStringList { - type Element = NonNull>; + type Element = TCString; unsafe fn from_raw_parts(items: *const Self::Element, len: usize, cap: usize) -> Self { TCStringList { @@ -185,7 +397,7 @@ impl CList for TCStringList { /// free(url); // string is no longer referenced and can be freed /// ``` #[no_mangle] -pub unsafe extern "C" fn tc_string_borrow(cstr: *const libc::c_char) -> *mut TCString<'static> { +pub unsafe extern "C" fn tc_string_borrow(cstr: *const libc::c_char) -> TCString { debug_assert!(!cstr.is_null()); // SAFETY: // - cstr is not NULL (promised by caller, verified by assertion) @@ -195,13 +407,13 @@ pub unsafe extern "C" fn tc_string_borrow(cstr: *const libc::c_char) -> *mut TCS let cstr: &CStr = unsafe { CStr::from_ptr(cstr) }; // SAFETY: // - caller promises to free this string - unsafe { TCString::CStr(cstr).return_ptr() } + unsafe { TCString::return_val(RustString::CStr(cstr)) } } /// Create a new TCString by cloning the content of the given C string. The resulting TCString /// is independent of the given string, which can be freed or overwritten immediately. #[no_mangle] -pub unsafe extern "C" fn tc_string_clone(cstr: *const libc::c_char) -> *mut TCString<'static> { +pub unsafe extern "C" fn tc_string_clone(cstr: *const libc::c_char) -> TCString { debug_assert!(!cstr.is_null()); // SAFETY: // - cstr is not NULL (promised by caller, verified by assertion) @@ -209,21 +421,24 @@ pub unsafe extern "C" fn tc_string_clone(cstr: *const libc::c_char) -> *mut TCSt // - cstr contains a valid NUL terminator (promised by caller) // - cstr's content will not change before it is destroyed (by C convention) let cstr: &CStr = unsafe { CStr::from_ptr(cstr) }; + let cstring: CString = cstr.into(); // SAFETY: // - caller promises to free this string - unsafe { TCString::CString(cstr.into()).return_ptr() } + unsafe { TCString::return_val(RustString::CString(cstring)) } } /// Create a new TCString containing the given string with the given length. This allows creation /// of strings containing embedded NUL characters. As with `tc_string_clone`, the resulting /// TCString is independent of the passed buffer, which may be reused or freed immediately. /// +/// The length should _not_ include any trailing NUL. +/// /// The given length must be less than half the maximum value of usize. #[no_mangle] pub unsafe extern "C" fn tc_string_clone_with_len( buf: *const libc::c_char, len: usize, -) -> *mut TCString<'static> { +) -> TCString { debug_assert!(!buf.is_null()); debug_assert!(len < isize::MAX as usize); // SAFETY: @@ -237,47 +452,45 @@ pub unsafe extern "C" fn tc_string_clone_with_len( // allocate and copy into Rust-controlled memory let vec = slice.to_vec(); - // try converting to a string, which is the only variant that can contain embedded NULs. If - // the bytes are not valid utf-8, store that information for reporting later. - let tcstring = match String::from_utf8(vec) { - Ok(string) => TCString::String(string), - Err(e) => { - let (e, vec) = (e.utf8_error(), e.into_bytes()); - TCString::InvalidUtf8(e, vec) - } - }; - // SAFETY: // - caller promises to free this string - unsafe { tcstring.return_ptr() } + unsafe { TCString::return_val(RustString::Bytes(vec)) } } -/// Get the content of the string as a regular C string. The given string must not be NULL. The +/// Get the content of the string as a regular C string. The given string must be valid. The /// returned value is NULL if the string contains NUL bytes or (in some cases) invalid UTF-8. The /// returned C string is valid until the TCString is freed or passed to another TC API function. /// /// In general, prefer [`tc_string_content_with_len`] except when it's certain that the string is /// valid and NUL-free. /// +/// This function takes the TCString by pointer because it may be modified in-place to add a NUL +/// terminator. The pointer must not be NULL. +/// /// This function does _not_ take ownership of the TCString. #[no_mangle] -pub unsafe extern "C" fn tc_string_content(tcstring: *mut TCString) -> *const libc::c_char { - // SAFETY: +pub unsafe extern "C" fn tc_string_content(tcstring: *const TCString) -> *const libc::c_char { + // SAFETY; // - tcstring is not NULL (promised by caller) - // - lifetime of tcstring outlives the lifetime of this function - // - lifetime of tcstring outlives the lifetime of the returned pointer (promised by caller) - let tcstring = unsafe { TCString::from_ptr_arg_ref_mut(tcstring) }; + // - *tcstring is valid (promised by caller) + // - *tcstring is not accessed concurrently (single-threaded) + unsafe { + wrap(tcstring, |rstring| { + // try to eliminate the Bytes variant. If this fails, we'll return NULL + // below, so the error is ignorable. + let _ = rstring.bytes_to_string(); - // if we have a String, we need to consume it and turn it into - // a CString. - tcstring.to_c_string_mut(); + // and eliminate the String variant + rstring.string_to_cstring(); - match tcstring { - TCString::CString(cstring) => cstring.as_ptr(), - TCString::String(_) => std::ptr::null(), // to_c_string_mut failed - TCString::CStr(cstr) => cstr.as_ptr(), - TCString::InvalidUtf8(_, _) => std::ptr::null(), - TCString::None => unreachable!(), + match &rstring { + RustString::CString(cstring) => cstring.as_ptr(), + RustString::String(_) => std::ptr::null(), // string_to_cstring failed + RustString::CStr(cstr) => cstr.as_ptr(), + RustString::Bytes(_) => std::ptr::null(), // already returned above + RustString::Null => unreachable!(), + } + }) } } @@ -286,26 +499,31 @@ pub unsafe extern "C" fn tc_string_content(tcstring: *mut TCString) -> *const li /// returned buffer is valid until the TCString is freed or passed to another TaskChampio /// function. /// +/// This function takes the TCString by pointer because it may be modified in-place to add a NUL +/// terminator. The pointer must not be NULL. +/// /// This function does _not_ take ownership of the TCString. #[no_mangle] pub unsafe extern "C" fn tc_string_content_with_len( - tcstring: *mut TCString, + tcstring: *const TCString, len_out: *mut usize, ) -> *const libc::c_char { - // SAFETY: + // SAFETY; // - tcstring is not NULL (promised by caller) - // - lifetime of tcstring outlives the lifetime of this function - // - lifetime of tcstring outlives the lifetime of the returned pointer (promised by caller) - let tcstring = unsafe { TCString::from_ptr_arg_ref(tcstring) }; + // - *tcstring is valid (promised by caller) + // - *tcstring is not accessed concurrently (single-threaded) + unsafe { + wrap(tcstring, |rstring| { + let bytes = rstring.as_bytes(); - let bytes = tcstring.as_bytes(); - - // SAFETY: - // - len_out is not NULL (promised by caller) - // - len_out points to valid memory (promised by caller) - // - len_out is properly aligned (C convention) - unsafe { usize::val_to_arg_out(bytes.len(), len_out) }; - bytes.as_ptr() as *const libc::c_char + // SAFETY: + // - len_out is not NULL (promised by caller) + // - len_out points to valid memory (promised by caller) + // - len_out is properly aligned (C convention) + usize::val_to_arg_out(bytes.len(), len_out); + bytes.as_ptr() as *const libc::c_char + }) + } } /// Free a TCString. The given string must not be NULL. The string must not be used @@ -315,7 +533,7 @@ pub unsafe extern "C" fn tc_string_free(tcstring: *mut TCString) { // SAFETY: // - tcstring is not NULL (promised by caller) // - caller is exclusive owner of tcstring (promised by caller) - drop(unsafe { TCString::take_from_ptr_arg(tcstring) }); + drop(unsafe { TCString::take_val_from_arg(tcstring, TCString::default()) }); } /// Free a TCStringList instance. The instance, and all TCStringList it contains, must not be used after @@ -328,7 +546,7 @@ pub unsafe extern "C" fn tc_string_list_free(tcstrings: *mut TCStringList) { // - 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_list(tcstrings) }; + unsafe { drop_value_list(tcstrings) }; } #[cfg(test)] @@ -338,7 +556,7 @@ mod test { #[test] fn empty_list_has_non_null_pointer() { - let tcstrings = TCStringList::return_val(Vec::new()); + let tcstrings = unsafe { TCStringList::return_val(Vec::new()) }; assert!(!tcstrings.items.is_null()); assert_eq!(tcstrings.len, 0); assert_eq!(tcstrings._capacity, 0); @@ -346,7 +564,7 @@ mod test { #[test] fn free_sets_null_pointer() { - let mut tcstrings = TCStringList::return_val(Vec::new()); + let mut tcstrings = unsafe { TCStringList::return_val(Vec::new()) }; // SAFETY: testing expected behavior unsafe { tc_string_list_free(&mut tcstrings) }; assert!(tcstrings.items.is_null()); @@ -356,26 +574,29 @@ mod test { const INVALID_UTF8: &[u8] = b"abc\xf0\x28\x8c\x28"; - fn make_cstring() -> TCString<'static> { - TCString::CString(CString::new("a string").unwrap()) + fn make_cstring() -> RustString<'static> { + RustString::CString(CString::new("a string").unwrap()) } - fn make_cstr() -> TCString<'static> { + fn make_cstr() -> RustString<'static> { let cstr = CStr::from_bytes_with_nul(b"a string\0").unwrap(); - TCString::CStr(&cstr) + RustString::CStr(&cstr) } - fn make_string() -> TCString<'static> { - TCString::String("a string".into()) + fn make_string() -> RustString<'static> { + RustString::String("a string".into()) } - fn make_string_with_nul() -> TCString<'static> { - TCString::String("a \0 nul!".into()) + fn make_string_with_nul() -> RustString<'static> { + RustString::String("a \0 nul!".into()) } - fn make_invalid() -> TCString<'static> { - let e = String::from_utf8(INVALID_UTF8.to_vec()).unwrap_err(); - TCString::InvalidUtf8(e.utf8_error(), e.into_bytes()) + fn make_invalid_bytes() -> RustString<'static> { + RustString::Bytes(INVALID_UTF8.to_vec()) + } + + fn make_bytes() -> RustString<'static> { + RustString::Bytes(b"bytes".to_vec()) } #[test] @@ -399,11 +620,16 @@ mod test { } #[test] - fn invalid_as_str() { - let as_str_err = make_invalid().as_str().unwrap_err(); + fn invalid_bytes_as_str() { + let as_str_err = make_invalid_bytes().as_str().unwrap_err(); assert_eq!(as_str_err.valid_up_to(), 3); // "abc" is valid } + #[test] + fn valid_bytes_as_str() { + assert_eq!(make_bytes().as_str().unwrap(), "bytes"); + } + #[test] fn cstring_as_bytes() { assert_eq!(make_cstring().as_bytes(), b"a string"); @@ -425,42 +651,42 @@ mod test { } #[test] - fn invalid_as_bytes() { - assert_eq!(make_invalid().as_bytes(), INVALID_UTF8); + fn invalid_bytes_as_bytes() { + assert_eq!(make_invalid_bytes().as_bytes(), INVALID_UTF8); } #[test] - fn cstring_to_c_string_mut() { + fn cstring_string_to_cstring() { let mut tcstring = make_cstring(); - tcstring.to_c_string_mut(); + tcstring.string_to_cstring(); assert_eq!(tcstring, make_cstring()); // unchanged } #[test] - fn cstr_to_c_string_mut() { + fn cstr_string_to_cstring() { let mut tcstring = make_cstr(); - tcstring.to_c_string_mut(); + tcstring.string_to_cstring(); assert_eq!(tcstring, make_cstr()); // unchanged } #[test] - fn string_to_c_string_mut() { + fn string_string_to_cstring() { let mut tcstring = make_string(); - tcstring.to_c_string_mut(); + tcstring.string_to_cstring(); assert_eq!(tcstring, make_cstring()); // converted to CString, same content } #[test] - fn string_with_nul_to_c_string_mut() { + fn string_with_nul_string_to_cstring() { let mut tcstring = make_string_with_nul(); - tcstring.to_c_string_mut(); + tcstring.string_to_cstring(); assert_eq!(tcstring, make_string_with_nul()); // unchanged } #[test] - fn invalid_to_c_string_mut() { - let mut tcstring = make_invalid(); - tcstring.to_c_string_mut(); - assert_eq!(tcstring, make_invalid()); // unchanged + fn bytes_string_to_cstring() { + let mut tcstring = make_bytes(); + tcstring.string_to_cstring(); + assert_eq!(tcstring, make_bytes()); // unchanged } } diff --git a/lib/src/task.rs b/lib/src/task.rs index ccea9fa20..c3c0b896b 100644 --- a/lib/src/task.rs +++ b/lib/src/task.rs @@ -1,6 +1,6 @@ use crate::traits::*; use crate::types::*; -use crate::util::err_to_tcstring; +use crate::util::err_to_ruststring; use chrono::{TimeZone, Utc}; use std::convert::TryFrom; use std::ops::Deref; @@ -42,7 +42,7 @@ pub struct TCTask { inner: Inner, /// The error from the most recent operation, if any - error: Option>, + error: Option>, } enum Inner { @@ -154,17 +154,17 @@ where match f(task) { Ok(rv) => rv, Err(e) => { - tctask.error = Some(err_to_tcstring(e)); + tctask.error = Some(err_to_ruststring(e)); err_value } } } -impl TryFrom> for Tag { +impl TryFrom> for Tag { type Error = anyhow::Error; - fn try_from(tcstring: TCString) -> Result { - let tagstr = tcstring.as_str()?; + fn try_from(mut rstring: RustString) -> Result { + let tagstr = rstring.as_str()?; Tag::from_str(tagstr) } } @@ -248,7 +248,11 @@ pub unsafe extern "C" fn tc_task_to_immut(task: *mut TCTask) { /// Get a task's UUID. #[no_mangle] pub unsafe extern "C" fn tc_task_get_uuid(task: *mut TCTask) -> TCUuid { - wrap(task, |task| TCUuid::return_val(task.get_uuid())) + wrap(task, |task| { + // SAFETY: + // - value is not allocated and need not be freed + unsafe { TCUuid::return_val(task.get_uuid()) } + }) } /// Get a task's status. @@ -259,7 +263,7 @@ pub unsafe extern "C" fn tc_task_get_status(task: *mut TCTask) -> TCStatus { /// Get the underlying key/value pairs for this task. The returned TCKVList is /// a "snapshot" of the task and will not be updated if the task is subsequently -/// modified. +/// modified. It is the caller's responsibility to free the TCKVList. #[no_mangle] pub unsafe extern "C" fn tc_task_get_taskmap(task: *mut TCTask) -> TCKVList { wrap(task, |task| { @@ -267,24 +271,26 @@ pub unsafe extern "C" fn tc_task_get_taskmap(task: *mut TCTask) -> TCKVList { .get_taskmap() .iter() .map(|(k, v)| { - let key = TCString::from(k.as_ref()); - let value = TCString::from(v.as_ref()); + let key = RustString::from(k.as_ref()); + let value = RustString::from(v.as_ref()); TCKV::as_ctype((key, value)) }) .collect(); - TCKVList::return_val(vec) + // SAFETY: + // - caller will free this list + unsafe { TCKVList::return_val(vec) } }) } /// Get a task's description, or NULL if the task cannot be represented as a C string (e.g., if it /// contains embedded NUL characters). #[no_mangle] -pub unsafe extern "C" fn tc_task_get_description(task: *mut TCTask) -> *mut TCString<'static> { +pub unsafe extern "C" fn tc_task_get_description(task: *mut TCTask) -> TCString { wrap(task, |task| { - let descr: TCString = task.get_description().into(); + let descr = task.get_description(); // SAFETY: // - caller promises to free this string - unsafe { descr.return_ptr() } + unsafe { TCString::return_val(descr.into()) } }) } @@ -321,12 +327,11 @@ pub unsafe extern "C" fn tc_task_is_active(task: *mut TCTask) -> bool { /// Check if a task has the given tag. If the tag is invalid, this function will return false, as /// that (invalid) tag is not present. No error will be reported via `tc_task_error`. #[no_mangle] -pub unsafe extern "C" fn tc_task_has_tag(task: *mut TCTask, tag: *mut TCString) -> bool { +pub unsafe extern "C" fn tc_task_has_tag(task: *mut TCTask, tag: TCString) -> bool { // SAFETY: - // - tag is not NULL (promised by caller) - // - tag is return from a tc_string_.. so is valid + // - tag is valid (promised by caller) // - caller will not use tag after this call (convention) - let tcstring = unsafe { TCString::take_from_ptr_arg(tag) }; + let tcstring = unsafe { TCString::val_from_arg(tag) }; wrap(task, |task| { if let Ok(tag) = Tag::try_from(tcstring) { task.has_tag(&tag) @@ -343,18 +348,17 @@ pub unsafe extern "C" fn tc_task_has_tag(task: *mut TCTask, tag: *mut TCString) #[no_mangle] pub unsafe extern "C" fn tc_task_get_tags(task: *mut TCTask) -> TCStringList { wrap(task, |task| { - let vec: Vec>> = task + let vec: Vec = task .get_tags() .map(|t| { - NonNull::new( - // SAFETY: - // - this TCString will be freed via tc_string_list_free. - unsafe { TCString::from(t.as_ref()).return_ptr() }, - ) - .expect("TCString::return_ptr() returned NULL") + // SAFETY: + // - this TCString will be freed via tc_string_list_free. + unsafe { TCString::return_val(t.as_ref().into()) } }) .collect(); - TCStringList::return_val(vec) + // SAFETY: + // - caller will free the list + unsafe { TCStringList::return_val(vec) } }) } @@ -368,39 +372,40 @@ pub unsafe extern "C" fn tc_task_get_annotations(task: *mut TCTask) -> TCAnnotat let vec: Vec = task .get_annotations() .map(|a| { - let description = TCString::from(a.description); + let description = RustString::from(a.description); TCAnnotation::as_ctype((a.entry, description)) }) .collect(); - TCAnnotationList::return_val(vec) + // SAFETY: + // - caller will free the list + unsafe { TCAnnotationList::return_val(vec) } }) } /// Get the named UDA from the task. /// -/// Returns NULL if the UDA does not exist. +/// Returns a TCString with NULL ptr field if the UDA does not exist. #[no_mangle] pub unsafe extern "C" fn tc_task_get_uda<'a>( task: *mut TCTask, - ns: *mut TCString<'a>, - key: *mut TCString<'a>, -) -> *mut TCString<'static> { + ns: TCString, + key: TCString, +) -> TCString { wrap(task, |task| { // SAFETY: - // - ns is not NULL (promised by caller) - // - ns is return from a tc_string_.. so is valid + // - ns is valid (promised by caller) // - caller will not use ns after this call (convention) - if let Ok(ns) = unsafe { TCString::take_from_ptr_arg(ns) }.as_str() { + if let Ok(ns) = unsafe { TCString::val_from_arg(ns) }.as_str() { // SAFETY: same - if let Ok(key) = unsafe { TCString::take_from_ptr_arg(key) }.as_str() { + if let Ok(key) = unsafe { TCString::val_from_arg(key) }.as_str() { if let Some(value) = task.get_uda(ns, key) { // SAFETY: // - caller will free this string (caller promises) - return unsafe { TCString::return_ptr(value.into()) }; + return unsafe { TCString::return_val(value.into()) }; } } } - std::ptr::null_mut() + TCString::default() }) } @@ -408,23 +413,19 @@ pub unsafe extern "C" fn tc_task_get_uda<'a>( /// /// Returns NULL if the UDA does not exist. #[no_mangle] -pub unsafe extern "C" fn tc_task_get_legacy_uda<'a>( - task: *mut TCTask, - key: *mut TCString<'a>, -) -> *mut TCString<'static> { +pub unsafe extern "C" fn tc_task_get_legacy_uda<'a>(task: *mut TCTask, key: TCString) -> TCString { wrap(task, |task| { // SAFETY: - // - key is not NULL (promised by caller) - // - key is return from a tc_string_.. so is valid + // - key is valid (promised by caller) // - caller will not use key after this call (convention) - if let Ok(key) = unsafe { TCString::take_from_ptr_arg(key) }.as_str() { + if let Ok(key) = unsafe { TCString::val_from_arg(key) }.as_str() { if let Some(value) = task.get_legacy_uda(key) { // SAFETY: // - caller will free this string (caller promises) - return unsafe { TCString::return_ptr(value.into()) }; + return unsafe { TCString::return_val(value.into()) }; } } - std::ptr::null_mut() + TCString::default() }) } @@ -437,35 +438,47 @@ pub unsafe extern "C" fn tc_task_get_udas(task: *mut TCTask) -> TCUdaList { let vec: Vec = task .get_udas() .map(|((ns, key), value)| { - TCUda::return_val(Uda { - ns: Some(ns.into()), - key: key.into(), - value: value.into(), - }) + // SAFETY: + // - will be freed by tc_uda_list_free + unsafe { + TCUda::return_val(Uda { + ns: Some(ns.into()), + key: key.into(), + value: value.into(), + }) + } }) .collect(); - TCUdaList::return_val(vec) + // SAFETY: + // - caller will free this list + unsafe { TCUdaList::return_val(vec) } }) } /// Get all UDAs for this task. /// /// All TCUdas in this list have a NULL ns field. The entire UDA key is -/// included in the key field. +/// included in the key field. The caller must free the returned list. #[no_mangle] pub unsafe extern "C" fn tc_task_get_legacy_udas(task: *mut TCTask) -> TCUdaList { wrap(task, |task| { let vec: Vec = task .get_legacy_udas() .map(|(key, value)| { - TCUda::return_val(Uda { - ns: None, - key: key.into(), - value: value.into(), - }) + // SAFETY: + // - will be freed by tc_uda_list_free + unsafe { + TCUda::return_val(Uda { + ns: None, + key: key.into(), + value: value.into(), + }) + } }) .collect(); - TCUdaList::return_val(vec) + // SAFETY: + // - caller will free this list + unsafe { TCUdaList::return_val(vec) } }) } @@ -486,13 +499,12 @@ pub unsafe extern "C" fn tc_task_set_status(task: *mut TCTask, status: TCStatus) #[no_mangle] pub unsafe extern "C" fn tc_task_set_description( task: *mut TCTask, - description: *mut TCString, + description: TCString, ) -> TCResult { // SAFETY: - // - description is not NULL (promised by caller) - // - description is return from a tc_string_.. so is valid + // - description is valid (promised by caller) // - caller will not use description after this call (convention) - let description = unsafe { TCString::take_from_ptr_arg(description) }; + let mut description = unsafe { TCString::val_from_arg(description) }; wrap_mut( task, |task| { @@ -606,12 +618,11 @@ pub unsafe extern "C" fn tc_task_delete(task: *mut TCTask) -> TCResult { /// Add a tag to a mutable task. #[no_mangle] -pub unsafe extern "C" fn tc_task_add_tag(task: *mut TCTask, tag: *mut TCString) -> TCResult { +pub unsafe extern "C" fn tc_task_add_tag(task: *mut TCTask, tag: TCString) -> TCResult { // SAFETY: - // - tag is not NULL (promised by caller) - // - tag is return from a tc_string_.. so is valid + // - tag is valid (promised by caller) // - caller will not use tag after this call (convention) - let tcstring = unsafe { TCString::take_from_ptr_arg(tag) }; + let tcstring = unsafe { TCString::val_from_arg(tag) }; wrap_mut( task, |task| { @@ -625,12 +636,11 @@ pub unsafe extern "C" fn tc_task_add_tag(task: *mut TCTask, tag: *mut TCString) /// Remove a tag from a mutable task. #[no_mangle] -pub unsafe extern "C" fn tc_task_remove_tag(task: *mut TCTask, tag: *mut TCString) -> TCResult { +pub unsafe extern "C" fn tc_task_remove_tag(task: *mut TCTask, tag: TCString) -> TCResult { // SAFETY: - // - tag is not NULL (promised by caller) - // - tag is return from a tc_string_.. so is valid + // - tag is valid (promised by caller) // - caller will not use tag after this call (convention) - let tcstring = unsafe { TCString::take_from_ptr_arg(tag) }; + let tcstring = unsafe { TCString::val_from_arg(tag) }; wrap_mut( task, |task| { @@ -683,19 +693,18 @@ pub unsafe extern "C" fn tc_task_remove_annotation(task: *mut TCTask, entry: i64 #[no_mangle] pub unsafe extern "C" fn tc_task_set_uda( task: *mut TCTask, - ns: *mut TCString, - key: *mut TCString, - value: *mut TCString, + ns: TCString, + key: TCString, + value: TCString, ) -> TCResult { // safety: - // - ns is not null (promised by caller) - // - ns is return from a tc_string_.. so is valid + // - ns is valid (promised by caller) // - caller will not use ns after this call (convention) - let ns = unsafe { TCString::take_from_ptr_arg(ns) }; + let mut ns = unsafe { TCString::val_from_arg(ns) }; // SAFETY: same - let key = unsafe { TCString::take_from_ptr_arg(key) }; + let mut key = unsafe { TCString::val_from_arg(key) }; // SAFETY: same - let value = unsafe { TCString::take_from_ptr_arg(value) }; + let mut value = unsafe { TCString::val_from_arg(value) }; wrap_mut( task, |task| { @@ -714,16 +723,15 @@ pub unsafe extern "C" fn tc_task_set_uda( #[no_mangle] pub unsafe extern "C" fn tc_task_remove_uda( task: *mut TCTask, - ns: *mut TCString, - key: *mut TCString, + ns: TCString, + key: TCString, ) -> TCResult { // safety: - // - ns is not null (promised by caller) - // - ns is return from a tc_string_.. so is valid + // - ns is valid (promised by caller) // - caller will not use ns after this call (convention) - let ns = unsafe { TCString::take_from_ptr_arg(ns) }; + let mut ns = unsafe { TCString::val_from_arg(ns) }; // SAFETY: same - let key = unsafe { TCString::take_from_ptr_arg(key) }; + let mut key = unsafe { TCString::val_from_arg(key) }; wrap_mut( task, |task| { @@ -738,16 +746,15 @@ pub unsafe extern "C" fn tc_task_remove_uda( #[no_mangle] pub unsafe extern "C" fn tc_task_set_legacy_uda( task: *mut TCTask, - key: *mut TCString, - value: *mut TCString, + key: TCString, + value: TCString, ) -> TCResult { // safety: - // - key is not null (promised by caller) - // - key is return from a tc_string_.. so is valid + // - key is valid (promised by caller) // - caller will not use key after this call (convention) - let key = unsafe { TCString::take_from_ptr_arg(key) }; + let mut key = unsafe { TCString::val_from_arg(key) }; // SAFETY: same - let value = unsafe { TCString::take_from_ptr_arg(value) }; + let mut value = unsafe { TCString::val_from_arg(value) }; wrap_mut( task, |task| { @@ -760,15 +767,11 @@ pub unsafe extern "C" fn tc_task_set_legacy_uda( /// Remove a UDA fraom a mutable task. #[no_mangle] -pub unsafe extern "C" fn tc_task_remove_legacy_uda( - task: *mut TCTask, - key: *mut TCString, -) -> TCResult { +pub unsafe extern "C" fn tc_task_remove_legacy_uda(task: *mut TCTask, key: TCString) -> TCResult { // safety: - // - key is not null (promised by caller) - // - key is return from a tc_string_.. so is valid + // - key is valid (promised by caller) // - caller will not use key after this call (convention) - let key = unsafe { TCString::take_from_ptr_arg(key) }; + let mut key = unsafe { TCString::val_from_arg(key) }; wrap_mut( task, |task| { @@ -779,21 +782,21 @@ pub unsafe extern "C" fn tc_task_remove_legacy_uda( ) } -/// Get the latest error for a task, or NULL if the last operation succeeded. Subsequent calls -/// to this function will return NULL. The task pointer must not be NULL. The caller must free the -/// returned string. +/// Get the latest error for a task, or a string NULL ptr field if the last operation succeeded. +/// Subsequent calls to this function will return NULL. The task pointer must not be NULL. The +/// caller must free the returned string. #[no_mangle] -pub unsafe extern "C" fn tc_task_error(task: *mut TCTask) -> *mut TCString<'static> { +pub unsafe extern "C" fn tc_task_error(task: *mut TCTask) -> TCString { // SAFETY: // - task is not null (promised by caller) // - task outlives 'a (promised by caller) let task: &mut TCTask = unsafe { TCTask::from_ptr_arg_ref_mut(task) }; - if let Some(tcstring) = task.error.take() { + if let Some(rstring) = task.error.take() { // SAFETY: // - caller promises to free this value - unsafe { tcstring.return_ptr() } + unsafe { TCString::return_val(rstring) } } else { - std::ptr::null_mut() + TCString::default() } } @@ -833,7 +836,7 @@ mod test { #[test] fn empty_list_has_non_null_pointer() { - let tctasks = TCTaskList::return_val(Vec::new()); + let tctasks = unsafe { TCTaskList::return_val(Vec::new()) }; assert!(!tctasks.items.is_null()); assert_eq!(tctasks.len, 0); assert_eq!(tctasks._capacity, 0); @@ -841,7 +844,7 @@ mod test { #[test] fn free_sets_null_pointer() { - let mut tctasks = TCTaskList::return_val(Vec::new()); + let mut tctasks = unsafe { TCTaskList::return_val(Vec::new()) }; // SAFETY: testing expected behavior unsafe { tc_task_list_free(&mut tctasks) }; assert!(tctasks.items.is_null()); diff --git a/lib/src/traits.rs b/lib/src/traits.rs index e71ab0b9f..ffbdf9a79 100644 --- a/lib/src/traits.rs +++ b/lib/src/traits.rs @@ -28,8 +28,6 @@ pub(crate) trait PassByValue: Sized { /// /// - `self` must be a valid instance of the C type. This is typically ensured either by /// requiring that C code not modify it, or by defining the valid values in C comments. - /// - if RustType is not Copy, then arg must not be used by the caller after calling this - /// function unsafe fn val_from_arg(arg: Self) -> Self::RustType { // SAFETY: // - arg is a valid CType (promised by caller) @@ -54,7 +52,11 @@ pub(crate) trait PassByValue: Sized { } /// Return a value to C - fn return_val(arg: Self::RustType) -> Self { + /// + /// # Safety + /// + /// - if the value is allocated, the caller must ensure that the value is eventually freed + unsafe fn return_val(arg: Self::RustType) -> Self { Self::as_ctype(arg) } diff --git a/lib/src/uda.rs b/lib/src/uda.rs index 3712932ad..607d77e27 100644 --- a/lib/src/uda.rs +++ b/lib/src/uda.rs @@ -4,18 +4,18 @@ use crate::types::*; /// TCUda contains the details of a UDA. #[repr(C)] pub struct TCUda { - /// Namespace of the UDA. For legacy UDAs, this is NULL. - pub ns: *mut TCString<'static>, + /// Namespace of the UDA. For legacy UDAs, this may have a NULL ptr field. + pub ns: TCString, /// UDA key. Must not be NULL. - pub key: *mut TCString<'static>, + pub key: TCString, /// Content of the UDA. Must not be NULL. - pub value: *mut TCString<'static>, + pub value: TCString, } pub(crate) struct Uda { - pub ns: Option>, - pub key: TCString<'static>, - pub value: TCString<'static>, + pub ns: Option>, + pub key: RustString<'static>, + pub value: RustString<'static>, } impl PassByValue for TCUda { @@ -29,16 +29,16 @@ impl PassByValue for TCUda { // SAFETY: // - self is owned, so we can take ownership of this TCString // - self.ns is a valid, non-null TCString (NULL just checked) - Some(unsafe { TCString::take_from_ptr_arg(self.ns) }) + Some(unsafe { TCString::val_from_arg(self.ns) }) }, // SAFETY: // - self is owned, so we can take ownership of this TCString // - self.key is a valid, non-null TCString (see type docstring) - key: unsafe { TCString::take_from_ptr_arg(self.key) }, + key: unsafe { TCString::val_from_arg(self.key) }, // SAFETY: // - self is owned, so we can take ownership of this TCString // - self.value is a valid, non-null TCString (see type docstring) - value: unsafe { TCString::take_from_ptr_arg(self.value) }, + value: unsafe { TCString::val_from_arg(self.value) }, } } @@ -46,14 +46,14 @@ impl PassByValue for TCUda { TCUda { // SAFETY: caller assumes ownership of this value ns: if let Some(ns) = uda.ns { - unsafe { ns.return_ptr() } + unsafe { TCString::return_val(ns) } } else { - std::ptr::null_mut() + TCString::default() }, // SAFETY: caller assumes ownership of this value - key: unsafe { uda.key.return_ptr() }, + key: unsafe { TCString::return_val(uda.key) }, // SAFETY: caller assumes ownership of this value - value: unsafe { uda.value.return_ptr() }, + value: unsafe { TCString::return_val(uda.value) }, } } } @@ -61,9 +61,9 @@ impl PassByValue for TCUda { impl Default for TCUda { fn default() -> Self { TCUda { - ns: std::ptr::null_mut(), - key: std::ptr::null_mut(), - value: std::ptr::null_mut(), + ns: TCString::default(), + key: TCString::default(), + value: TCString::default(), } } } @@ -130,7 +130,7 @@ mod test { #[test] fn empty_list_has_non_null_pointer() { - let tcudas = TCUdaList::return_val(Vec::new()); + let tcudas = unsafe { TCUdaList::return_val(Vec::new()) }; assert!(!tcudas.items.is_null()); assert_eq!(tcudas.len, 0); assert_eq!(tcudas._capacity, 0); @@ -138,7 +138,7 @@ mod test { #[test] fn free_sets_null_pointer() { - let mut tcudas = TCUdaList::return_val(Vec::new()); + let mut tcudas = unsafe { TCUdaList::return_val(Vec::new()) }; // SAFETY: testing expected behavior unsafe { tc_uda_list_free(&mut tcudas) }; assert!(tcudas.items.is_null()); diff --git a/lib/src/util.rs b/lib/src/util.rs index 405a8b292..bfd739282 100644 --- a/lib/src/util.rs +++ b/lib/src/util.rs @@ -1,7 +1,7 @@ -use crate::string::TCString; +use crate::string::RustString; -pub(crate) fn err_to_tcstring(e: impl std::string::ToString) -> TCString<'static> { - TCString::from(e.to_string()) +pub(crate) fn err_to_ruststring(e: impl std::string::ToString) -> RustString<'static> { + RustString::from(e.to_string()) } /// An implementation of Vec::into_raw_parts, which is still unstable. Returns ptr, len, cap. @@ -12,3 +12,12 @@ pub(crate) fn vec_into_raw_parts(vec: Vec) -> (*mut T, usize, usize) { let mut vec = std::mem::ManuallyDrop::new(vec); (vec.as_mut_ptr(), vec.len(), vec.capacity()) } + +/// An implementation of String::into_raw_parts, which is still unstable. Returns ptr, len, cap. +pub(crate) fn string_into_raw_parts(string: String) -> (*mut u8, usize, usize) { + // emulate String::into_raw_parts(): + // - disable dropping the String with ManuallyDrop + // - extract ptr, len, and capacity using those methods + let mut string = std::mem::ManuallyDrop::new(string); + (string.as_mut_ptr(), string.len(), string.capacity()) +} diff --git a/lib/src/uuid.rs b/lib/src/uuid.rs index 1bc300cf1..c4ec8f474 100644 --- a/lib/src/uuid.rs +++ b/lib/src/uuid.rs @@ -31,13 +31,17 @@ impl PassByValue for TCUuid { /// Create a new, randomly-generated UUID. #[no_mangle] pub unsafe extern "C" fn tc_uuid_new_v4() -> TCUuid { - TCUuid::return_val(Uuid::new_v4()) + // SAFETY: + // - value is not allocated + unsafe { TCUuid::return_val(Uuid::new_v4()) } } /// Create a new UUID with the nil value. #[no_mangle] pub unsafe extern "C" fn tc_uuid_nil() -> TCUuid { - TCUuid::return_val(Uuid::nil()) + // SAFETY: + // - value is not allocated + unsafe { TCUuid::return_val(Uuid::nil()) } } /// TCUuidList represents a list of uuids. @@ -95,27 +99,26 @@ pub unsafe extern "C" fn tc_uuid_to_buf(tcuuid: TCUuid, buf: *mut libc::c_char) /// Return the hyphenated string representation of a TCUuid. The returned string /// must be freed with tc_string_free. #[no_mangle] -pub unsafe extern "C" fn tc_uuid_to_str(tcuuid: TCUuid) -> *mut TCString<'static> { +pub unsafe extern "C" fn tc_uuid_to_str(tcuuid: TCUuid) -> TCString { // SAFETY: // - tcuuid is a valid TCUuid (all byte patterns are valid) let uuid: Uuid = unsafe { TCUuid::val_from_arg(tcuuid) }; let s = uuid.to_string(); // SAFETY: // - caller promises to free this value. - unsafe { TCString::from(s).return_ptr() } + unsafe { TCString::return_val(s.into()) } } /// Parse the given string as a UUID. Returns TC_RESULT_ERROR on parse failure or if the given /// string is not valid. #[no_mangle] -pub unsafe extern "C" fn tc_uuid_from_str(s: *mut TCString, uuid_out: *mut TCUuid) -> TCResult { +pub unsafe extern "C" fn tc_uuid_from_str(s: TCString, uuid_out: *mut TCUuid) -> TCResult { debug_assert!(!s.is_null()); debug_assert!(!uuid_out.is_null()); // SAFETY: - // - s is not NULL (promised by caller) - // - s is return from a tc_string_.. so is valid + // - s is valid (promised by caller) // - caller will not use s after this call (convention) - let s = unsafe { TCString::take_from_ptr_arg(s) }; + let mut s = unsafe { TCString::val_from_arg(s) }; if let Ok(s) = s.as_str() { if let Ok(u) = Uuid::parse_str(s) { // SAFETY: @@ -147,7 +150,7 @@ mod test { #[test] fn empty_list_has_non_null_pointer() { - let tcuuids = TCUuidList::return_val(Vec::new()); + let tcuuids = unsafe { TCUuidList::return_val(Vec::new()) }; assert!(!tcuuids.items.is_null()); assert_eq!(tcuuids.len, 0); assert_eq!(tcuuids._capacity, 0); @@ -155,7 +158,7 @@ mod test { #[test] fn free_sets_null_pointer() { - let mut tcuuids = TCUuidList::return_val(Vec::new()); + let mut tcuuids = unsafe { TCUuidList::return_val(Vec::new()) }; // SAFETY: testing expected behavior unsafe { tc_uuid_list_free(&mut tcuuids) }; assert!(tcuuids.items.is_null()); diff --git a/lib/taskchampion.h b/lib/taskchampion.h index 702e24446..f09bddfe1 100644 --- a/lib/taskchampion.h +++ b/lib/taskchampion.h @@ -151,40 +151,6 @@ typedef struct TCReplica TCReplica; */ typedef struct TCServer TCServer; -/** - * TCString supports passing strings into and out of the TaskChampion API. - * - * # Rust Strings and C Strings - * - * A Rust string can contain embedded NUL characters, while C considers such a character to mark - * the end of a string. Strings containing embedded NULs cannot be represented as a "C string" - * and must be accessed using `tc_string_content_and_len` and `tc_string_clone_with_len`. In - * general, these two functions should be used for handling arbitrary data, while more convenient - * forms may be used where embedded NUL characters are impossible, such as in static strings. - * - * # UTF-8 - * - * TaskChampion expects all strings to be valid UTF-8. `tc_string_…` functions will fail if given - * a `*TCString` containing invalid UTF-8. - * - * # Safety - * - * When a `*TCString` appears as a return value or output argument, ownership is passed to the - * caller. The caller must pass that ownership back to another function or free the string. - * - * Any function taking a `*TCString` requires: - * - the pointer must not be NUL; - * - the pointer must be one previously returned from a tc_… function; and - * - the memory referenced by the pointer must never be modified by C code. - * - * Unless specified otherwise, TaskChampion functions take ownership of a `*TCString` when it is - * given as a function argument, and the pointer is invalid when the function returns. Callers - * must not use or free TCStrings after passing them to such API functions. - * - * TCString is not threadsafe. - */ -typedef struct TCString TCString; - /** * A task, as publicly exposed by this library. * @@ -243,6 +209,52 @@ typedef struct TCTask TCTask; */ typedef struct TCWorkingSet TCWorkingSet; +/** + * TCString supports passing strings into and out of the TaskChampion API. + * + * # Rust Strings and C Strings + * + * A Rust string can contain embedded NUL characters, while C considers such a character to mark + * the end of a string. Strings containing embedded NULs cannot be represented as a "C string" + * and must be accessed using `tc_string_content_and_len` and `tc_string_clone_with_len`. In + * general, these two functions should be used for handling arbitrary data, while more convenient + * forms may be used where embedded NUL characters are impossible, such as in static strings. + * + * # UTF-8 + * + * TaskChampion expects all strings to be valid UTF-8. `tc_string_…` functions will fail if given + * a `*TCString` containing invalid UTF-8. + * + * # Safety + * + * The `ptr` field may be checked for NULL, where documentation indicates this is possible. All + * other fields in a TCString are private and must not be used from C. They exist in the struct + * to ensure proper allocation and alignment. + * + * When a `TCString` appears as a return value or output argument, ownership is passed to the + * caller. The caller must pass that ownership back to another function or free the string. + * + * Any function taking a `TCString` requires: + * - the pointer must not be NUL; + * - the pointer must be one previously returned from a tc_… function; and + * - the memory referenced by the pointer must never be modified by C code. + * + * Unless specified otherwise, TaskChampion functions take ownership of a `TCString` when it is + * given as a function argument, and the caller must not use or free TCStrings after passing them + * to such API functions. + * + * A TCString with a NULL `ptr` field need not be freed, although tc_free_string will not fail + * for such a value. + * + * TCString is not threadsafe. + */ +typedef struct TCString { + void *ptr; + size_t _u1; + size_t _u2; + uint8_t _u3; +} TCString; + /** * TCAnnotation contains the details of an annotation. * @@ -268,7 +280,7 @@ typedef struct TCAnnotation { /** * Content of the annotation. Must not be NULL. */ - struct TCString *description; + struct TCString description; } TCAnnotation; /** @@ -299,8 +311,8 @@ typedef struct TCAnnotationList { * will be freed when it is freed with tc_kv_list_free. */ typedef struct TCKV { - struct TCString *key; - struct TCString *value; + struct TCString key; + struct TCString value; } TCKV; /** @@ -395,7 +407,7 @@ typedef struct TCStringList { * be freed by tc_string_list_free. This pointer is never NULL for a valid TCStringList, and the * *TCStringList at indexes 0..len-1 are not NULL. */ - struct TCString *const *items; + const struct TCString *items; } TCStringList; /** @@ -403,17 +415,17 @@ typedef struct TCStringList { */ typedef struct TCUda { /** - * Namespace of the UDA. For legacy UDAs, this is NULL. + * Namespace of the UDA. For legacy UDAs, this may have a NULL ptr field. */ - struct TCString *ns; + struct TCString ns; /** * UDA key. Must not be NULL. */ - struct TCString *key; + struct TCString key; /** * Content of the UDA. Must not be NULL. */ - struct TCString *value; + struct TCString value; } TCUda; /** @@ -474,7 +486,7 @@ struct TCReplica *tc_replica_new_in_memory(void); * is written to the error_out parameter (if it is not NULL) and NULL is returned. The caller * must free this string. */ -struct TCReplica *tc_replica_new_on_disk(struct TCString *path, struct TCString **error_out); +struct TCReplica *tc_replica_new_on_disk(struct TCString path, struct TCString *error_out); /** * Get a list of all tasks in the replica. @@ -487,6 +499,8 @@ struct TCTaskList tc_replica_all_tasks(struct TCReplica *rep); * Get a list of all uuids for tasks in the replica. * * Returns a TCUuidList with a NULL items field on error. + * + * The caller must free the UUID list with `tc_uuid_list_free`. */ struct TCUuidList tc_replica_all_task_uuids(struct TCReplica *rep); @@ -513,7 +527,7 @@ struct TCTask *tc_replica_get_task(struct TCReplica *rep, struct TCUuid tcuuid); */ struct TCTask *tc_replica_new_task(struct TCReplica *rep, enum TCStatus status, - struct TCString *description); + struct TCString description); /** * Create a new task. The task must not already exist. @@ -553,11 +567,11 @@ TCResult tc_replica_add_undo_point(struct TCReplica *rep, bool force); TCResult tc_replica_rebuild_working_set(struct TCReplica *rep, bool renumber); /** - * 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. + * Get the latest error for a replica, or a string with NULL ptr if no error exists. 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); +struct TCString tc_replica_error(struct TCReplica *rep); /** * Free a replica. The replica may not be used after this function returns and must not be freed @@ -574,7 +588,7 @@ void tc_replica_free(struct TCReplica *rep); * * The server must be freed after it is used - tc_replica_sync does not automatically free it. */ -struct TCServer *tc_server_new_local(struct TCString *server_dir, struct TCString **error_out); +struct TCServer *tc_server_new_local(struct TCString server_dir, struct TCString *error_out); /** * Create a new TCServer that connects to a remote server. See the TaskChampion docs for the @@ -585,10 +599,10 @@ struct TCServer *tc_server_new_local(struct TCString *server_dir, struct TCStrin * * The server must be freed after it is used - tc_replica_sync does not automatically free it. */ -struct TCServer *tc_server_new_remote(struct TCString *origin, +struct TCServer *tc_server_new_remote(struct TCString origin, struct TCUuid client_key, - struct TCString *encryption_secret, - struct TCString **error_out); + struct TCString encryption_secret, + struct TCString *error_out); /** * Free a server. The server may not be used after this function returns and must not be freed @@ -612,34 +626,39 @@ void tc_server_free(struct TCServer *server); * free(url); // string is no longer referenced and can be freed * ``` */ -struct TCString *tc_string_borrow(const char *cstr); +struct TCString tc_string_borrow(const char *cstr); /** * Create a new TCString by cloning the content of the given C string. The resulting TCString * is independent of the given string, which can be freed or overwritten immediately. */ -struct TCString *tc_string_clone(const char *cstr); +struct TCString tc_string_clone(const char *cstr); /** * Create a new TCString containing the given string with the given length. This allows creation * of strings containing embedded NUL characters. As with `tc_string_clone`, the resulting * TCString is independent of the passed buffer, which may be reused or freed immediately. * + * The length should _not_ include any trailing NUL. + * * The given length must be less than half the maximum value of usize. */ -struct TCString *tc_string_clone_with_len(const char *buf, size_t len); +struct TCString tc_string_clone_with_len(const char *buf, size_t len); /** - * Get the content of the string as a regular C string. The given string must not be NULL. The + * Get the content of the string as a regular C string. The given string must be valid. The * returned value is NULL if the string contains NUL bytes or (in some cases) invalid UTF-8. The * returned C string is valid until the TCString is freed or passed to another TC API function. * * In general, prefer [`tc_string_content_with_len`] except when it's certain that the string is * valid and NUL-free. * + * This function takes the TCString by pointer because it may be modified in-place to add a NUL + * terminator. The pointer must not be NULL. + * * This function does _not_ take ownership of the TCString. */ -const char *tc_string_content(struct TCString *tcstring); +const char *tc_string_content(const struct TCString *tcstring); /** * Get the content of the string as a pointer and length. The given string must not be NULL. @@ -647,9 +666,12 @@ const char *tc_string_content(struct TCString *tcstring); * returned buffer is valid until the TCString is freed or passed to another TaskChampio * function. * + * This function takes the TCString by pointer because it may be modified in-place to add a NUL + * terminator. The pointer must not be NULL. + * * This function does _not_ take ownership of the TCString. */ -const char *tc_string_content_with_len(struct TCString *tcstring, size_t *len_out); +const char *tc_string_content_with_len(const struct TCString *tcstring, size_t *len_out); /** * Free a TCString. The given string must not be NULL. The string must not be used @@ -707,7 +729,7 @@ enum TCStatus tc_task_get_status(struct TCTask *task); /** * Get the underlying key/value pairs for this task. The returned TCKVList is * a "snapshot" of the task and will not be updated if the task is subsequently - * modified. + * modified. It is the caller's responsibility to free the TCKVList. */ struct TCKVList tc_task_get_taskmap(struct TCTask *task); @@ -715,7 +737,7 @@ struct TCKVList tc_task_get_taskmap(struct TCTask *task); * Get a task's description, or NULL if the task cannot be represented as a C string (e.g., if it * contains embedded NUL characters). */ -struct TCString *tc_task_get_description(struct TCTask *task); +struct TCString tc_task_get_description(struct TCTask *task); /** * Get the entry timestamp for a task (when it was created), or 0 if not set. @@ -746,7 +768,7 @@ bool tc_task_is_active(struct TCTask *task); * Check if a task has the given tag. If the tag is invalid, this function will return false, as * that (invalid) tag is not present. No error will be reported via `tc_task_error`. */ -bool tc_task_has_tag(struct TCTask *task, struct TCString *tag); +bool tc_task_has_tag(struct TCTask *task, struct TCString tag); /** * Get the tags for the task. @@ -767,16 +789,16 @@ struct TCAnnotationList tc_task_get_annotations(struct TCTask *task); /** * Get the named UDA from the task. * - * Returns NULL if the UDA does not exist. + * Returns a TCString with NULL ptr field if the UDA does not exist. */ -struct TCString *tc_task_get_uda(struct TCTask *task, struct TCString *ns, struct TCString *key); +struct TCString tc_task_get_uda(struct TCTask *task, struct TCString ns, struct TCString key); /** * Get the named legacy UDA from the task. * * Returns NULL if the UDA does not exist. */ -struct TCString *tc_task_get_legacy_uda(struct TCTask *task, struct TCString *key); +struct TCString tc_task_get_legacy_uda(struct TCTask *task, struct TCString key); /** * Get all UDAs for this task. @@ -789,7 +811,7 @@ struct TCUdaList tc_task_get_udas(struct TCTask *task); * Get all UDAs for this task. * * All TCUdas in this list have a NULL ns field. The entire UDA key is - * included in the key field. + * included in the key field. The caller must free the returned list. */ struct TCUdaList tc_task_get_legacy_udas(struct TCTask *task); @@ -801,7 +823,7 @@ TCResult tc_task_set_status(struct TCTask *task, enum TCStatus status); /** * Set a mutable task's description. */ -TCResult tc_task_set_description(struct TCTask *task, struct TCString *description); +TCResult tc_task_set_description(struct TCTask *task, struct TCString description); /** * Set a mutable task's entry (creation time). Pass entry=0 to unset @@ -842,12 +864,12 @@ TCResult tc_task_delete(struct TCTask *task); /** * Add a tag to a mutable task. */ -TCResult tc_task_add_tag(struct TCTask *task, struct TCString *tag); +TCResult tc_task_add_tag(struct TCTask *task, struct TCString tag); /** * Remove a tag from a mutable task. */ -TCResult tc_task_remove_tag(struct TCTask *task, struct TCString *tag); +TCResult tc_task_remove_tag(struct TCTask *task, struct TCString tag); /** * Add an annotation to a mutable task. This call takes ownership of the @@ -864,31 +886,31 @@ TCResult tc_task_remove_annotation(struct TCTask *task, int64_t entry); * Set a UDA on a mutable task. */ TCResult tc_task_set_uda(struct TCTask *task, - struct TCString *ns, - struct TCString *key, - struct TCString *value); + struct TCString ns, + struct TCString key, + struct TCString value); /** * Remove a UDA fraom a mutable task. */ -TCResult tc_task_remove_uda(struct TCTask *task, struct TCString *ns, struct TCString *key); +TCResult tc_task_remove_uda(struct TCTask *task, struct TCString ns, struct TCString key); /** * Set a legacy UDA on a mutable task. */ -TCResult tc_task_set_legacy_uda(struct TCTask *task, struct TCString *key, struct TCString *value); +TCResult tc_task_set_legacy_uda(struct TCTask *task, struct TCString key, struct TCString value); /** * Remove a UDA fraom a mutable task. */ -TCResult tc_task_remove_legacy_uda(struct TCTask *task, struct TCString *key); +TCResult tc_task_remove_legacy_uda(struct TCTask *task, struct TCString key); /** - * Get the latest error for a task, or NULL if the last operation succeeded. Subsequent calls - * to this function will return NULL. The task pointer must not be NULL. The caller must free the - * returned string. + * Get the latest error for a task, or a string NULL ptr field if the last operation succeeded. + * Subsequent calls to this function will return NULL. The task pointer must not be NULL. The + * caller must free the returned string. */ -struct TCString *tc_task_error(struct TCTask *task); +struct TCString tc_task_error(struct TCTask *task); /** * Free a task. The given task must not be NULL. The task must not be used after this function @@ -940,13 +962,13 @@ void tc_uuid_to_buf(struct TCUuid tcuuid, char *buf); * Return the hyphenated string representation of a TCUuid. The returned string * must be freed with tc_string_free. */ -struct TCString *tc_uuid_to_str(struct TCUuid tcuuid); +struct TCString tc_uuid_to_str(struct TCUuid tcuuid); /** * Parse the given string as a UUID. Returns TC_RESULT_ERROR on parse failure or if the given * string is not valid. */ -TCResult tc_uuid_from_str(struct TCString *s, struct TCUuid *uuid_out); +TCResult tc_uuid_from_str(struct TCString s, struct TCUuid *uuid_out); /** * Free a TCUuidList instance. The instance, and all TCUuids it contains, must not be used after