StarMirror/toxcore
1
0
Fork 0
mirror of https://github.com/irungentoo/toxcore.git synced 2024-03-22 13:30:51 +08:00
Code Issues Projects Releases Wiki Activity

refactor: Simplify locking logic in DHT

Browse Source 
This fixes an error reported by cppcheck about overflowing the callback
array. The locking logic probably was broken, so I replaced it with
something simpler and more robust.
This commit is contained in:
sudden6 2022-05-24 22:22:21 +02:00
parent 0f1db935a9
commit 4062c3a2ba
No known key found for this signature in database
GPG Key ID: 279509B499E032B9
4 changed files with 96 additions and 48 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
auto_tests/onion_test.c
View File

@ -500,10 +500,10 @@ static void dht_pk_callback(void *object, int32_t number, const uint8_t *dht_pub
{
if ((NUM_FIRST == number && !first) || (NUM_LAST == number && !last)) {
Onions *on = (Onions *)object;
uint16_t count = 0;
int ret = dht_addfriend(on->onion->dht, dht_public_key, &dht_ip_callback, object, number, &count);
uint32_t token = 0;
int ret = dht_addfriend(on->onion->dht, dht_public_key, &dht_ip_callback, object, number, &token);
ck_assert_msg(ret == 0, "dht_addfriend() did not return 0");
ck_assert_msg(count == 1, "Count not 1, count is %u", count);
ck_assert_msg(token == 1, "Count not 1, count is %u", token);
if (NUM_FIRST == number && !first) {
first = 1;

93
toxcore/DHT.c
View File

@ -43,6 +43,9 @@
/** Number of get node requests to send to quickly find close nodes. */
#define MAX_BOOTSTRAP_TIMES 5
// TODO(sudden6): find out why we need multiple callbacks and if we really need 32
#define DHT_FRIEND_MAX_LOCKS 32
typedef struct DHT_Friend_Callback {
dht_ip_cb *ip_callback;
void *data;
@ -61,7 +64,8 @@ struct DHT_Friend {
/* Symmetric NAT hole punching stuff. */
NAT nat;
uint16_t lock_count;
/* Each set bit represents one installed callback */
uint32_t lock_flags;
DHT_Friend_Callback callbacks[DHT_FRIEND_MAX_LOCKS];
Node_format to_bootstrap[MAX_SENT_NODES];
@ -71,6 +75,8 @@ struct DHT_Friend {
static const DHT_Friend empty_dht_friend = {{0}};
const Node_format empty_node_format = {{0}};
static_assert(sizeof (empty_dht_friend.lock_flags) * 8 == DHT_FRIEND_MAX_LOCKS, "Bitfield size and number of locks don't match");
typedef struct Cryptopacket_Handler {
cryptopacket_handler_cb *function;
void *object;
@ -1419,8 +1425,9 @@ uint32_t addto_lists(DHT *dht, const IP_Port *ip_port, const uint8_t *public_key
return used;
}
for (uint32_t i = 0; i < friend_foundip->lock_count; ++i) {
if (friend_foundip->callbacks[i].ip_callback != nullptr) {
for (uint32_t i = 0; i < DHT_FRIEND_MAX_LOCKS; ++i) {
const bool has_lock = (friend_foundip->lock_flags & (UINT32_C(1) << i)) > 0;
if (has_lock && friend_foundip->callbacks[i].ip_callback != nullptr) {
friend_foundip->callbacks[i].ip_callback(friend_foundip->callbacks[i].data,
friend_foundip->callbacks[i].number, &ipp_copy);
}
@ -1745,35 +1752,75 @@ static int handle_sendnodes_ipv6(void *object, const IP_Port *source, const uint
/*----------------------------------------------------------------------------------*/
/*------------------------END of packet handling functions--------------------------*/
non_null(1) nullable(2, 3, 5)
static void dht_friend_lock(DHT_Friend *const dht_friend, dht_ip_cb *ip_callback,
void *data, int32_t number, uint16_t *lock_count)
non_null(1) nullable(2, 3)
static uint32_t dht_friend_lock(DHT_Friend *const dht_friend, dht_ip_cb *ip_callback,
void *data, int32_t number)
{
const uint16_t lock_num = dht_friend->lock_count;
++dht_friend->lock_count;
// find first free slot
uint8_t lock_num;
uint32_t lock_token = 0;
for (lock_num = 0; lock_num < DHT_FRIEND_MAX_LOCKS; ++lock_num) {
lock_token = UINT32_C(1) << lock_num;
if ((dht_friend->lock_flags & lock_token) == 0) {
break;
}
}
// One of the conditions would be enough, but static analyzers don't get that
if (lock_token == 0 || lock_num == DHT_FRIEND_MAX_LOCKS) {
return 0;
}
// Claim that slot
dht_friend->lock_flags |= lock_token;
dht_friend->callbacks[lock_num].ip_callback = ip_callback;
dht_friend->callbacks[lock_num].data = data;
dht_friend->callbacks[lock_num].number = number;
if (lock_count != nullptr) {
*lock_count = lock_num + 1;
return lock_token;
}
non_null()
static void dht_friend_unlock(DHT_Friend *const dht_friend, uint32_t lock_token)
{
// If this triggers, there was a double free
assert((lock_token & dht_friend->lock_flags) > 0);
// find used slot
uint8_t lock_num;
for (lock_num = 0; lock_num < DHT_FRIEND_MAX_LOCKS; ++lock_num) {
if ((dht_friend->lock_flags & lock_token) > 0) {
break;
}
}
if (lock_num == DHT_FRIEND_MAX_LOCKS) {
// Gracefully handle double unlock
return;
}
// Clear the slot
dht_friend->lock_flags &= ~lock_token;
dht_friend->callbacks[lock_num].ip_callback = nullptr;
dht_friend->callbacks[lock_num].data = nullptr;
dht_friend->callbacks[lock_num].number = 0;
}
int dht_addfriend(DHT *dht, const uint8_t *public_key, dht_ip_cb *ip_callback,
void *data, int32_t number, uint16_t *lock_count)
void *data, int32_t number, uint32_t *lock_token)
{
const uint32_t friend_num = index_of_friend_pk(dht->friends_list, dht->num_friends, public_key);
if (friend_num != UINT32_MAX) { /* Is friend already in DHT? */
DHT_Friend *const dht_friend = &dht->friends_list[friend_num];
const uint32_t tmp_lock_token = dht_friend_lock(dht_friend, ip_callback, data, number);
if (dht_friend->lock_count == DHT_FRIEND_MAX_LOCKS) {
if (tmp_lock_token == 0) {
return -1;
}
dht_friend_lock(dht_friend, ip_callback, data, number, lock_count);
return 0;
}
@ -1791,7 +1838,8 @@ int dht_addfriend(DHT *dht, const uint8_t *public_key, dht_ip_cb *ip_callback,
dht_friend->nat.nat_ping_id = random_u64(dht->rng);
++dht->num_friends;
dht_friend_lock(dht_friend, ip_callback, data, number, lock_count);
*lock_token = dht_friend_lock(dht_friend, ip_callback, data, number);
assert(*lock_token != 0); // Friend was newly allocated
dht_friend->num_to_bootstrap = get_close_nodes(dht, dht_friend->public_key, dht_friend->to_bootstrap, net_family_unspec(),
true, false);
@ -1799,7 +1847,7 @@ int dht_addfriend(DHT *dht, const uint8_t *public_key, dht_ip_cb *ip_callback,
return 0;
}
int dht_delfriend(DHT *dht, const uint8_t *public_key, uint16_t lock_count)
int dht_delfriend(DHT *dht, const uint8_t *public_key, uint32_t lock_token)
{
const uint32_t friend_num = index_of_friend_pk(dht->friends_list, dht->num_friends, public_key);
@ -1808,13 +1856,9 @@ int dht_delfriend(DHT *dht, const uint8_t *public_key, uint16_t lock_count)
}
DHT_Friend *const dht_friend = &dht->friends_list[friend_num];
--dht_friend->lock_count;
if (dht_friend->lock_count > 0 && lock_count > 0) { /* DHT friend is still in use.*/
--lock_count;
dht_friend->callbacks[lock_count].ip_callback = nullptr;
dht_friend->callbacks[lock_count].data = nullptr;
dht_friend->callbacks[lock_count].number = 0;
dht_friend_unlock(dht_friend, lock_token);
if (dht_friend->lock_flags > 0) {
/* DHT friend is still in use.*/
return 0;
}
@ -2765,7 +2809,8 @@ DHT *new_dht(const Logger *log, const Random *rng, const Network *ns, Mono_Time
crypto_new_keypair(rng, random_public_key_bytes, random_secret_key_bytes);
if (dht_addfriend(dht, random_public_key_bytes, nullptr, nullptr, 0, nullptr) != 0) {
uint32_t token; // We don't intend to delete these ever, but need to pass the token
if (dht_addfriend(dht, random_public_key_bytes, nullptr, nullptr, 0, &token) != 0) {
kill_dht(dht);
return nullptr;
}

19
toxcore/DHT.h
View File

@ -172,8 +172,6 @@ typedef struct NAT {
uint64_t nat_ping_timestamp;
} NAT;
#define DHT_FRIEND_MAX_LOCKS 32
typedef struct Node_format {
uint8_t public_key[CRYPTO_PUBLIC_KEY_SIZE];
IP_Port ip_port;
@ -327,29 +325,34 @@ non_null(1) nullable(2)
void dht_callback_get_nodes_response(DHT *dht, dht_get_nodes_response_cb *function);
/** @brief Add a new friend to the friends list.
* public_key must be CRYPTO_PUBLIC_KEY_SIZE bytes long.
* @param public_key must be CRYPTO_PUBLIC_KEY_SIZE bytes long.
*
* ip_callback is the callback of a function that will be called when the ip address
* @param ip_callback is the callback of a function that will be called when the ip address
* is found along with arguments data and number.
* @param data User data for the callback
* @param number Will be passed to ip_callback
*
* lock_count will be set to a non zero number that must be passed to `dht_delfriend()`
* @param lock_token will be set to a non zero number that must be passed to `dht_delfriend()`
* to properly remove the callback.
*
* @retval 0 if success.
* @retval -1 if failure (friends list is full).
*/
non_null(1, 2) nullable(3, 4, 6)
non_null(1, 2, 6) nullable(3, 4)
int dht_addfriend(DHT *dht, const uint8_t *public_key, dht_ip_cb *ip_callback,
void *data, int32_t number, uint16_t *lock_count);
void *data, int32_t number, uint32_t *lock_token);
/** @brief Delete a friend from the friends list.
* public_key must be CRYPTO_PUBLIC_KEY_SIZE bytes long.
* @param dht The DHT object
* @param public_key The public key of the friend
* @param lock_token The token received by dht_addfriend(...)
*
* @retval 0 if success.
* @retval -1 if failure (public_key not in friends list).
*/
non_null()
int dht_delfriend(DHT *dht, const uint8_t *public_key, uint16_t lock_count);
int dht_delfriend(DHT *dht, const uint8_t *public_key, uint32_t lock_token);
/** @brief Get ip of friend.
*

26
toxcore/friend_connection.c
View File

@ -31,7 +31,7 @@ struct Friend_Conn {
uint8_t real_public_key[CRYPTO_PUBLIC_KEY_SIZE];
uint8_t dht_temp_pk[CRYPTO_PUBLIC_KEY_SIZE];
uint16_t dht_lock;
uint32_t dht_lock_token;
IP_Port dht_ip_port;
uint64_t dht_pk_lastrecv;
uint64_t dht_ip_port_lastrecv;
@ -377,16 +377,15 @@ static void change_dht_pk(Friend_Connections *fr_c, int friendcon_id, const uint
friend_con->dht_pk_lastrecv = mono_time_get(fr_c->mono_time);
if (friend_con->dht_lock > 0) {
if (dht_delfriend(fr_c->dht, friend_con->dht_temp_pk, friend_con->dht_lock) != 0) {
if (friend_con->dht_lock_token > 0) {
if (dht_delfriend(fr_c->dht, friend_con->dht_temp_pk, friend_con->dht_lock_token) != 0) {
LOGGER_ERROR(fr_c->logger, "a. Could not delete dht peer. Please report this.");
return;
}
friend_con->dht_lock = 0;
friend_con->dht_lock_token = 0;
}
dht_addfriend(fr_c->dht, dht_public_key, dht_ip_callback, fr_c, friendcon_id, &friend_con->dht_lock);
dht_addfriend(fr_c->dht, dht_public_key, dht_ip_callback, fr_c, friendcon_id, &friend_con->dht_lock_token);
memcpy(friend_con->dht_temp_pk, dht_public_key, CRYPTO_PUBLIC_KEY_SIZE);
}
@ -609,7 +608,7 @@ static int friend_new_connection(Friend_Connections *fr_c, int friendcon_id)
}
/* If dht_temp_pk does not contains a pk. */
if (friend_con->dht_lock == 0) {
if (friend_con->dht_lock_token == 0) {
return -1;
}
@ -838,8 +837,9 @@ int kill_friend_connection(Friend_Connections *fr_c, int friendcon_id)
onion_delfriend(fr_c->onion_c, friend_con->onion_friendnum);
crypto_kill(fr_c->net_crypto, friend_con->crypt_connection_id);
if (friend_con->dht_lock > 0) {
dht_delfriend(fr_c->dht, friend_con->dht_temp_pk, friend_con->dht_lock);
if (friend_con->dht_lock_token > 0) {
dht_delfriend(fr_c->dht, friend_con->dht_temp_pk, friend_con->dht_lock_token);
friend_con->dht_lock_token = 0;
}
return wipe_friend_conn(fr_c, friendcon_id);
@ -967,9 +967,9 @@ void do_friend_connections(Friend_Connections *fr_c, void *userdata)
if (friend_con != nullptr) {
if (friend_con->status == FRIENDCONN_STATUS_CONNECTING) {
if (friend_con->dht_pk_lastrecv + FRIEND_DHT_TIMEOUT < temp_time) {
if (friend_con->dht_lock > 0) {
dht_delfriend(fr_c->dht, friend_con->dht_temp_pk, friend_con->dht_lock);
friend_con->dht_lock = 0;
if (friend_con->dht_lock_token > 0) {
dht_delfriend(fr_c->dht, friend_con->dht_temp_pk, friend_con->dht_lock_token);
friend_con->dht_lock_token = 0;
memset(friend_con->dht_temp_pk, 0, CRYPTO_PUBLIC_KEY_SIZE);
}
}
@ -978,7 +978,7 @@ void do_friend_connections(Friend_Connections *fr_c, void *userdata)
friend_con->dht_ip_port.ip.family = net_family_unspec();
}
if (friend_con->dht_lock > 0) {
if (friend_con->dht_lock_token > 0) {
if (friend_new_connection(fr_c, i) == 0) {
set_direct_ip_port(fr_c->net_crypto, friend_con->crypt_connection_id, &friend_con->dht_ip_port, false);
connect_to_saved_tcp_relays(fr_c, i, MAX_FRIEND_TCP_CONNECTIONS / 2); /* Only fill it half up. */