Remove validation threads tracking in PrivateDnsConfiguration
The validation threads tracking is not necessary since DnsTlsServer
itself can provide sufficient information to PrivateDnsConfiguration
to decide if a validation should start.
This change comprises:
[1] Remove mPrivateDnsValidateThreads and related functions
[2] Extend DnsTlsServer to tell if it is a present server for a
network.
[3] PrivateDnsConfiguration reserves every DnsTlsServer object until
being set to OFF mode or the network is destroyed. Make use of
[2] to determine which servers should be active for the network.
[4] DnsTlsServers with identical IP address but different private
DNS provider hostname are treated as different servers, and
thus they have their own validation thread.
[5] Add a new state, Validation::success_but_expired, which is used
to determine if a server which had passed the validation should
be revalidated again.
[6] To fit in with [4], some related tests are modified.
Bug: 79727473
Test: cd packages/modules/DnsResolver && atest
Change-Id: I78afce543ea05be39c36d268576824e9ec798b12
diff --git a/DnsTlsServer.h b/DnsTlsServer.h
index 18c1ddd..0dd136a 100644
--- a/DnsTlsServer.h
+++ b/DnsTlsServer.h
@@ -28,7 +28,14 @@
namespace net {
// Validation status of a DNS over TLS server (on a specific netId).
-enum class Validation : uint8_t { in_process, success, fail, unknown_server, unknown_netid };
+enum class Validation : uint8_t {
+ in_process,
+ success,
+ success_but_expired,
+ fail,
+ unknown_server,
+ unknown_netid,
+};
// DnsTlsServer represents a recursive resolver that supports, or may support, a
// secure protocol.
@@ -66,9 +73,15 @@
Validation validationState() const { return mValidation; }
void setValidationState(Validation val) { mValidation = val; }
+ // Return whether or not the server can be used for a network. It depends on
+ // the resolver configuration.
+ bool active() const { return mActive; }
+ void setActive(bool val) { mActive = val; }
+
private:
// State, unrelated to the comparison of DnsTlsServer objects.
Validation mValidation = Validation::unknown_server;
+ bool mActive = false;
};
// This comparison only checks the IP address. It ignores ports, names, and fingerprints.
diff --git a/PrivateDnsConfiguration.cpp b/PrivateDnsConfiguration.cpp
index 13bef15..43596bb 100644
--- a/PrivateDnsConfiguration.cpp
+++ b/PrivateDnsConfiguration.cpp
@@ -89,61 +89,30 @@
} else {
mPrivateDnsModes[netId] = PrivateDnsMode::OFF;
mPrivateDnsTransports.erase(netId);
- mPrivateDnsValidateThreads.erase(netId);
- // TODO: As mPrivateDnsValidateThreads is reset, validation threads which haven't yet
- // finished are considered outdated. Consider signaling the outdated validation threads to
- // stop them from updating the state of PrivateDnsConfiguration (possibly disallow them to
- // report onPrivateDnsValidationEvent).
+ // TODO: signal validation threads to stop.
return 0;
}
// Create the tracker if it was not present
- auto netPair = mPrivateDnsTransports.find(netId);
- if (netPair == mPrivateDnsTransports.end()) {
- // No TLS tracker yet for this netId.
- bool added;
- std::tie(netPair, added) = mPrivateDnsTransports.emplace(netId, PrivateDnsTracker());
- if (!added) {
- LOG(ERROR) << "Memory error while recording private DNS for netId " << netId;
- return -ENOMEM;
- }
- }
- auto& tracker = netPair->second;
+ auto& tracker = mPrivateDnsTransports[netId];
- // Remove any servers from the tracker that are not in |servers| exactly.
- for (auto it = tracker.begin(); it != tracker.end();) {
- if (tmp.find(it->first) == tmp.end()) {
- it = tracker.erase(it);
- } else {
- ++it;
- }
- }
-
- // Add any new or changed servers to the tracker, and initiate async checks for them.
+ // Add the servers if not contained in tracker.
for (const auto& [identity, server] : tmp) {
- if (needsValidation(tracker, server)) {
- // This is temporarily required. Consider the following scenario, for example,
- // Step 1) A DoTServer (s1) is set for the network. A validation (v1) for s1 starts.
- // tracker has s1 alone.
- // Step 2) The configuration changes. DotServer2 (s2) is set for the network. A
- // validation (v2) for s2 starts. tracker has s2 alone.
- // Step 3) Assume v1 and v2 somehow block. Now, if the configuration changes back to
- // set s1, there won't be a v1' starts because needValidateThread() will
- // return false.
- //
- // If we didn't add servers to tracker before needValidateThread(), tracker would
- // become empty. We would report s1 validation failed.
- if (tracker.find(identity) == tracker.end()) {
- tracker[identity] = server;
- }
- tracker[identity].setValidationState(Validation::in_process);
- LOG(DEBUG) << "Server " << addrToString(&server.ss) << " marked as in_process on netId "
- << netId << ". Tracker now has size " << tracker.size();
- // This judge must be after "tracker[server] = Validation::in_process;"
- if (!needValidateThread(server, netId)) {
- continue;
- }
+ if (tracker.find(identity) == tracker.end()) {
+ tracker[identity] = server;
+ }
+ }
+ for (auto& [identity, server] : tracker) {
+ const bool active = tmp.find(identity) != tmp.end();
+ server.setActive(active);
+
+ // For simplicity, deem the validation result of inactive servers as unreliable.
+ if (!server.active() && server.validationState() == Validation::success) {
+ updateServerState(identity, Validation::success_but_expired, netId);
+ }
+
+ if (needsValidation(server)) {
updateServerState(identity, Validation::in_process, netId);
startValidation(server, netId, mark);
}
@@ -163,7 +132,9 @@
const auto netPair = mPrivateDnsTransports.find(netId);
if (netPair != mPrivateDnsTransports.end()) {
for (const auto& [_, server] : netPair->second) {
- status.serversMap.emplace(server, server.validationState());
+ if (server.active()) {
+ status.serversMap.emplace(server, server.validationState());
+ }
}
}
@@ -175,7 +146,6 @@
std::lock_guard guard(mPrivateDnsLock);
mPrivateDnsModes.erase(netId);
mPrivateDnsTransports.erase(netId);
- mPrivateDnsValidateThreads.erase(netId);
}
void PrivateDnsConfiguration::startValidation(const DnsTlsServer& server, unsigned netId,
@@ -216,12 +186,12 @@
}
if (backoff.hasNextTimeout()) {
+ // TODO: make the thread able to receive signals to shutdown early.
std::this_thread::sleep_for(backoff.getNextTimeout());
} else {
break;
}
}
- this->cleanValidateThreadTracker(server, netId);
});
validate_thread.detach();
}
@@ -266,6 +236,11 @@
<< " was changed during private DNS validation";
success = false;
reevaluationStatus = DONT_REEVALUATE;
+ } else if (!serverPair->second.active()) {
+ LOG(WARNING) << "Server " << addrToString(&server.ss)
+ << " was removed from the configuration";
+ success = false;
+ reevaluationStatus = DONT_REEVALUATE;
}
// Send a validation event to NetdEventListenerService.
@@ -297,34 +272,6 @@
return reevaluationStatus;
}
-bool PrivateDnsConfiguration::needValidateThread(const DnsTlsServer& server, unsigned netId)
- REQUIRES(mPrivateDnsLock) {
- // Create the thread tracker if it was not present
- auto threadPair = mPrivateDnsValidateThreads.find(netId);
- if (threadPair == mPrivateDnsValidateThreads.end()) {
- // No thread tracker yet for this netId.
- bool added;
- std::tie(threadPair, added) = mPrivateDnsValidateThreads.emplace(netId, ThreadTracker());
- if (!added) {
- LOG(ERROR) << "Memory error while needValidateThread for netId " << netId;
- return true;
- }
- }
- auto& threadTracker = threadPair->second;
- if (threadTracker.count(server)) {
- LOG(DEBUG) << "Server " << addrToString(&(server.ss))
- << " validate thread is already running. Thread tracker now has size "
- << threadTracker.size();
- return false;
- } else {
- threadTracker.insert(server);
- LOG(DEBUG) << "Server " << addrToString(&(server.ss))
- << " validate thread is not running. Thread tracker now has size "
- << threadTracker.size();
- return true;
- }
-}
-
void PrivateDnsConfiguration::updateServerState(const ServerIdentity& identity, Validation state,
uint32_t netId) {
auto netPair = mPrivateDnsTransports.find(netId);
@@ -343,27 +290,20 @@
maybeNotifyObserver(identity.ip.toString(), state, netId);
}
-void PrivateDnsConfiguration::cleanValidateThreadTracker(const DnsTlsServer& server,
- unsigned netId) {
- std::lock_guard<std::mutex> guard(mPrivateDnsLock);
- LOG(DEBUG) << "cleanValidateThreadTracker Server " << addrToString(&(server.ss))
- << " validate thread is stopped.";
+bool PrivateDnsConfiguration::needsValidation(const DnsTlsServer& server) {
+ // The server is not expected to be used on the network.
+ if (!server.active()) return false;
- auto threadPair = mPrivateDnsValidateThreads.find(netId);
- if (threadPair != mPrivateDnsValidateThreads.end()) {
- auto& threadTracker = threadPair->second;
- threadTracker.erase(server);
- LOG(DEBUG) << "Server " << addrToString(&(server.ss))
- << " validate thread is stopped. Thread tracker now has size "
- << threadTracker.size();
- }
-}
+ // The server is newly added.
+ if (server.validationState() == Validation::unknown_server) return true;
-bool PrivateDnsConfiguration::needsValidation(const PrivateDnsTracker& tracker,
- const DnsTlsServer& server) {
- const ServerIdentity identity = ServerIdentity(server);
- const auto& iter = tracker.find(identity);
- return (iter == tracker.end()) || (iter->second.validationState() == Validation::fail);
+ // The server has failed at least one validation attempt. Give it another try.
+ if (server.validationState() == Validation::fail) return true;
+
+ // The previous validation result might be unreliable.
+ if (server.validationState() == Validation::success_but_expired) return true;
+
+ return false;
}
void PrivateDnsConfiguration::setObserver(Observer* observer) {
diff --git a/PrivateDnsConfiguration.h b/PrivateDnsConfiguration.h
index 341c691..722ed71 100644
--- a/PrivateDnsConfiguration.h
+++ b/PrivateDnsConfiguration.h
@@ -95,26 +95,22 @@
bool recordPrivateDnsValidation(const DnsTlsServer& server, unsigned netId, bool success)
EXCLUDES(mPrivateDnsLock);
- bool needValidateThread(const DnsTlsServer& server, unsigned netId) REQUIRES(mPrivateDnsLock);
- void cleanValidateThreadTracker(const DnsTlsServer& server, unsigned netId)
- EXCLUDES(mPrivateDnsLock);
-
- // Start validation for newly added servers as well as any servers that have
- // landed in Validation::fail state. Note that servers that have failed
+ // Decide if a validation for |server| is needed. Note that servers that have failed
// multiple validation attempts but for which there is still a validating
// thread running are marked as being Validation::in_process.
- bool needsValidation(const PrivateDnsTracker& tracker, const DnsTlsServer& server)
- REQUIRES(mPrivateDnsLock);
+ bool needsValidation(const DnsTlsServer& server) REQUIRES(mPrivateDnsLock);
void updateServerState(const ServerIdentity& identity, Validation state, uint32_t netId)
REQUIRES(mPrivateDnsLock);
std::mutex mPrivateDnsLock;
std::map<unsigned, PrivateDnsMode> mPrivateDnsModes GUARDED_BY(mPrivateDnsLock);
- // Structure for tracking the validation status of servers on a specific netId.
- // Using the AddressComparator ensures at most one entry per IP address.
+
+ // Contains all servers for a network, along with their current validation status.
+ // In case a server is removed due to a configuration change, it remains in this map,
+ // but is marked inactive.
+ // Any pending validation threads will continue running because we have no way to cancel them.
std::map<unsigned, PrivateDnsTracker> mPrivateDnsTransports GUARDED_BY(mPrivateDnsLock);
- std::map<unsigned, ThreadTracker> mPrivateDnsValidateThreads GUARDED_BY(mPrivateDnsLock);
// For testing. The observer is notified of onValidationStateUpdate 1) when a validation is
// about to begin or 2) when a validation finishes. If a validation finishes when in OFF mode
diff --git a/ResolverController.cpp b/ResolverController.cpp
index 051f095..b7ac8eb 100644
--- a/ResolverController.cpp
+++ b/ResolverController.cpp
@@ -70,6 +70,8 @@
return "in_process";
case Validation::success:
return "success";
+ case Validation::success_but_expired:
+ return "success_but_expired";
case Validation::fail:
return "fail";
case Validation::unknown_server:
diff --git a/resolv_tls_unit_test.cpp b/resolv_tls_unit_test.cpp
index 919ff6f..f384ead 100644
--- a/resolv_tls_unit_test.cpp
+++ b/resolv_tls_unit_test.cpp
@@ -892,9 +892,15 @@
checkEqual(s1, s2);
s2.setValidationState(Validation::fail);
checkEqual(s1, s2);
+ s1.setActive(true);
+ checkEqual(s1, s2);
+ s2.setActive(false);
+ checkEqual(s1, s2);
EXPECT_EQ(s1.validationState(), Validation::success);
EXPECT_EQ(s2.validationState(), Validation::fail);
+ EXPECT_TRUE(s1.active());
+ EXPECT_FALSE(s2.active());
}
TEST(QueryMapTest, Basic) {
diff --git a/tests/resolv_integration_test.cpp b/tests/resolv_integration_test.cpp
index b53d2ba..c994162 100644
--- a/tests/resolv_integration_test.cpp
+++ b/tests/resolv_integration_test.cpp
@@ -4750,6 +4750,7 @@
StartDns(dns2, {});
test::DnsTlsFrontend workableTls(addr1, "853", addr1, "53");
test::DnsTlsFrontend unresponsiveTls(addr2, "853", addr2, "53");
+ int validationAttemptsToUnresponsiveTls = 1;
unresponsiveTls.setHangOnHandshakeForTesting(true);
ASSERT_TRUE(workableTls.startServer());
ASSERT_TRUE(unresponsiveTls.startServer());
@@ -4763,7 +4764,9 @@
// Check the validation results.
EXPECT_TRUE(WaitForPrivateDnsValidation(workableTls.listen_address(), true));
EXPECT_TRUE(WaitForPrivateDnsValidation(unusable_addr, false));
- EXPECT_EQ(unresponsiveTls.acceptConnectionsCount(), 1); // The validation is still in progress.
+
+ // The validation is still in progress.
+ EXPECT_EQ(unresponsiveTls.acceptConnectionsCount(), validationAttemptsToUnresponsiveTls);
static const struct TestConfig {
std::vector<std::string> tlsServers;
@@ -4793,13 +4796,23 @@
SCOPED_TRACE(serverAddr);
if (serverAddr == workableTls.listen_address()) {
if (dnsModeChanged) {
- // In despite of the identical IP address, the server is regarded as a different
+ // Despite the identical IP address, the server is regarded as a different
// server when DnsTlsServer.name is different. The resolver treats it as a
// different object and begins the validation process.
EXPECT_TRUE(WaitForPrivateDnsValidation(serverAddr, true));
}
} else if (serverAddr == unresponsiveTls.listen_address()) {
- // No revalidation needed for the server which have been marked as in_progesss.
+ if (dnsModeChanged) {
+ // Despite the identical IP address, the server is regarded as a different
+ // server when DnsTlsServer.name is different. The resolver treats it as a
+ // different object and begins the validation process.
+ validationAttemptsToUnresponsiveTls++;
+
+ // This is the limitation from DnsTlsFrontend. DnsTlsFrontend can't operate
+ // concurrently. As soon as there's another connection request,
+ // DnsTlsFrontend resets the unique_fd to the new connection.
+ EXPECT_TRUE(WaitForPrivateDnsValidation(serverAddr, false));
+ }
} else {
// Must be unusable_addr.
// In opportunistic mode, when a validation for a private DNS server fails, the
@@ -4826,7 +4839,7 @@
EXPECT_TRUE(WaitForPrivateDnsValidation(unusable_addr, false));
}
- EXPECT_EQ(unresponsiveTls.acceptConnectionsCount(), 1);
+ EXPECT_EQ(unresponsiveTls.acceptConnectionsCount(), validationAttemptsToUnresponsiveTls);
TlsNameLastTime = config.tlsName;
}
@@ -4872,17 +4885,18 @@
{{addr2}, "", false, false},
{{addr1}, "", false, true},
{{addr2}, "", false, true},
- {{addr1}, kDefaultPrivateDnsHostName, false, true},
- {{addr2}, kDefaultPrivateDnsHostName, false, true},
+
+ // expectNothingHappenWhenServerUnresponsive is false in the two cases because of the
+ // limitation from DnsTlsFrontend which can't operate concurrently.
+ {{addr1}, kDefaultPrivateDnsHostName, false, false},
+ {{addr2}, kDefaultPrivateDnsHostName, false, false},
{{addr1}, kDefaultPrivateDnsHostName, true, true},
{{addr2}, kDefaultPrivateDnsHostName, true, true},
- // There's no new validation to start because there are already two validation threads
- // running (one is for addr1, the other is for addr2). This is because the comparator
- // doesn't compare DnsTlsServer.name. Keep the design as-is until it's known to be
- // harmful.
- {{addr1}, "", true, true},
- {{addr2}, "", true, true},
+ // expectNothingHappenWhenServerUnresponsive is true in the two cases because of the
+ // limitation from DnsTlsFrontend which can't operate concurrently.
+ {{addr1}, "", true, false},
+ {{addr2}, "", true, false},
{{addr1}, "", true, true},
{{addr2}, "", true, true},
};
@@ -4931,6 +4945,8 @@
// It's possible that the resolver hasn't yet started to
// connect. Wait a while.
std::this_thread::sleep_for(100ms);
+ } else {
+ EXPECT_TRUE(WaitForPrivateDnsValidation(config.tlsServer, false));
}
const auto condition = [&]() {
return tls.acceptConnectionsCount() == connectCountsBefore + expectCountDiff;