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

Commit 16b2e637 authored by Mike Yu's avatar Mike Yu
Browse files

Revert "Add a flag dot_keep_unusable_xport_sec" 

This reverts commit 9fc1b07c.

The test added in this change is flaky in R and blocks the train
release. This change also caused a bug that DnsTlsDispatcher::cleanup()
is called every DoT query if dot_keep_unusable_xport_sec is -1.

Bug: 233345976
Bug: 235965058
Test: atest
Change-Id: I884bc6ae611de474ca337eaa1174fef0da7215f5
parent 6283df0e

DnsTlsDispatcher.cpp

+3 −23
Original line number Diff line number Diff line
@@ -26,7 +26,6 @@ 
#include "resolv_cache.h"

#include "resolv_private.h"

#include "stats.pb.h"

#include "util.h"



#include <android-base/logging.h>
@@ -244,7 +243,7 @@ DnsTlsTransport::Response DnsTlsDispatcher::query(const DnsTlsServer& server, un 


void DnsTlsDispatcher::forceCleanup(unsigned netId) {

    std::lock_guard guard(sLock);

    cleanup(std::chrono::steady_clock::now(), std::chrono::seconds(-1), netId);

    cleanup(std::chrono::steady_clock::now(), netId);

}



DnsTlsTransport::Result DnsTlsDispatcher::queryInternal(Transport& xport,
@@ -277,41 +276,22 @@ DnsTlsTransport::Result DnsTlsDispatcher::queryInternal(Transport& xport, 
// This timeout effectively controls how long to keep SSL session tickets.

static constexpr std::chrono::minutes IDLE_TIMEOUT(5);

void DnsTlsDispatcher::maybeCleanup(std::chrono::time_point<std::chrono::steady_clock> now) {

    // Make the timeout tunable via experiment flag for testing.

    std::chrono::seconds unusable_xport_idle_timeout{-1};

    const int value = Experiments::getInstance()->getFlag("dot_keep_unusable_xport_sec", -1);

    if (value > -1 && isUserDebugBuild() && std::chrono::seconds(value) < IDLE_TIMEOUT) {

        unusable_xport_idle_timeout = std::chrono::seconds(value);

    }



    // To avoid scanning mStore after every query, return early if a cleanup has been

    // performed recently.

    const std::chrono::seconds timeout = (unusable_xport_idle_timeout < IDLE_TIMEOUT)

                                                 ? unusable_xport_idle_timeout

                                                 : IDLE_TIMEOUT;

    if (now - mLastCleanup < timeout) {

    if (now - mLastCleanup < IDLE_TIMEOUT) {

        return;

    }

    cleanup(now, unusable_xport_idle_timeout, std::nullopt);

    cleanup(now, std::nullopt);

    mLastCleanup = now;

}



void DnsTlsDispatcher::cleanup(std::chrono::time_point<std::chrono::steady_clock> now,

                               std::chrono::seconds unusable_xport_idle_timeout,

                               std::optional<unsigned> netId) {

    std::erase_if(mStore, [&](const auto& item) REQUIRES(sLock) {

        auto const& [_, xport] = item;

        if (xport->useCount == 0) {

            // Remove the Transports of the associated network.

            if (netId.has_value() && xport->mNetId == netId.value()) return true;



            // Remove all expired Transports.

            if (now - xport->lastUsed > IDLE_TIMEOUT) return true;



            // Unusable Transports should be removed earlier.

            if (!xport->usable() && unusable_xport_idle_timeout.count() >= 0 &&

                now - xport->lastUsed > unusable_xport_idle_timeout)

                return true;

        }

        return false;

    });

DnsTlsDispatcher.h

+1 −2
Original line number Diff line number Diff line
@@ -165,8 +165,7 @@ class DnsTlsDispatcher : public PrivateDnsValidationObserver { 
    // Drop any cache entries whose useCount is zero and which have not been used recently.

    // This function performs a linear scan of mStore.

    void cleanup(std::chrono::time_point<std::chrono::steady_clock> now,

                 std::chrono::seconds unusable_xport_idle_timeout, std::optional<unsigned> netId)

            REQUIRES(sLock);

                 std::optional<unsigned> netId) REQUIRES(sLock);



    // Return a sorted list of usable DnsTlsServers in preference order.

    std::list<DnsTlsServer> getOrderedAndUsableServerList(const std::list<DnsTlsServer>& tlsServers,

Experiments.h

+0 −1
Original line number Diff line number Diff line
@@ -55,7 +55,6 @@ class Experiments { 
            "sort_nameservers",

            "dot_async_handshake",

            "dot_connect_timeout_ms",

            "dot_keep_unusable_xport_sec",

            "dot_maxtries",

            "dot_revalidation_threshold",

            "dot_xport_unusable_threshold",

tests/resolv_integration_test.cpp

+0 −84
Original line number Diff line number Diff line
@@ -94,9 +94,6 @@ const std::string kDotValidationLatencyFactorFlag( 
const std::string kDotValidationLatencyOffsetMsFlag(

        "persist.device_config.netd_native.dot_validation_latency_offset_ms");

const std::string kDotQuickFallbackFlag("persist.device_config.netd_native.dot_quick_fallback");

const std::string kDotKeepUnusableXportSecFlag(

        "persist.device_config.netd_native.dot_keep_unusable_xport_sec");



// Semi-public Bionic hook used by the NDK (frameworks/base/native/android/net.c)

// Tested here for convenience.

extern "C" int android_getaddrinfofornet(const char* hostname, const char* servname,
@@ -4912,87 +4909,6 @@ TEST_F(ResolverTest, SkipUnusableTlsServer) { 
    }

}



// Tests that, even if all DoT servers are mark as unusable, DnsResolver will try to send DNS

// queries to DoT servers after their expiration.

TEST_F(ResolverTest, UnusableTlsServersWorkAgain) {

    const auto sendQueryAndCheckResult = [&]() {

        int fd = resNetworkQuery(TEST_NETID, kHelloExampleCom, ns_c_in, ns_t_aaaa,

                                 ANDROID_RESOLV_NO_CACHE_LOOKUP);

        expectAnswersValid(fd, AF_INET6, kHelloExampleComAddrV6);

    };



    constexpr int DOT_CONNECT_TIMEOUT_MS = 1000;

    constexpr int DOT_XPORT_UNUSABLE_THRESHOLD = 1;

    constexpr int DOT_KEEP_UNUSABLE_XPORT_SEC = 2;



    const std::string addr1 = getUniqueIPv4Address();

    const std::string addr2 = getUniqueIPv4Address();

    test::DNSResponder dns1(addr1);

    test::DNSResponder dns2(addr2);

    test::DnsTlsFrontend dot1(addr1, "853", addr1, "53");

    test::DnsTlsFrontend dot2(addr2, "853", addr2, "53");

    dns1.addMapping(kHelloExampleCom, ns_type::ns_t_aaaa, kHelloExampleComAddrV6);

    dns2.addMapping(kHelloExampleCom, ns_type::ns_t_aaaa, kHelloExampleComAddrV6);

    ASSERT_TRUE(dns1.startServer());

    ASSERT_TRUE(dns2.startServer());

    ASSERT_TRUE(dot1.startServer());

    ASSERT_TRUE(dot2.startServer());



    ScopedSystemProperties sp1(kDotConnectTimeoutMsFlag, std::to_string(DOT_CONNECT_TIMEOUT_MS));

    ScopedSystemProperties sp2(kDotXportUnusableThresholdFlag,

                               std::to_string(DOT_XPORT_UNUSABLE_THRESHOLD));

    ScopedSystemProperties sp3(kDotQuickFallbackFlag, "1");

    ScopedSystemProperties sp4(kDotRevalidationThresholdFlag, "-1");

    ScopedSystemProperties sp5(kDotKeepUnusableXportSecFlag,

                               std::to_string(DOT_KEEP_UNUSABLE_XPORT_SEC));

    resetNetwork();



    // Private DNS opportunistic mode.

    auto parcel = DnsResponderClient::GetDefaultResolverParamsParcel();

    parcel.servers = {addr1, addr2};

    parcel.tlsServers = {addr1, addr2};

    ASSERT_TRUE(mDnsClient.SetResolversFromParcel(parcel));



    EXPECT_TRUE(WaitForPrivateDnsValidation(dot1.listen_address(), true));

    EXPECT_TRUE(WaitForPrivateDnsValidation(dot2.listen_address(), true));

    EXPECT_TRUE(dot1.waitForQueries(1));

    EXPECT_TRUE(dot2.waitForQueries(1));

    dot1.clearQueries();

    dot2.clearQueries();

    dot1.clearConnectionsCount();

    dot2.clearConnectionsCount();



    // Set the DoT servers as unresponsive to connection handshake.

    dot1.setHangOnHandshakeForTesting(true);

    dot2.setHangOnHandshakeForTesting(true);



    // Send two queries in series. The two DoT servers will be marked as unusable.

    sendQueryAndCheckResult();

    sendQueryAndCheckResult();

    EXPECT_EQ(dot1.acceptConnectionsCount(), 1);

    EXPECT_EQ(dot2.acceptConnectionsCount(), 1);

    EXPECT_EQ(dot1.queries(), 0);

    EXPECT_EQ(dot2.queries(), 0);



    // Wait for a bit more than `dot_keep_unusable_xport_sec` seconds, and then send one

    // DNS query. DnsResolver should remove the unusable mark from the two DoT servers.

    std::this_thread::sleep_for(std::chrono::milliseconds(2500));

    sendQueryAndCheckResult();

    EXPECT_EQ(dot1.acceptConnectionsCount(), 1);

    EXPECT_EQ(dot2.acceptConnectionsCount(), 1);

    EXPECT_EQ(dot1.queries(), 0);

    EXPECT_EQ(dot2.queries(), 0);



    // Recover the DoT servers, and then send one DNS query. Expect that DnsResolver uses DoT again.

    dot1.setHangOnHandshakeForTesting(false);

    dot2.setHangOnHandshakeForTesting(false);

    sendQueryAndCheckResult();

    EXPECT_EQ(dot1.acceptConnectionsCount(), 2);

    EXPECT_EQ(dot2.acceptConnectionsCount(), 1);

    EXPECT_EQ(dot1.queries(), 1);

    EXPECT_EQ(dot2.queries(), 0);

}



// Verifies that the DnsResolver re-validates the DoT server when several DNS queries to

// the server fails in a row.

TEST_F(ResolverTest, TlsServerRevalidation) {