From b630121f2f659027e1c9f00cd97087ef34e95677 Mon Sep 17 00:00:00 2001 From: zugz Date: Thu, 19 Jan 2017 18:51:14 +0100 Subject: reduce thread-unsafe use of static variables - rework ip_ntoa() to avoid use of static variables - rework sort_client_list() to avoid use of static variables - move static 'lastdump' into Messenger struct - rework ID2String() to avoid use of static variables; rename to id_to_string() - fetch_broadcast_info(): attempt to mitigate risks from concurrent execution - current_time_monotonic(): attempt to mitigate risks from concurrent execution - comment on non-thread-safety of unix_time_update --- toxcore/LAN_discovery.c | 56 +++++++++++++++++++++++++++++++++++-------------- 1 file changed, 40 insertions(+), 16 deletions(-) (limited to 'toxcore/LAN_discovery.c') diff --git a/toxcore/LAN_discovery.c b/toxcore/LAN_discovery.c index 75dca605..40c64cda 100644 --- a/toxcore/LAN_discovery.c +++ b/toxcore/LAN_discovery.c @@ -39,8 +39,10 @@ #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 + * behaviour. Consider storing the data in per-instance variables instead. */ static int broadcast_count = -1; -static IP_Port broadcast_ip_port[MAX_INTERFACES]; +static IP_Port broadcast_ip_ports[MAX_INTERFACES]; #if defined(_WIN32) || defined(__WIN32__) || defined (WIN32) @@ -48,8 +50,6 @@ static IP_Port broadcast_ip_port[MAX_INTERFACES]; static void fetch_broadcast_info(uint16_t port) { - broadcast_count = 0; - IP_ADAPTER_INFO *pAdapterInfo = (IP_ADAPTER_INFO *)malloc(sizeof(IP_ADAPTER_INFO)); unsigned long ulOutBufLen = sizeof(IP_ADAPTER_INFO); @@ -66,6 +66,13 @@ static void fetch_broadcast_info(uint16_t port) } } + /* 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]; + int ret; if ((ret = GetAdaptersInfo(pAdapterInfo, &ulOutBufLen)) == NO_ERROR) { @@ -77,16 +84,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 (gateway.family == AF_INET && subnet_mask.family == AF_INET) { - IP_Port *ip_port = &broadcast_ip_port[broadcast_count]; + IP_Port *ip_port = &ip_ports[count]; ip_port->ip.family = AF_INET; uint32_t gateway_ip = ntohl(gateway.ip4.uint32), subnet_ip = ntohl(subnet_mask.ip4.uint32); uint32_t broadcast_ip = gateway_ip + ~subnet_ip - 1; ip_port->ip.ip4.uint32 = htonl(broadcast_ip); ip_port->port = port; - broadcast_count++; + count++; - if (broadcast_count >= MAX_INTERFACES) { - return; + if (count >= MAX_INTERFACES) { + break; } } } @@ -98,6 +105,12 @@ static void fetch_broadcast_info(uint16_t port) if (pAdapterInfo) { free(pAdapterInfo); } + + broadcast_count = count; + + for (uint32_t i = 0; i < count; i++) { + broadcast_ip_ports[i] = ip_ports[i]; + } } #elif defined(__linux__) @@ -108,7 +121,6 @@ static void fetch_broadcast_info(uint16_t port) * so it's wrapped in __linux for now. * Definitely won't work like this on Windows... */ - broadcast_count = 0; sock_t sock = 0; if ((sock = socket(AF_INET, SOCK_STREAM, 0)) < 0) { @@ -128,14 +140,21 @@ static void fetch_broadcast_info(uint16_t port) return; } + /* 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]; + /* ifconf.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 * enough, for everybody!) */ - int i, count = ifconf.ifc_len / sizeof(struct ifreq); + int i, n = ifconf.ifc_len / sizeof(struct ifreq); - for (i = 0; i < count; i++) { + for (i = 0; i < n; i++) { /* there are interfaces with are incapable of broadcast */ if (ioctl(sock, SIOCGIFBRDADDR, &i_faces[i]) < 0) { continue; @@ -148,12 +167,11 @@ static void fetch_broadcast_info(uint16_t port) struct sockaddr_in *sock4 = (struct sockaddr_in *)&i_faces[i].ifr_broadaddr; - if (broadcast_count >= MAX_INTERFACES) { - close(sock); - return; + if (count >= MAX_INTERFACES) { + break; } - IP_Port *ip_port = &broadcast_ip_port[broadcast_count]; + IP_Port *ip_port = &ip_ports[count]; ip_port->ip.family = AF_INET; ip_port->ip.ip4.in_addr = sock4->sin_addr; @@ -162,10 +180,16 @@ static void fetch_broadcast_info(uint16_t port) } ip_port->port = port; - broadcast_count++; + count++; } close(sock); + + broadcast_count = count; + + for (uint32_t i = 0; i < count; i++) { + broadcast_ip_ports[i] = ip_ports[i]; + } } #else // TODO(irungentoo): Other platforms? @@ -196,7 +220,7 @@ static uint32_t send_broadcasts(Networking_Core *net, uint16_t port, const uint8 int i; for (i = 0; i < broadcast_count; i++) { - sendpacket(net, broadcast_ip_port[i], data, length); + sendpacket(net, broadcast_ip_ports[i], data, length); } return 1; -- cgit v1.2.3