diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index ec6aa995b64a..278f0d325f87 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -5,6 +5,7 @@ Version history ================ Changes ------- +* udp: fixed issue in which receiving truncated UDP datagrams would cause Envoy to crash. 1.13.6 (September 29, 2020) =========================== diff --git a/include/envoy/network/io_handle.h b/include/envoy/network/io_handle.h index d5a63e571376..fede1f2ec588 100644 --- a/include/envoy/network/io_handle.h +++ b/include/envoy/network/io_handle.h @@ -4,6 +4,8 @@ #include "envoy/common/platform.h" #include "envoy/common/pure.h" +#include "absl/container/fixed_array.h" + namespace Envoy { namespace Buffer { struct RawSlice; @@ -74,13 +76,26 @@ class IoHandle { int flags, const Address::Ip* self_ip, const Address::Instance& peer_address) PURE; + struct RecvMsgPerPacketInfo { + // If true indicates a successful syscall, but the packet was dropped due to truncation. We do + // not support receiving truncated packets. + bool truncated_and_dropped_{false}; + }; + + /** + * The output parameter type for recvmsg and recvmmsg. + */ struct RecvMsgOutput { /* + * @param num_packets_per_call is the max number of packets allowed per + * recvmmsg call. For recvmsg call, any value larger than 0 is allowed, but + * only one packet will be returned. * @param dropped_packets points to a variable to store how many packets are * dropped so far. If nullptr, recvmsg() won't try to get this information * from transport header. */ - RecvMsgOutput(uint32_t* dropped_packets) : dropped_packets_(dropped_packets) {} + RecvMsgOutput(size_t num_packets_per_call, uint32_t* dropped_packets) + : dropped_packets_(dropped_packets), msg_(num_packets_per_call) {} // If not nullptr, its value is the total number of packets dropped. recvmsg() will update it // when more packets are dropped. @@ -89,6 +104,8 @@ class IoHandle { std::shared_ptr local_address_; // The the source address from transport header. std::shared_ptr peer_address_; + // Per packet info. + absl::FixedArray msg_; }; /** diff --git a/source/common/network/io_socket_handle_impl.cc b/source/common/network/io_socket_handle_impl.cc index ec1e3529e0f6..61b2219e853c 100644 --- a/source/common/network/io_socket_handle_impl.cc +++ b/source/common/network/io_socket_handle_impl.cc @@ -16,6 +16,21 @@ using Envoy::Api::SysCallIntResult; using Envoy::Api::SysCallSizeResult; namespace Envoy { + +namespace { + +constexpr int messageTruncatedOption() { +#if defined(__APPLE__) + // OSX does not support passing `MSG_TRUNC` to recvmsg and recvmmsg. This does not effect + // functionality and it primarily used for logging. + return 0; +#else + return MSG_TRUNC; +#endif +} + +} // namespace + namespace Network { IoSocketHandleImpl::~IoSocketHandleImpl() { @@ -243,11 +258,18 @@ Api::IoCallUint64Result IoSocketHandleImpl::recvmsg(Buffer::RawSlice* slices, cmsg->cmsg_len = cmsg_space; hdr.msg_control = cmsg; hdr.msg_controllen = cmsg_space; - auto& os_sys_calls = Api::OsSysCallsSingleton::get(); - const Api::SysCallSizeResult result = os_sys_calls.recvmsg(fd_, &hdr, 0); + Api::SysCallSizeResult result = + Api::OsSysCallsSingleton::get().recvmsg(fd_, &hdr, messageTruncatedOption()); if (result.rc_ < 0) { return sysCallResultToIoCallResult(result); } + if ((hdr.msg_flags & MSG_TRUNC) != 0) { + ENVOY_LOG_MISC(debug, "Dropping truncated UDP packet with size: {}.", result.rc_); + result.rc_ = 0; + (*output.dropped_packets_)++; + output.msg_[0].truncated_and_dropped_ = true; + return sysCallResultToIoCallResult(result); + } RELEASE_ASSERT((hdr.msg_flags & MSG_CTRUNC) == 0, fmt::format("Incorrectly set control message length: {}", hdr.msg_controllen)); @@ -287,7 +309,8 @@ Api::IoCallUint64Result IoSocketHandleImpl::recvmsg(Buffer::RawSlice* slices, if (output.dropped_packets_ != nullptr) { absl::optional maybe_dropped = maybeGetPacketsDroppedFromHeader(*cmsg); if (maybe_dropped) { - *output.dropped_packets_ = *maybe_dropped; + *output.dropped_packets_ += *maybe_dropped; + continue; } } } diff --git a/source/common/network/udp_listener_impl.h b/source/common/network/udp_listener_impl.h index 2184b4419c10..8c0f6a4e1735 100644 --- a/source/common/network/udp_listener_impl.h +++ b/source/common/network/udp_listener_impl.h @@ -24,8 +24,8 @@ class UdpListenerImpl : public BaseListenerImpl, public: UdpListenerImpl(Event::DispatcherImpl& dispatcher, SocketSharedPtr socket, UdpListenerCallbacks& cb, TimeSource& time_source); - ~UdpListenerImpl() override; + uint32_t packetsDropped() { return packets_dropped_; } // Network::Listener Interface void disable() override; diff --git a/source/common/network/utility.cc b/source/common/network/utility.cc index 1f3e8ab98d97..cd540c3b2de6 100644 --- a/source/common/network/utility.cc +++ b/source/common/network/utility.cc @@ -536,11 +536,11 @@ Api::IoCallUint64Result Utility::readFromSocket(IoHandle& handle, const uint64_t num_slices = buffer->reserve(udp_packet_processor.maxPacketSize(), &slice, 1); ASSERT(num_slices == 1); - IoHandle::RecvMsgOutput output(packets_dropped); + IoHandle::RecvMsgOutput output(1, packets_dropped); Api::IoCallUint64Result result = handle.recvmsg(&slice, num_slices, local_address.ip()->port(), output); - if (!result.ok()) { + if (!result.ok() || output.msg_[0].truncated_and_dropped_) { return result; } @@ -581,12 +581,6 @@ Api::IoErrorPtr Utility::readPacketsFromSocket(IoHandle& handle, return std::move(result.err_); } - if (result.rc_ == 0) { - // TODO(conqerAtapple): Is zero length packet interesting? If so add stats - // for it. Otherwise remove the warning log below. - ENVOY_LOG_MISC(trace, "received 0-length packet"); - } - if (packets_dropped != old_packets_dropped) { // The kernel tracks SO_RXQ_OVFL as a uint32 which can overflow to a smaller // value. So as long as this count differs from previously recorded value, diff --git a/test/common/network/udp_listener_impl_test.cc b/test/common/network/udp_listener_impl_test.cc index 7682aa27cc9c..257c2bd73299 100644 --- a/test/common/network/udp_listener_impl_test.cc +++ b/test/common/network/udp_listener_impl_test.cc @@ -94,7 +94,7 @@ class UdpListenerImplTest : public ListenerImplTestBase { SocketSharedPtr server_socket_; SocketPtr client_socket_; Address::InstanceConstSharedPtr send_to_addr_; - MockUdpListenerCallbacks listener_callbacks_; + NiceMock listener_callbacks_; std::unique_ptr listener_; }; @@ -172,6 +172,44 @@ TEST_P(UdpListenerImplTest, UseActualDstUdp) { dispatcher_->run(Event::Dispatcher::RunType::Block); } +// Test a large datagram that gets dropped using recvmsg. +TEST_P(UdpListenerImplTest, LargeDatagramRecvmsg) { + client_socket_ = createClientSocket(false); + + // This will get dropped. + const std::string first(4096, 'a'); + const void* void_pointer = static_cast(first.c_str()); + Buffer::RawSlice first_slice{const_cast(void_pointer), first.length()}; + const std::string second("second"); + void_pointer = static_cast(second.c_str()); + Buffer::RawSlice second_slice{const_cast(void_pointer), second.length()}; + // This will get dropped. + const std::string third(4096, 'b'); + void_pointer = static_cast(third.c_str()); + Buffer::RawSlice third_slice{const_cast(void_pointer), third.length()}; + + auto send_rc = Network::Utility::writeToSocket(client_socket_->ioHandle(), &first_slice, 1, + nullptr, *send_to_addr_); + ASSERT_EQ(send_rc.rc_, first.length()); + send_rc = Network::Utility::writeToSocket(client_socket_->ioHandle(), &second_slice, 1, nullptr, + *send_to_addr_); + ASSERT_EQ(send_rc.rc_, second.length()); + send_rc = Network::Utility::writeToSocket(client_socket_->ioHandle(), &third_slice, 1, nullptr, + *send_to_addr_); + ASSERT_EQ(send_rc.rc_, third.length()); + + EXPECT_CALL(listener_callbacks_, onReadReady()); + EXPECT_CALL(listener_callbacks_, onData(_)).WillOnce(Invoke([&](const UdpRecvData& data) -> void { + validateRecvCallbackParams(data); + EXPECT_EQ(data.buffer_->toString(), second); + + dispatcher_->exit(); + })); + + dispatcher_->run(Event::Dispatcher::RunType::Block); + EXPECT_EQ(2, listener_->packetsDropped()); +} + /** * Tests UDP listener for read and write callbacks with actual data. */ diff --git a/test/extensions/quic_listeners/quiche/quic_io_handle_wrapper_test.cc b/test/extensions/quic_listeners/quiche/quic_io_handle_wrapper_test.cc index ccf8caa930b1..bf8bb6cdf12e 100644 --- a/test/extensions/quic_listeners/quiche/quic_io_handle_wrapper_test.cc +++ b/test/extensions/quic_listeners/quiche/quic_io_handle_wrapper_test.cc @@ -1,5 +1,6 @@ #include +#include #include #include "common/network/address_impl.h" @@ -18,6 +19,20 @@ using testing::Return; namespace Envoy { namespace Quic { +namespace { + +constexpr int messageTruncatedOption() { +#if defined(__APPLE__) + // OSX does not support passing `MSG_TRUNC` to recvmsg and recvmmsg. This does not effect + // functionality and it primarily used for logging. + return 0; +#else + return MSG_TRUNC; +#endif +} + +} // namespace + class QuicIoHandleWrapperTest : public testing::Test { public: QuicIoHandleWrapperTest() : wrapper_(std::make_unique(socket_.ioHandle())) { @@ -56,18 +71,20 @@ TEST_F(QuicIoHandleWrapperTest, DelegateIoHandleCalls) { EXPECT_CALL(os_sys_calls_, sendmsg(fd, _, 0)).WillOnce(Return(Api::SysCallSizeResult{5u, 0})); wrapper_->sendmsg(&slice, 1, 0, /*self_ip=*/nullptr, *addr); - Network::IoHandle::RecvMsgOutput output(nullptr); - EXPECT_CALL(os_sys_calls_, recvmsg(fd, _, 0)).WillOnce(Invoke([](int, struct msghdr* msg, int) { - sockaddr_storage ss; - auto ipv6_addr = reinterpret_cast(&ss); - memset(ipv6_addr, 0, sizeof(sockaddr_in6)); - ipv6_addr->sin6_family = AF_INET6; - ipv6_addr->sin6_addr = in6addr_loopback; - ipv6_addr->sin6_port = htons(54321); - *reinterpret_cast(msg->msg_name) = *ipv6_addr; - msg->msg_namelen = sizeof(sockaddr_in6); - return Api::SysCallSizeResult{5u, 0}; - })); + Network::IoHandle::RecvMsgOutput output(1, nullptr); + EXPECT_CALL(os_sys_calls_, recvmsg(fd, _, messageTruncatedOption())) + .WillOnce(Invoke([](int, msghdr* msg, int) { + sockaddr_storage ss; + auto ipv6_addr = reinterpret_cast(&ss); + memset(ipv6_addr, 0, sizeof(sockaddr_in6)); + ipv6_addr->sin6_family = AF_INET6; + ipv6_addr->sin6_addr = in6addr_loopback; + ipv6_addr->sin6_port = htons(54321); + *reinterpret_cast(msg->msg_name) = *ipv6_addr; + msg->msg_namelen = sizeof(sockaddr_in6); + msg->msg_controllen = 0; + return Api::SysCallSizeResult{5u, 0}; + })); wrapper_->recvmsg(&slice, 1, /*self_port=*/12345, output); EXPECT_TRUE(wrapper_->close().ok()); diff --git a/test/test_common/threadsafe_singleton_injector.h b/test/test_common/threadsafe_singleton_injector.h index 627efa1390e6..b9a0020dc88a 100644 --- a/test/test_common/threadsafe_singleton_injector.h +++ b/test/test_common/threadsafe_singleton_injector.h @@ -12,6 +12,7 @@ template class TestThreadsafeSingletonInjector { ThreadSafeSingleton::instance_ = instance; } ~TestThreadsafeSingletonInjector() { ThreadSafeSingleton::instance_ = latched_instance_; } + T& latched() { return *latched_instance_; } private: T* latched_instance_; diff --git a/tools/spelling_dictionary.txt b/tools/spelling_dictionary.txt index dfca8c13997e..4c6e01123b54 100644 --- a/tools/spelling_dictionary.txt +++ b/tools/spelling_dictionary.txt @@ -285,6 +285,7 @@ TLV TMPDIR TODO TPM +TRUNC TSAN TSI TTL