fix: Don't use memcmp to compare IP_Ports.

`memcmp` compares padding bytes as well, which may be arbitrary or
uninitialised.
This commit is contained in:
iphydf 2024-01-26 16:12:40 +00:00
parent d94246a906
commit 39aadf8922
No known key found for this signature in database
GPG Key ID: 3855DBA2D74403C9
13 changed files with 197 additions and 18 deletions

View File

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

View File

@ -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;
}

View File

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

View File

@ -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,7 +135,7 @@ 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;
@ -143,6 +143,7 @@ int bs_list_init(BS_List *list, uint32_t element_size, uint32_t initial_capacity
list->capacity = 0;
list->data = nullptr;
list->ids = nullptr;
list->cmp_callback = cmp_callback;
if (initial_capacity != 0) {
if (!resize(list, initial_capacity)) {

View File

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

View File

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

View File

@ -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;
}

View File

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

View File

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

View File

@ -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

View File

@ -161,6 +161,11 @@ uint64_t min_u64(uint64_t a, uint64_t b)
return a < b ? a : b;
}
int cmp_uint(uint64_t a, uint64_t b)
{
return (a > b ? 1 : 0) - (a < b ? 1 : 0);
}
uint32_t jenkins_one_at_a_time_hash(const uint8_t *key, size_t len)
{
uint32_t hash = 0;

View File

@ -79,6 +79,9 @@ uint16_t min_u16(uint16_t a, uint16_t b);
uint32_t min_u32(uint32_t a, uint32_t b);
uint64_t min_u64(uint64_t a, uint64_t b);
// Comparison function: return -1 if a<b, 0 if a==b, 1 if a>b.
int cmp_uint(uint64_t a, uint64_t b);
/** @brief Returns a 32-bit hash of key of size len */
non_null()
uint32_t jenkins_one_at_a_time_hash(const uint8_t *key, size_t len);

View File

@ -34,4 +34,15 @@ TEST(Util, IdCopyMakesKeysEqual)
EXPECT_TRUE(pk_equal(pk1, pk2));
}
TEST(Cmp, OrdersNumbersCorrectly)
{
EXPECT_EQ(cmp_uint(1, 2), -1);
EXPECT_EQ(cmp_uint(0, UINT32_MAX), -1);
EXPECT_EQ(cmp_uint(UINT32_MAX, 0), 1);
EXPECT_EQ(cmp_uint(UINT32_MAX, UINT32_MAX), 0);
EXPECT_EQ(cmp_uint(0, UINT64_MAX), -1);
EXPECT_EQ(cmp_uint(UINT64_MAX, 0), 1);
EXPECT_EQ(cmp_uint(UINT64_MAX, UINT64_MAX), 0);
}
} // namespace