fix: Don't use memcmp to compare IP_Ports.

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

Author: iphydf

other/bootstrap_daemon/docker/tox-bootstrapd.sha256

@@ -1 +1 @@
-e5ccf844b7ece48d1153c226bdf8462d0e85d517dfcd720c8d6c4abad7da6929  /usr/local/bin/tox-bootstrapd
+af83f07bb96eb17a7ee69f174c9960d23758c9c9314144d73e0f57fdef5d55e4  /usr/local/bin/tox-bootstrapd

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/group.c

@@ -956,11 +956,6 @@ static bool delpeer(Group_Chats *g_c, uint32_t groupnumber, int peer_index, void
     return true;
 }
 
-static int cmp_u64(uint64_t a, uint64_t b)
-{
-    return (a > b ? 1 : 0) - (a < b ? 1 : 0);
-}
-
 /** Order peers with friends first and with more recently active earlier */
 non_null()
 static int cmp_frozen(const void *a, const void *b)
@@ -972,7 +967,7 @@ static int cmp_frozen(const void *a, const void *b)
         return pa->is_friend ? -1 : 1;
     }
 
-    return cmp_u64(pb->last_active, pa->last_active);
+    return cmp_uint(pb->last_active, pa->last_active);
 }
 
 /** @brief Delete frozen peers as necessary to ensure at most `g->maxfrozen` remain.

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 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,61 @@ bool ipport_equal(const IP_Port *a, const IP_Port *b)
     return ip_equal(&a->ip, &b->ip);
 }
 
+non_null()
+static int ip4_cmp(const IP4 *a, const IP4 *b)
+{
+    return cmp_uint(a->uint32, b->uint32);
+}
+
+non_null()
+static int ip6_cmp(const IP6 *a, const IP6 *b)
+{
+    const int res = cmp_uint(a->uint64[0], b->uint64[0]);
+    if (res != 0) {
+        return res;
+    }
+    return cmp_uint(a->uint64[1], b->uint64[1]);
+}
+
+non_null()
+static int ip_cmp(const IP *a, const IP *b)
+{
+    const int res = cmp_uint(a->family.value, b->family.value);
+    if (res != 0) {
+        return res;
+    }
+    switch (a->family.value) {
+        case TOX_AF_UNSPEC:
+            return 0;
+        case TOX_AF_INET:
+        case TCP_INET:
+        case TOX_TCP_INET:
+            return ip4_cmp(&a->ip.v4, &b->ip.v4);
+        case TOX_AF_INET6:
+        case TCP_INET6:
+        case TOX_TCP_INET6:
+        case TCP_SERVER_FAMILY:  // these happen to be ipv6 according to TCP_server.c.
+        case TCP_CLIENT_FAMILY:
+            return ip6_cmp(&a->ip.v6, &b->ip.v6);
+    }
+    // Invalid, we don't compare any further and consider them equal.
+    return 0;
+}
+
+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 cmp_uint(ipp_a->port, ipp_b->port);
+}
+
 /** nulls out ip */
 void ip_reset(IP *ip)
 {
@@ -1508,7 +1563,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,18 @@ 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.
+ *
+ * @retval -1 if `a < b`
+ * @retval 0 if `a == b`
+ * @retval 1 if `a > b`
+ */
+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

@@ -82,4 +82,90 @@ 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 = net_family_ipv4();
+    b.ip.family = net_family_ipv4();
+
+    a.port = 10;
+    b.port = 20;
+
+    EXPECT_EQ(  //
+        ipport_cmp_handler(&a, &b, sizeof(IP_Port)), -1)
+        << "a=" << a << "\n"
+        << "b=" << b;
+    EXPECT_EQ(  //
+        ipport_cmp_handler(&a, &b, sizeof(IP_Port)),  //
+        cmp_val(memcmp(&a, &b, sizeof(IP_Port))))
+        << "a=" << a << "\n"
+        << "b=" << b;
+
+    a.ip.ip.v4.uint8[0] = 192;
+    b.ip.ip.v4.uint8[0] = 10;
+
+    EXPECT_EQ(  //
+        ipport_cmp_handler(&a, &b, sizeof(IP_Port)), 1)
+        << "a=" << a << "\n"
+        << "b=" << b;
+    EXPECT_EQ(  //
+        ipport_cmp_handler(&a, &b, sizeof(IP_Port)),  //
+        cmp_val(memcmp(&a, &b, sizeof(IP_Port))))
+        << "a=" << a << "\n"
+        << "b=" << b;
+}
+
+TEST(IpportCmp, Ipv6BeginAndEndCompareCorrectly)
+{
+    IP_Port a = {0};
+    IP_Port b = {0};
+
+    a.ip.family = net_family_ipv6();
+    b.ip.family = net_family_ipv6();
+
+    a.ip.ip.v6.uint8[0] = 0xab;
+    b.ip.ip.v6.uint8[0] = 0xba;
+
+    EXPECT_EQ(ipport_cmp_handler(&a, &b, sizeof(IP_Port)), -1);
+
+    a.ip.ip.v6.uint8[0] = 0;
+    b.ip.ip.v6.uint8[0] = 0;
+
+    a.ip.ip.v6.uint8[15] = 0xba;
+
+    EXPECT_EQ(ipport_cmp_handler(&a, &b, sizeof(IP_Port)), 1);
+}
+
+TEST(IpportCmp, UnspecAlwaysComparesEqual)
+{
+    IP_Port a = {0};
+    IP_Port b = {0};
+
+    a.ip.family = net_family_unspec();
+    b.ip.family = net_family_unspec();
+
+    a.ip.ip.v4.uint8[0] = 0xab;
+    b.ip.ip.v4.uint8[0] = 0xba;
+
+    EXPECT_EQ(ipport_cmp_handler(&a, &b, sizeof(IP_Port)), 0);
+}
+
+TEST(IpportCmp, InvalidAlwaysComparesEqual)
+{
+    IP_Port a = {0};
+    IP_Port b = {0};
+
+    a.ip.family.value = 0xff;
+    b.ip.family.value = 0xff;
+
+    a.ip.ip.v4.uint8[0] = 0xab;
+    b.ip.ip.v4.uint8[0] = 0xba;
+
+    EXPECT_EQ(ipport_cmp_handler(&a, &b, sizeof(IP_Port)), 0);
+}
+
 }  // namespace