cleanup: Make LAN discovery thread-safe without data races.

It was kind of thread-safe, maybe, but there was a data race that makes
tsan unhappy. We now do interface detection once per Tox instance
instead of once per process.

other/DHT_bootstrap.c

@@ -217,7 +217,7 @@ int main(int argc, char *argv[])
     int is_waiting_for_dht_connection = 1;
 
     uint64_t last_LANdiscovery = 0;
-    lan_discovery_init(dht);
+    Broadcast_Info *broadcast = lan_discovery_init(dht);
 
     while (1) {
         mono_time_update(mono_time);
@@ -230,7 +230,7 @@ int main(int argc, char *argv[])
         do_dht(dht);
 
         if (mono_time_is_timeout(mono_time, last_LANdiscovery, is_waiting_for_dht_connection ? 5 : LAN_DISCOVERY_INTERVAL)) {
-            lan_discovery_send(dht_get_net(dht), dht_get_self_public_key(dht), net_htons(PORT));
+            lan_discovery_send(dht_get_net(dht), broadcast, dht_get_self_public_key(dht), net_htons(PORT));
             last_LANdiscovery = mono_time_get(mono_time);
         }
 

other/bootstrap_daemon/docker/tox-bootstrapd.sha256

@@ -1 +1 @@
-21bbf4e49911d31cef0f50195affe86138630703c7072c67ffd7802f7e588190  /usr/local/bin/tox-bootstrapd
+dd0f0cc84ecb5ce44ca56ff47793fca162e12b1a542c34e39fae4d3e53996874  /usr/local/bin/tox-bootstrapd

other/bootstrap_daemon/src/tox-bootstrapd.c

@@ -474,8 +474,10 @@ int main(int argc, char *argv[])
 
     int waiting_for_dht_connection = 1;
 
+    Broadcast_Info *broadcast = nullptr;
+
     if (enable_lan_discovery) {
-        lan_discovery_init(dht);
+        broadcast = lan_discovery_init(dht);
         log_write(LOG_LEVEL_INFO, "Initialized LAN discovery successfully.\n");
     }
 
@@ -503,7 +505,7 @@ int main(int argc, char *argv[])
         do_dht(dht);
 
         if (enable_lan_discovery && mono_time_is_timeout(mono_time, last_LANdiscovery, LAN_DISCOVERY_INTERVAL)) {
-            lan_discovery_send(dht_get_net(dht), dht_get_self_public_key(dht), net_htons_port);
+            lan_discovery_send(dht_get_net(dht), broadcast, dht_get_self_public_key(dht), net_htons_port);
             last_LANdiscovery = mono_time_get(mono_time);
         }
 
@@ -535,7 +537,7 @@ int main(int argc, char *argv[])
     }
 
     if (enable_lan_discovery) {
-        lan_discovery_kill(dht);
+        lan_discovery_kill(dht, broadcast);
     }
 
     kill_TCP_server(tcp_server);

toxcore/LAN_discovery.c

@@ -8,6 +8,7 @@
  */
 #include "LAN_discovery.h"
 
+#include <stdlib.h>
 #include <string.h>
 
 #if defined(_WIN32) || defined(__WIN32__) || defined(WIN32)
@@ -43,23 +44,27 @@
 #define MAX_INTERFACES 16
 
 
-/* TODO: multiple threads might concurrently try to set these, and it isn't clear that this couldn't lead to undesirable
+struct Broadcast_Info {
- * behaviour. Consider storing the data in per-instance variables instead. */
+    uint32_t count;
-//!TOKSTYLE-
+    IP_Port ip_ports[MAX_INTERFACES];
-// No global mutable state in Tokstyle.
+};
-static int     broadcast_count = -1;
-static IP_Port broadcast_ip_ports[MAX_INTERFACES];
-//!TOKSTYLE+
 
 #if defined(_WIN32) || defined(__WIN32__) || defined(WIN32)
 
-static void fetch_broadcast_info(uint16_t port)
+static Broadcast_Info *fetch_broadcast_info(uint16_t port)
 {
+    Broadcast_Info *broadcast = (Broadcast_Info *)calloc(1, sizeof(Broadcast_Info));
+
+    if (broadcast == nullptr) {
+        return nullptr;
+    }
+
     IP_ADAPTER_INFO *pAdapterInfo = (IP_ADAPTER_INFO *)malloc(sizeof(IP_ADAPTER_INFO));
     unsigned long ulOutBufLen = sizeof(IP_ADAPTER_INFO);
 
     if (pAdapterInfo == nullptr) {
-        return;
+        free(broadcast);
+        return nullptr;
     }
 
     if (GetAdaptersInfo(pAdapterInfo, &ulOutBufLen) == ERROR_BUFFER_OVERFLOW) {
@@ -67,17 +72,11 @@ static void fetch_broadcast_info(uint16_t port)
         pAdapterInfo = (IP_ADAPTER_INFO *)malloc(ulOutBufLen);
 
         if (pAdapterInfo == nullptr) {
-            return;
+            free(broadcast);
+            return nullptr;
         }
     }
 
-    /* We copy these to the static variables `broadcast_*` only at the end of `fetch_broadcast_info()`.
-     * The intention is to ensure that even if multiple threads enter `fetch_broadcast_info()` concurrently, only valid
-     * interfaces will be set to be broadcast to.
-     * */
-    int count = 0;
-    IP_Port ip_ports[MAX_INTERFACES];
-
     const int ret = GetAdaptersInfo(pAdapterInfo, &ulOutBufLen);
 
     if (ret == NO_ERROR) {
@@ -90,16 +89,16 @@ static void fetch_broadcast_info(uint16_t port)
             if (addr_parse_ip(pAdapter->IpAddressList.IpMask.String, &subnet_mask)
                     && addr_parse_ip(pAdapter->GatewayList.IpAddress.String, &gateway)) {
                 if (net_family_is_ipv4(gateway.family) && net_family_is_ipv4(subnet_mask.family)) {
-                    IP_Port *ip_port = &ip_ports[count];
+                    IP_Port *ip_port = &broadcast->ip_ports[broadcast->count];
                     ip_port->ip.family = net_family_ipv4;
                     uint32_t gateway_ip = net_ntohl(gateway.ip.v4.uint32);
                     uint32_t subnet_ip = net_ntohl(subnet_mask.ip.v4.uint32);
                     uint32_t broadcast_ip = gateway_ip + ~subnet_ip - 1;
                     ip_port->ip.ip.v4.uint32 = net_htonl(broadcast_ip);
                     ip_port->port = port;
-                    ++count;
+                    ++broadcast->count;
 
-                    if (count >= MAX_INTERFACES) {
+                    if (broadcast->count >= MAX_INTERFACES) {
                         break;
                     }
                 }
@@ -113,26 +112,28 @@ static void fetch_broadcast_info(uint16_t port)
         free(pAdapterInfo);
     }
 
-    broadcast_count = count;
+    return broadcast;
-
-    for (uint32_t i = 0; i < count; ++i) {
-        broadcast_ip_ports[i] = ip_ports[i];
-    }
 }
 
 #elif !defined(FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION) && (defined(__linux__) || defined(__FreeBSD__) || defined(__DragonFly__))
 
-static void fetch_broadcast_info(uint16_t port)
+static Broadcast_Info *fetch_broadcast_info(uint16_t port)
 {
+    Broadcast_Info *broadcast = (Broadcast_Info *)calloc(1, sizeof(Broadcast_Info));
+
+    if (broadcast == nullptr) {
+        return nullptr;
+    }
+
     /* Not sure how many platforms this will run on,
      * so it's wrapped in `__linux__` for now.
      * Definitely won't work like this on Windows...
      */
-    broadcast_count = 0;
     const Socket sock = net_socket(net_family_ipv4, TOX_SOCK_STREAM, 0);
 
     if (!sock_valid(sock)) {
-        return;
+        free(broadcast);
+        return nullptr;
     }
 
     /* Configure ifconf for the ioctl call. */
@@ -145,16 +146,10 @@ static void fetch_broadcast_info(uint16_t port)
 
     if (ioctl(sock.socket, SIOCGIFCONF, &ifc) < 0) {
         kill_sock(sock);
-        return;
+        free(broadcast);
+        return nullptr;
     }
 
-    /* We copy these to the static variables `broadcast_*` only at the end of `fetch_broadcast_info()`.
-     * The intention is to ensure that even if multiple threads enter `fetch_broadcast_info()` concurrently, only valid
-     * interfaces will be set to be broadcast to.
-     * */
-    int count = 0;
-    IP_Port ip_ports[MAX_INTERFACES];
-
     /* `ifc.ifc_len` is set by the `ioctl()` to the actual length used.
      * On usage of the complete array the call should be repeated with
      * a larger array, not done (640kB and 16 interfaces shall be
@@ -175,11 +170,11 @@ static void fetch_broadcast_info(uint16_t port)
 
         const struct sockaddr_in *sock4 = (const struct sockaddr_in *)(void *)&i_faces[i].ifr_broadaddr;
 
-        if (count >= MAX_INTERFACES) {
+        if (broadcast->count >= MAX_INTERFACES) {
             break;
         }
 
-        IP_Port *ip_port = &ip_ports[count];
+        IP_Port *ip_port = &broadcast->ip_ports[broadcast->count];
         ip_port->ip.family = net_family_ipv4;
         ip_port->ip.ip.v4.uint32 = sock4->sin_addr.s_addr;
 
@@ -188,49 +183,40 @@ static void fetch_broadcast_info(uint16_t port)
         }
 
         ip_port->port = port;
-        ++count;
+        ++broadcast->count;
     }
 
     kill_sock(sock);
 
-    broadcast_count = count;
+    return broadcast;
-
-    for (uint32_t i = 0; i < count; ++i) {
-        broadcast_ip_ports[i] = ip_ports[i];
-    }
 }
 
 #else // TODO(irungentoo): Other platforms?
 
-static void fetch_broadcast_info(uint16_t port)
+static Broadcast_Info *fetch_broadcast_info(uint16_t port)
 {
-    broadcast_count = 0;
+    return (Broadcast_Info *)calloc(1, sizeof(Broadcast_Info));
 }
 
 #endif
 
 /** Send packet to all IPv4 broadcast addresses
  *
- *  return 1 if sent to at least one broadcast target.
+ * @retval true if sent to at least one broadcast target.
- *  return 0 on failure to find any valid broadcast target.
+ * @retval false on failure to find any valid broadcast target.
  */
-static uint32_t send_broadcasts(const Networking_Core *net, uint16_t port, const uint8_t *data, uint16_t length)
+static bool send_broadcasts(const Networking_Core *net, const Broadcast_Info *broadcast, uint16_t port,
+                            const uint8_t *data, uint16_t length)
 {
-    /* fetch only once? on every packet? every X seconds?
+    if (broadcast->count == 0) {
-     * old: every packet, new: once */
+        return false;
-    if (broadcast_count < 0) {
-        fetch_broadcast_info(port);
     }
 
-    if (!broadcast_count) {
+    for (uint32_t i = 0; i < broadcast->count; ++i) {
-        return 0;
+        sendpacket(net, &broadcast->ip_ports[i], data, length);
     }
 
-    for (int i = 0; i < broadcast_count; ++i) {
+    return true;
-        sendpacket(net, &broadcast_ip_ports[i], data, length);
-    }
-
-    return 1;
 }
 
 /** Return the broadcast ip. */
@@ -369,13 +355,17 @@ static int handle_LANdiscovery(void *object, const IP_Port *source, const uint8_
 }
 
 
-bool lan_discovery_send(Networking_Core *net, const uint8_t *dht_pk, uint16_t port)
+bool lan_discovery_send(Networking_Core *net, Broadcast_Info *broadcast, const uint8_t *dht_pk, uint16_t port)
 {
+    if (broadcast == nullptr) {
+        return false;
+    }
+
     uint8_t data[CRYPTO_PUBLIC_KEY_SIZE + 1];
     data[0] = NET_PACKET_LAN_DISCOVERY;
     id_copy(data + 1, dht_pk);
 
-    send_broadcasts(net, port, data, 1 + CRYPTO_PUBLIC_KEY_SIZE);
+    send_broadcasts(net, broadcast, port, data, 1 + CRYPTO_PUBLIC_KEY_SIZE);
 
     bool res = false;
     IP_Port ip_port;
@@ -405,12 +395,15 @@ bool lan_discovery_send(Networking_Core *net, const uint8_t *dht_pk, uint16_t po
 }
 
 
-void lan_discovery_init(DHT *dht)
+Broadcast_Info *lan_discovery_init(DHT *dht)
 {
+    Broadcast_Info *broadcast = fetch_broadcast_info(net_htons(TOX_PORT_DEFAULT));
     networking_registerhandler(dht_get_net(dht), NET_PACKET_LAN_DISCOVERY, &handle_LANdiscovery, dht);
+    return broadcast;
 }
 
-void lan_discovery_kill(DHT *dht)
+void lan_discovery_kill(DHT *dht, Broadcast_Info *broadcast)
 {
     networking_registerhandler(dht_get_net(dht), NET_PACKET_LAN_DISCOVERY, nullptr, nullptr);
+    free(broadcast);
 }

toxcore/LAN_discovery.h

@@ -16,22 +16,24 @@
  */
 #define LAN_DISCOVERY_INTERVAL         10
 
+typedef struct Broadcast_Info Broadcast_Info;
+
 /**
  * Send a LAN discovery pcaket to the broadcast address with port port.
  *
  * @return true on success, false on failure.
  */
-bool lan_discovery_send(Networking_Core *net, const uint8_t *dht_pk, uint16_t port);
+bool lan_discovery_send(Networking_Core *net, Broadcast_Info *broadcast, const uint8_t *dht_pk, uint16_t port);
 
 /**
  * Sets up packet handlers.
  */
-void lan_discovery_init(DHT *dht);
+Broadcast_Info *lan_discovery_init(DHT *dht);
 
 /**
  * Clear packet handlers.
  */
-void lan_discovery_kill(DHT *dht);
+void lan_discovery_kill(DHT *dht, Broadcast_Info *info);
 
 /**
  * Is IP a local ip or not.

toxcore/friend_connection.c

@@ -58,6 +58,7 @@ struct Friend_Connections {
     const Logger *logger;
     Net_Crypto *net_crypto;
     DHT *dht;
+    Broadcast_Info *broadcast;
     Onion_Client *onion_c;
 
     Friend_Conn *conns;
@@ -882,7 +883,11 @@ Friend_Connections *new_friend_connections(const Logger *logger, const Mono_Time
     new_connection_handler(temp->net_crypto, &handle_new_connections, temp);
 
     if (temp->local_discovery_enabled) {
-        lan_discovery_init(temp->dht);
+        temp->broadcast = lan_discovery_init(temp->dht);
+
+        if (temp->broadcast == nullptr) {
+            LOGGER_ERROR(logger, "could not initialise LAN discovery");
+        }
     }
 
     return temp;
@@ -897,11 +902,12 @@ static void lan_discovery(Friend_Connections *fr_c)
         last = last > TOX_PORTRANGE_TO ? TOX_PORTRANGE_TO : last;
 
         // Always send to default port
-        lan_discovery_send(dht_get_net(fr_c->dht), dht_get_self_public_key(fr_c->dht), net_htons(TOX_PORT_DEFAULT));
+        lan_discovery_send(dht_get_net(fr_c->dht), fr_c->broadcast, dht_get_self_public_key(fr_c->dht),
+                           net_htons(TOX_PORT_DEFAULT));
 
         // And check some extra ports
         for (uint16_t port = first; port < last; ++port) {
-            lan_discovery_send(dht_get_net(fr_c->dht), dht_get_self_public_key(fr_c->dht), net_htons(port));
+            lan_discovery_send(dht_get_net(fr_c->dht), fr_c->broadcast, dht_get_self_public_key(fr_c->dht), net_htons(port));
         }
 
         // Don't include default port in port range
@@ -974,7 +980,7 @@ void kill_friend_connections(Friend_Connections *fr_c)
     }
 
     if (fr_c->local_discovery_enabled) {
-        lan_discovery_kill(fr_c->dht);
+        lan_discovery_kill(fr_c->dht, fr_c->broadcast);
     }
 
     free(fr_c);