fix: Don't use memcmp to compare IP_Ports.

`memcmp` compares padding bytes as well, which may be arbitrary or
uninitialised.

toxcore/BUILD.bazel

@@ -321,6 +321,8 @@ cc_test(
     size = "small",
     srcs = ["network_test.cc"],
     deps = [
+        ":crypto_core",
+        ":crypto_core_test_util",
         ":network",
         ":network_test_util",
         "@com_google_googletest//:gtest",

toxcore/TCP_server.c

@@ -1043,7 +1043,7 @@ TCP_Server *new_tcp_server(const Logger *logger, const Memory *mem, const Random
     memcpy(temp->secret_key, secret_key, CRYPTO_SECRET_KEY_SIZE);
     crypto_derive_public_key(temp->public_key, temp->secret_key);
 
-    bs_list_init(&temp->accepted_key_list, CRYPTO_PUBLIC_KEY_SIZE, 8);
+    bs_list_init(&temp->accepted_key_list, CRYPTO_PUBLIC_KEY_SIZE, 8, memcmp);
 
     return temp;
 }

toxcore/list.c

@@ -60,7 +60,7 @@ static int find(const BS_List *list, const uint8_t *data)
     // closest match is found if we move back to where we have already been
 
     while (true) {
-        const int r = memcmp(data, list->data + list->element_size * i, list->element_size);
+        const int r = list->cmp_callback(data, list->data + list->element_size * i, list->element_size);
 
         if (r == 0) {
             return i;
@@ -135,14 +135,15 @@ static bool resize(BS_List *list, uint32_t new_size)
 }
 
 
-int bs_list_init(BS_List *list, uint32_t element_size, uint32_t initial_capacity)
+int bs_list_init(BS_List *list, uint32_t element_size, uint32_t initial_capacity, bs_list_cmp_cb *cmp_callback)
 {
     // set initial values
     list->n = 0;
     list->element_size = element_size;
     list->capacity = 0;
     list->data = nullptr;
     list->ids = nullptr;
+    list->cmp_callback = cmp_callback;
 
     if (initial_capacity != 0) {
         if (!resize(list, initial_capacity)) {

toxcore/list.h

@@ -12,6 +12,7 @@
 #define C_TOXCORE_TOXCORE_LIST_H
 
 #include <stdbool.h>
+#include <stddef.h>  // size_t
 #include <stdint.h>
 
 #include "attributes.h"
@@ -20,12 +21,15 @@
 extern "C" {
 #endif
 
+typedef int bs_list_cmp_cb(const void *a, const void *b, size_t element_size);
+
 typedef struct BS_List {
     uint32_t n; // number of elements
     uint32_t capacity; // number of elements memory is allocated for
     uint32_t element_size; // size of the elements
     uint8_t *data; // array of elements
     int *ids; // array of element ids
+    bs_list_cmp_cb *cmp_callback;
 } BS_List;
 
 /** @brief Initialize a list.
@@ -37,7 +41,7 @@ typedef struct BS_List {
  * @retval 0 failure
  */
 non_null()
-int bs_list_init(BS_List *list, uint32_t element_size, uint32_t initial_capacity);
+int bs_list_init(BS_List *list, uint32_t element_size, uint32_t initial_capacity, bs_list_cmp_cb *cmp_callback);
 
 /** Free a list initiated with list_init */
 nullable(1)

toxcore/list_test.cc

@@ -7,21 +7,21 @@ namespace {
 TEST(List, CreateAndDestroyWithNonZeroSize)
 {
     BS_List list;
-    bs_list_init(&list, sizeof(int), 10);
+    bs_list_init(&list, sizeof(int), 10, memcmp);
     bs_list_free(&list);
 }
 
 TEST(List, CreateAndDestroyWithZeroSize)
 {
     BS_List list;
-    bs_list_init(&list, sizeof(int), 0);
+    bs_list_init(&list, sizeof(int), 0, memcmp);
     bs_list_free(&list);
 }
 
 TEST(List, DeleteFromEmptyList)
 {
     BS_List list;
-    bs_list_init(&list, sizeof(int), 0);
+    bs_list_init(&list, sizeof(int), 0, memcmp);
     const uint8_t data[sizeof(int)] = {0};
     bs_list_remove(&list, data, 0);
     bs_list_free(&list);

toxcore/net_crypto.c

@@ -3163,7 +3163,7 @@ Net_Crypto *new_net_crypto(const Logger *log, const Memory *mem, const Random *r
     networking_registerhandler(dht_get_net(dht), NET_PACKET_CRYPTO_HS, &udp_handle_packet, temp);
     networking_registerhandler(dht_get_net(dht), NET_PACKET_CRYPTO_DATA, &udp_handle_packet, temp);
 
-    bs_list_init(&temp->ip_port_list, sizeof(IP_Port), 8);
+    bs_list_init(&temp->ip_port_list, sizeof(IP_Port), 8, ipport_cmp_handler);
 
     return temp;
 }

toxcore/network.c

@@ -1435,6 +1435,45 @@ bool ipport_equal(const IP_Port *a, const IP_Port *b)
     return ip_equal(&a->ip, &b->ip);
 }
 
+static int ip4_cmp(const IP4 *a, const IP4 *b)
+{
+    return a->uint32 - b->uint32;
+}
+
+static int ip6_cmp(const IP6 *a, const IP6 *b)
+{
+    if (a->uint64[0] != b->uint64[0]) {
+        return a->uint64[0] - b->uint64[0];
+    }
+    return a->uint64[1] - b->uint64[1];
+}
+
+static int ip_cmp(const IP *a, const IP *b)
+{
+    if (a->family.value != b->family.value) {
+        return a->family.value - b->family.value;
+    }
+    if (net_family_is_ipv4(a->family)) {
+        return ip4_cmp(&a->ip.v4, &b->ip.v4);
+    }
+    // If family isn't ipv4, assume ipv6 and compare as many bytes as we can.
+    return ip6_cmp(&a->ip.v6, &b->ip.v6);
+}
+
+int ipport_cmp_handler(const void *a, const void *b, size_t size)
+{
+    const IP_Port *ipp_a = (const IP_Port *)a;
+    const IP_Port *ipp_b = (const IP_Port *)b;
+    assert(size == sizeof(IP_Port));
+
+    const int ip_res = ip_cmp(&ipp_a->ip, &ipp_b->ip);
+    if (ip_res != 0) {
+        return ip_res;
+    }
+
+    return ipp_a->port - ipp_b->port;
+}
+
 /** nulls out ip */
 void ip_reset(IP *ip)
 {
@@ -1508,7 +1547,14 @@ void ipport_copy(IP_Port *target, const IP_Port *source)
         return;
     }
 
-    *target = *source;
+    // Write to a temporary object first, so that padding bytes are
+    // uninitialised and msan can catch mistakes in downstream code.
+    IP_Port tmp;
+    tmp.ip.family = source->ip.family;
+    tmp.ip.ip = source->ip.ip;
+    tmp.port = source->port;
+
+    *target = tmp;
 }
 
 const char *net_ip_ntoa(const IP *ip, Ip_Ntoa *ip_str)

toxcore/network.h

@@ -337,7 +337,7 @@ non_null()
 bool addr_parse_ip(const char *address, IP *to);
 
 /**
- * Compares two IPAny structures.
+ * Compares two IP structures.
  *
  * Unset means unequal.
  *
@@ -347,7 +347,7 @@ nullable(1, 2)
 bool ip_equal(const IP *a, const IP *b);
 
 /**
- * Compares two IPAny_Port structures.
+ * Compares two IP_Port structures.
  *
  * Unset means unequal.
  *
@@ -356,6 +356,14 @@ bool ip_equal(const IP *a, const IP *b);
 nullable(1, 2)
 bool ipport_equal(const IP_Port *a, const IP_Port *b);
 
+/**
+ * @brief IP_Port comparison function with `memcmp` signature.
+ *
+ * Casts the void pointers to `IP_Port *` for comparison.
+ */
+non_null()
+int ipport_cmp_handler(const void *a, const void *b, size_t size);
+
 /** nulls out ip */
 non_null()
 void ip_reset(IP *ip);

toxcore/network_test.cc

@@ -2,10 +2,14 @@
 
 #include <gtest/gtest.h>
 
+#include "crypto_core.h"
+#include "crypto_core_test_util.hh"
 #include "network_test_util.hh"
 
 namespace {
 
+using ::testing::PrintToString;
+
 TEST(TestUtil, ProducesNonNullNetwork)
 {
     Test_Network net;
@@ -82,4 +86,43 @@ TEST(IpParseAddr, FormatsIPv6)
     EXPECT_EQ(std::string(ip_str), "ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff");
 }
 
+TEST(IpportCmp, BehavesLikeMemcmp)
+{
+    auto cmp_val = [](int val) { return val < 0 ? -1 : val > 0 ? 1 : 0; };
+
+    IP_Port a = {0};
+    IP_Port b = {0};
+
+    a.ip.family.value = 10;
+    b.ip.family.value = 10;
+
+    a.port = 10;
+    b.port = 20;
+
+    EXPECT_EQ(  //
+        cmp_val(ipport_cmp_handler(&a, &b, sizeof(IP_Port))),
+        -1)
+        << "a=" << a << "\n"
+        << "b=" << b;
+    EXPECT_EQ(  //
+        cmp_val(ipport_cmp_handler(&a, &b, sizeof(IP_Port))),
+        cmp_val(memcmp(&a, &b, sizeof(IP_Port))))
+        << "a=" << a << "\n"
+        << "b=" << b;
+
+    a.ip.ip.v6.uint8[0] = 0xba;
+    b.ip.ip.v6.uint8[0] = 0xab;
+
+    EXPECT_EQ(  //
+        cmp_val(ipport_cmp_handler(&a, &b, sizeof(IP_Port))),
+        1)
+        << "a=" << a << "\n"
+        << "b=" << b;
+    EXPECT_EQ(  //
+        cmp_val(ipport_cmp_handler(&a, &b, sizeof(IP_Port))),
+        cmp_val(memcmp(&a, &b, sizeof(IP_Port))))
+        << "a=" << a << "\n"
+        << "b=" << b;
+}
+
 }  // namespace