refactor: Simplify locking logic in DHT

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.

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;

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,15 +1838,16 @@ 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);
 
     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;
         }

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

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. */