Clean up DnsTlsDispatcher when a network is disconnected

DnsTlsDispatcher used to keep Transport in the map for at least 5
minutes. Since Transport is no longer a stateless object, it needs
to be removed as soon as the associated network, testing network
particularly, is disconnected.

This change actually fixes two known test bugs:
- bug 189384775.
  How to reproduce it: run TlsServerRevalidation twice.
    $ ./resolv_integration_test64 --gtest_filter="*TlsServerRevalidation"
    $ ./resolv_integration_test64 --gtest_filter="*TlsServerRevalidation"

- bug 189132684
  How to reproduce it: run ConnectTlsServerTimeout and QueryTlsServerTimeout
  before before ConnectTlsServerTimeout_ConcurrentQueries.
    $ ./resolv_integration_test64 --gtest_filter="*ConnectTlsServerTimeout:*QueryTlsServerTimeout"
    $ ./resolv_integration_test64 --gtest_filter="*ConnectTlsServerTimeout_ConcurrentQueries"

Original change: https://android-review.googlesource.com/c/platform/packages/modules/DnsResolver/+/1722073

Bug: 189384775
Bug: 189132684
Bug: 189161918
Test: run atest on R platform with the dnsresolver built from master
Test: run atest on Q platform with the dnsresolver built from mainline-prod
Test: verified that the two mentioned bugs are not reproducible
Test: run resolv_integration_test twice
Change-Id: I3bf3b7ddec7818c4fcf3ffaa0f97173d876d3642
Merged-In: I3bf3b7ddec7818c4fcf3ffaa0f97173d876d3642

3 files changed, 41 insertions, 7 deletions

diff --git a/DnsTlsDispatcher.cpp b/DnsTlsDispatcher.cpp
index ca577e72..40093e68 100644
--- a/DnsTlsDispatcher.cpp
+++ b/DnsTlsDispatcher.cpp
@@ -172,7 +172,7 @@ DnsTlsTransport::Response DnsTlsDispatcher::query(const DnsTlsServer& server, un
     {
         std::lock_guard guard(sLock);
         if (xport = getTransport(key); xport == nullptr) {
-            xport = addTransport(server, mark);
+            xport = addTransport(server, mark, netId);
         }
         ++xport->useCount;
     }
@@ -226,6 +226,11 @@ DnsTlsTransport::Response DnsTlsDispatcher::query(const DnsTlsServer& server, un
     return code;
 }
 
+void DnsTlsDispatcher::forceCleanup(unsigned netId) {
+    std::lock_guard guard(sLock);
+    forceCleanupLocked(netId);
+}
+
 DnsTlsTransport::Result DnsTlsDispatcher::queryInternal(Transport& xport,
                                                         const netdutils::Slice query) {
     LOG(DEBUG) << "Sending query of length " << query.size();
@@ -272,8 +277,20 @@ void DnsTlsDispatcher::cleanup(std::chrono::time_point<std::chrono::steady_clock
     mLastCleanup = now;
 }
 
+// TODO: unify forceCleanupLocked() and cleanup().
+void DnsTlsDispatcher::forceCleanupLocked(unsigned netId) {
+    for (auto it = mStore.begin(); it != mStore.end();) {
+        auto& s = it->second;
+        if (s->useCount == 0 && s->mNetId == netId) {
+            it = mStore.erase(it);
+        } else {
+            ++it;
+        }
+    }
+}
+
 DnsTlsDispatcher::Transport* DnsTlsDispatcher::addTransport(const DnsTlsServer& server,
-                                                            unsigned mark) {
+                                                            unsigned mark, unsigned netId) {
     const Key key = std::make_pair(mark, server);
     Transport* ret = getTransport(key);
     if (ret != nullptr) return ret;
@@ -300,8 +317,8 @@ DnsTlsDispatcher::Transport* DnsTlsDispatcher::addTransport(const DnsTlsServer&
         queryTimeout = 1000;
     }
 
-    ret = new Transport(server, mark, mFactory.get(), revalidationEnabled, triggerThr, unusableThr,
-                        queryTimeout);
+    ret = new Transport(server, mark, netId, mFactory.get(), revalidationEnabled, triggerThr,
+                        unusableThr, queryTimeout);
     LOG(DEBUG) << "Transport is initialized with { " << triggerThr << ", " << unusableThr << ", "
                << queryTimeout << "ms }"
                << " for server { " << server.toIpString() << "/" << server.name << " }";
diff --git a/DnsTlsDispatcher.h b/DnsTlsDispatcher.h
index f7b27dff..b0af889f 100644
--- a/DnsTlsDispatcher.h
+++ b/DnsTlsDispatcher.h
@@ -65,6 +65,8 @@ class DnsTlsDispatcher : public PrivateDnsValidationObserver {
     // Implement PrivateDnsValidationObserver.
     void onValidationStateUpdate(const std::string&, Validation, uint32_t) override{};
 
+    void forceCleanup(unsigned netId) EXCLUDES(sLock);
+
   private:
     DnsTlsDispatcher();
 
@@ -79,15 +81,22 @@ class DnsTlsDispatcher : public PrivateDnsValidationObserver {
     // Transport is a thin wrapper around DnsTlsTransport, adding reference counting and
     // usage monitoring so we can expire idle sessions from the cache.
     struct Transport {
-        Transport(const DnsTlsServer& server, unsigned mark, IDnsTlsSocketFactory* _Nonnull factory,
-                  bool revalidationEnabled, int triggerThr, int unusableThr, int timeout)
+        Transport(const DnsTlsServer& server, unsigned mark, unsigned netId,
+                  IDnsTlsSocketFactory* _Nonnull factory, bool revalidationEnabled, int triggerThr,
+                  int unusableThr, int timeout)
             : transport(server, mark, factory),
+              mNetId(netId),
               revalidationEnabled(revalidationEnabled),
               triggerThreshold(triggerThr),
               unusableThreshold(unusableThr),
               mTimeout(timeout) {}
+
         // DnsTlsTransport is thread-safe, so it doesn't need to be guarded.
         DnsTlsTransport transport;
+
+        // The expected network, assigned from dns_netid, to which Transport will send DNS packets.
+        const unsigned mNetId;
+
         // This use counter and timestamp are used to ensure that only idle sessions are
         // destroyed.
         int useCount GUARDED_BY(sLock) = 0;
@@ -134,7 +143,8 @@ class DnsTlsDispatcher : public PrivateDnsValidationObserver {
         const std::chrono::milliseconds mTimeout;
     };
 
-    Transport* _Nullable addTransport(const DnsTlsServer& server, unsigned mark) REQUIRES(sLock);
+    Transport* _Nullable addTransport(const DnsTlsServer& server, unsigned mark, unsigned netId)
+            REQUIRES(sLock);
     Transport* _Nullable getTransport(const Key& key) REQUIRES(sLock);
 
     // Cache of reusable DnsTlsTransports.  Transports stay in cache as long as
@@ -152,6 +162,9 @@ class DnsTlsDispatcher : public PrivateDnsValidationObserver {
     // This function performs a linear scan of mStore.
     void cleanup(std::chrono::time_point<std::chrono::steady_clock> now) REQUIRES(sLock);
 
+    // Force dropping any Transports whose useCount is zero.
+    void forceCleanupLocked(unsigned netId) REQUIRES(sLock);
+
     // Return a sorted list of usable DnsTlsServers in preference order.
     std::list<DnsTlsServer> getOrderedAndUsableServerList(const std::list<DnsTlsServer>& tlsServers,
                                                           unsigned netId, unsigned mark);
diff --git a/ResolverController.cpp b/ResolverController.cpp
index 0d01d2b3..6c7bed49 100644
--- a/ResolverController.cpp
+++ b/ResolverController.cpp
@@ -28,6 +28,7 @@
 
 #include "Dns64Configuration.h"
 #include "DnsResolver.h"
+#include "DnsTlsDispatcher.h"
 #include "PrivateDnsConfiguration.h"
 #include "ResolverEventReporter.h"
 #include "ResolverStats.h"
@@ -166,6 +167,9 @@ void ResolverController::destroyNetworkCache(unsigned netId) {
     resolv_delete_cache_for_net(netId);
     mDns64Configuration.stopPrefixDiscovery(netId);
     PrivateDnsConfiguration::getInstance().clear(netId);
+
+    // Don't get this instance in PrivateDnsConfiguration. It's probe to deadlock.
+    DnsTlsDispatcher::getInstance().forceCleanup(netId);
 }
 
 int ResolverController::createNetworkCache(unsigned netId) {