diff --git a/auto_tests/onion_test.c b/auto_tests/onion_test.c index 34f5e21b..36fcc202 100644 --- a/auto_tests/onion_test.c +++ b/auto_tests/onion_test.c @@ -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; diff --git a/toxcore/DHT.c b/toxcore/DHT.c index 8aa16110..b1f444f1 100644 --- a/toxcore/DHT.c +++ b/toxcore/DHT.c @@ -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; } diff --git a/toxcore/DHT.h b/toxcore/DHT.h index 34ecf9dd..3fc5c87b 100644 --- a/toxcore/DHT.h +++ b/toxcore/DHT.h @@ -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. * diff --git a/toxcore/friend_connection.c b/toxcore/friend_connection.c index 9e5dee7c..c65e3105 100644 --- a/toxcore/friend_connection.c +++ b/toxcore/friend_connection.c @@ -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. */