refactor: Use merge_sort instead of qsort for sorting.

toxcore/BUILD.bazel

@@ -100,8 +100,6 @@ cc_test(
     size = "small",
     srcs = ["util_test.cc"],
     deps = [
-        ":crypto_core",
-        ":crypto_core_test_util",
         ":util",
         "@com_google_googletest//:gtest",
         "@com_google_googletest//:gtest_main",
@@ -953,6 +951,7 @@ cc_library(
         ":crypto_core",
         ":friend_connection",
         ":logger",
+        ":mem",
         ":mono_time",
         ":net_crypto",
         ":network",

toxcore/DHT.c

@@ -25,6 +25,7 @@
 #include "ping_array.h"
 #include "shared_key_cache.h"
 #include "state.h"
+#include "util.h"
 
 /** The timeout after which a node is discarded completely. */
 #define KILL_NODE_TIMEOUT (BAD_NODE_TIMEOUT + PING_INTERVAL)
@@ -755,49 +756,6 @@ int get_close_nodes(
                is_lan, want_announce);
 }
 
-typedef struct DHT_Cmp_Data {
-    uint64_t cur_time;
-    const uint8_t *base_public_key;
-    Client_data entry;
-} DHT_Cmp_Data;
-
-non_null()
-static int dht_cmp_entry(const void *a, const void *b)
-{
-    const DHT_Cmp_Data *cmp1 = (const DHT_Cmp_Data *)a;
-    const DHT_Cmp_Data *cmp2 = (const DHT_Cmp_Data *)b;
-    const Client_data entry1 = cmp1->entry;
-    const Client_data entry2 = cmp2->entry;
-    const uint8_t *cmp_public_key = cmp1->base_public_key;
-
-    const bool t1 = assoc_timeout(cmp1->cur_time, &entry1.assoc4) && assoc_timeout(cmp1->cur_time, &entry1.assoc6);
-    const bool t2 = assoc_timeout(cmp2->cur_time, &entry2.assoc4) && assoc_timeout(cmp2->cur_time, &entry2.assoc6);
-
-    if (t1 && t2) {
-        return 0;
-    }
-
-    if (t1) {
-        return -1;
-    }
-
-    if (t2) {
-        return 1;
-    }
-
-    const int closest = id_closest(cmp_public_key, entry1.public_key, entry2.public_key);
-
-    if (closest == 1) {
-        return 1;
-    }
-
-    if (closest == 2) {
-        return -1;
-    }
-
-    return 0;
-}
-
 #ifdef CHECK_ANNOUNCE_NODE
 non_null()
 static void set_announce_node_in_list(Client_data *list, uint32_t list_len, const uint8_t *public_key)
@@ -914,31 +872,117 @@ static bool store_node_ok(const Client_data *client, uint64_t cur_time, const ui
            || id_closest(comp_public_key, client->public_key, public_key) == 2;
 }
 
+typedef struct Client_data_Cmp {
+    const Memory *mem;
+    uint64_t cur_time;
+    const uint8_t *comp_public_key;
+} Client_data_Cmp;
+
 non_null()
-static void sort_client_list(const Memory *mem, Client_data *list, uint64_t cur_time, unsigned int length,
-                             const uint8_t *comp_public_key)
+static int client_data_cmp(const Client_data_Cmp *cmp, const Client_data *entry1, const Client_data *entry2)
 {
-    // Pass comp_public_key to qsort with each Client_data entry, so the
-    // comparison function can use it as the base of comparison.
-    DHT_Cmp_Data *cmp_list = (DHT_Cmp_Data *)mem_valloc(mem, length, sizeof(DHT_Cmp_Data));
+    const bool t1 = assoc_timeout(cmp->cur_time, &entry1->assoc4) && assoc_timeout(cmp->cur_time, &entry1->assoc6);
+    const bool t2 = assoc_timeout(cmp->cur_time, &entry2->assoc4) && assoc_timeout(cmp->cur_time, &entry2->assoc6);
 
-    if (cmp_list == nullptr) {
-        return;
+    if (t1 && t2) {
+        return 0;
     }
 
-    for (uint32_t i = 0; i < length; ++i) {
-        cmp_list[i].cur_time = cur_time;
-        cmp_list[i].base_public_key = comp_public_key;
-        cmp_list[i].entry = list[i];
+    if (t1) {
+        return -1;
     }
 
-    qsort(cmp_list, length, sizeof(DHT_Cmp_Data), dht_cmp_entry);
+    if (t2) {
+        return 1;
+    }
 
-    for (uint32_t i = 0; i < length; ++i) {
-        list[i] = cmp_list[i].entry;
+    const int closest = id_closest(cmp->comp_public_key, entry1->public_key, entry2->public_key);
+
+    if (closest == 1) {
+        return 1;
+    }
+
+    if (closest == 2) {
+        return -1;
+    }
+
+    return 0;
+}
+
+non_null()
+static bool client_data_less_handler(const void *object, const void *a, const void *b)
+{
+    const Client_data_Cmp *cmp = (const Client_data_Cmp *)object;
+    const Client_data *entry1 = (const Client_data *)a;
+    const Client_data *entry2 = (const Client_data *)b;
+
+    return client_data_cmp(cmp, entry1, entry2) < 0;
+}
+
+non_null()
+static const void *client_data_get_handler(const void *arr, uint32_t index)
+{
+    const Client_data *entries = (const Client_data *)arr;
+    return &entries[index];
+}
+
+non_null()
+static void client_data_set_handler(void *arr, uint32_t index, const void *val)
+{
+    Client_data *entries = (Client_data *)arr;
+    const Client_data *entry = (const Client_data *)val;
+    entries[index] = *entry;
+}
+
+non_null()
+static void *client_data_subarr_handler(void *arr, uint32_t index, uint32_t size)
+{
+    Client_data *entries = (Client_data *)arr;
+    return &entries[index];
+}
+
+non_null()
+static void *client_data_alloc_handler(const void *object, uint32_t size)
+{
+    const Client_data_Cmp *cmp = (const Client_data_Cmp *)object;
+    Client_data *tmp = (Client_data *)mem_valloc(cmp->mem, size, sizeof(Client_data));
+
+    if (tmp == nullptr) {
+        return nullptr;
     }
 
-    mem_delete(mem, cmp_list);
+    return tmp;
+}
+
+non_null()
+static void client_data_delete_handler(const void *object, void *arr, uint32_t size)
+{
+    const Client_data_Cmp *cmp = (const Client_data_Cmp *)object;
+    mem_delete(cmp->mem, arr);
+}
+
+static const Sort_Funcs client_data_cmp_funcs = {
+    client_data_less_handler,
+    client_data_get_handler,
+    client_data_set_handler,
+    client_data_subarr_handler,
+    client_data_alloc_handler,
+    client_data_delete_handler,
+};
+
+non_null()
+static void sort_client_list(const Memory *mem, Client_data *list, uint64_t cur_time, unsigned int length,
+                             const uint8_t *comp_public_key)
+{
+    // Pass comp_public_key to merge_sort with each Client_data entry, so the
+    // comparison function can use it as the base of comparison.
+    const Client_data_Cmp cmp = {
+        mem,
+        cur_time,
+        comp_public_key,
+    };
+
+    merge_sort(list, length, &cmp, &client_data_cmp_funcs);
 }
 
 non_null()

toxcore/crypto_core_test.cc

@@ -19,6 +19,35 @@ using ExtSecretKey = std::array<uint8_t, EXT_SECRET_KEY_SIZE>;
 using Signature = std::array<uint8_t, CRYPTO_SIGNATURE_SIZE>;
 using Nonce = std::array<uint8_t, CRYPTO_NONCE_SIZE>;
 
+TEST(PkEqual, TwoRandomIdsAreNotEqual)
+{
+    std::mt19937 rng;
+    std::uniform_int_distribution<uint8_t> dist{0, UINT8_MAX};
+
+    uint8_t pk1[CRYPTO_PUBLIC_KEY_SIZE];
+    uint8_t pk2[CRYPTO_PUBLIC_KEY_SIZE];
+
+    std::generate(std::begin(pk1), std::end(pk1), [&]() { return dist(rng); });
+    std::generate(std::begin(pk2), std::end(pk2), [&]() { return dist(rng); });
+
+    EXPECT_FALSE(pk_equal(pk1, pk2));
+}
+
+TEST(PkEqual, IdCopyMakesKeysEqual)
+{
+    std::mt19937 rng;
+    std::uniform_int_distribution<uint8_t> dist{0, UINT8_MAX};
+
+    uint8_t pk1[CRYPTO_PUBLIC_KEY_SIZE];
+    uint8_t pk2[CRYPTO_PUBLIC_KEY_SIZE] = {0};
+
+    std::generate(std::begin(pk1), std::end(pk1), [&]() { return dist(rng); });
+
+    pk_copy(pk2, pk1);
+
+    EXPECT_TRUE(pk_equal(pk1, pk2));
+}
+
 TEST(CryptoCore, EncryptLargeData)
 {
     Test_Random rng;

toxcore/group.c

@@ -20,6 +20,7 @@
 #include "friend_connection.h"
 #include "group_common.h"
 #include "logger.h"
+#include "mem.h"
 #include "mono_time.h"
 #include "net_crypto.h"
 #include "network.h"
@@ -957,24 +958,75 @@ static bool delpeer(Group_Chats *g_c, uint32_t groupnumber, int peer_index, void
 
 /** Order peers with friends first and with more recently active earlier */
 non_null()
-static int cmp_frozen(const void *a, const void *b)
+static bool group_peer_less_handler(const void *object, const void *a, const void *b)
 {
     const Group_Peer *pa = (const Group_Peer *)a;
     const Group_Peer *pb = (const Group_Peer *)b;
 
-    if (pa->is_friend ^ pb->is_friend) {
-        return pa->is_friend ? -1 : 1;
+    if (((pa->is_friend ? 1 : 0) ^ (pb->is_friend ? 1 : 0)) != 0) {
+        return pa->is_friend;
     }
 
-    return cmp_uint(pb->last_active, pa->last_active);
+    return cmp_uint(pb->last_active, pa->last_active) < 0;
 }
 
+non_null()
+static const void *group_peer_get_handler(const void *arr, uint32_t index)
+{
+    const Group_Peer *entries = (const Group_Peer *)arr;
+    return &entries[index];
+}
+
+non_null()
+static void group_peer_set_handler(void *arr, uint32_t index, const void *val)
+{
+    Group_Peer *entries = (Group_Peer *)arr;
+    const Group_Peer *entry = (const Group_Peer *)val;
+    entries[index] = *entry;
+}
+
+non_null()
+static void *group_peer_subarr_handler(void *arr, uint32_t index, uint32_t size)
+{
+    Group_Peer *entries = (Group_Peer *)arr;
+    return &entries[index];
+}
+
+non_null()
+static void *group_peer_alloc_handler(const void *object, uint32_t size)
+{
+    const Memory *mem = (const Memory *)object;
+    Group_Peer *tmp = (Group_Peer *)mem_valloc(mem, size, sizeof(Group_Peer));
+
+    if (tmp == nullptr) {
+        return nullptr;
+    }
+
+    return tmp;
+}
+
+non_null()
+static void group_peer_delete_handler(const void *object, void *arr, uint32_t size)
+{
+    const Memory *mem = (const Memory *)object;
+    mem_delete(mem, arr);
+}
+
+static const Sort_Funcs group_peer_cmp_funcs = {
+    group_peer_less_handler,
+    group_peer_get_handler,
+    group_peer_set_handler,
+    group_peer_subarr_handler,
+    group_peer_alloc_handler,
+    group_peer_delete_handler,
+};
+
 /** @brief Delete frozen peers as necessary to ensure at most `g->maxfrozen` remain.
  *
  * @retval true if any frozen peers are removed.
  */
 non_null()
-static bool delete_old_frozen(Group_c *g)
+static bool delete_old_frozen(Group_c *g, const Memory *mem)
 {
     if (g->numfrozen <= g->maxfrozen) {
         return false;
@@ -987,7 +1039,7 @@ static bool delete_old_frozen(Group_c *g)
         return true;
     }
 
-    qsort(g->frozen, g->numfrozen, sizeof(Group_Peer), cmp_frozen);
+    merge_sort(g->frozen, g->numfrozen, mem, &group_peer_cmp_funcs);
 
     Group_Peer *temp = (Group_Peer *)realloc(g->frozen, g->maxfrozen * sizeof(Group_Peer));
 
@@ -1032,7 +1084,7 @@ static bool freeze_peer(Group_Chats *g_c, uint32_t groupnumber, int peer_index,
 
     ++g->numfrozen;
 
-    delete_old_frozen(g);
+    delete_old_frozen(g, g_c->m->mem);
 
     return true;
 }
@@ -1519,7 +1571,7 @@ int group_set_max_frozen(const Group_Chats *g_c, uint32_t groupnumber, uint32_t
     }
 
     g->maxfrozen = maxfrozen;
-    delete_old_frozen(g);
+    delete_old_frozen(g, g_c->m->mem);
     return 0;
 }