Merge pull request #66 from jagerman/merge-hash-return-stable

Return good hashes from merge() [stable backport]

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).

include/session/config/base.hpp

@@ -758,19 +758,25 @@ class ConfigBase {
     ///
     /// Declaration:
     /// ```cpp
-    /// int merge(const std::vector<std::pair<std::string, ustring_view>>& configs);
-    /// int merge(const std::vector<std::pair<std::string, ustring>>& configs);
+    /// std::vector<std::string> merge(
+    ///     const std::vector<std::pair<std::string, ustring_view>>& configs);
+    /// std::vector<std::string> merge(
+    ///     const std::vector<std::pair<std::string, ustring>>& 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<std::pair<std::string, ustring_view>>& 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<std::string> merge(
+            const std::vector<std::pair<std::string, ustring_view>>& configs);
 
     // Same as merge (above )but takes the values as ustring's as sometimes that is more convenient.
-    int merge(const std::vector<std::pair<std::string, ustring>>& configs);
+    std::vector<std::string> merge(const std::vector<std::pair<std::string, ustring>>& configs);
 
     /// API: base/ConfigBase::is_dirty
     ///

src/config/base.cpp

@@ -9,6 +9,7 @@
 #include <string>
 #include <vector>
 
+#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<std::pair<std::string, ustring>>& configs) {
+std::vector<std::string> ConfigBase::merge(
+        const std::vector<std::pair<std::string, ustring>>& configs) {
     std::vector<std::pair<std::string, ustring_view>> config_views;
     config_views.reserve(configs.size());
     for (auto& [hash, data] : configs)
@@ -53,7 +55,8 @@ std::unique_ptr<ConfigMessage> make_config_message(bool from_dirty, Args&&... ar
     return std::make_unique<ConfigMessage>(std::forward<Args>(args)...);
 }
 
-int ConfigBase::merge(const std::vector<std::pair<std::string, ustring_view>>& configs) {
+std::vector<std::string> ConfigBase::merge(
+        const std::vector<std::pair<std::string, ustring_view>>& 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<std::pair<std::string, ustring_view>>& 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<std::string> 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<std::string> ConfigBase::current_hashes() const {
@@ -476,7 +484,7 @@ LIBSESSION_EXPORT int16_t config_storage_namespace(const config_object* conf) {
     return static_cast<int16_t>(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<config_string_list*>(buf);
-    ret->len = hashes.size();
-
-    static_assert(alignof(config_string_list) >= alignof(char*));
-    ret->value = reinterpret_cast<char**>(ret + 1);
-    char** next_ptr = ret->value;
-    char* next_str = reinterpret_cast<char*>(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) {

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<std::is_same_v<typename Container::value_type, std::string>>>
+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<config_string_list*>(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<char**>(ret + 1);
+    char** next_ptr = ret->value;
+    char* next_str = reinterpret_cast<char*>(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);
 

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");
 

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));

tests/test_config_user_groups.cpp

@@ -544,7 +544,7 @@ TEST_CASE("User Groups members C API", "[config][groups][c]") {
 
     std::vector<std::pair<std::string, ustring_view>> 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<std::string>{{"fakehash1"}});
 
     auto grp = c2.get_legacy_group(definitely_real_id);
     REQUIRE(grp);

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