From 17d594238f55d918c7828153f05a9c750503cd1d Mon Sep 17 00:00:00 2001 From: Fredy Wijaya Date: Wed, 9 Oct 2024 17:24:53 -0500 Subject: [PATCH] getaddrinfo: Fix TSAN issue when trace is enabled (#36503) Commit Message: Fixes https://github.com/envoyproxy/envoy/issues/36499 Additional Description: Doing `std::transform` on a shared `std::vector` reference without proper locking isn't thread safe and can cause a UB, especially for the `getaddrinfo` resolver where adding traces is done in a separate thread. This PR fixes by introducing a new `getTraces` API that builds the string traces in an atomic way. Risk Level: low (only enabled with `enable_dfp_dns_trace` runtime flag) Testing: existing tests Docs Changes: n/a Release Notes: n/a Platform Specific Features: getaddrinfo, mobile --------- Signed-off-by: Fredy Wijaya --- envoy/network/BUILD | 1 - envoy/network/dns.h | 4 +- .../dynamic_forward_proxy/dns_cache_impl.cc | 14 +-- .../dns_resolver/apple/apple_dns_impl.h | 2 +- .../network/dns_resolver/cares/dns_impl.h | 2 +- .../dns_resolver/getaddrinfo/getaddrinfo.h | 11 ++- .../dynamic_forward_proxy/test_resolver.h | 2 + .../getaddrinfo/getaddrinfo_test.cc | 95 +++++++++++-------- test/mocks/network/mocks.h | 2 +- 9 files changed, 71 insertions(+), 62 deletions(-) diff --git a/envoy/network/BUILD b/envoy/network/BUILD index 13bbd4457026..160559ab8ad8 100644 --- a/envoy/network/BUILD +++ b/envoy/network/BUILD @@ -86,7 +86,6 @@ envoy_cc_library( name = "dns_interface", hdrs = ["dns.h"], deps = [ - "//envoy/common:optref_lib", "//envoy/common:time_interface", "//envoy/network:address_interface", ], diff --git a/envoy/network/dns.h b/envoy/network/dns.h index d2781c853e7b..3b5fcf893c49 100644 --- a/envoy/network/dns.h +++ b/envoy/network/dns.h @@ -6,11 +6,11 @@ #include #include -#include "envoy/common/optref.h" #include "envoy/common/pure.h" #include "envoy/common/time.h" #include "envoy/network/address.h" +#include "absl/types/optional.h" #include "absl/types/variant.h" namespace Envoy { @@ -56,7 +56,7 @@ class ActiveDnsQuery { virtual void addTrace(uint8_t trace) PURE; /** Return the DNS query traces. */ - virtual OptRef> getTraces() PURE; + virtual std::string getTraces() PURE; }; /** diff --git a/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc b/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc index 44b2b98e8f9d..fffd784255a6 100644 --- a/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc +++ b/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc @@ -421,18 +421,8 @@ void DnsCacheImpl::finishResolve(const std::string& host, std::string details_with_maybe_trace = std::string(details); if (primary_host_info != nullptr && primary_host_info->active_query_ != nullptr) { if (enable_dfp_dns_trace_) { - OptRef> traces = - primary_host_info->active_query_->getTraces(); - if (traces.has_value()) { - std::vector string_traces; - string_traces.reserve(traces.ref().size()); - std::transform(traces.ref().begin(), traces.ref().end(), std::back_inserter(string_traces), - [](const auto& trace) { - return absl::StrCat(trace.trace_, "=", - trace.time_.time_since_epoch().count()); - }); - details_with_maybe_trace = absl::StrCat(details, ":", absl::StrJoin(string_traces, ",")); - } + std::string traces = primary_host_info->active_query_->getTraces(); + details_with_maybe_trace = absl::StrCat(details, ":", traces); } // `cancel` must be called last because the `ActiveQuery` will be destroyed afterward. if (is_timeout) { diff --git a/source/extensions/network/dns_resolver/apple/apple_dns_impl.h b/source/extensions/network/dns_resolver/apple/apple_dns_impl.h index 448c0a59465b..7b32cb468017 100644 --- a/source/extensions/network/dns_resolver/apple/apple_dns_impl.h +++ b/source/extensions/network/dns_resolver/apple/apple_dns_impl.h @@ -101,7 +101,7 @@ class AppleDnsResolverImpl : public DnsResolver, protected Logger::Loggable> getTraces() override { return {}; } + std::string getTraces() override { return {}; } static DnsResponse buildDnsResponse(const struct sockaddr* address, uint32_t ttl); diff --git a/source/extensions/network/dns_resolver/cares/dns_impl.h b/source/extensions/network/dns_resolver/cares/dns_impl.h index e61efc3c52ab..a95ff3e2b032 100644 --- a/source/extensions/network/dns_resolver/cares/dns_impl.h +++ b/source/extensions/network/dns_resolver/cares/dns_impl.h @@ -77,7 +77,7 @@ class DnsResolverImpl : public DnsResolver, protected Logger::Loggable> getTraces() override { return {}; } + std::string getTraces() override { return {}; } // Does the object own itself? Resource reclamation occurs via self-deleting // on query completion or error. diff --git a/source/extensions/network/dns_resolver/getaddrinfo/getaddrinfo.h b/source/extensions/network/dns_resolver/getaddrinfo/getaddrinfo.h index 27f29a6c6adb..2357fdb4ace6 100644 --- a/source/extensions/network/dns_resolver/getaddrinfo/getaddrinfo.h +++ b/source/extensions/network/dns_resolver/getaddrinfo/getaddrinfo.h @@ -62,9 +62,16 @@ class GetAddrInfoDnsResolver : public DnsResolver, public Logger::Loggable> getTraces() override { + std::string getTraces() override { absl::MutexLock lock(&mutex_); - return traces_; + std::vector string_traces; + string_traces.reserve(traces_.size()); + std::transform(traces_.begin(), traces_.end(), std::back_inserter(string_traces), + [](const Trace& trace) { + return absl::StrCat(trace.trace_, "=", + trace.time_.time_since_epoch().count()); + }); + return absl::StrJoin(string_traces, ","); } bool isCancelled() { diff --git a/test/extensions/filters/http/dynamic_forward_proxy/test_resolver.h b/test/extensions/filters/http/dynamic_forward_proxy/test_resolver.h index 41b47d10eb0f..738c5bc6d954 100644 --- a/test/extensions/filters/http/dynamic_forward_proxy/test_resolver.h +++ b/test/extensions/filters/http/dynamic_forward_proxy/test_resolver.h @@ -46,6 +46,8 @@ class TestResolver : public GetAddrInfoDnsResolver { if (dns_override.has_value()) { *const_cast(&query->dns_name_) = dns_override.value(); } + // Add a dummy trace for test coverage. + query->addTrace(100); pending_queries_.push_back(PendingQueryInfo{std::move(query), absl::nullopt}); }); return raw_new_query; diff --git a/test/extensions/network/dns_resolver/getaddrinfo/getaddrinfo_test.cc b/test/extensions/network/dns_resolver/getaddrinfo/getaddrinfo_test.cc index 840e11c0c1c2..2dcbc115d96b 100644 --- a/test/extensions/network/dns_resolver/getaddrinfo/getaddrinfo_test.cc +++ b/test/extensions/network/dns_resolver/getaddrinfo/getaddrinfo_test.cc @@ -110,7 +110,9 @@ class GetAddrInfoDnsImplTest : public testing::Test { }; MATCHER_P(HasTrace, expected_trace, "") { - return arg.trace_ == static_cast(expected_trace); + std::vector v = absl::StrSplit(arg, '='); + uint8_t trace = std::stoi(v.at(0)); + return trace == static_cast(expected_trace); } TEST_F(GetAddrInfoDnsImplTest, LocalhostResolve) { @@ -124,11 +126,12 @@ TEST_F(GetAddrInfoDnsImplTest, LocalhostResolve) { [this](DnsResolver::ResolutionStatus status, absl::string_view, std::list&& response) { verifyRealGaiResponse(status, std::move(response)); - EXPECT_THAT(active_dns_query_->getTraces().ref(), - ElementsAre(HasTrace(GetAddrInfoTrace::NotStarted), - HasTrace(GetAddrInfoTrace::Starting), - HasTrace(GetAddrInfoTrace::Success), - HasTrace(GetAddrInfoTrace::Callback))); + std::vector traces = + absl::StrSplit(active_dns_query_->getTraces(), ','); + EXPECT_THAT(traces, ElementsAre(HasTrace(GetAddrInfoTrace::NotStarted), + HasTrace(GetAddrInfoTrace::Starting), + HasTrace(GetAddrInfoTrace::Success), + HasTrace(GetAddrInfoTrace::Callback))); dispatcher_->exit(); }); @@ -152,11 +155,12 @@ TEST_F(GetAddrInfoDnsImplTest, Cancel) { [this](DnsResolver::ResolutionStatus status, absl::string_view, std::list&& response) { verifyRealGaiResponse(status, std::move(response)); - EXPECT_THAT(active_dns_query_->getTraces().ref(), - ElementsAre(HasTrace(GetAddrInfoTrace::NotStarted), - HasTrace(GetAddrInfoTrace::Starting), - HasTrace(GetAddrInfoTrace::Success), - HasTrace(GetAddrInfoTrace::Callback))); + std::vector traces = + absl::StrSplit(active_dns_query_->getTraces(), ','); + EXPECT_THAT(traces, ElementsAre(HasTrace(GetAddrInfoTrace::NotStarted), + HasTrace(GetAddrInfoTrace::Starting), + HasTrace(GetAddrInfoTrace::Success), + HasTrace(GetAddrInfoTrace::Callback))); dispatcher_->exit(); }); @@ -177,11 +181,12 @@ TEST_F(GetAddrInfoDnsImplTest, Failure) { EXPECT_EQ(status, DnsResolver::ResolutionStatus::Failure); EXPECT_EQ("Non-recoverable failure in name resolution", details); EXPECT_TRUE(response.empty()); - EXPECT_THAT(active_dns_query_->getTraces().ref(), - ElementsAre(HasTrace(GetAddrInfoTrace::NotStarted), - HasTrace(GetAddrInfoTrace::Starting), - HasTrace(GetAddrInfoTrace::Failed), - HasTrace(GetAddrInfoTrace::Callback))); + std::vector traces = + absl::StrSplit(active_dns_query_->getTraces(), ','); + EXPECT_THAT(traces, ElementsAre(HasTrace(GetAddrInfoTrace::NotStarted), + HasTrace(GetAddrInfoTrace::Starting), + HasTrace(GetAddrInfoTrace::Failed), + HasTrace(GetAddrInfoTrace::Callback))); dispatcher_->exit(); }); @@ -201,11 +206,12 @@ TEST_F(GetAddrInfoDnsImplTest, NoData) { std::list&& response) { EXPECT_EQ(status, DnsResolver::ResolutionStatus::Completed); EXPECT_TRUE(response.empty()); - EXPECT_THAT(active_dns_query_->getTraces().ref(), - ElementsAre(HasTrace(GetAddrInfoTrace::NotStarted), - HasTrace(GetAddrInfoTrace::Starting), - HasTrace(GetAddrInfoTrace::NoResult), - HasTrace(GetAddrInfoTrace::Callback))); + std::vector traces = + absl::StrSplit(active_dns_query_->getTraces(), ','); + EXPECT_THAT(traces, ElementsAre(HasTrace(GetAddrInfoTrace::NotStarted), + HasTrace(GetAddrInfoTrace::Starting), + HasTrace(GetAddrInfoTrace::NoResult), + HasTrace(GetAddrInfoTrace::Callback))); dispatcher_->exit(); }); @@ -225,11 +231,12 @@ TEST_F(GetAddrInfoDnsImplTest, NoName) { std::list&& response) { EXPECT_EQ(status, DnsResolver::ResolutionStatus::Completed); EXPECT_TRUE(response.empty()); - EXPECT_THAT(active_dns_query_->getTraces().ref(), - ElementsAre(HasTrace(GetAddrInfoTrace::NotStarted), - HasTrace(GetAddrInfoTrace::Starting), - HasTrace(GetAddrInfoTrace::NoResult), - HasTrace(GetAddrInfoTrace::Callback))); + std::vector traces = + absl::StrSplit(active_dns_query_->getTraces(), ','); + EXPECT_THAT(traces, ElementsAre(HasTrace(GetAddrInfoTrace::NotStarted), + HasTrace(GetAddrInfoTrace::Starting), + HasTrace(GetAddrInfoTrace::NoResult), + HasTrace(GetAddrInfoTrace::Callback))); dispatcher_->exit(); }); @@ -246,20 +253,22 @@ TEST_F(GetAddrInfoDnsImplTest, TryAgainIndefinitelyAndSuccess) { .Times(2) .WillOnce(Return(Api::SysCallIntResult{EAI_AGAIN, 0})) .WillOnce(Return(Api::SysCallIntResult{0, 0})); - active_dns_query_ = resolver_->resolve( - "localhost", DnsLookupFamily::All, - [this](DnsResolver::ResolutionStatus status, absl::string_view, - std::list&& response) { - EXPECT_EQ(status, DnsResolver::ResolutionStatus::Completed); - EXPECT_TRUE(response.empty()); - EXPECT_THAT( - active_dns_query_->getTraces().ref(), - ElementsAre(HasTrace(GetAddrInfoTrace::NotStarted), - HasTrace(GetAddrInfoTrace::Starting), HasTrace(GetAddrInfoTrace::Retrying), - HasTrace(GetAddrInfoTrace::Starting), HasTrace(GetAddrInfoTrace::Success), - HasTrace(GetAddrInfoTrace::Callback))); - dispatcher_->exit(); - }); + active_dns_query_ = + resolver_->resolve("localhost", DnsLookupFamily::All, + [this](DnsResolver::ResolutionStatus status, absl::string_view, + std::list&& response) { + EXPECT_EQ(status, DnsResolver::ResolutionStatus::Completed); + EXPECT_TRUE(response.empty()); + std::vector traces = + absl::StrSplit(active_dns_query_->getTraces(), ','); + EXPECT_THAT(traces, ElementsAre(HasTrace(GetAddrInfoTrace::NotStarted), + HasTrace(GetAddrInfoTrace::Starting), + HasTrace(GetAddrInfoTrace::Retrying), + HasTrace(GetAddrInfoTrace::Starting), + HasTrace(GetAddrInfoTrace::Success), + HasTrace(GetAddrInfoTrace::Callback))); + dispatcher_->exit(); + }); dispatcher_->run(Event::Dispatcher::RunType::RunUntilExit); } @@ -304,8 +313,9 @@ TEST_F(GetAddrInfoDnsImplTest, TryAgainWithNumRetriesAndSuccess) { std::list&& response) { EXPECT_EQ(status, DnsResolver::ResolutionStatus::Completed); EXPECT_TRUE(response.empty()); + std::vector traces = absl::StrSplit(active_dns_query_->getTraces(), ','); EXPECT_THAT( - active_dns_query_->getTraces().ref(), + traces, ElementsAre(HasTrace(GetAddrInfoTrace::NotStarted), HasTrace(GetAddrInfoTrace::Starting), HasTrace(GetAddrInfoTrace::Retrying), HasTrace(GetAddrInfoTrace::Starting), HasTrace(GetAddrInfoTrace::Retrying), @@ -340,8 +350,9 @@ TEST_F(GetAddrInfoDnsImplTest, TryAgainWithNumRetriesAndFailure) { EXPECT_EQ(status, DnsResolver::ResolutionStatus::Failure); EXPECT_FALSE(details.empty()); EXPECT_TRUE(response.empty()); + std::vector traces = absl::StrSplit(active_dns_query_->getTraces(), ','); EXPECT_THAT( - active_dns_query_->getTraces().ref(), + traces, ElementsAre( HasTrace(GetAddrInfoTrace::NotStarted), HasTrace(GetAddrInfoTrace::Starting), HasTrace(GetAddrInfoTrace::Retrying), HasTrace(GetAddrInfoTrace::Starting), diff --git a/test/mocks/network/mocks.h b/test/mocks/network/mocks.h index c3f4bfd5c5a3..c7260c34a8c9 100644 --- a/test/mocks/network/mocks.h +++ b/test/mocks/network/mocks.h @@ -44,7 +44,7 @@ class MockActiveDnsQuery : public ActiveDnsQuery { // Network::ActiveDnsQuery MOCK_METHOD(void, cancel, (CancelReason reason)); MOCK_METHOD(void, addTrace, (uint8_t)); - MOCK_METHOD(OptRef>, getTraces, ()); + MOCK_METHOD(std::string, getTraces, ()); }; class MockFilterManager : public FilterManager {