From 317194447f027ff80178948e6585ffe305026b17 Mon Sep 17 00:00:00 2001 From: Jason Rhinelander Date: Thu, 5 Oct 2023 22:57:14 -0300 Subject: [PATCH 1/2] Return good hashes from merge() [stable backport] Session clients required the list of hashes from a merge() to perform various operations based on the messages that were accepted. This updates `merge()` to return a vector of accepted hashes. For the C API, this returns a config_string_list* pointer that needs to be freed, but contains all the good hashes (the same way current_hashes() works). --- include/session/config/base.h | 38 +++++++++++++------- include/session/config/base.hpp | 16 ++++++--- src/config/base.cpp | 42 +++++++++------------- src/config/internal.hpp | 44 +++++++++++++++++++++++ tests/test_config_contacts.cpp | 10 ++++-- tests/test_config_convo_info_volatile.cpp | 11 ++++-- tests/test_config_user_groups.cpp | 2 +- tests/test_config_userprofile.cpp | 16 ++++++--- 8 files changed, 125 insertions(+), 54 deletions(-) diff --git a/include/session/config/base.h b/include/session/config/base.h index de9806e3..6b6ae027 100644 --- a/include/session/config/base.h +++ b/include/session/config/base.h @@ -91,6 +91,17 @@ LIBSESSION_EXPORT void config_set_logger( /// - `int16_t` -- integer of the namespace LIBSESSION_EXPORT int16_t config_storage_namespace(const config_object* conf); +/// Struct containing a list of C strings. Typically where this is returned by this API it must be +/// freed (via `free()`) when done with it. +/// +/// When returned as a pointer by a libsession-util function this is allocated in such a way that +/// just the outer config_string_list can be free()d to free both the list *and* the inner `value` +/// and pointed-at values. +typedef struct config_string_list { + char** value; // array of null-terminated C strings + size_t len; // length of `value` +} config_string_list; + /// API: base/config_merge /// /// Merges the config object with one or more remotely obtained config strings. After this call the @@ -117,13 +128,19 @@ LIBSESSION_EXPORT int16_t config_storage_namespace(const config_object* conf); /// - `count` -- [in] is the length of all three arrays. /// /// Outputs: -/// - `int` -- -LIBSESSION_EXPORT int config_merge( +/// - `config_string_list*` -- pointer to the list of successfully parsed hashes; the pointer +/// belongs to the caller and must be freed when done with it. + +LIBSESSION_EXPORT config_string_list* config_merge( config_object* conf, const char** msg_hashes, const unsigned char** configs, const size_t* lengths, - size_t count); + size_t count) +#ifdef __GNUC__ + __attribute__((warn_unused_result)) +#endif + ; /// API: base/config_needs_push /// @@ -251,13 +268,6 @@ LIBSESSION_EXPORT void config_dump(config_object* conf, unsigned char** out, siz /// - `bool` -- True if config has changed since last call to `dump()` LIBSESSION_EXPORT bool config_needs_dump(const config_object* conf); -/// Struct containing a list of C strings. Typically where this is returned by this API it must be -/// freed (via `free()`) when done with it. -typedef struct config_string_list { - char** value; // array of null-terminated C strings - size_t len; // length of `value` -} config_string_list; - /// API: base/config_current_hashes /// /// Obtains the current active hashes. Note that this will be empty if the current hash is unknown @@ -278,8 +288,12 @@ typedef struct config_string_list { /// - `conf` -- [in] Pointer to config_object object /// /// Outputs: -/// - `config_string_list*` -- point to the list of hashes, pointer belongs to the caller -LIBSESSION_EXPORT config_string_list* config_current_hashes(const config_object* conf); +/// - `config_string_list*` -- pointer to the list of hashes; the pointer belongs to the caller +LIBSESSION_EXPORT config_string_list* config_current_hashes(const config_object* conf) +#ifdef __GNUC__ + __attribute__((warn_unused_result)) +#endif + ; /// Config key management; see the corresponding method docs in base.hpp. All `key` arguments here /// are 32-byte binary buffers (and since fixed-length, there is no keylen argument). diff --git a/include/session/config/base.hpp b/include/session/config/base.hpp index 987e587d..caec633b 100644 --- a/include/session/config/base.hpp +++ b/include/session/config/base.hpp @@ -758,19 +758,25 @@ class ConfigBase { /// /// Declaration: /// ```cpp - /// int merge(const std::vector>& configs); - /// int merge(const std::vector>& configs); + /// std::vector merge( + /// const std::vector>& configs); + /// std::vector merge( + /// const std::vector>& configs); /// ``` /// /// Inputs: /// - `configs` -- vector of pairs containing the message hash and the raw message body /// /// Outputs: - /// - `int` -- Returns how many config messages that were successfully parsed - virtual int merge(const std::vector>& configs); + /// - vector of successfully parsed hashes. Note that this does not mean the hash was recent or + /// that it changed the config, merely that the returned hash was properly parsed and + /// processed as a config message, even if it was too old to be useful (or was already known + /// to be included). The hashes will be in the same order as in the input vector. + virtual std::vector merge( + const std::vector>& configs); // Same as merge (above )but takes the values as ustring's as sometimes that is more convenient. - int merge(const std::vector>& configs); + std::vector merge(const std::vector>& configs); /// API: base/ConfigBase::is_dirty /// diff --git a/src/config/base.cpp b/src/config/base.cpp index 3f80d5e2..5ed8c822 100644 --- a/src/config/base.cpp +++ b/src/config/base.cpp @@ -9,6 +9,7 @@ #include #include +#include "internal.hpp" #include "session/config/base.h" #include "session/config/encrypt.hpp" #include "session/export.h" @@ -38,7 +39,8 @@ MutableConfigMessage& ConfigBase::dirty() { throw std::runtime_error{"Internal error: unexpected dirty but non-mutable ConfigMessage"}; } -int ConfigBase::merge(const std::vector>& configs) { +std::vector ConfigBase::merge( + const std::vector>& configs) { std::vector> config_views; config_views.reserve(configs.size()); for (auto& [hash, data] : configs) @@ -53,7 +55,8 @@ std::unique_ptr make_config_message(bool from_dirty, Args&&... ar return std::make_unique(std::forward(args)...); } -int ConfigBase::merge(const std::vector>& configs) { +std::vector ConfigBase::merge( + const std::vector>& configs) { if (_keys_size == 0) throw std::logic_error{"Cannot merge configs without any decryption keys"}; @@ -218,8 +221,13 @@ int ConfigBase::merge(const std::vector>& c assert(new_conf->unmerged_index() == 0); } - return all_confs.size() - bad_confs.size() - - 1; // -1 because we don't count the first one (reparsing ourself). + std::vector good_hashes; + good_hashes.reserve(all_hashes.size() - (mine.empty() ? 0 : 1) - bad_confs.size()); + for (size_t i = mine.empty() ? 0 : 1; i < all_hashes.size(); i++) + if (!bad_confs.count(i)) + good_hashes.emplace_back(all_hashes[i]); + + return good_hashes; } std::vector ConfigBase::current_hashes() const { @@ -476,7 +484,7 @@ LIBSESSION_EXPORT int16_t config_storage_namespace(const config_object* conf) { return static_cast(unbox(conf)->storage_namespace()); } -LIBSESSION_EXPORT int config_merge( +LIBSESSION_EXPORT config_string_list* config_merge( config_object* conf, const char** msg_hashes, const unsigned char** configs, @@ -487,7 +495,8 @@ LIBSESSION_EXPORT int config_merge( confs.reserve(count); for (size_t i = 0; i < count; i++) confs.emplace_back(msg_hashes[i], ustring_view{configs[i], lengths[i]}); - return config.merge(confs); + + return make_string_list(config.merge(confs)); } LIBSESSION_EXPORT bool config_needs_push(const config_object* conf) { @@ -548,26 +557,7 @@ LIBSESSION_EXPORT bool config_needs_dump(const config_object* conf) { } LIBSESSION_EXPORT config_string_list* config_current_hashes(const config_object* conf) { - auto hashes = unbox(conf)->current_hashes(); - size_t sz = sizeof(config_string_list) + hashes.size() * sizeof(char*); - for (auto& h : hashes) - sz += h.size() + 1; - void* buf = std::malloc(sz); - auto* ret = static_cast(buf); - ret->len = hashes.size(); - - static_assert(alignof(config_string_list) >= alignof(char*)); - ret->value = reinterpret_cast(ret + 1); - char** next_ptr = ret->value; - char* next_str = reinterpret_cast(next_ptr + ret->len); - - for (size_t i = 0; i < ret->len; i++) { - *(next_ptr++) = next_str; - std::memcpy(next_str, hashes[i].c_str(), hashes[i].size() + 1); - next_str += hashes[i].size() + 1; - } - - return ret; + return make_string_list(unbox(conf)->current_hashes()); } LIBSESSION_EXPORT void config_add_key(config_object* conf, const unsigned char* key) { diff --git a/src/config/internal.hpp b/src/config/internal.hpp index 5bbab837..38cd8759 100644 --- a/src/config/internal.hpp +++ b/src/config/internal.hpp @@ -50,6 +50,50 @@ void copy_c_str(char (&dest)[N], std::string_view src) { dest[src.size()] = 0; } +// Copies a container of std::strings into a self-contained malloc'ed config_string_list for +// returning to C code with the strings and pointers of the string list in the same malloced space, +// hanging off the end (so that everything, including string values, is freed by a single `free()`). +template < + typename Container, + typename = std::enable_if_t>> +config_string_list* make_string_list(Container vals) { + // We malloc space for the config_string_list struct itself, plus the required number of string + // pointers to store its strings, and the space to actually contain a copy of the string data. + // When we're done, the malloced memory we grab is going to look like this: + // + // {config_string_list} + // {pointer1}{pointer2}... + // {string data 1\0}{string data 2\0}... + // + // where config_string_list.value points at the beginning of {pointer1}, and each pointerN + // points at the beginning of the {string data N\0} c string. + // + // Since we malloc it all at once, when the user frees it, they also free the entire thing. + size_t sz = sizeof(config_string_list) + vals.size() * sizeof(char*); + // plus, for each string, the space to store it (including the null) + for (auto& v : vals) + sz += v.size() + 1; + + auto* ret = static_cast(std::malloc(sz)); + ret->len = vals.size(); + + static_assert(alignof(config_string_list) >= alignof(char*)); + + // value points at the space immediately after the struct itself, which is the first element in + // the array of c string pointers. + ret->value = reinterpret_cast(ret + 1); + char** next_ptr = ret->value; + char* next_str = reinterpret_cast(next_ptr + ret->len); + + for (const auto& v : vals) { + *(next_ptr++) = next_str; + std::memcpy(next_str, v.c_str(), v.size() + 1); + next_str += v.size() + 1; + } + + return ret; +} + // Throws std::invalid_argument if session_id doesn't look valid void check_session_id(std::string_view session_id); diff --git a/tests/test_config_contacts.cpp b/tests/test_config_contacts.cpp index db6b9510..318e76c8 100644 --- a/tests/test_config_contacts.cpp +++ b/tests/test_config_contacts.cpp @@ -291,8 +291,10 @@ TEST_CASE("Contacts (C API)", "[config][contacts][c]") { merge_hash[0] = "fakehash1"; merge_data[0] = to_push->config; merge_size[0] = to_push->config_len; - int accepted = config_merge(conf2, merge_hash, merge_data, merge_size, 1); - REQUIRE(accepted == 1); + config_string_list* accepted = config_merge(conf2, merge_hash, merge_data, merge_size, 1); + REQUIRE(accepted->len == 1); + CHECK(accepted->value[0] == "fakehash1"sv); + free(accepted); config_confirm_pushed(conf, to_push->seqno, "fakehash1"); free(to_push); @@ -325,7 +327,9 @@ TEST_CASE("Contacts (C API)", "[config][contacts][c]") { merge_data[0] = to_push->config; merge_size[0] = to_push->config_len; accepted = config_merge(conf, merge_hash, merge_data, merge_size, 1); - REQUIRE(accepted == 1); + REQUIRE(accepted->len == 1); + CHECK(accepted->value[0] == "fakehash2"sv); + free(accepted); config_confirm_pushed(conf2, to_push->seqno, "fakehash2"); diff --git a/tests/test_config_convo_info_volatile.cpp b/tests/test_config_convo_info_volatile.cpp index 0edc9720..8511bd39 100644 --- a/tests/test_config_convo_info_volatile.cpp +++ b/tests/test_config_convo_info_volatile.cpp @@ -342,8 +342,10 @@ TEST_CASE("Conversations (C API)", "[config][conversations][c]") { hash_data[0] = "hash123"; merge_data[0] = to_push->config; merge_size[0] = to_push->config_len; - int accepted = config_merge(conf, hash_data, merge_data, merge_size, 1); - REQUIRE(accepted == 1); + config_string_list* accepted = config_merge(conf, hash_data, merge_data, merge_size, 1); + REQUIRE(accepted->len == 1); + CHECK(accepted->value[0] == "hash123"sv); + free(accepted); config_confirm_pushed(conf2, seqno, "hash123"); free(to_push); @@ -610,7 +612,10 @@ TEST_CASE("Conversation dump/load state bug", "[config][conversations][dump-load merge_data[0] = to_push->config; merge_size[0] = to_push->config_len; - config_merge(conf2, merge_hash, merge_data, merge_size, 1); + config_string_list* accepted = config_merge(conf2, merge_hash, merge_data, merge_size, 1); + REQUIRE(accepted->len == 1); + CHECK(accepted->value[0] == "hash5235"sv); + free(accepted); free(to_push); CHECK(config_needs_push(conf2)); diff --git a/tests/test_config_user_groups.cpp b/tests/test_config_user_groups.cpp index cccf18bf..4a685f32 100644 --- a/tests/test_config_user_groups.cpp +++ b/tests/test_config_user_groups.cpp @@ -544,7 +544,7 @@ TEST_CASE("User Groups members C API", "[config][groups][c]") { std::vector> to_merge; to_merge.emplace_back("fakehash1", ustring_view{to_push->config, to_push->config_len}); - CHECK(c2.merge(to_merge) == 1); + CHECK(c2.merge(to_merge) == std::vector{{"fakehash1"}}); auto grp = c2.get_legacy_group(definitely_real_id); REQUIRE(grp); diff --git a/tests/test_config_userprofile.cpp b/tests/test_config_userprofile.cpp index 47c547b2..f718d346 100644 --- a/tests/test_config_userprofile.cpp +++ b/tests/test_config_userprofile.cpp @@ -215,8 +215,10 @@ TEST_CASE("user profile C API", "[config][user_profile][c]") { merge_hash[0] = "fakehash1"; merge_data[0] = exp_push1_encrypted.data(); merge_size[0] = exp_push1_encrypted.size(); - int accepted = config_merge(conf2, merge_hash, merge_data, merge_size, 1); - REQUIRE(accepted == 1); + config_string_list* accepted = config_merge(conf2, merge_hash, merge_data, merge_size, 1); + REQUIRE(accepted->len == 1); + CHECK(accepted->value[0] == "fakehash1"sv); + free(accepted); // Our state has changed, so we need to dump: CHECK(config_needs_dump(conf2)); @@ -283,12 +285,18 @@ TEST_CASE("user profile C API", "[config][user_profile][c]") { merge_hash[0] = "fakehash2"; merge_data[0] = to_push->config; merge_size[0] = to_push->config_len; - config_merge(conf2, merge_hash, merge_data, merge_size, 1); + accepted = config_merge(conf2, merge_hash, merge_data, merge_size, 1); free(to_push); + REQUIRE(accepted->len == 1); + CHECK(accepted->value[0] == "fakehash2"sv); + free(accepted); merge_hash[0] = "fakehash3"; merge_data[0] = to_push2->config; merge_size[0] = to_push2->config_len; - config_merge(conf, merge_hash, merge_data, merge_size, 1); + accepted = config_merge(conf, merge_hash, merge_data, merge_size, 1); + REQUIRE(accepted->len == 1); + CHECK(accepted->value[0] == "fakehash3"sv); + free(accepted); free(to_push2); // Now after the merge we *will* want to push from both client, since both will have generated a From 612efe826bed9135513cc7a335d1e6a2dcae584b Mon Sep 17 00:00:00 2001 From: Jason Rhinelander Date: Mon, 16 Oct 2023 11:47:50 -0300 Subject: [PATCH 2/2] Update oxen-encoding & Catch2 submodules --- external/oxen-encoding | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/external/oxen-encoding b/external/oxen-encoding index fc85dfd3..10ffe949 160000 --- a/external/oxen-encoding +++ b/external/oxen-encoding @@ -1 +1 @@ -Subproject commit fc85dfd352e8474bc7195b0ba881838bd72ebea6 +Subproject commit 10ffe949f8e207be24f75d82632f12748ff4c253