From aca5540e13319c951ecb343eec21647ba7730e56 Mon Sep 17 00:00:00 2001
From: Fabio Alessandrelli <fabio.alessandrelli@gmail.com>
Date: Mon, 2 Aug 2021 19:25:41 +0200
Subject: [PATCH] [Net] Fix IP address resolution incorrectly locking the main
 thread.

This seems to be a pretty old bug, older then originally reported (at
least under certain circumstances).

The IP singleton uses a resolve queue so developers can queue hostnames
for resolution in a separate while keeping the main thread unlocked
(address-resolution OS functions are blocking, and could block for a long
time in case of network disruption).

In most places though, the address resolution function was called with
the mutex locked, causing other functions (querying status, queueing
another hostname, ecc) to block until that resolution ended.

This commit ensures that all calls to OS address resolution are done
with the mutex unlocked.
---
 core/io/ip.cpp | 63 +++++++++++++++++++++++++++++++++++---------------
 1 file changed, 44 insertions(+), 19 deletions(-)

diff --git a/core/io/ip.cpp b/core/io/ip.cpp
index cd1b6d19944..e3102508a34 100644
--- a/core/io/ip.cpp
+++ b/core/io/ip.cpp
@@ -83,8 +83,23 @@ struct _IP_ResolverPrivate {
 				continue;
 			}
 
-			IP::get_singleton()->_resolve_hostname(queue[i].response, queue[i].hostname, queue[i].type);
-			queue[i].status.set(queue[i].response.is_empty() ? IP::RESOLVER_STATUS_ERROR : IP::RESOLVER_STATUS_DONE);
+			mutex.lock();
+			List<IPAddress> response;
+			String hostname = queue[i].hostname;
+			IP::Type type = queue[i].type;
+			mutex.unlock();
+
+			// We should not lock while resolving the hostname,
+			// only when modifying the queue.
+			IP::get_singleton()->_resolve_hostname(response, hostname, type);
+
+			MutexLock lock(mutex);
+			// Could have been completed by another function, or deleted.
+			if (queue[i].status.get() != IP::RESOLVER_STATUS_WAITING) {
+				continue;
+			}
+			queue[i].response = response;
+			queue[i].status.set(response.is_empty() ? IP::RESOLVER_STATUS_ERROR : IP::RESOLVER_STATUS_DONE);
 		}
 	}
 
@@ -93,8 +108,6 @@ struct _IP_ResolverPrivate {
 
 		while (!ipr->thread_abort) {
 			ipr->sem.wait();
-
-			MutexLock lock(ipr->mutex);
 			ipr->resolve_queues();
 		}
 	}
@@ -107,17 +120,23 @@ struct _IP_ResolverPrivate {
 };
 
 IPAddress IP::resolve_hostname(const String &p_hostname, IP::Type p_type) {
-	MutexLock lock(resolver->mutex);
-
 	List<IPAddress> res;
-
 	String key = _IP_ResolverPrivate::get_cache_key(p_hostname, p_type);
+
+	resolver->mutex.lock();
 	if (resolver->cache.has(key)) {
 		res = resolver->cache[key];
 	} else {
+		// This should be run unlocked so the resolver thread can keep
+		// resolving other requests.
+		resolver->mutex.unlock();
 		_resolve_hostname(res, p_hostname, p_type);
+		resolver->mutex.lock();
+		// We might be overriding another result, but we don't care (they are the
+		// same hostname).
 		resolver->cache[key] = res;
 	}
+	resolver->mutex.unlock();
 
 	for (int i = 0; i < res.size(); ++i) {
 		if (res[i].is_valid()) {
@@ -128,14 +147,23 @@ IPAddress IP::resolve_hostname(const String &p_hostname, IP::Type p_type) {
 }
 
 Array IP::resolve_hostname_addresses(const String &p_hostname, Type p_type) {
-	MutexLock lock(resolver->mutex);
-
+	List<IPAddress> res;
 	String key = _IP_ResolverPrivate::get_cache_key(p_hostname, p_type);
-	if (!resolver->cache.has(key)) {
-		_resolve_hostname(resolver->cache[key], p_hostname, p_type);
-	}
 
-	List<IPAddress> res = resolver->cache[key];
+	resolver->mutex.lock();
+	if (resolver->cache.has(key)) {
+		res = resolver->cache[key];
+	} else {
+		// This should be run unlocked so the resolver thread can keep resolving
+		// other requests.
+		resolver->mutex.unlock();
+		_resolve_hostname(res, p_hostname, p_type);
+		resolver->mutex.lock();
+		// We might be overriding another result, but we don't care (they are the
+		// same hostname).
+		resolver->cache[key] = res;
+	}
+	resolver->mutex.unlock();
 
 	Array result;
 	for (int i = 0; i < res.size(); ++i) {
@@ -178,13 +206,12 @@ IP::ResolverID IP::resolve_hostname_queue_item(const String &p_hostname, IP::Typ
 IP::ResolverStatus IP::get_resolve_item_status(ResolverID p_id) const {
 	ERR_FAIL_INDEX_V(p_id, IP::RESOLVER_MAX_QUERIES, IP::RESOLVER_STATUS_NONE);
 
-	MutexLock lock(resolver->mutex);
-
-	if (resolver->queue[p_id].status.get() == IP::RESOLVER_STATUS_NONE) {
+	IP::ResolverStatus res = resolver->queue[p_id].status.get();
+	if (res == IP::RESOLVER_STATUS_NONE) {
 		ERR_PRINT("Condition status == IP::RESOLVER_STATUS_NONE");
 		return IP::RESOLVER_STATUS_NONE;
 	}
-	return resolver->queue[p_id].status.get();
+	return res;
 }
 
 IPAddress IP::get_resolve_item_address(ResolverID p_id) const {
@@ -230,8 +257,6 @@ Array IP::get_resolve_item_addresses(ResolverID p_id) const {
 void IP::erase_resolve_item(ResolverID p_id) {
 	ERR_FAIL_INDEX(p_id, IP::RESOLVER_MAX_QUERIES);
 
-	MutexLock lock(resolver->mutex);
-
 	resolver->queue[p_id].status.set(IP::RESOLVER_STATUS_NONE);
 }