From e389b73e0ab0f39ecba8b22dfdd8584c829fec67 Mon Sep 17 00:00:00 2001 From: Matt Klein Date: Tue, 20 Jul 2021 15:02:54 +0000 Subject: [PATCH 1/6] listener: add listen() error handling Related to https://github.com/envoyproxy/envoy/issues/14784 Signed-off-by: Matt Klein --- source/server/admin/admin.cc | 4 +-- source/server/listener_impl.cc | 8 ++--- test/common/quic/envoy_quic_writer_test.cc | 7 +++-- .../quic/quic_io_handle_wrapper_test.cc | 9 ++++-- test/mocks/network/BUILD | 1 + test/mocks/network/mocks.cc | 2 +- test/mocks/network/mocks.h | 3 +- test/mocks/server/BUILD | 1 + test/mocks/tcp/BUILD | 1 + test/server/listener_manager_impl_test.cc | 31 +++++++++++++++++++ 10 files changed, 54 insertions(+), 13 deletions(-) diff --git a/source/server/admin/admin.cc b/source/server/admin/admin.cc index 314e2c298bf86..f155cf5e8eeb5 100644 --- a/source/server/admin/admin.cc +++ b/source/server/admin/admin.cc @@ -127,8 +127,8 @@ void AdminImpl::startHttpListener(const std::list& } null_overload_manager_.start(); socket_ = std::make_shared(address, socket_options, true); - // TODO(mattklein123): We lost error handling along the way for the listen() call. Add it back. - socket_->ioHandle().listen(ENVOY_TCP_BACKLOG_SIZE); + RELEASE_ASSERT(0 == socket_->ioHandle().listen(ENVOY_TCP_BACKLOG_SIZE).rc_, + "listen() failed on admin listener"); socket_factory_ = std::make_unique(socket_); listener_ = std::make_unique(*this, std::move(listener_scope)); ENVOY_LOG(info, "admin address: {}", socket().addressProvider().localAddress()->asString()); diff --git a/source/server/listener_impl.cc b/source/server/listener_impl.cc index a01633616315d..89dbdb0cb932a 100644 --- a/source/server/listener_impl.cc +++ b/source/server/listener_impl.cc @@ -169,10 +169,10 @@ void ListenSocketFactoryImpl::doFinalPreWorkerInit() { } for (auto& socket : sockets_) { - // TODO(mattklein123): At some point we lost error handling on this call which I think can - // technically fail (at least according to lingering code comments). Add error handling on this - // in a follow up. - socket->ioHandle().listen(tcp_backlog_size_); + const auto rc = socket->ioHandle().listen(tcp_backlog_size_); + if (rc.rc_ != 0) { + throw EnvoyException(fmt::format("cannot listen() errno={}", rc.errno_)); + } if (!Network::Socket::applyOptions(socket->options(), *socket, envoy::config::core::v3::SocketOption::STATE_LISTENING)) { diff --git a/test/common/quic/envoy_quic_writer_test.cc b/test/common/quic/envoy_quic_writer_test.cc index a0ee8b3ed5edd..3908fb82ba568 100644 --- a/test/common/quic/envoy_quic_writer_test.cc +++ b/test/common/quic/envoy_quic_writer_test.cc @@ -5,16 +5,17 @@ #include "source/common/network/address_impl.h" #include "source/common/network/io_socket_error_impl.h" +#include "source/common/network/io_socket_handle_impl.h" #include "source/common/network/udp_packet_writer_handler_impl.h" #include "source/common/quic/envoy_quic_packet_writer.h" #include "test/mocks/api/mocks.h" -#include "test/mocks/network/mocks.h" #include "test/test_common/threadsafe_singleton_injector.h" #include "gmock/gmock.h" #include "gtest/gtest.h" +using testing::_; using testing::Return; namespace Envoy { @@ -23,7 +24,7 @@ namespace Quic { class EnvoyQuicWriterTest : public ::testing::Test { public: EnvoyQuicWriterTest() - : envoy_quic_writer_(std::make_unique(socket_.ioHandle())) { + : envoy_quic_writer_(std::make_unique(io_handle_)) { self_address_.FromString("::"); quic::QuicIpAddress peer_ip; peer_ip.FromString("::1"); @@ -50,7 +51,7 @@ class EnvoyQuicWriterTest : public ::testing::Test { protected: testing::NiceMock os_sys_calls_; TestThreadsafeSingletonInjector os_calls_{&os_sys_calls_}; - testing::NiceMock socket_; + Network::IoSocketHandleImpl io_handle_; quic::QuicIpAddress self_address_; quic::QuicSocketAddress peer_address_; EnvoyQuicPacketWriter envoy_quic_writer_; diff --git a/test/common/quic/quic_io_handle_wrapper_test.cc b/test/common/quic/quic_io_handle_wrapper_test.cc index 899f7903d5892..e9c7d566207ed 100644 --- a/test/common/quic/quic_io_handle_wrapper_test.cc +++ b/test/common/quic/quic_io_handle_wrapper_test.cc @@ -4,6 +4,7 @@ #include "envoy/common/platform.h" #include "source/common/network/address_impl.h" +#include "source/common/network/io_socket_handle_impl.h" #include "source/common/quic/quic_io_handle_wrapper.h" #include "test/mocks/api/mocks.h" @@ -16,19 +17,23 @@ using testing::ByMove; using testing::Return; +using testing::ReturnRef; namespace Envoy { namespace Quic { class QuicIoHandleWrapperTest : public testing::Test { public: - QuicIoHandleWrapperTest() : wrapper_(std::make_unique(socket_.ioHandle())) { + QuicIoHandleWrapperTest() { + real_io_handle_ = std::make_unique(); + ON_CALL(socket_, ioHandle()).WillByDefault(ReturnRef(*real_io_handle_)); + wrapper_ = std::make_unique(socket_.ioHandle()); EXPECT_TRUE(wrapper_->isOpen()); EXPECT_FALSE(socket_.ioHandle().isOpen()); } - ~QuicIoHandleWrapperTest() override = default; protected: + Network::IoHandlePtr real_io_handle_; testing::NiceMock socket_; std::unique_ptr wrapper_; testing::StrictMock os_sys_calls_; diff --git a/test/mocks/network/BUILD b/test/mocks/network/BUILD index 38cd2003b09d7..3e3757f1ea2f0 100644 --- a/test/mocks/network/BUILD +++ b/test/mocks/network/BUILD @@ -47,6 +47,7 @@ envoy_cc_mock( hdrs = ["mocks.h"], deps = [ ":connection_mocks", + ":io_handle_mocks", ":transport_socket_mocks", "//envoy/buffer:buffer_interface", "//envoy/network:connection_interface", diff --git a/test/mocks/network/mocks.cc b/test/mocks/network/mocks.cc index f07685c37f246..f4f20e1359b97 100644 --- a/test/mocks/network/mocks.cc +++ b/test/mocks/network/mocks.cc @@ -132,7 +132,7 @@ MockFilterChainFactory::MockFilterChainFactory() { MockFilterChainFactory::~MockFilterChainFactory() = default; MockListenSocket::MockListenSocket() - : io_handle_(std::make_unique()), + : io_handle_(std::make_unique>()), address_provider_(std::make_shared( std::make_shared(80), nullptr)) { ON_CALL(*this, options()).WillByDefault(ReturnRef(options_)); diff --git a/test/mocks/network/mocks.h b/test/mocks/network/mocks.h index 1145db10d1460..ce971d0b92cd3 100644 --- a/test/mocks/network/mocks.h +++ b/test/mocks/network/mocks.h @@ -25,6 +25,7 @@ #include "test/mocks/event/mocks.h" #include "test/mocks/network/connection.h" +#include "test/mocks/network/io_handle.h" #include "test/mocks/stream_info/mocks.h" #include "test/test_common/printers.h" @@ -270,7 +271,7 @@ class MockListenSocket : public Socket { (unsigned long, void*, unsigned long, void*, unsigned long, unsigned long*)); MOCK_METHOD(Api::SysCallIntResult, setBlockingForTest, (bool)); - IoHandlePtr io_handle_; + std::unique_ptr io_handle_; Network::SocketAddressSetterSharedPtr address_provider_; OptionsSharedPtr options_; bool socket_is_open_ = true; diff --git a/test/mocks/server/BUILD b/test/mocks/server/BUILD index 679f3e6d1b2d2..1229c88032b9c 100644 --- a/test/mocks/server/BUILD +++ b/test/mocks/server/BUILD @@ -121,6 +121,7 @@ envoy_cc_mock( deps = [ "//envoy/server:drain_manager_interface", "//envoy/server:listener_manager_interface", + "//test/mocks/network:io_handle_mocks", "//test/mocks/network:network_mocks", "@envoy_api//envoy/config/core/v3:pkg_cc_proto", "@envoy_api//envoy/config/listener/v3:pkg_cc_proto", diff --git a/test/mocks/tcp/BUILD b/test/mocks/tcp/BUILD index b3cbd6903f68f..f61c5af7d0d68 100644 --- a/test/mocks/tcp/BUILD +++ b/test/mocks/tcp/BUILD @@ -16,6 +16,7 @@ envoy_cc_mock( "//envoy/buffer:buffer_interface", "//envoy/tcp:conn_pool_interface", "//test/mocks:common_lib", + "//test/mocks/network:io_handle_mocks", "//test/mocks/network:network_mocks", "//test/mocks/upstream:host_mocks", ], diff --git a/test/server/listener_manager_impl_test.cc b/test/server/listener_manager_impl_test.cc index 5608db746f14c..2d5307c70a01b 100644 --- a/test/server/listener_manager_impl_test.cc +++ b/test/server/listener_manager_impl_test.cc @@ -1829,6 +1829,37 @@ TEST_F(ListenerManagerImplTest, NotSupportedDatagramUds) { "socket type SocketType::Datagram not supported for pipes"); } +TEST_F(ListenerManagerImplTest, CantListen) { + InSequence s; + + EXPECT_CALL(*worker_, start(_, _)); + manager_->startWorkers(guard_dog_, callback_.AsStdFunction()); + + const std::string listener_foo_yaml = R"EOF( +name: foo +address: + socket_address: + address: 127.0.0.1 + port_value: 1234 +filter_chains: +- filters: [] + )EOF"; + + ListenerHandle* listener_foo = expectListenerCreate(true, true); + EXPECT_CALL(listener_factory_, createListenSocket(_, _, _, default_bind_type, 0)); + EXPECT_CALL(listener_foo->target_, initialize()); + manager_->addOrUpdateListener(parseListenerFromV3Yaml(listener_foo_yaml), "", true); + + EXPECT_CALL(*listener_factory_.socket_->io_handle_, listen(_)) + .WillOnce(Return(Api::SysCallIntResult{-1, 100})); + EXPECT_CALL(*listener_foo, onDestroy()); + listener_foo->target_.ready(); + + EXPECT_EQ( + 1UL, + server_.stats_store_.counterFromString("listener_manager.listener_create_failure").value()); +} + TEST_F(ListenerManagerImplTest, CantBindSocket) { time_system_.setSystemTime(std::chrono::milliseconds(1001001001001)); InSequence s; From b96d769f55d555c4a81a7c89548c55e523b2764e Mon Sep 17 00:00:00 2001 From: Matt Klein Date: Thu, 22 Jul 2021 23:01:41 +0000 Subject: [PATCH 2/6] asan fix Signed-off-by: Matt Klein --- source/common/init/target_impl.cc | 8 +++++++- source/common/init/target_impl.h | 4 ++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/source/common/init/target_impl.cc b/source/common/init/target_impl.cc index 1c8ddfdea5cec..86a6b13b63e34 100644 --- a/source/common/init/target_impl.cc +++ b/source/common/init/target_impl.cc @@ -45,8 +45,14 @@ bool TargetImpl::ready() { if (watcher_handle_) { // If we have a handle for the ManagerImpl's watcher, signal it and then reset so it can't be // accidentally signaled again. + // NOTE: The still_alive_ guard is used here to avoid the scenario in which as a result of + // calling ready() this target is destroyed. This is possible in practice, for example when + // a listener is deleted as a result of a failure in the context of the ready() call. + std::weak_ptr weak_still_alive = still_alive_; const bool result = watcher_handle_->ready(); - watcher_handle_.reset(); + if (!weak_still_alive.expired()) { + watcher_handle_.reset(); + } return result; } return false; diff --git a/source/common/init/target_impl.h b/source/common/init/target_impl.h index 72bde7f8d32a9..6f84fb4b2a1c2 100644 --- a/source/common/init/target_impl.h +++ b/source/common/init/target_impl.h @@ -79,6 +79,10 @@ class TargetImpl : public Target, Logger::Loggable { bool ready(); private: + // This is a guard to allow ready() to verify that this object is still alive. See the comments + // there for more information. + std::shared_ptr still_alive_{std::make_shared(true)}; + // Human-readable name for logging const std::string name_; From 5bf1a3b60de1f7cb8d5e3a81e2863cd458a1e1b3 Mon Sep 17 00:00:00 2001 From: Matt Klein Date: Fri, 23 Jul 2021 20:34:24 +0000 Subject: [PATCH 3/6] fix Signed-off-by: Matt Klein --- source/common/init/target_impl.cc | 19 +++++++++---------- source/common/init/target_impl.h | 4 ---- 2 files changed, 9 insertions(+), 14 deletions(-) diff --git a/source/common/init/target_impl.cc b/source/common/init/target_impl.cc index 86a6b13b63e34..12fd61387254d 100644 --- a/source/common/init/target_impl.cc +++ b/source/common/init/target_impl.cc @@ -45,14 +45,11 @@ bool TargetImpl::ready() { if (watcher_handle_) { // If we have a handle for the ManagerImpl's watcher, signal it and then reset so it can't be // accidentally signaled again. - // NOTE: The still_alive_ guard is used here to avoid the scenario in which as a result of + // NOTE: We must move watcher_handle_ to a local to avoid the scenario in which as a result of // calling ready() this target is destroyed. This is possible in practice, for example when // a listener is deleted as a result of a failure in the context of the ready() call. - std::weak_ptr weak_still_alive = still_alive_; - const bool result = watcher_handle_->ready(); - if (!weak_still_alive.expired()) { - watcher_handle_.reset(); - } + auto local_watcher_handle = std::move(watcher_handle_); + const bool result = local_watcher_handle->ready(); return result; } return false; @@ -81,12 +78,14 @@ TargetHandlePtr SharedTargetImpl::createHandle(absl::string_view handle_name) co bool SharedTargetImpl::ready() { initialized_ = true; - bool all_notified = !watcher_handles_.empty(); - for (auto& watcher_handle : watcher_handles_) { + // NOTE: We must move watcher_handles_ to a local to avoid the scenario in which as a result of + // calling ready() this target is destroyed. This is possible in practice, for example when + // a listener is deleted as a result of a failure in the context of the ready() call. + auto local_watcher_handles = std::move(watcher_handles_); + bool all_notified = !local_watcher_handles.empty(); + for (auto& watcher_handle : local_watcher_handles) { all_notified = watcher_handle->ready() && all_notified; } - // save heap and avoid repeatedly invoke - watcher_handles_.clear(); return all_notified; } diff --git a/source/common/init/target_impl.h b/source/common/init/target_impl.h index 6f84fb4b2a1c2..72bde7f8d32a9 100644 --- a/source/common/init/target_impl.h +++ b/source/common/init/target_impl.h @@ -79,10 +79,6 @@ class TargetImpl : public Target, Logger::Loggable { bool ready(); private: - // This is a guard to allow ready() to verify that this object is still alive. See the comments - // there for more information. - std::shared_ptr still_alive_{std::make_shared(true)}; - // Human-readable name for logging const std::string name_; From 299adb820f4bacac9930ff2f28ebbfc4e486cf8c Mon Sep 17 00:00:00 2001 From: Matt Klein Date: Mon, 26 Jul 2021 16:52:44 +0000 Subject: [PATCH 4/6] comment Signed-off-by: Matt Klein --- source/common/init/target_impl.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/source/common/init/target_impl.cc b/source/common/init/target_impl.cc index 12fd61387254d..5e41e62f409c2 100644 --- a/source/common/init/target_impl.cc +++ b/source/common/init/target_impl.cc @@ -49,8 +49,7 @@ bool TargetImpl::ready() { // calling ready() this target is destroyed. This is possible in practice, for example when // a listener is deleted as a result of a failure in the context of the ready() call. auto local_watcher_handle = std::move(watcher_handle_); - const bool result = local_watcher_handle->ready(); - return result; + return local_watcher_handle->ready(); } return false; } From 8afc727b9627aaee65558957676a423fabf14acd Mon Sep 17 00:00:00 2001 From: Matt Klein Date: Mon, 26 Jul 2021 16:59:51 +0000 Subject: [PATCH 5/6] fix Signed-off-by: Matt Klein --- source/server/admin/admin.cc | 2 +- source/server/listener_impl.cc | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/source/server/admin/admin.cc b/source/server/admin/admin.cc index f155cf5e8eeb5..b54e9c8c4077e 100644 --- a/source/server/admin/admin.cc +++ b/source/server/admin/admin.cc @@ -127,7 +127,7 @@ void AdminImpl::startHttpListener(const std::list& } null_overload_manager_.start(); socket_ = std::make_shared(address, socket_options, true); - RELEASE_ASSERT(0 == socket_->ioHandle().listen(ENVOY_TCP_BACKLOG_SIZE).rc_, + RELEASE_ASSERT(0 == socket_->ioHandle().listen(ENVOY_TCP_BACKLOG_SIZE).return_value_, "listen() failed on admin listener"); socket_factory_ = std::make_unique(socket_); listener_ = std::make_unique(*this, std::move(listener_scope)); diff --git a/source/server/listener_impl.cc b/source/server/listener_impl.cc index d33883c8bf630..6287d2acfa958 100644 --- a/source/server/listener_impl.cc +++ b/source/server/listener_impl.cc @@ -170,7 +170,7 @@ void ListenSocketFactoryImpl::doFinalPreWorkerInit() { for (auto& socket : sockets_) { const auto rc = socket->ioHandle().listen(tcp_backlog_size_); - if (rc.rc_ != 0) { + if (rc.return_value_ != 0) { throw EnvoyException(fmt::format("cannot listen() errno={}", rc.errno_)); } From 8129c2a90e0d29d86cdf24f55849b021ba7a7cf5 Mon Sep 17 00:00:00 2001 From: Matt Klein Date: Mon, 2 Aug 2021 17:53:27 +0000 Subject: [PATCH 6/6] fix Signed-off-by: Matt Klein --- source/server/listener_impl.cc | 6 ++++++ test/server/listener_manager_impl_test.cc | 4 ++++ 2 files changed, 10 insertions(+) diff --git a/source/server/listener_impl.cc b/source/server/listener_impl.cc index 7ea7b2c98bb04..cd36b26627376 100644 --- a/source/server/listener_impl.cc +++ b/source/server/listener_impl.cc @@ -170,9 +170,15 @@ void ListenSocketFactoryImpl::doFinalPreWorkerInit() { for (auto& socket : sockets_) { const auto rc = socket->ioHandle().listen(tcp_backlog_size_); +#ifndef WIN32 if (rc.return_value_ != 0) { throw EnvoyException(fmt::format("cannot listen() errno={}", rc.errno_)); } +#else + // TODO(davinci26): listen() error handling and generally listening on multiple workers + // is broken right now. This needs follow up to do something better on Windows. + UNREFERENCED_PARAMETER(rc); +#endif if (!Network::Socket::applyOptions(socket->options(), *socket, envoy::config::core::v3::SocketOption::STATE_LISTENING)) { diff --git a/test/server/listener_manager_impl_test.cc b/test/server/listener_manager_impl_test.cc index dd1d25cff1b6e..c2c61c695c03c 100644 --- a/test/server/listener_manager_impl_test.cc +++ b/test/server/listener_manager_impl_test.cc @@ -1837,6 +1837,9 @@ TEST_F(ListenerManagerImplTest, NotSupportedDatagramUds) { "socket type SocketType::Datagram not supported for pipes"); } +// TODO(davinci26): See ListenSocketFactoryImpl::doFinalPreWorkerInit() for why this test is +// not run on Windows. +#ifndef WIN32 TEST_F(ListenerManagerImplTest, CantListen) { InSequence s; @@ -1867,6 +1870,7 @@ name: foo 1UL, server_.stats_store_.counterFromString("listener_manager.listener_create_failure").value()); } +#endif TEST_F(ListenerManagerImplTest, CantBindSocket) { time_system_.setSystemTime(std::chrono::milliseconds(1001001001001));