Skip to content

Commit

Permalink
getaddrinfo: Fix TSAN issue when trace is enabled (#36503)
Browse files Browse the repository at this point in the history 
Commit Message: Fixes #36499
Additional Description:
Doing `std::transform` on a shared `std::vector<Trace>` 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 <fredyw@google.com>
  • Loading branch information
@fredyw
fredyw authored Oct 9, 2024
1 parent 55b0fc4 commit 17d5942
Show file tree
Hide file tree
Showing 9 changed files with 71 additions and 62 deletions.
1 change: 0 additions & 1 deletion envoy/network/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
],
Expand Down
4 changes: 2 additions & 2 deletions envoy/network/dns.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,11 @@
#include <memory>
#include <string>

#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 {
Expand Down Expand Up @@ -56,7 +56,7 @@ class ActiveDnsQuery {
virtual void addTrace(uint8_t trace) PURE;

/** Return the DNS query traces. */
virtual OptRef<const std::vector<Trace>> getTraces() PURE;
virtual std::string getTraces() PURE;
};

/**
Expand Down
14 changes: 2 additions & 12 deletions source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<const std::vector<Network::ActiveDnsQuery::Trace>> traces =
primary_host_info->active_query_->getTraces();
if (traces.has_value()) {
std::vector<std::string> 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) {
Expand Down
2 changes: 1 addition & 1 deletion source/extensions/network/dns_resolver/apple/apple_dns_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ class AppleDnsResolverImpl : public DnsResolver, protected Logger::Loggable<Logg
// Network::ActiveDnsQuery
void cancel(Network::ActiveDnsQuery::CancelReason reason) override;
void addTrace(uint8_t) override {}
OptRef<const std::vector<Trace>> getTraces() override { return {}; }
std::string getTraces() override { return {}; }

static DnsResponse buildDnsResponse(const struct sockaddr* address, uint32_t ttl);

Expand Down
2 changes: 1 addition & 1 deletion source/extensions/network/dns_resolver/cares/dns_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ class DnsResolverImpl : public DnsResolver, protected Logger::Loggable<Logger::I
cancel_reason_ = reason;
}
void addTrace(uint8_t) override {}
OptRef<const std::vector<Trace>> getTraces() override { return {}; }
std::string getTraces() override { return {}; }

// Does the object own itself? Resource reclamation occurs via self-deleting
// on query completion or error.
Expand Down
11 changes: 9 additions & 2 deletions source/extensions/network/dns_resolver/getaddrinfo/getaddrinfo.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,9 +62,16 @@ class GetAddrInfoDnsResolver : public DnsResolver, public Logger::Loggable<Logge
Trace{trace, std::chrono::steady_clock::now()}); // NO_CHECK_FORMAT(real_time)
}

OptRef<const std::vector<Trace>> getTraces() override {
std::string getTraces() override {
absl::MutexLock lock(&mutex_);
return traces_;
std::vector<std::string> 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() {
Expand Down
2 changes: 2 additions & 0 deletions test/extensions/filters/http/dynamic_forward_proxy/test_resolver.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ class TestResolver : public GetAddrInfoDnsResolver {
if (dns_override.has_value()) {
*const_cast<std::string*>(&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;
Expand Down
95 changes: 53 additions & 42 deletions test/extensions/network/dns_resolver/getaddrinfo/getaddrinfo_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,9 @@ class GetAddrInfoDnsImplTest : public testing::Test {
};

MATCHER_P(HasTrace, expected_trace, "") {
return arg.trace_ == static_cast<uint8_t>(expected_trace);
std::vector<std::string> v = absl::StrSplit(arg, '=');
uint8_t trace = std::stoi(v.at(0));
return trace == static_cast<uint8_t>(expected_trace);
}

TEST_F(GetAddrInfoDnsImplTest, LocalhostResolve) {
Expand All @@ -124,11 +126,12 @@ TEST_F(GetAddrInfoDnsImplTest, LocalhostResolve) {
[this](DnsResolver::ResolutionStatus status, absl::string_view,
std::list<DnsResponse>&& 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<std::string> 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();
});

Expand All @@ -152,11 +155,12 @@ TEST_F(GetAddrInfoDnsImplTest, Cancel) {
[this](DnsResolver::ResolutionStatus status, absl::string_view,
std::list<DnsResponse>&& 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<std::string> 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();
});

Expand All @@ -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<std::string> 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();
});

Expand All @@ -201,11 +206,12 @@ TEST_F(GetAddrInfoDnsImplTest, NoData) {
std::list<DnsResponse>&& 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<std::string> 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();
});

Expand All @@ -225,11 +231,12 @@ TEST_F(GetAddrInfoDnsImplTest, NoName) {
std::list<DnsResponse>&& 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<std::string> 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();
});

Expand All @@ -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<DnsResponse>&& 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<DnsResponse>&& response) {
EXPECT_EQ(status, DnsResolver::ResolutionStatus::Completed);
EXPECT_TRUE(response.empty());
std::vector<std::string> 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);
}
Expand Down Expand Up @@ -304,8 +313,9 @@ TEST_F(GetAddrInfoDnsImplTest, TryAgainWithNumRetriesAndSuccess) {
std::list<DnsResponse>&& response) {
EXPECT_EQ(status, DnsResolver::ResolutionStatus::Completed);
EXPECT_TRUE(response.empty());
std::vector<std::string> 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),
Expand Down Expand Up @@ -340,8 +350,9 @@ TEST_F(GetAddrInfoDnsImplTest, TryAgainWithNumRetriesAndFailure) {
EXPECT_EQ(status, DnsResolver::ResolutionStatus::Failure);
EXPECT_FALSE(details.empty());
EXPECT_TRUE(response.empty());
std::vector<std::string> 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),
Expand Down
2 changes: 1 addition & 1 deletion test/mocks/network/mocks.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<const std::vector<Trace>>, getTraces, ());
MOCK_METHOD(std::string, getTraces, ());
};

class MockFilterManager : public FilterManager {
Expand Down

0 comments on commit 17d5942

Please sign in to comment.