Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit b46f8fae authored by Mike Yu's avatar Mike Yu
Browse files

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
parent b0d27faa

DnsTlsDispatcher.cpp

+21 −4
Original line number Diff line number Diff line
@@ -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 << " }";

DnsTlsDispatcher.h

+16 −3
Original line number Diff line number Diff line
@@ -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);

ResolverController.cpp

+4 −0
Original line number Diff line number Diff line
@@ -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) {