From 19bde102a292752458194b85c03514f0f3dcaa41 Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Thu, 6 Sep 2018 17:01:21 -0400 Subject: [PATCH 1/4] Revert "dns: fix exception unsafe behavior in c-ares callbacks. (#4307)" This reverts commit 1d34172bd058f31956741ec5f3346339fc624fd0. Signed-off-by: Harvey Tuch --- include/envoy/network/dns.h | 3 +- source/common/network/dns_impl.cc | 5 +- source/common/network/dns_impl.h | 7 +- source/common/upstream/logical_dns_cluster.cc | 4 +- source/common/upstream/upstream_impl.cc | 7 +- test/common/network/dns_impl_test.cc | 265 +++++++++--------- ...testcase-server_fuzz_test-6288786894880768 | 39 --- test/server/server_fuzz_test.cc | 8 +- 8 files changed, 139 insertions(+), 199 deletions(-) delete mode 100644 test/server/server_corpus/clusterfuzz-testcase-server_fuzz_test-6288786894880768 diff --git a/include/envoy/network/dns.h b/include/envoy/network/dns.h index 64d79bd8d7d85..f89cac1ef4617 100644 --- a/include/envoy/network/dns.h +++ b/include/envoy/network/dns.h @@ -38,8 +38,7 @@ class DnsResolver { * @param address_list supplies the list of resolved IP addresses. The list will be empty if * the resolution failed. */ - typedef std::function&& address_list)> - ResolveCb; + typedef std::function&& address_list)> ResolveCb; /** * Initiate an async DNS resolution. diff --git a/source/common/network/dns_impl.cc b/source/common/network/dns_impl.cc index bb317ae397f03..33b721e59155e 100644 --- a/source/common/network/dns_impl.cc +++ b/source/common/network/dns_impl.cc @@ -120,8 +120,7 @@ void DnsResolverImpl::PendingResolution::onAresHostCallback(int status, int time if (completed_) { if (!cancelled_) { - dispatcher_.post( - [callback = callback_, al = std::move(address_list)] { callback(std::move(al)); }); + callback_(std::move(address_list)); } if (owned_) { delete this; @@ -186,7 +185,7 @@ ActiveDnsQuery* DnsResolverImpl::resolve(const std::string& dns_name, // failed intial call to getHostbyName followed by a synchronous IPv4 // resolution. std::unique_ptr pending_resolution( - new PendingResolution(callback, dispatcher_, channel_, dns_name)); + new PendingResolution(callback, channel_, dns_name)); if (dns_lookup_family == DnsLookupFamily::Auto) { pending_resolution->fallback_if_failed_ = true; } diff --git a/source/common/network/dns_impl.h b/source/common/network/dns_impl.h index 2f79190e8e18b..b10fd067d8d21 100644 --- a/source/common/network/dns_impl.h +++ b/source/common/network/dns_impl.h @@ -39,9 +39,8 @@ class DnsResolverImpl : public DnsResolver, protected Logger::Loggableresolve( dns_address, dns_lookup_family_, - [this, dns_address]( - const std::list&& address_list) -> void { + [this, + dns_address](std::list&& address_list) -> void { active_dns_query_ = nullptr; ENVOY_LOG(debug, "async DNS resolution complete for {}", dns_address); info_->stats().update_success_.inc(); diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index 5be2051da71f4..529c9bb1257a8 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -373,11 +373,6 @@ ClusterInfoImpl::ClusterInfoImpl(const envoy::api::v2::Cluster& config, idle_timeout_ = std::chrono::milliseconds( DurationUtil::durationToMilliseconds(config.common_http_protocol_options().idle_timeout())); } - - // TODO(htuch): Remove this temporary workaround when we have - // https://github.com/lyft/protoc-gen-validate/issues/97 resolved. This just provides early - // validation of sanity of fields that we should catch at config ingestion. - DurationUtil::durationToMilliseconds(common_lb_config_.update_merge_window()); } ProtocolOptionsConfigConstSharedPtr @@ -1147,7 +1142,7 @@ void StrictDnsClusterImpl::ResolveTarget::startResolve() { active_query_ = parent_.dns_resolver_->resolve( dns_address_, parent_.dns_lookup_family_, - [this](const std::list&& address_list) -> void { + [this](std::list&& address_list) -> void { active_query_ = nullptr; ENVOY_LOG(debug, "async DNS resolution complete for {}", dns_address_); parent_.info_->stats().update_success_.inc(); diff --git a/test/common/network/dns_impl_test.cc b/test/common/network/dns_impl_test.cc index d90029cd576be..136c686a6348a 100644 --- a/test/common/network/dns_impl_test.cc +++ b/test/common/network/dns_impl_test.cc @@ -459,12 +459,12 @@ INSTANTIATE_TEST_CASE_P(IpVersions, DnsImplTest, // development, where segfaults were encountered due to callback invocations on // destruction. TEST_P(DnsImplTest, DestructPending) { - EXPECT_NE(nullptr, resolver_->resolve( - "", DnsLookupFamily::V4Only, - [&](const std::list&& results) -> void { - FAIL(); - UNREFERENCED_PARAMETER(results); - })); + EXPECT_NE(nullptr, + resolver_->resolve("", DnsLookupFamily::V4Only, + [&](std::list&& results) -> void { + FAIL(); + UNREFERENCED_PARAMETER(results); + })); // Also validate that pending events are around to exercise the resource // reclamation path. EXPECT_GT(peer_->events().size(), 0U); @@ -475,24 +475,22 @@ TEST_P(DnsImplTest, DestructPending) { // asynchronous behavior or network events. TEST_P(DnsImplTest, LocalLookup) { std::list address_list; - EXPECT_NE(nullptr, resolver_->resolve( - "", DnsLookupFamily::V4Only, - [&](const std::list&& results) -> void { - address_list = results; - dispatcher_.exit(); - })); + EXPECT_NE(nullptr, + resolver_->resolve("", DnsLookupFamily::V4Only, + [&](std::list&& results) -> void { + address_list = results; + dispatcher_.exit(); + })); dispatcher_.run(Event::Dispatcher::RunType::Block); EXPECT_TRUE(address_list.empty()); if (GetParam() == Address::IpVersion::v4) { - // EXPECT_CALL(dispatcher_, post(_)); - EXPECT_EQ(nullptr, resolver_->resolve( - "localhost", DnsLookupFamily::V4Only, - [&](const std::list&& results) -> void { - address_list = results; - })); - dispatcher_.run(Event::Dispatcher::RunType::NonBlock); + EXPECT_EQ(nullptr, + resolver_->resolve("localhost", DnsLookupFamily::V4Only, + [&](std::list&& results) -> void { + address_list = results; + })); EXPECT_TRUE(hasAddress(address_list, "127.0.0.1")); EXPECT_FALSE(hasAddress(address_list, "::1")); } @@ -501,23 +499,21 @@ TEST_P(DnsImplTest, LocalLookup) { const std::string error_msg = "Synchronous DNS IPv6 localhost resolution failed. Please verify localhost resolves to ::1 " "in /etc/hosts, since this misconfiguration is a common cause of these failures."; - EXPECT_EQ(nullptr, resolver_->resolve( - "localhost", DnsLookupFamily::V6Only, - [&](const std::list&& results) -> void { - address_list = results; - })) + EXPECT_EQ(nullptr, + resolver_->resolve("localhost", DnsLookupFamily::V6Only, + [&](std::list&& results) -> void { + address_list = results; + })) << error_msg; - dispatcher_.run(Event::Dispatcher::RunType::NonBlock); EXPECT_TRUE(hasAddress(address_list, "::1")) << error_msg; EXPECT_FALSE(hasAddress(address_list, "127.0.0.1")); - EXPECT_EQ(nullptr, resolver_->resolve( - "localhost", DnsLookupFamily::Auto, - [&](const std::list&& results) -> void { - address_list = results; - })) + EXPECT_EQ(nullptr, + resolver_->resolve("localhost", DnsLookupFamily::Auto, + [&](std::list&& results) -> void { + address_list = results; + })) << error_msg; - dispatcher_.run(Event::Dispatcher::RunType::NonBlock); EXPECT_FALSE(hasAddress(address_list, "127.0.0.1")); EXPECT_TRUE(hasAddress(address_list, "::1")) << error_msg; } @@ -526,32 +522,32 @@ TEST_P(DnsImplTest, LocalLookup) { TEST_P(DnsImplTest, DnsIpAddressVersionV6) { std::list address_list; server_->addHosts("some.good.domain", {"1::2"}, AAAA); - EXPECT_NE(nullptr, resolver_->resolve( - "some.good.domain", DnsLookupFamily::Auto, - [&](const std::list&& results) -> void { - address_list = results; - dispatcher_.exit(); - })); + EXPECT_NE(nullptr, + resolver_->resolve("some.good.domain", DnsLookupFamily::Auto, + [&](std::list&& results) -> void { + address_list = results; + dispatcher_.exit(); + })); dispatcher_.run(Event::Dispatcher::RunType::Block); EXPECT_TRUE(hasAddress(address_list, "1::2")); - EXPECT_NE(nullptr, resolver_->resolve( - "some.good.domain", DnsLookupFamily::V4Only, - [&](const std::list&& results) -> void { - address_list = results; - dispatcher_.exit(); - })); + EXPECT_NE(nullptr, + resolver_->resolve("some.good.domain", DnsLookupFamily::V4Only, + [&](std::list&& results) -> void { + address_list = results; + dispatcher_.exit(); + })); dispatcher_.run(Event::Dispatcher::RunType::Block); EXPECT_FALSE(hasAddress(address_list, "1::2")); - EXPECT_NE(nullptr, resolver_->resolve( - "some.good.domain", DnsLookupFamily::V6Only, - [&](const std::list&& results) -> void { - address_list = results; - dispatcher_.exit(); - })); + EXPECT_NE(nullptr, + resolver_->resolve("some.good.domain", DnsLookupFamily::V6Only, + [&](std::list&& results) -> void { + address_list = results; + dispatcher_.exit(); + })); dispatcher_.run(Event::Dispatcher::RunType::Block); EXPECT_TRUE(hasAddress(address_list, "1::2")); @@ -560,32 +556,32 @@ TEST_P(DnsImplTest, DnsIpAddressVersionV6) { TEST_P(DnsImplTest, DnsIpAddressVersion) { std::list address_list; server_->addHosts("some.good.domain", {"1.2.3.4"}, A); - EXPECT_NE(nullptr, resolver_->resolve( - "some.good.domain", DnsLookupFamily::Auto, - [&](const std::list&& results) -> void { - address_list = results; - dispatcher_.exit(); - })); + EXPECT_NE(nullptr, + resolver_->resolve("some.good.domain", DnsLookupFamily::Auto, + [&](std::list&& results) -> void { + address_list = results; + dispatcher_.exit(); + })); dispatcher_.run(Event::Dispatcher::RunType::Block); EXPECT_TRUE(hasAddress(address_list, "1.2.3.4")); - EXPECT_NE(nullptr, resolver_->resolve( - "some.good.domain", DnsLookupFamily::V4Only, - [&](const std::list&& results) -> void { - address_list = results; - dispatcher_.exit(); - })); + EXPECT_NE(nullptr, + resolver_->resolve("some.good.domain", DnsLookupFamily::V4Only, + [&](std::list&& results) -> void { + address_list = results; + dispatcher_.exit(); + })); dispatcher_.run(Event::Dispatcher::RunType::Block); EXPECT_TRUE(hasAddress(address_list, "1.2.3.4")); - EXPECT_NE(nullptr, resolver_->resolve( - "some.good.domain", DnsLookupFamily::V6Only, - [&](const std::list&& results) -> void { - address_list = results; - dispatcher_.exit(); - })); + EXPECT_NE(nullptr, + resolver_->resolve("some.good.domain", DnsLookupFamily::V6Only, + [&](std::list&& results) -> void { + address_list = results; + dispatcher_.exit(); + })); dispatcher_.run(Event::Dispatcher::RunType::Block); EXPECT_FALSE(hasAddress(address_list, "1.2.3.4")); @@ -596,22 +592,22 @@ TEST_P(DnsImplTest, DnsIpAddressVersion) { TEST_P(DnsImplTest, RemoteAsyncLookup) { server_->addHosts("some.good.domain", {"201.134.56.7"}, A); std::list address_list; - EXPECT_NE(nullptr, resolver_->resolve( - "some.bad.domain", DnsLookupFamily::Auto, - [&](const std::list&& results) -> void { - address_list = results; - dispatcher_.exit(); - })); + EXPECT_NE(nullptr, + resolver_->resolve("some.bad.domain", DnsLookupFamily::Auto, + [&](std::list&& results) -> void { + address_list = results; + dispatcher_.exit(); + })); dispatcher_.run(Event::Dispatcher::RunType::Block); EXPECT_TRUE(address_list.empty()); - EXPECT_NE(nullptr, resolver_->resolve( - "some.good.domain", DnsLookupFamily::Auto, - [&](const std::list&& results) -> void { - address_list = results; - dispatcher_.exit(); - })); + EXPECT_NE(nullptr, + resolver_->resolve("some.good.domain", DnsLookupFamily::Auto, + [&](std::list&& results) -> void { + address_list = results; + dispatcher_.exit(); + })); dispatcher_.run(Event::Dispatcher::RunType::Block); EXPECT_TRUE(hasAddress(address_list, "201.134.56.7")); @@ -621,12 +617,12 @@ TEST_P(DnsImplTest, RemoteAsyncLookup) { TEST_P(DnsImplTest, MultiARecordLookup) { server_->addHosts("some.good.domain", {"201.134.56.7", "123.4.5.6", "6.5.4.3"}, A); std::list address_list; - EXPECT_NE(nullptr, resolver_->resolve( - "some.good.domain", DnsLookupFamily::V4Only, - [&](const std::list&& results) -> void { - address_list = results; - dispatcher_.exit(); - })); + EXPECT_NE(nullptr, + resolver_->resolve("some.good.domain", DnsLookupFamily::V4Only, + [&](std::list&& results) -> void { + address_list = results; + dispatcher_.exit(); + })); dispatcher_.run(Event::Dispatcher::RunType::Block); EXPECT_TRUE(hasAddress(address_list, "201.134.56.7")); @@ -638,12 +634,12 @@ TEST_P(DnsImplTest, CNameARecordLookupV4) { server_->addCName("root.cnam.domain", "result.cname.domain"); server_->addHosts("result.cname.domain", {"201.134.56.7"}, A); std::list address_list; - EXPECT_NE(nullptr, resolver_->resolve( - "root.cnam.domain", DnsLookupFamily::V4Only, - [&](const std::list&& results) -> void { - address_list = results; - dispatcher_.exit(); - })); + EXPECT_NE(nullptr, + resolver_->resolve("root.cnam.domain", DnsLookupFamily::V4Only, + [&](std::list&& results) -> void { + address_list = results; + dispatcher_.exit(); + })); dispatcher_.run(Event::Dispatcher::RunType::Block); EXPECT_TRUE(hasAddress(address_list, "201.134.56.7")); @@ -653,12 +649,12 @@ TEST_P(DnsImplTest, CNameARecordLookupWithV6) { server_->addCName("root.cnam.domain", "result.cname.domain"); server_->addHosts("result.cname.domain", {"201.134.56.7"}, A); std::list address_list; - EXPECT_NE(nullptr, resolver_->resolve( - "root.cnam.domain", DnsLookupFamily::Auto, - [&](const std::list&& results) -> void { - address_list = results; - dispatcher_.exit(); - })); + EXPECT_NE(nullptr, + resolver_->resolve("root.cnam.domain", DnsLookupFamily::Auto, + [&](std::list&& results) -> void { + address_list = results; + dispatcher_.exit(); + })); dispatcher_.run(Event::Dispatcher::RunType::Block); EXPECT_TRUE(hasAddress(address_list, "201.134.56.7")); @@ -668,36 +664,36 @@ TEST_P(DnsImplTest, MultiARecordLookupWithV6) { server_->addHosts("some.good.domain", {"201.134.56.7", "123.4.5.6", "6.5.4.3"}, A); server_->addHosts("some.good.domain", {"1::2", "1::2:3", "1::2:3:4"}, AAAA); std::list address_list; - EXPECT_NE(nullptr, resolver_->resolve( - "some.good.domain", DnsLookupFamily::V4Only, - [&](const std::list&& results) -> void { - address_list = results; - dispatcher_.exit(); - })); + EXPECT_NE(nullptr, + resolver_->resolve("some.good.domain", DnsLookupFamily::V4Only, + [&](std::list&& results) -> void { + address_list = results; + dispatcher_.exit(); + })); dispatcher_.run(Event::Dispatcher::RunType::Block); EXPECT_TRUE(hasAddress(address_list, "201.134.56.7")); EXPECT_TRUE(hasAddress(address_list, "123.4.5.6")); EXPECT_TRUE(hasAddress(address_list, "6.5.4.3")); - EXPECT_NE(nullptr, resolver_->resolve( - "some.good.domain", DnsLookupFamily::Auto, - [&](const std::list&& results) -> void { - address_list = results; - dispatcher_.exit(); - })); + EXPECT_NE(nullptr, + resolver_->resolve("some.good.domain", DnsLookupFamily::Auto, + [&](std::list&& results) -> void { + address_list = results; + dispatcher_.exit(); + })); dispatcher_.run(Event::Dispatcher::RunType::Block); EXPECT_TRUE(hasAddress(address_list, "1::2")); EXPECT_TRUE(hasAddress(address_list, "1::2:3")); EXPECT_TRUE(hasAddress(address_list, "1::2:3:4")); - EXPECT_NE(nullptr, resolver_->resolve( - "some.good.domain", DnsLookupFamily::V6Only, - [&](const std::list&& results) -> void { - address_list = results; - dispatcher_.exit(); - })); + EXPECT_NE(nullptr, + resolver_->resolve("some.good.domain", DnsLookupFamily::V6Only, + [&](std::list&& results) -> void { + address_list = results; + dispatcher_.exit(); + })); dispatcher_.run(Event::Dispatcher::RunType::Block); EXPECT_TRUE(hasAddress(address_list, "1::2")); @@ -709,17 +705,17 @@ TEST_P(DnsImplTest, MultiARecordLookupWithV6) { TEST_P(DnsImplTest, Cancel) { server_->addHosts("some.good.domain", {"201.134.56.7"}, A); - ActiveDnsQuery* query = resolver_->resolve( - "some.domain", DnsLookupFamily::Auto, - [](const std::list &&) -> void { FAIL(); }); + ActiveDnsQuery* query = + resolver_->resolve("some.domain", DnsLookupFamily::Auto, + [](std::list &&) -> void { FAIL(); }); std::list address_list; - EXPECT_NE(nullptr, resolver_->resolve( - "some.good.domain", DnsLookupFamily::Auto, - [&](const std::list&& results) -> void { - address_list = results; - dispatcher_.exit(); - })); + EXPECT_NE(nullptr, + resolver_->resolve("some.good.domain", DnsLookupFamily::Auto, + [&](std::list&& results) -> void { + address_list = results; + dispatcher_.exit(); + })); ASSERT_NE(nullptr, query); query->cancel(); @@ -742,12 +738,12 @@ INSTANTIATE_TEST_CASE_P(IpVersions, DnsImplZeroTimeoutTest, TEST_P(DnsImplZeroTimeoutTest, Timeout) { server_->addHosts("some.good.domain", {"201.134.56.7"}, A); std::list address_list; - EXPECT_NE(nullptr, resolver_->resolve( - "some.good.domain", DnsLookupFamily::V4Only, - [&](const std::list&& results) -> void { - address_list = results; - dispatcher_.exit(); - })); + EXPECT_NE(nullptr, + resolver_->resolve("some.good.domain", DnsLookupFamily::V4Only, + [&](std::list&& results) -> void { + address_list = results; + dispatcher_.exit(); + })); dispatcher_.run(Event::Dispatcher::RunType::Block); EXPECT_TRUE(address_list.empty()); @@ -764,11 +760,10 @@ TEST(DnsImplUnitTest, PendingTimerEnable) { Event::FileEvent* file_event = new NiceMock(); EXPECT_CALL(dispatcher, createFileEvent_(_, _, _, _)).WillOnce(Return(file_event)); EXPECT_CALL(*timer, enableTimer(_)); - EXPECT_NE(nullptr, - resolver.resolve("some.bad.domain.invalid", DnsLookupFamily::V4Only, - [&](const std::list&& results) { - UNREFERENCED_PARAMETER(results); - })); + EXPECT_NE(nullptr, resolver.resolve("some.bad.domain.invalid", DnsLookupFamily::V4Only, + [&](std::list&& results) { + UNREFERENCED_PARAMETER(results); + })); } } // namespace Network diff --git a/test/server/server_corpus/clusterfuzz-testcase-server_fuzz_test-6288786894880768 b/test/server/server_corpus/clusterfuzz-testcase-server_fuzz_test-6288786894880768 deleted file mode 100644 index 14c7c52760c70..0000000000000 --- a/test/server/server_corpus/clusterfuzz-testcase-server_fuzz_test-6288786894880768 +++ /dev/null @@ -1,39 +0,0 @@ -node { - locality { - } -} -static_resources { - clusters { - name: "x" - type: STRICT_DNS - connect_timeout { - nanos: 250000000 - } - lb_policy: RING_HASH - hosts { - socket_address { - address: "123.1.0.1" - named_port: "x" - } - } - hosts { - socket_address { - address: "127.0.0.2" - named_port: "3" - } - } - common_lb_config { - update_merge_window { - seconds: 281474976710656 - } - } - } -} -admin { - access_log_path: "@-" - address { - pipe { - path: " " - } - } -} diff --git a/test/server/server_fuzz_test.cc b/test/server/server_fuzz_test.cc index af38634b42cf2..40426d1ea0b58 100644 --- a/test/server/server_fuzz_test.cc +++ b/test/server/server_fuzz_test.cc @@ -74,21 +74,15 @@ DEFINE_PROTO_FUZZER(const envoy::config::bootstrap::v2::Bootstrap& input) { options.log_level_ = Fuzz::Runner::logLevel(); } - std::unique_ptr server; try { - server = std::make_unique( + auto server = std::make_unique( options, test_time.timeSource(), std::make_shared("127.0.0.1"), hooks, restart, stats_store, fakelock, component_factory, std::make_unique(), thread_local_instance); } catch (const EnvoyException& ex) { ENVOY_LOG_MISC(debug, "Controlled EnvoyException exit: {}", ex.what()); - return; } - // If we were successful, run any pending events on the main thread's dispatcher loop. These might - // be, for example, pending DNS resolution callbacks. If they generate exceptions, we want to - // explode and fail the test, hence we do this outside of the try-catch above. - server->dispatcher().run(Event::Dispatcher::RunType::NonBlock); } } // namespace Server From cf88b8d8fdbfb5f1c2f8905efa1b3a30bc06f9eb Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Thu, 6 Sep 2018 17:01:55 -0400 Subject: [PATCH 2/4] Preserve non-controversial aspects of #4307. Signed-off-by: Harvey Tuch --- include/envoy/network/dns.h | 3 +- source/common/upstream/logical_dns_cluster.cc | 4 +- source/common/upstream/upstream_impl.cc | 7 +- test/common/network/dns_impl_test.cc | 262 +++++++++--------- ...testcase-server_fuzz_test-6288786894880768 | 39 +++ test/server/server_fuzz_test.cc | 8 +- 6 files changed, 188 insertions(+), 135 deletions(-) create mode 100644 test/server/server_corpus/clusterfuzz-testcase-server_fuzz_test-6288786894880768 diff --git a/include/envoy/network/dns.h b/include/envoy/network/dns.h index f89cac1ef4617..64d79bd8d7d85 100644 --- a/include/envoy/network/dns.h +++ b/include/envoy/network/dns.h @@ -38,7 +38,8 @@ class DnsResolver { * @param address_list supplies the list of resolved IP addresses. The list will be empty if * the resolution failed. */ - typedef std::function&& address_list)> ResolveCb; + typedef std::function&& address_list)> + ResolveCb; /** * Initiate an async DNS resolution. diff --git a/source/common/upstream/logical_dns_cluster.cc b/source/common/upstream/logical_dns_cluster.cc index c8bf6c73f3c50..7a26b09546533 100644 --- a/source/common/upstream/logical_dns_cluster.cc +++ b/source/common/upstream/logical_dns_cluster.cc @@ -82,8 +82,8 @@ void LogicalDnsCluster::startResolve() { active_dns_query_ = dns_resolver_->resolve( dns_address, dns_lookup_family_, - [this, - dns_address](std::list&& address_list) -> void { + [this, dns_address]( + const std::list&& address_list) -> void { active_dns_query_ = nullptr; ENVOY_LOG(debug, "async DNS resolution complete for {}", dns_address); info_->stats().update_success_.inc(); diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index 529c9bb1257a8..5be2051da71f4 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -373,6 +373,11 @@ ClusterInfoImpl::ClusterInfoImpl(const envoy::api::v2::Cluster& config, idle_timeout_ = std::chrono::milliseconds( DurationUtil::durationToMilliseconds(config.common_http_protocol_options().idle_timeout())); } + + // TODO(htuch): Remove this temporary workaround when we have + // https://github.com/lyft/protoc-gen-validate/issues/97 resolved. This just provides early + // validation of sanity of fields that we should catch at config ingestion. + DurationUtil::durationToMilliseconds(common_lb_config_.update_merge_window()); } ProtocolOptionsConfigConstSharedPtr @@ -1142,7 +1147,7 @@ void StrictDnsClusterImpl::ResolveTarget::startResolve() { active_query_ = parent_.dns_resolver_->resolve( dns_address_, parent_.dns_lookup_family_, - [this](std::list&& address_list) -> void { + [this](const std::list&& address_list) -> void { active_query_ = nullptr; ENVOY_LOG(debug, "async DNS resolution complete for {}", dns_address_); parent_.info_->stats().update_success_.inc(); diff --git a/test/common/network/dns_impl_test.cc b/test/common/network/dns_impl_test.cc index 136c686a6348a..96358d6137a84 100644 --- a/test/common/network/dns_impl_test.cc +++ b/test/common/network/dns_impl_test.cc @@ -459,12 +459,12 @@ INSTANTIATE_TEST_CASE_P(IpVersions, DnsImplTest, // development, where segfaults were encountered due to callback invocations on // destruction. TEST_P(DnsImplTest, DestructPending) { - EXPECT_NE(nullptr, - resolver_->resolve("", DnsLookupFamily::V4Only, - [&](std::list&& results) -> void { - FAIL(); - UNREFERENCED_PARAMETER(results); - })); + EXPECT_NE(nullptr, resolver_->resolve( + "", DnsLookupFamily::V4Only, + [&](const std::list&& results) -> void { + FAIL(); + UNREFERENCED_PARAMETER(results); + })); // Also validate that pending events are around to exercise the resource // reclamation path. EXPECT_GT(peer_->events().size(), 0U); @@ -475,22 +475,23 @@ TEST_P(DnsImplTest, DestructPending) { // asynchronous behavior or network events. TEST_P(DnsImplTest, LocalLookup) { std::list address_list; - EXPECT_NE(nullptr, - resolver_->resolve("", DnsLookupFamily::V4Only, - [&](std::list&& results) -> void { - address_list = results; - dispatcher_.exit(); - })); + EXPECT_NE(nullptr, resolver_->resolve( + "", DnsLookupFamily::V4Only, + [&](const std::list&& results) -> void { + address_list = results; + dispatcher_.exit(); + })); dispatcher_.run(Event::Dispatcher::RunType::Block); EXPECT_TRUE(address_list.empty()); if (GetParam() == Address::IpVersion::v4) { - EXPECT_EQ(nullptr, - resolver_->resolve("localhost", DnsLookupFamily::V4Only, - [&](std::list&& results) -> void { - address_list = results; - })); + // EXPECT_CALL(dispatcher_, post(_)); + EXPECT_EQ(nullptr, resolver_->resolve( + "localhost", DnsLookupFamily::V4Only, + [&](const std::list&& results) -> void { + address_list = results; + })); EXPECT_TRUE(hasAddress(address_list, "127.0.0.1")); EXPECT_FALSE(hasAddress(address_list, "::1")); } @@ -499,20 +500,20 @@ TEST_P(DnsImplTest, LocalLookup) { const std::string error_msg = "Synchronous DNS IPv6 localhost resolution failed. Please verify localhost resolves to ::1 " "in /etc/hosts, since this misconfiguration is a common cause of these failures."; - EXPECT_EQ(nullptr, - resolver_->resolve("localhost", DnsLookupFamily::V6Only, - [&](std::list&& results) -> void { - address_list = results; - })) + EXPECT_EQ(nullptr, resolver_->resolve( + "localhost", DnsLookupFamily::V6Only, + [&](const std::list&& results) -> void { + address_list = results; + })) << error_msg; EXPECT_TRUE(hasAddress(address_list, "::1")) << error_msg; EXPECT_FALSE(hasAddress(address_list, "127.0.0.1")); - EXPECT_EQ(nullptr, - resolver_->resolve("localhost", DnsLookupFamily::Auto, - [&](std::list&& results) -> void { - address_list = results; - })) + EXPECT_EQ(nullptr, resolver_->resolve( + "localhost", DnsLookupFamily::Auto, + [&](const std::list&& results) -> void { + address_list = results; + })) << error_msg; EXPECT_FALSE(hasAddress(address_list, "127.0.0.1")); EXPECT_TRUE(hasAddress(address_list, "::1")) << error_msg; @@ -522,32 +523,32 @@ TEST_P(DnsImplTest, LocalLookup) { TEST_P(DnsImplTest, DnsIpAddressVersionV6) { std::list address_list; server_->addHosts("some.good.domain", {"1::2"}, AAAA); - EXPECT_NE(nullptr, - resolver_->resolve("some.good.domain", DnsLookupFamily::Auto, - [&](std::list&& results) -> void { - address_list = results; - dispatcher_.exit(); - })); + EXPECT_NE(nullptr, resolver_->resolve( + "some.good.domain", DnsLookupFamily::Auto, + [&](const std::list&& results) -> void { + address_list = results; + dispatcher_.exit(); + })); dispatcher_.run(Event::Dispatcher::RunType::Block); EXPECT_TRUE(hasAddress(address_list, "1::2")); - EXPECT_NE(nullptr, - resolver_->resolve("some.good.domain", DnsLookupFamily::V4Only, - [&](std::list&& results) -> void { - address_list = results; - dispatcher_.exit(); - })); + EXPECT_NE(nullptr, resolver_->resolve( + "some.good.domain", DnsLookupFamily::V4Only, + [&](const std::list&& results) -> void { + address_list = results; + dispatcher_.exit(); + })); dispatcher_.run(Event::Dispatcher::RunType::Block); EXPECT_FALSE(hasAddress(address_list, "1::2")); - EXPECT_NE(nullptr, - resolver_->resolve("some.good.domain", DnsLookupFamily::V6Only, - [&](std::list&& results) -> void { - address_list = results; - dispatcher_.exit(); - })); + EXPECT_NE(nullptr, resolver_->resolve( + "some.good.domain", DnsLookupFamily::V6Only, + [&](const std::list&& results) -> void { + address_list = results; + dispatcher_.exit(); + })); dispatcher_.run(Event::Dispatcher::RunType::Block); EXPECT_TRUE(hasAddress(address_list, "1::2")); @@ -556,32 +557,32 @@ TEST_P(DnsImplTest, DnsIpAddressVersionV6) { TEST_P(DnsImplTest, DnsIpAddressVersion) { std::list address_list; server_->addHosts("some.good.domain", {"1.2.3.4"}, A); - EXPECT_NE(nullptr, - resolver_->resolve("some.good.domain", DnsLookupFamily::Auto, - [&](std::list&& results) -> void { - address_list = results; - dispatcher_.exit(); - })); + EXPECT_NE(nullptr, resolver_->resolve( + "some.good.domain", DnsLookupFamily::Auto, + [&](const std::list&& results) -> void { + address_list = results; + dispatcher_.exit(); + })); dispatcher_.run(Event::Dispatcher::RunType::Block); EXPECT_TRUE(hasAddress(address_list, "1.2.3.4")); - EXPECT_NE(nullptr, - resolver_->resolve("some.good.domain", DnsLookupFamily::V4Only, - [&](std::list&& results) -> void { - address_list = results; - dispatcher_.exit(); - })); + EXPECT_NE(nullptr, resolver_->resolve( + "some.good.domain", DnsLookupFamily::V4Only, + [&](const std::list&& results) -> void { + address_list = results; + dispatcher_.exit(); + })); dispatcher_.run(Event::Dispatcher::RunType::Block); EXPECT_TRUE(hasAddress(address_list, "1.2.3.4")); - EXPECT_NE(nullptr, - resolver_->resolve("some.good.domain", DnsLookupFamily::V6Only, - [&](std::list&& results) -> void { - address_list = results; - dispatcher_.exit(); - })); + EXPECT_NE(nullptr, resolver_->resolve( + "some.good.domain", DnsLookupFamily::V6Only, + [&](const std::list&& results) -> void { + address_list = results; + dispatcher_.exit(); + })); dispatcher_.run(Event::Dispatcher::RunType::Block); EXPECT_FALSE(hasAddress(address_list, "1.2.3.4")); @@ -592,22 +593,22 @@ TEST_P(DnsImplTest, DnsIpAddressVersion) { TEST_P(DnsImplTest, RemoteAsyncLookup) { server_->addHosts("some.good.domain", {"201.134.56.7"}, A); std::list address_list; - EXPECT_NE(nullptr, - resolver_->resolve("some.bad.domain", DnsLookupFamily::Auto, - [&](std::list&& results) -> void { - address_list = results; - dispatcher_.exit(); - })); + EXPECT_NE(nullptr, resolver_->resolve( + "some.bad.domain", DnsLookupFamily::Auto, + [&](const std::list&& results) -> void { + address_list = results; + dispatcher_.exit(); + })); dispatcher_.run(Event::Dispatcher::RunType::Block); EXPECT_TRUE(address_list.empty()); - EXPECT_NE(nullptr, - resolver_->resolve("some.good.domain", DnsLookupFamily::Auto, - [&](std::list&& results) -> void { - address_list = results; - dispatcher_.exit(); - })); + EXPECT_NE(nullptr, resolver_->resolve( + "some.good.domain", DnsLookupFamily::Auto, + [&](const std::list&& results) -> void { + address_list = results; + dispatcher_.exit(); + })); dispatcher_.run(Event::Dispatcher::RunType::Block); EXPECT_TRUE(hasAddress(address_list, "201.134.56.7")); @@ -617,12 +618,12 @@ TEST_P(DnsImplTest, RemoteAsyncLookup) { TEST_P(DnsImplTest, MultiARecordLookup) { server_->addHosts("some.good.domain", {"201.134.56.7", "123.4.5.6", "6.5.4.3"}, A); std::list address_list; - EXPECT_NE(nullptr, - resolver_->resolve("some.good.domain", DnsLookupFamily::V4Only, - [&](std::list&& results) -> void { - address_list = results; - dispatcher_.exit(); - })); + EXPECT_NE(nullptr, resolver_->resolve( + "some.good.domain", DnsLookupFamily::V4Only, + [&](const std::list&& results) -> void { + address_list = results; + dispatcher_.exit(); + })); dispatcher_.run(Event::Dispatcher::RunType::Block); EXPECT_TRUE(hasAddress(address_list, "201.134.56.7")); @@ -634,12 +635,12 @@ TEST_P(DnsImplTest, CNameARecordLookupV4) { server_->addCName("root.cnam.domain", "result.cname.domain"); server_->addHosts("result.cname.domain", {"201.134.56.7"}, A); std::list address_list; - EXPECT_NE(nullptr, - resolver_->resolve("root.cnam.domain", DnsLookupFamily::V4Only, - [&](std::list&& results) -> void { - address_list = results; - dispatcher_.exit(); - })); + EXPECT_NE(nullptr, resolver_->resolve( + "root.cnam.domain", DnsLookupFamily::V4Only, + [&](const std::list&& results) -> void { + address_list = results; + dispatcher_.exit(); + })); dispatcher_.run(Event::Dispatcher::RunType::Block); EXPECT_TRUE(hasAddress(address_list, "201.134.56.7")); @@ -649,12 +650,12 @@ TEST_P(DnsImplTest, CNameARecordLookupWithV6) { server_->addCName("root.cnam.domain", "result.cname.domain"); server_->addHosts("result.cname.domain", {"201.134.56.7"}, A); std::list address_list; - EXPECT_NE(nullptr, - resolver_->resolve("root.cnam.domain", DnsLookupFamily::Auto, - [&](std::list&& results) -> void { - address_list = results; - dispatcher_.exit(); - })); + EXPECT_NE(nullptr, resolver_->resolve( + "root.cnam.domain", DnsLookupFamily::Auto, + [&](const std::list&& results) -> void { + address_list = results; + dispatcher_.exit(); + })); dispatcher_.run(Event::Dispatcher::RunType::Block); EXPECT_TRUE(hasAddress(address_list, "201.134.56.7")); @@ -664,36 +665,36 @@ TEST_P(DnsImplTest, MultiARecordLookupWithV6) { server_->addHosts("some.good.domain", {"201.134.56.7", "123.4.5.6", "6.5.4.3"}, A); server_->addHosts("some.good.domain", {"1::2", "1::2:3", "1::2:3:4"}, AAAA); std::list address_list; - EXPECT_NE(nullptr, - resolver_->resolve("some.good.domain", DnsLookupFamily::V4Only, - [&](std::list&& results) -> void { - address_list = results; - dispatcher_.exit(); - })); + EXPECT_NE(nullptr, resolver_->resolve( + "some.good.domain", DnsLookupFamily::V4Only, + [&](const std::list&& results) -> void { + address_list = results; + dispatcher_.exit(); + })); dispatcher_.run(Event::Dispatcher::RunType::Block); EXPECT_TRUE(hasAddress(address_list, "201.134.56.7")); EXPECT_TRUE(hasAddress(address_list, "123.4.5.6")); EXPECT_TRUE(hasAddress(address_list, "6.5.4.3")); - EXPECT_NE(nullptr, - resolver_->resolve("some.good.domain", DnsLookupFamily::Auto, - [&](std::list&& results) -> void { - address_list = results; - dispatcher_.exit(); - })); + EXPECT_NE(nullptr, resolver_->resolve( + "some.good.domain", DnsLookupFamily::Auto, + [&](const std::list&& results) -> void { + address_list = results; + dispatcher_.exit(); + })); dispatcher_.run(Event::Dispatcher::RunType::Block); EXPECT_TRUE(hasAddress(address_list, "1::2")); EXPECT_TRUE(hasAddress(address_list, "1::2:3")); EXPECT_TRUE(hasAddress(address_list, "1::2:3:4")); - EXPECT_NE(nullptr, - resolver_->resolve("some.good.domain", DnsLookupFamily::V6Only, - [&](std::list&& results) -> void { - address_list = results; - dispatcher_.exit(); - })); + EXPECT_NE(nullptr, resolver_->resolve( + "some.good.domain", DnsLookupFamily::V6Only, + [&](const std::list&& results) -> void { + address_list = results; + dispatcher_.exit(); + })); dispatcher_.run(Event::Dispatcher::RunType::Block); EXPECT_TRUE(hasAddress(address_list, "1::2")); @@ -705,17 +706,17 @@ TEST_P(DnsImplTest, MultiARecordLookupWithV6) { TEST_P(DnsImplTest, Cancel) { server_->addHosts("some.good.domain", {"201.134.56.7"}, A); - ActiveDnsQuery* query = - resolver_->resolve("some.domain", DnsLookupFamily::Auto, - [](std::list &&) -> void { FAIL(); }); + ActiveDnsQuery* query = resolver_->resolve( + "some.domain", DnsLookupFamily::Auto, + [](const std::list &&) -> void { FAIL(); }); std::list address_list; - EXPECT_NE(nullptr, - resolver_->resolve("some.good.domain", DnsLookupFamily::Auto, - [&](std::list&& results) -> void { - address_list = results; - dispatcher_.exit(); - })); + EXPECT_NE(nullptr, resolver_->resolve( + "some.good.domain", DnsLookupFamily::Auto, + [&](const std::list&& results) -> void { + address_list = results; + dispatcher_.exit(); + })); ASSERT_NE(nullptr, query); query->cancel(); @@ -738,12 +739,12 @@ INSTANTIATE_TEST_CASE_P(IpVersions, DnsImplZeroTimeoutTest, TEST_P(DnsImplZeroTimeoutTest, Timeout) { server_->addHosts("some.good.domain", {"201.134.56.7"}, A); std::list address_list; - EXPECT_NE(nullptr, - resolver_->resolve("some.good.domain", DnsLookupFamily::V4Only, - [&](std::list&& results) -> void { - address_list = results; - dispatcher_.exit(); - })); + EXPECT_NE(nullptr, resolver_->resolve( + "some.good.domain", DnsLookupFamily::V4Only, + [&](const std::list&& results) -> void { + address_list = results; + dispatcher_.exit(); + })); dispatcher_.run(Event::Dispatcher::RunType::Block); EXPECT_TRUE(address_list.empty()); @@ -760,10 +761,11 @@ TEST(DnsImplUnitTest, PendingTimerEnable) { Event::FileEvent* file_event = new NiceMock(); EXPECT_CALL(dispatcher, createFileEvent_(_, _, _, _)).WillOnce(Return(file_event)); EXPECT_CALL(*timer, enableTimer(_)); - EXPECT_NE(nullptr, resolver.resolve("some.bad.domain.invalid", DnsLookupFamily::V4Only, - [&](std::list&& results) { - UNREFERENCED_PARAMETER(results); - })); + EXPECT_NE(nullptr, + resolver.resolve("some.bad.domain.invalid", DnsLookupFamily::V4Only, + [&](const std::list&& results) { + UNREFERENCED_PARAMETER(results); + })); } } // namespace Network diff --git a/test/server/server_corpus/clusterfuzz-testcase-server_fuzz_test-6288786894880768 b/test/server/server_corpus/clusterfuzz-testcase-server_fuzz_test-6288786894880768 new file mode 100644 index 0000000000000..14c7c52760c70 --- /dev/null +++ b/test/server/server_corpus/clusterfuzz-testcase-server_fuzz_test-6288786894880768 @@ -0,0 +1,39 @@ +node { + locality { + } +} +static_resources { + clusters { + name: "x" + type: STRICT_DNS + connect_timeout { + nanos: 250000000 + } + lb_policy: RING_HASH + hosts { + socket_address { + address: "123.1.0.1" + named_port: "x" + } + } + hosts { + socket_address { + address: "127.0.0.2" + named_port: "3" + } + } + common_lb_config { + update_merge_window { + seconds: 281474976710656 + } + } + } +} +admin { + access_log_path: "@-" + address { + pipe { + path: " " + } + } +} diff --git a/test/server/server_fuzz_test.cc b/test/server/server_fuzz_test.cc index 40426d1ea0b58..af38634b42cf2 100644 --- a/test/server/server_fuzz_test.cc +++ b/test/server/server_fuzz_test.cc @@ -74,15 +74,21 @@ DEFINE_PROTO_FUZZER(const envoy::config::bootstrap::v2::Bootstrap& input) { options.log_level_ = Fuzz::Runner::logLevel(); } + std::unique_ptr server; try { - auto server = std::make_unique( + server = std::make_unique( options, test_time.timeSource(), std::make_shared("127.0.0.1"), hooks, restart, stats_store, fakelock, component_factory, std::make_unique(), thread_local_instance); } catch (const EnvoyException& ex) { ENVOY_LOG_MISC(debug, "Controlled EnvoyException exit: {}", ex.what()); + return; } + // If we were successful, run any pending events on the main thread's dispatcher loop. These might + // be, for example, pending DNS resolution callbacks. If they generate exceptions, we want to + // explode and fail the test, hence we do this outside of the try-catch above. + server->dispatcher().run(Event::Dispatcher::RunType::NonBlock); } } // namespace Server From f9e03415f346fcdb02ff1282b404ec07d72d7fd9 Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Thu, 6 Sep 2018 17:16:18 -0400 Subject: [PATCH 3/4] Exception wrapper for c-ares callback. Signed-off-by: Harvey Tuch --- source/common/network/dns_impl.cc | 15 +++++++++++++-- source/common/network/dns_impl.h | 7 +++++-- test/common/network/dns_impl_test.cc | 23 +++++++++++++++++++++++ 3 files changed, 41 insertions(+), 4 deletions(-) diff --git a/source/common/network/dns_impl.cc b/source/common/network/dns_impl.cc index 33b721e59155e..c0fed8acf2ddf 100644 --- a/source/common/network/dns_impl.cc +++ b/source/common/network/dns_impl.cc @@ -120,7 +120,18 @@ void DnsResolverImpl::PendingResolution::onAresHostCallback(int status, int time if (completed_) { if (!cancelled_) { - callback_(std::move(address_list)); + try { + callback_(std::move(address_list)); + } catch (const EnvoyException& e) { + ENVOY_LOG_MISC(critical, "EnvoyException in c-ares callback"); + dispatcher_.post([e] { throw e; }); + } catch (const std::exception& e) { + ENVOY_LOG_MISC(critical, "std::exception in c-ares callback"); + dispatcher_.post([s = std::string(e.what())] { throw EnvoyException(s); }); + } catch (...) { + ENVOY_LOG_MISC(critical, "Unknown exception in c-ares callback"); + dispatcher_.post([] { throw EnvoyException("unknown"); }); + } } if (owned_) { delete this; @@ -185,7 +196,7 @@ ActiveDnsQuery* DnsResolverImpl::resolve(const std::string& dns_name, // failed intial call to getHostbyName followed by a synchronous IPv4 // resolution. std::unique_ptr pending_resolution( - new PendingResolution(callback, channel_, dns_name)); + new PendingResolution(callback, dispatcher_, channel_, dns_name)); if (dns_lookup_family == DnsLookupFamily::Auto) { pending_resolution->fallback_if_failed_ = true; } diff --git a/source/common/network/dns_impl.h b/source/common/network/dns_impl.h index b10fd067d8d21..1dad26e11a017 100644 --- a/source/common/network/dns_impl.h +++ b/source/common/network/dns_impl.h @@ -39,8 +39,9 @@ class DnsResolverImpl : public DnsResolver, protected Logger::Loggableresolve( + "1.2.3.4", DnsLookupFamily::V4Only, + [&](const std::list && + /*results*/) -> void { throw EnvoyException("Envoy exception"); })); + EXPECT_THROW_WITH_MESSAGE(dispatcher_.run(Event::Dispatcher::RunType::Block), EnvoyException, + "Envoy exception"); + EXPECT_EQ(nullptr, resolver_->resolve( + "1.2.3.4", DnsLookupFamily::V4Only, + [&](const std::list && + /*results*/) -> void { throw std::runtime_error("runtime error"); })); + EXPECT_THROW_WITH_MESSAGE(dispatcher_.run(Event::Dispatcher::RunType::Block), EnvoyException, + "runtime error"); + EXPECT_EQ(nullptr, resolver_->resolve("1.2.3.4", DnsLookupFamily::V4Only, + [&](const std::list && + /*results*/) -> void { throw std::string(); })); + EXPECT_THROW_WITH_MESSAGE(dispatcher_.run(Event::Dispatcher::RunType::Block), EnvoyException, + "unknown"); +} + TEST_P(DnsImplTest, DnsIpAddressVersion) { std::list address_list; server_->addHosts("some.good.domain", {"1.2.3.4"}, A); From f4a071b01bd4494e46fdd32c1051c81d97ae7e64 Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Thu, 6 Sep 2018 17:51:59 -0400 Subject: [PATCH 4/4] Remove _MISC FROM ENVOY_LOG. Signed-off-by: Harvey Tuch --- source/common/network/dns_impl.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/source/common/network/dns_impl.cc b/source/common/network/dns_impl.cc index c0fed8acf2ddf..5153c35ffcabb 100644 --- a/source/common/network/dns_impl.cc +++ b/source/common/network/dns_impl.cc @@ -123,13 +123,13 @@ void DnsResolverImpl::PendingResolution::onAresHostCallback(int status, int time try { callback_(std::move(address_list)); } catch (const EnvoyException& e) { - ENVOY_LOG_MISC(critical, "EnvoyException in c-ares callback"); + ENVOY_LOG(critical, "EnvoyException in c-ares callback"); dispatcher_.post([e] { throw e; }); } catch (const std::exception& e) { - ENVOY_LOG_MISC(critical, "std::exception in c-ares callback"); + ENVOY_LOG(critical, "std::exception in c-ares callback"); dispatcher_.post([s = std::string(e.what())] { throw EnvoyException(s); }); } catch (...) { - ENVOY_LOG_MISC(critical, "Unknown exception in c-ares callback"); + ENVOY_LOG(critical, "Unknown exception in c-ares callback"); dispatcher_.post([] { throw EnvoyException("unknown"); }); } }