fix invalid use of mutex

- Moving a pthread_mutex in memory (e.g. via realloc()) is undefined
  behavior.
- add a state for allocated but not yet used crypto connections
- change crypto_connection_status signature

3 files changed, 116 insertions, 89 deletions

diff --git a/toxcore/Messenger.c b/toxcore/Messenger.c
index ae2d1d3b..2dccd4a8 100644
--- a/toxcore/Messenger.c
+++ b/toxcore/Messenger.c
@@ -458,6 +458,8 @@ int m_get_friend_connectionstatus(const Messenger *m, int32_t friendnumber)
         bool direct_connected = 0;
         unsigned int num_online_relays = 0;
         int crypt_conn_id = friend_connection_crypt_connection_id(m->fr_c, m->friendlist[friendnumber].friendcon_id);
+
+        // FIXME(sudden6): handle return value
         crypto_connection_status(m->net_crypto, crypt_conn_id, &direct_connected, &num_online_relays);
 
         if (direct_connected) {
diff --git a/toxcore/net_crypto.c b/toxcore/net_crypto.c
index 3ace3bea..da525a1f 100644
--- a/toxcore/net_crypto.c
+++ b/toxcore/net_crypto.c
@@ -48,6 +48,18 @@ typedef struct Packets_Array {
     uint32_t  buffer_end; /* packet numbers in array: {buffer_start, buffer_end) */
 } Packets_Array;
 
+typedef enum Crypto_Conn_State {
+    CRYPTO_CONN_FREE = 0,            /* the connection slot is free; this value is 0 so it is valid after
+                                      * crypto_memzero(...) of the parent struct
+                                      */
+    CRYPTO_CONN_NO_CONNECTION,       /* the connection is allocated, but not yet used */
+    CRYPTO_CONN_COOKIE_REQUESTING,   /* we are sending cookie request packets */
+    CRYPTO_CONN_HANDSHAKE_SENT,      /* we are sending handshake packets */
+    CRYPTO_CONN_NOT_CONFIRMED,       /* we are sending handshake packets;
+                                      * we have received one from the other, but no data */
+    CRYPTO_CONN_ESTABLISHED,         /* the connection is established */
+} Crypto_Conn_State;
+
 typedef struct Crypto_Connection {
     uint8_t public_key[CRYPTO_PUBLIC_KEY_SIZE]; /* The real public key of the peer. */
     uint8_t recv_nonce[CRYPTO_NONCE_SIZE]; /* Nonce of received packets. */
@@ -56,14 +68,7 @@ typedef struct Crypto_Connection {
     uint8_t sessionsecret_key[CRYPTO_SECRET_KEY_SIZE]; /* Our private key for this session. */
     uint8_t peersessionpublic_key[CRYPTO_PUBLIC_KEY_SIZE]; /* The public key of the peer. */
     uint8_t shared_key[CRYPTO_SHARED_KEY_SIZE]; /* The precomputed shared key from encrypt_precompute. */
-    /**
-     * 0 if no connection,
-     * 1 we are sending cookie request packets,
-     * 2 if we are sending handshake packets,
-     * 3 if connection is not confirmed yet (we have received a handshake but no data packets yet),
-     * 4 if the connection is established.
-     */
-    Crypto_Conn_State status;
+    Crypto_Conn_State status; /* See Crypto_Conn_State documentation */
     uint64_t cookie_request_number; /* number used in the cookie request packets for this connection */
     uint8_t dht_public_key[CRYPTO_PUBLIC_KEY_SIZE]; /* The dht public key of the peer */
 
@@ -125,7 +130,8 @@ typedef struct Crypto_Connection {
 
     uint8_t maximum_speed_reached;
 
-    pthread_mutex_t mutex;
+    /* Must be a pointer, because the struct is moved in memory */
+    pthread_mutex_t *mutex;
 
     dht_pk_cb *dht_pk_callback;
     void *dht_pk_callback_object;
@@ -193,7 +199,9 @@ static uint8_t crypt_connection_id_not_valid(const Net_Crypto *c, int crypt_conn
         return 1;
     }
 
-    if (c->crypto_connections[crypt_connection_id].status == CRYPTO_CONN_NO_CONNECTION) {
+    const Crypto_Conn_State status = c->crypto_connections[crypt_connection_id].status;
+
+    if (status == CRYPTO_CONN_NO_CONNECTION || status == CRYPTO_CONN_FREE) {
         return 1;
     }
 
@@ -680,21 +688,23 @@ static int send_packet_to(Net_Crypto *c, int crypt_connection_id, const uint8_t
 
     int direct_send_attempt = 0;
 
-    pthread_mutex_lock(&conn->mutex);
+    pthread_mutex_lock(conn->mutex);
     IP_Port ip_port = return_ip_port_connection(c, crypt_connection_id);
 
     // TODO(irungentoo): on bad networks, direct connections might not last indefinitely.
     if (!net_family_is_unspec(ip_port.ip.family)) {
         bool direct_connected = 0;
+
+        // FIXME(sudden6): handle return value
         crypto_connection_status(c, crypt_connection_id, &direct_connected, nullptr);
 
         if (direct_connected) {
             if ((uint32_t)sendpacket(dht_get_net(c->dht), ip_port, data, length) == length) {
-                pthread_mutex_unlock(&conn->mutex);
+                pthread_mutex_unlock(conn->mutex);
                 return 0;
             }
 
-            pthread_mutex_unlock(&conn->mutex);
+            pthread_mutex_unlock(conn->mutex);
             return -1;
         }
 
@@ -710,18 +720,18 @@ static int send_packet_to(Net_Crypto *c, int crypt_connection_id, const uint8_t
         }
     }
 
-    pthread_mutex_unlock(&conn->mutex);
+    pthread_mutex_unlock(conn->mutex);
     pthread_mutex_lock(&c->tcp_mutex);
     int ret = send_packet_tcp_connection(c->tcp_c, conn->connection_number_tcp, data, length);
     pthread_mutex_unlock(&c->tcp_mutex);
 
-    pthread_mutex_lock(&conn->mutex);
+    pthread_mutex_lock(conn->mutex);
 
     if (ret == 0) {
         conn->last_tcp_sent = current_time_monotonic(c->mono_time);
     }
 
-    pthread_mutex_unlock(&conn->mutex);
+    pthread_mutex_unlock(conn->mutex);
 
     if (ret == 0 || direct_send_attempt) {
         return 0;
@@ -1073,19 +1083,19 @@ static int send_data_packet(Net_Crypto *c, int crypt_connection_id, const uint8_
         return -1;
     }
 
-    pthread_mutex_lock(&conn->mutex);
+    pthread_mutex_lock(conn->mutex);
     VLA(uint8_t, packet, 1 + sizeof(uint16_t) + length + CRYPTO_MAC_SIZE);
     packet[0] = NET_PACKET_CRYPTO_DATA;
     memcpy(packet + 1, conn->sent_nonce + (CRYPTO_NONCE_SIZE - sizeof(uint16_t)), sizeof(uint16_t));
     const int len = encrypt_data_symmetric(conn->shared_key, conn->sent_nonce, data, length, packet + 1 + sizeof(uint16_t));
 
     if (len + 1 + sizeof(uint16_t) != SIZEOF_VLA(packet)) {
-        pthread_mutex_unlock(&conn->mutex);
+        pthread_mutex_unlock(conn->mutex);
         return -1;
     }
 
     increment_nonce(conn->sent_nonce);
-    pthread_mutex_unlock(&conn->mutex);
+    pthread_mutex_unlock(conn->mutex);
 
     return send_packet_to(c, crypt_connection_id, packet, SIZEOF_VLA(packet));
 }
@@ -1172,9 +1182,9 @@ static int64_t send_lossless_packet(Net_Crypto *c, int crypt_connection_id, cons
     dt.sent_time = 0;
     dt.length = length;
     memcpy(dt.data, data, length);
-    pthread_mutex_lock(&conn->mutex);
+    pthread_mutex_lock(conn->mutex);
     int64_t packet_num = add_data_end_of_buffer(c->log, &conn->send_array, &dt);
-    pthread_mutex_unlock(&conn->mutex);
+    pthread_mutex_unlock(conn->mutex);
 
     if (packet_num == -1) {
         return -1;
@@ -1578,9 +1588,9 @@ static int handle_data_packet_core(Net_Crypto *c, int crypt_connection_id, const
         }
 
         while (1) {
-            pthread_mutex_lock(&conn->mutex);
+            pthread_mutex_lock(conn->mutex);
             int ret = read_data_beg_buffer(c->log, &conn->recv_array, &dt);
-            pthread_mutex_unlock(&conn->mutex);
+            pthread_mutex_unlock(conn->mutex);
 
             if (ret == -1) {
                 break;
@@ -1748,12 +1758,6 @@ static int realloc_cryptoconnection(Net_Crypto *c, uint32_t num)
  */
 static int create_crypto_connection(Net_Crypto *c)
 {
-    for (uint32_t i = 0; i < c->crypto_connections_length; ++i) {
-        if (c->crypto_connections[i].status == CRYPTO_CONN_NO_CONNECTION) {
-            return i;
-        }
-    }
-
     while (1) { /* TODO(irungentoo): is this really the best way to do this? */
         pthread_mutex_lock(&c->connections_mutex);
 
@@ -1766,21 +1770,42 @@ static int create_crypto_connection(Net_Crypto *c)
 
     int id = -1;
 
-    if (realloc_cryptoconnection(c, c->crypto_connections_length + 1) == 0) {
-        id = c->crypto_connections_length;
-        ++c->crypto_connections_length;
-        memset(&c->crypto_connections[id], 0, sizeof(Crypto_Connection));
+    for (uint32_t i = 0; i < c->crypto_connections_length; ++i) {
+        if (c->crypto_connections[i].status == CRYPTO_CONN_FREE) {
+            id = i;
+            break;
+        }
+    }
+
+    if (id == -1) {
+        if (realloc_cryptoconnection(c, c->crypto_connections_length + 1) == 0) {
+            id = c->crypto_connections_length;
+            ++c->crypto_connections_length;
+            memset(&c->crypto_connections[id], 0, sizeof(Crypto_Connection));
+        }
+    }
+
+    if (id != -1) {
         // Memsetting float/double to 0 is non-portable, so we explicitly set them to 0
         c->crypto_connections[id].packet_recv_rate = 0;
         c->crypto_connections[id].packet_send_rate = 0;
         c->crypto_connections[id].last_packets_left_rem = 0;
         c->crypto_connections[id].packet_send_rate_requested = 0;
         c->crypto_connections[id].last_packets_left_requested_rem = 0;
+        c->crypto_connections[id].mutex = (pthread_mutex_t *) malloc(sizeof(pthread_mutex_t));
+
+        if (c->crypto_connections[id].mutex == nullptr) {
+            pthread_mutex_unlock(&c->connections_mutex);
+            return -1;
+        }
 
-        if (pthread_mutex_init(&c->crypto_connections[id].mutex, nullptr) != 0) {
+        if (pthread_mutex_init(c->crypto_connections[id].mutex, nullptr) != 0) {
+            free(c->crypto_connections[id].mutex);
             pthread_mutex_unlock(&c->connections_mutex);
             return -1;
         }
+
+        c->crypto_connections[id].status = CRYPTO_CONN_NO_CONNECTION;
     }
 
     pthread_mutex_unlock(&c->connections_mutex);
@@ -1794,21 +1819,29 @@ static int create_crypto_connection(Net_Crypto *c)
  */
 static int wipe_crypto_connection(Net_Crypto *c, int crypt_connection_id)
 {
-    if (crypt_connection_id_not_valid(c, crypt_connection_id)) {
+    if ((uint32_t)crypt_connection_id >= c->crypto_connections_length) {
+        return -1;
+    }
+
+    if (c->crypto_connections == nullptr) {
+        return -1;
+    }
+
+    const Crypto_Conn_State status = c->crypto_connections[crypt_connection_id].status;
+
+    if (status == CRYPTO_CONN_FREE) {
         return -1;
     }
 
     uint32_t i;
 
-    /* Keep mutex, only destroy it when connection is realloced out. */
-    pthread_mutex_t mutex = c->crypto_connections[crypt_connection_id].mutex;
+    pthread_mutex_destroy(c->crypto_connections[crypt_connection_id].mutex);
+    free(c->crypto_connections[crypt_connection_id].mutex);
     crypto_memzero(&c->crypto_connections[crypt_connection_id], sizeof(Crypto_Connection));
-    c->crypto_connections[crypt_connection_id].mutex = mutex;
 
+    /* check if we can resize the connections array */
     for (i = c->crypto_connections_length; i != 0; --i) {
-        if (c->crypto_connections[i - 1].status == CRYPTO_CONN_NO_CONNECTION) {
-            pthread_mutex_destroy(&c->crypto_connections[i - 1].mutex);
-        } else {
+        if (c->crypto_connections[i - 1].status != CRYPTO_CONN_FREE) {
             break;
         }
     }
@@ -1829,10 +1862,12 @@ static int wipe_crypto_connection(Net_Crypto *c, int crypt_connection_id)
 static int getcryptconnection_id(const Net_Crypto *c, const uint8_t *public_key)
 {
     for (uint32_t i = 0; i < c->crypto_connections_length; ++i) {
-        if (c->crypto_connections[i].status != CRYPTO_CONN_NO_CONNECTION) {
-            if (public_key_cmp(public_key, c->crypto_connections[i].public_key) == 0) {
-                return i;
-            }
+        if (crypt_connection_id_not_valid(c, i)) {
+            continue;
+        }
+
+        if (public_key_cmp(public_key, c->crypto_connections[i].public_key) == 0) {
+            return i;
         }
     }
 
@@ -1975,6 +2010,7 @@ int accept_crypto_connection(Net_Crypto *c, New_Connection *n_c)
     Crypto_Connection *conn = &c->crypto_connections[crypt_connection_id];
 
     if (n_c->cookie_length != COOKIE_LENGTH) {
+        wipe_crypto_connection(c, crypt_connection_id);
         return -1;
     }
 
@@ -1983,6 +2019,7 @@ int accept_crypto_connection(Net_Crypto *c, New_Connection *n_c)
     pthread_mutex_unlock(&c->tcp_mutex);
 
     if (connection_number_tcp == -1) {
+        wipe_crypto_connection(c, crypt_connection_id);
         return -1;
     }
 
@@ -1999,7 +2036,7 @@ int accept_crypto_connection(Net_Crypto *c, New_Connection *n_c)
         pthread_mutex_lock(&c->tcp_mutex);
         kill_tcp_connection_to(c->tcp_c, conn->connection_number_tcp);
         pthread_mutex_unlock(&c->tcp_mutex);
-        conn->status = CRYPTO_CONN_NO_CONNECTION;
+        wipe_crypto_connection(c, crypt_connection_id);
         return -1;
     }
 
@@ -2039,6 +2076,7 @@ int new_crypto_connection(Net_Crypto *c, const uint8_t *real_public_key, const u
     pthread_mutex_unlock(&c->tcp_mutex);
 
     if (connection_number_tcp == -1) {
+        wipe_crypto_connection(c, crypt_connection_id);
         return -1;
     }
 
@@ -2062,7 +2100,7 @@ int new_crypto_connection(Net_Crypto *c, const uint8_t *real_public_key, const u
         pthread_mutex_lock(&c->tcp_mutex);
         kill_tcp_connection_to(c->tcp_c, conn->connection_number_tcp);
         pthread_mutex_unlock(&c->tcp_mutex);
-        conn->status = CRYPTO_CONN_NO_CONNECTION;
+        wipe_crypto_connection(c, crypt_connection_id);
         return -1;
     }
 
@@ -2259,19 +2297,24 @@ static void do_tcp(Net_Crypto *c, void *userdata)
             continue;
         }
 
-        if (conn->status == CRYPTO_CONN_ESTABLISHED) {
-            bool direct_connected = 0;
-            crypto_connection_status(c, i, &direct_connected, nullptr);
+        if (conn->status != CRYPTO_CONN_ESTABLISHED) {
+            continue;
+        }
 
-            if (direct_connected) {
-                pthread_mutex_lock(&c->tcp_mutex);
-                set_tcp_connection_to_status(c->tcp_c, conn->connection_number_tcp, 0);
-                pthread_mutex_unlock(&c->tcp_mutex);
-            } else {
-                pthread_mutex_lock(&c->tcp_mutex);
-                set_tcp_connection_to_status(c->tcp_c, conn->connection_number_tcp, 1);
-                pthread_mutex_unlock(&c->tcp_mutex);
-            }
+        bool direct_connected = 0;
+
+        if (!crypto_connection_status(c, i, &direct_connected, nullptr)) {
+            continue;
+        }
+
+        if (direct_connected) {
+            pthread_mutex_lock(&c->tcp_mutex);
+            set_tcp_connection_to_status(c->tcp_c, conn->connection_number_tcp, 0);
+            pthread_mutex_unlock(&c->tcp_mutex);
+        } else {
+            pthread_mutex_lock(&c->tcp_mutex);
+            set_tcp_connection_to_status(c->tcp_c, conn->connection_number_tcp, 1);
+            pthread_mutex_unlock(&c->tcp_mutex);
         }
     }
 }
@@ -2425,7 +2468,7 @@ static int udp_handle_packet(void *object, IP_Port source, const uint8_t *packet
         return -1;
     }
 
-    pthread_mutex_lock(&conn->mutex);
+    pthread_mutex_lock(conn->mutex);
 
     if (net_family_is_ipv4(source.ip.family)) {
         conn->direct_lastrecv_timev4 = mono_time_get(c->mono_time);
@@ -2433,7 +2476,7 @@ static int udp_handle_packet(void *object, IP_Port source, const uint8_t *packet
         conn->direct_lastrecv_timev6 = mono_time_get(c->mono_time);
     }
 
-    pthread_mutex_unlock(&conn->mutex);
+    pthread_mutex_unlock(conn->mutex);
     return 0;
 }
 
@@ -2542,6 +2585,7 @@ static void send_crypto_packets(Net_Crypto *c)
                                                (CONGESTION_QUEUE_ARRAY_SIZE * CONGESTION_LAST_SENT_ARRAY_SIZE);
 
                 bool direct_connected = 0;
+                /* return value can be ignored since the `if` above ensures the connection is established */
                 crypto_connection_status(c, i, &direct_connected, nullptr);
 
                 /* When switching from TCP to UDP, don't change the packet send rate for CONGESTION_EVENT_TIMEOUT ms. */
@@ -2824,10 +2868,10 @@ int send_lossy_cryptpacket(Net_Crypto *c, int crypt_connection_id, const uint8_t
     int ret = -1;
 
     if (conn) {
-        pthread_mutex_lock(&conn->mutex);
+        pthread_mutex_lock(conn->mutex);
         uint32_t buffer_start = conn->recv_array.buffer_start;
         uint32_t buffer_end = conn->send_array.buffer_end;
-        pthread_mutex_unlock(&conn->mutex);
+        pthread_mutex_unlock(conn->mutex);
         ret = send_data_packet_helper(c, crypt_connection_id, buffer_start, buffer_end, data, length);
     }
 
@@ -2881,18 +2925,13 @@ int crypto_kill(Net_Crypto *c, int crypt_connection_id)
     return ret;
 }
 
-/* return one of CRYPTO_CONN_* values indicating the state of the connection.
- *
- * sets direct_connected to 1 if connection connects directly to other, 0 if it isn't.
- * sets online_tcp_relays to the number of connected tcp relays this connection has.
- */
-Crypto_Conn_State crypto_connection_status(const Net_Crypto *c, int crypt_connection_id, bool *direct_connected,
-        unsigned int *online_tcp_relays)
+bool crypto_connection_status(const Net_Crypto *c, int crypt_connection_id, bool *direct_connected,
+                              unsigned int *online_tcp_relays)
 {
     Crypto_Connection *conn = get_crypto_connection(c, crypt_connection_id);
 
     if (conn == nullptr) {
-        return CRYPTO_CONN_NO_CONNECTION;
+        return false;
     }
 
     if (direct_connected) {
@@ -2913,7 +2952,7 @@ Crypto_Conn_State crypto_connection_status(const Net_Crypto *c, int crypt_connec
         *online_tcp_relays = tcp_connection_to_online_tcp_relays(c->tcp_c, conn->connection_number_tcp);
     }
 
-    return conn->status;
+    return true;
 }
 
 void new_keys(Net_Crypto *c)
@@ -3002,10 +3041,6 @@ static void kill_timedout(Net_Crypto *c, void *userdata)
             continue;
         }
 
-        if (conn->status == CRYPTO_CONN_NO_CONNECTION) {
-            continue;
-        }
-
         if (conn->status == CRYPTO_CONN_COOKIE_REQUESTING || conn->status == CRYPTO_CONN_HANDSHAKE_SENT
                 || conn->status == CRYPTO_CONN_NOT_CONFIRMED) {
             if (conn->temp_packet_num_sent < MAX_NUM_SENDPACKET_TRIES) {
diff --git a/toxcore/net_crypto.h b/toxcore/net_crypto.h
index 45e1a05e..6917301b 100644
--- a/toxcore/net_crypto.h
+++ b/toxcore/net_crypto.h
@@ -81,16 +81,6 @@
 #define PACKET_ID_REJOIN_CONFERENCE 100
 #define PACKET_ID_LOSSY_CONFERENCE 199
 
-/*** Crypto connections. ***/
-
-typedef enum Crypto_Conn_State {
-    CRYPTO_CONN_NO_CONNECTION = 0,
-    CRYPTO_CONN_COOKIE_REQUESTING = 1,  // send cookie request packets
-    CRYPTO_CONN_HANDSHAKE_SENT = 2,     // send handshake packets
-    CRYPTO_CONN_NOT_CONFIRMED = 3,      // send handshake packets, we have received one from the other
-    CRYPTO_CONN_ESTABLISHED = 4,
-} Crypto_Conn_State;
-
 /* Maximum size of receiving and sending packet buffers. */
 #define CRYPTO_PACKET_BUFFER_SIZE 32768 // Must be a power of 2
 
@@ -319,13 +309,13 @@ unsigned int copy_connected_tcp_relays(Net_Crypto *c, Node_format *tcp_relays, u
  */
 int crypto_kill(Net_Crypto *c, int crypt_connection_id);
 
-/* return one of CRYPTO_CONN_* values indicating the state of the connection.
+/* return true if connection is valid, false otherwise
  *
  * sets direct_connected to 1 if connection connects directly to other, 0 if it isn't.
  * sets online_tcp_relays to the number of connected tcp relays this connection has.
  */
-Crypto_Conn_State crypto_connection_status(const Net_Crypto *c, int crypt_connection_id, bool *direct_connected,
-        unsigned int *online_tcp_relays);
+bool crypto_connection_status(const Net_Crypto *c, int crypt_connection_id, bool *direct_connected,
+                              unsigned int *online_tcp_relays);
 
 /* Generate our public and private keys.
  *  Only call this function the first time the program starts.