From 309ddc5abc1c944621f97b8e310a8106cd044178 Mon Sep 17 00:00:00 2001 From: Matt Klein Date: Fri, 16 Oct 2020 15:40:11 +0000 Subject: [PATCH 1/8] udp: properly handle truncated/dropped datagrams Signed-off-by: Matt Klein --- .../common/network/io_socket_handle_impl.cc | 23 +++++--- source/common/network/udp_listener_impl.h | 2 +- source/common/network/utility.cc | 16 ++--- test/common/network/udp_listener_impl_test.cc | 58 +++++++++++++++++-- .../threadsafe_singleton_injector.h | 1 + 5 files changed, 76 insertions(+), 24 deletions(-) diff --git a/source/common/network/io_socket_handle_impl.cc b/source/common/network/io_socket_handle_impl.cc index 52a6ae6e5ddb..b0c92a9d4f19 100644 --- a/source/common/network/io_socket_handle_impl.cc +++ b/source/common/network/io_socket_handle_impl.cc @@ -292,10 +292,16 @@ Api::IoCallUint64Result IoSocketHandleImpl::recvmsg(Buffer::RawSlice* slices, hdr.msg_flags = 0; hdr.msg_control = cbuf.begin(); hdr.msg_controllen = cmsg_space_; - const Api::SysCallSizeResult result = Api::OsSysCallsSingleton::get().recvmsg(fd_, &hdr, 0); + Api::SysCallSizeResult result = Api::OsSysCallsSingleton::get().recvmsg(fd_, &hdr, MSG_TRUNC); 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_)++; + return sysCallResultToIoCallResult(result); + } RELEASE_ASSERT((hdr.msg_flags & MSG_CTRUNC) == 0, fmt::format("Incorrectly set control message length: {}", hdr.msg_controllen)); @@ -320,7 +326,7 @@ 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; } } @@ -383,18 +389,17 @@ Api::IoCallUint64Result IoSocketHandleImpl::recvmmsg(RawSliceArrays& slices, uin int num_packets_read = result.rc_; for (int i = 0; i < num_packets_read; ++i) { - if (mmsg_hdr[i].msg_len == 0) { + msghdr& hdr = mmsg_hdr[i].msg_hdr; + if ((hdr.msg_flags & MSG_TRUNC) != 0) { + ENVOY_LOG_MISC(debug, "Dropping truncated UDP packet with size: {}.", mmsg_hdr[i].msg_len); + (*output.dropped_packets_)++; continue; } - msghdr& hdr = mmsg_hdr[i].msg_hdr; + RELEASE_ASSERT((hdr.msg_flags & MSG_CTRUNC) == 0, fmt::format("Incorrectly set control message length: {}", hdr.msg_controllen)); RELEASE_ASSERT(hdr.msg_namelen > 0, fmt::format("Unable to get remote address from recvmmsg() for fd: {}", fd_)); - if ((hdr.msg_flags & MSG_TRUNC) != 0) { - ENVOY_LOG_MISC(warn, "Dropping truncated UDP packet with size: {}.", mmsg_hdr[i].msg_len); - continue; - } output.msg_[i].msg_len_ = mmsg_hdr[i].msg_len; // Get local and peer addresses for each packet. @@ -420,7 +425,7 @@ Api::IoCallUint64Result IoSocketHandleImpl::recvmmsg(RawSliceArrays& slices, uin for (cmsg = CMSG_FIRSTHDR(&hdr); cmsg != nullptr; cmsg = CMSG_NXTHDR(&hdr, cmsg)) { absl::optional maybe_dropped = maybeGetPacketsDroppedFromHeader(*cmsg); if (maybe_dropped) { - *output.dropped_packets_ = *maybe_dropped; + *output.dropped_packets_ += *maybe_dropped; } } } diff --git a/source/common/network/udp_listener_impl.h b/source/common/network/udp_listener_impl.h index d555649833bc..e3c2cd5d45c3 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 426598ace956..e3445090b69d 100644 --- a/source/common/network/utility.cc +++ b/source/common/network/utility.cc @@ -632,6 +632,11 @@ Api::IoCallUint64Result Utility::readFromSocket(IoHandle& handle, Buffer::RawSlice* slice = slices[i].data(); const uint64_t msg_len = output.msg_[i].msg_len_; ASSERT(msg_len <= slice->len_); + if (msg_len == 0) { + // 0 length packets at this point are truncated and dropped. + continue; + } + ENVOY_LOG_MISC(debug, "Receive a packet with {} bytes from {}", msg_len, output.msg_[i].peer_address_->asString()); @@ -651,7 +656,8 @@ Api::IoCallUint64Result Utility::readFromSocket(IoHandle& handle, Api::IoCallUint64Result result = receiveMessage(udp_packet_processor.maxPacketSize(), buffer, output, handle, local_address); - if (!result.ok()) { + if (!result.ok() || result.rc_ == 0) { + // 0 length packets at this point are truncated and dropped. return result; } @@ -678,12 +684,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, @@ -695,7 +695,7 @@ Api::IoErrorPtr Utility::readPacketsFromSocket(IoHandle& handle, 1); // TODO(danzh) add stats for this. ENVOY_LOG_MISC( - debug, "Kernel dropped {} more packets. Consider increase receive buffer size.", delta); + debug, "Kernel dropped {} more packet(s). Consider increase receive buffer size.", delta); } } while (true); } diff --git a/test/common/network/udp_listener_impl_test.cc b/test/common/network/udp_listener_impl_test.cc index 1ee20f8e93a9..61c65a8d0b58 100644 --- a/test/common/network/udp_listener_impl_test.cc +++ b/test/common/network/udp_listener_impl_test.cc @@ -40,17 +40,21 @@ namespace { // packets are sent from a network namespace different to that of // the client. Currently, the testing framework does not support // this behavior. -// This helper allows to intercept the supportsUdpGro syscall and -// toggle the gro behavior as per individual test requirements. -class MockSupportsUdpGro : public Api::OsSysCallsImpl { +// This helper allows to intercept syscalls and +// toggle the behavior as per individual test requirements. +class OverrideOsSysCallsImpl : public Api::OsSysCallsImpl { public: MOCK_METHOD(bool, supportsUdpGro, (), (const)); + MOCK_METHOD(bool, supportsMmsg, (), (const)); }; class UdpListenerImplTest : public UdpListenerImplTestBase { public: void SetUp() override { - ON_CALL(udp_gro_syscall_, supportsUdpGro()).WillByDefault(Return(false)); + ON_CALL(override_syscall_, supportsUdpGro()).WillByDefault(Return(false)); + // Return the real version by default. + ON_CALL(override_syscall_, supportsMmsg()) + .WillByDefault(Return(os_calls.latched().supportsMmsg())); // Set listening socket options. server_socket_->addOptions(SocketOptionFactory::buildIpPacketInfoOptions()); @@ -64,8 +68,8 @@ class UdpListenerImplTest : public UdpListenerImplTestBase { ON_CALL(listener_callbacks_, udpPacketWriter()).WillByDefault(ReturnRef(*udp_packet_writer_)); } - NiceMock udp_gro_syscall_; - TestThreadsafeSingletonInjector os_calls{&udp_gro_syscall_}; + NiceMock override_syscall_; + TestThreadsafeSingletonInjector os_calls{&override_syscall_}; }; INSTANTIATE_TEST_SUITE_P(IpVersions, UdpListenerImplTest, @@ -126,6 +130,48 @@ TEST_P(UdpListenerImplTest, UseActualDstUdp) { dispatcher_->run(Event::Dispatcher::RunType::Block); } +// Test a large datagram that gets dropped using recvmmsg if supported. +TEST_P(UdpListenerImplTest, LargeDatagram) { + // This will get dropped. + const std::string first(4096, 'a'); + client_.write(first, *send_to_addr_); + const std::string second("second"); + client_.write(second, *send_to_addr_); + + EXPECT_CALL(listener_callbacks_, onReadReady()); + EXPECT_CALL(listener_callbacks_, onData(_)).WillOnce(Invoke([&](const UdpRecvData& data) -> void { + validateRecvCallbackParams(data, Api::OsSysCallsSingleton::get().supportsMmsg() ? 16u : 1u); + EXPECT_EQ(data.buffer_->toString(), second); + + dispatcher_->exit(); + })); + + dispatcher_->run(Event::Dispatcher::RunType::Block); + EXPECT_EQ(1, listener_->packetsDropped()); +} + +// Test a large datagram that gets dropped using recvmsg. +TEST_P(UdpListenerImplTest, LargeDatagramNoMmsg) { + ON_CALL(override_syscall_, supportsMmsg()).WillByDefault(Return(false)); + + // This will get dropped. + const std::string first(4096, 'a'); + client_.write(first, *send_to_addr_); + const std::string second("second"); + client_.write(second, *send_to_addr_); + + EXPECT_CALL(listener_callbacks_, onReadReady()); + EXPECT_CALL(listener_callbacks_, onData(_)).WillOnce(Invoke([&](const UdpRecvData& data) -> void { + validateRecvCallbackParams(data, Api::OsSysCallsSingleton::get().supportsMmsg() ? 16u : 1u); + EXPECT_EQ(data.buffer_->toString(), second); + + dispatcher_->exit(); + })); + + dispatcher_->run(Event::Dispatcher::RunType::Block); + EXPECT_EQ(1, listener_->packetsDropped()); +} + /** * Tests UDP listener for read and write callbacks with actual data. */ 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_; From ccb53ae2108eaa44cf0008b7c082b386998228a6 Mon Sep 17 00:00:00 2001 From: Matt Klein Date: Fri, 16 Oct 2020 21:04:35 +0000 Subject: [PATCH 2/8] comments Signed-off-by: Matt Klein --- include/envoy/network/io_handle.h | 3 +++ source/common/network/io_socket_handle_impl.cc | 2 ++ source/common/network/utility.cc | 14 ++++++-------- test/common/network/udp_listener_impl_test.cc | 4 ++-- 4 files changed, 13 insertions(+), 10 deletions(-) diff --git a/include/envoy/network/io_handle.h b/include/envoy/network/io_handle.h index f11855014a3e..aecb98fdbbf9 100644 --- a/include/envoy/network/io_handle.h +++ b/include/envoy/network/io_handle.h @@ -117,6 +117,9 @@ class IoHandle { unsigned int msg_len_{0}; // The gso_size, if specified in the transport header unsigned int gso_size_{0}; + // 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}; }; /** diff --git a/source/common/network/io_socket_handle_impl.cc b/source/common/network/io_socket_handle_impl.cc index b0c92a9d4f19..7817780bd40d 100644 --- a/source/common/network/io_socket_handle_impl.cc +++ b/source/common/network/io_socket_handle_impl.cc @@ -300,6 +300,7 @@ Api::IoCallUint64Result IoSocketHandleImpl::recvmsg(Buffer::RawSlice* slices, 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); } @@ -393,6 +394,7 @@ Api::IoCallUint64Result IoSocketHandleImpl::recvmmsg(RawSliceArrays& slices, uin if ((hdr.msg_flags & MSG_TRUNC) != 0) { ENVOY_LOG_MISC(debug, "Dropping truncated UDP packet with size: {}.", mmsg_hdr[i].msg_len); (*output.dropped_packets_)++; + output.msg_[0].truncated_and_dropped_ = true; continue; } diff --git a/source/common/network/utility.cc b/source/common/network/utility.cc index e3445090b69d..0574c040a1d1 100644 --- a/source/common/network/utility.cc +++ b/source/common/network/utility.cc @@ -629,14 +629,13 @@ Api::IoCallUint64Result Utility::readFromSocket(IoHandle& handle, uint64_t packets_read = result.rc_; ENVOY_LOG_MISC(trace, "recvmmsg read {} packets", packets_read); for (uint64_t i = 0; i < packets_read; ++i) { - Buffer::RawSlice* slice = slices[i].data(); - const uint64_t msg_len = output.msg_[i].msg_len_; - ASSERT(msg_len <= slice->len_); - if (msg_len == 0) { - // 0 length packets at this point are truncated and dropped. + if (output.msg_[i].truncated_and_dropped_) { continue; } + Buffer::RawSlice* slice = slices[i].data(); + const uint64_t msg_len = output.msg_[i].msg_len_; + ASSERT(msg_len <= slice->len_); ENVOY_LOG_MISC(debug, "Receive a packet with {} bytes from {}", msg_len, output.msg_[i].peer_address_->asString()); @@ -656,8 +655,7 @@ Api::IoCallUint64Result Utility::readFromSocket(IoHandle& handle, Api::IoCallUint64Result result = receiveMessage(udp_packet_processor.maxPacketSize(), buffer, output, handle, local_address); - if (!result.ok() || result.rc_ == 0) { - // 0 length packets at this point are truncated and dropped. + if (!result.ok() || output.msg_[0].truncated_and_dropped_) { return result; } @@ -695,7 +693,7 @@ Api::IoErrorPtr Utility::readPacketsFromSocket(IoHandle& handle, 1); // TODO(danzh) add stats for this. ENVOY_LOG_MISC( - debug, "Kernel dropped {} more packet(s). Consider increase receive buffer size.", delta); + debug, "Kernel dropped {} more packets. Consider increase receive buffer size.", delta); } } while (true); } diff --git a/test/common/network/udp_listener_impl_test.cc b/test/common/network/udp_listener_impl_test.cc index 61c65a8d0b58..192ea0719614 100644 --- a/test/common/network/udp_listener_impl_test.cc +++ b/test/common/network/udp_listener_impl_test.cc @@ -131,7 +131,7 @@ TEST_P(UdpListenerImplTest, UseActualDstUdp) { } // Test a large datagram that gets dropped using recvmmsg if supported. -TEST_P(UdpListenerImplTest, LargeDatagram) { +TEST_P(UdpListenerImplTest, LargeDatagramRecvmmsg) { // This will get dropped. const std::string first(4096, 'a'); client_.write(first, *send_to_addr_); @@ -151,7 +151,7 @@ TEST_P(UdpListenerImplTest, LargeDatagram) { } // Test a large datagram that gets dropped using recvmsg. -TEST_P(UdpListenerImplTest, LargeDatagramNoMmsg) { +TEST_P(UdpListenerImplTest, LargeDatagramRecvmsg) { ON_CALL(override_syscall_, supportsMmsg()).WillByDefault(Return(false)); // This will get dropped. From f51548769ccec7023a294d9f1fc2bafc9c04ba69 Mon Sep 17 00:00:00 2001 From: Matt Klein Date: Mon, 19 Oct 2020 15:39:54 +0000 Subject: [PATCH 3/8] fix Signed-off-by: Matt Klein --- .../quiche/quic_io_handle_wrapper_test.cc | 27 ++++++++++--------- 1 file changed, 14 insertions(+), 13 deletions(-) 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 da1d7b1aeae5..7a711ef39d65 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 @@ -77,18 +77,19 @@ TEST_F(QuicIoHandleWrapperTest, DelegateIoHandleCalls) { addr = wrapper_->peerAddress(); Network::IoHandle::RecvMsgOutput output(1, nullptr); - EXPECT_CALL(os_sys_calls_, recvmsg(fd, _, 0)).WillOnce(Invoke([](os_fd_t, 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}; - })); + EXPECT_CALL(os_sys_calls_, recvmsg(fd, _, MSG_TRUNC)) + .WillOnce(Invoke([](os_fd_t, 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); size_t num_packet_per_call = 1u; @@ -97,7 +98,7 @@ TEST_F(QuicIoHandleWrapperTest, DelegateIoHandleCalls) { absl::FixedArray({Buffer::RawSlice{data, 5}})); EXPECT_CALL(os_sys_calls_, recvmmsg(fd, _, num_packet_per_call, _, nullptr)) .WillOnce(Invoke([](os_fd_t, struct mmsghdr*, unsigned int, int, struct timespec*) { - return Api::SysCallIntResult{1u, 0}; + return Api::SysCallIntResult{-1, SOCKET_ERROR_AGAIN}; })); wrapper_->recvmmsg(slices, /*self_port=*/12345, output2); From 0616a2546c4029cc3a7ca82ac17324a3e75df9be Mon Sep 17 00:00:00 2001 From: Matt Klein Date: Thu, 22 Oct 2020 17:14:23 +0000 Subject: [PATCH 4/8] windows fix Signed-off-by: Matt Klein --- source/common/api/win32/os_sys_calls_impl.cc | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/source/common/api/win32/os_sys_calls_impl.cc b/source/common/api/win32/os_sys_calls_impl.cc index 5c49d5439d21..27f9257bf55f 100644 --- a/source/common/api/win32/os_sys_calls_impl.cc +++ b/source/common/api/win32/os_sys_calls_impl.cc @@ -144,14 +144,22 @@ SysCallSizeResult OsSysCallsImpl::recv(os_fd_t socket, void* buffer, size_t leng } SysCallSizeResult OsSysCallsImpl::recvmsg(os_fd_t sockfd, msghdr* msg, int flags) { - DWORD bytes_received; + DWORD bytes_received = 0; LPFN_WSARECVMSG recvmsg_fn_ptr = getFnPtrWSARecvMsg(); wsamsgResult wsamsg = msghdrToWSAMSG(msg); // Windows supports only a single flag on input to WSARecvMsg wsamsg.wsamsg_->dwFlags = flags & MSG_PEEK; const int rc = recvmsg_fn_ptr(sockfd, wsamsg.wsamsg_.get(), &bytes_received, nullptr, nullptr); if (rc == SOCKET_ERROR) { - return {-1, ::WSAGetLastError()}; + // We try to match the UNIX behavior for truncated packages. In that case the return code is + // the length of the allocated buffer and we get the value from `dwFlags`. + auto last_error = ::WSAGetLastError(); + if (last_error == WSAEMSGSIZE) { + msg->msg_flags = wsamsg.wsamsg_->dwFlags; + return {bytes_received, 0}; + } + + return {rc, last_error}; } msg->msg_namelen = wsamsg.wsamsg_->namelen; msg->msg_flags = wsamsg.wsamsg_->dwFlags; From 060e89832d71715629835693dc9a5ebe7929346e Mon Sep 17 00:00:00 2001 From: Matt Klein Date: Thu, 12 Nov 2020 22:40:04 +0000 Subject: [PATCH 5/8] fix Signed-off-by: Matt Klein --- source/common/network/io_socket_handle_impl.cc | 2 +- test/common/network/udp_listener_impl_test.cc | 10 ++++++++-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/source/common/network/io_socket_handle_impl.cc b/source/common/network/io_socket_handle_impl.cc index b30079685488..5a43368f86b7 100644 --- a/source/common/network/io_socket_handle_impl.cc +++ b/source/common/network/io_socket_handle_impl.cc @@ -398,7 +398,7 @@ Api::IoCallUint64Result IoSocketHandleImpl::recvmmsg(RawSliceArrays& slices, uin if ((hdr.msg_flags & MSG_TRUNC) != 0) { ENVOY_LOG_MISC(debug, "Dropping truncated UDP packet with size: {}.", mmsg_hdr[i].msg_len); (*output.dropped_packets_)++; - output.msg_[0].truncated_and_dropped_ = true; + output.msg_[i].truncated_and_dropped_ = true; continue; } diff --git a/test/common/network/udp_listener_impl_test.cc b/test/common/network/udp_listener_impl_test.cc index 192ea0719614..b6e4bd670510 100644 --- a/test/common/network/udp_listener_impl_test.cc +++ b/test/common/network/udp_listener_impl_test.cc @@ -137,6 +137,9 @@ TEST_P(UdpListenerImplTest, LargeDatagramRecvmmsg) { client_.write(first, *send_to_addr_); const std::string second("second"); client_.write(second, *send_to_addr_); + // This will get dropped. + const std::string third(4096, 'b'); + client_.write(third, *send_to_addr_); EXPECT_CALL(listener_callbacks_, onReadReady()); EXPECT_CALL(listener_callbacks_, onData(_)).WillOnce(Invoke([&](const UdpRecvData& data) -> void { @@ -147,7 +150,7 @@ TEST_P(UdpListenerImplTest, LargeDatagramRecvmmsg) { })); dispatcher_->run(Event::Dispatcher::RunType::Block); - EXPECT_EQ(1, listener_->packetsDropped()); + EXPECT_EQ(2, listener_->packetsDropped()); } // Test a large datagram that gets dropped using recvmsg. @@ -159,6 +162,9 @@ TEST_P(UdpListenerImplTest, LargeDatagramRecvmsg) { client_.write(first, *send_to_addr_); const std::string second("second"); client_.write(second, *send_to_addr_); + // This will get dropped. + const std::string third(4096, 'b'); + client_.write(third, *send_to_addr_); EXPECT_CALL(listener_callbacks_, onReadReady()); EXPECT_CALL(listener_callbacks_, onData(_)).WillOnce(Invoke([&](const UdpRecvData& data) -> void { @@ -169,7 +175,7 @@ TEST_P(UdpListenerImplTest, LargeDatagramRecvmsg) { })); dispatcher_->run(Event::Dispatcher::RunType::Block); - EXPECT_EQ(1, listener_->packetsDropped()); + EXPECT_EQ(2, listener_->packetsDropped()); } /** From e876a47fe13ebb3e57e3693a0a1ad2f481f3b8ff Mon Sep 17 00:00:00 2001 From: Matt Klein Date: Tue, 17 Nov 2020 19:24:08 +0000 Subject: [PATCH 6/8] comments Signed-off-by: Matt Klein --- docs/root/version_history/v1.16.1.rst | 6 ++++++ docs/root/version_history/version_history.rst | 1 + source/common/network/io_socket_handle_impl.cc | 18 +++++++++++++++--- 3 files changed, 22 insertions(+), 3 deletions(-) create mode 100644 docs/root/version_history/v1.16.1.rst diff --git a/docs/root/version_history/v1.16.1.rst b/docs/root/version_history/v1.16.1.rst new file mode 100644 index 000000000000..4522f107f518 --- /dev/null +++ b/docs/root/version_history/v1.16.1.rst @@ -0,0 +1,6 @@ +1.16.1 (TBD) +============ + +Changes +------- +* udp: fixed issue in which receiving truncated UDP datagrams would cause Envoy to crash. \ No newline at end of file diff --git a/docs/root/version_history/version_history.rst b/docs/root/version_history/version_history.rst index 453bda753f1f..c01875afafcc 100644 --- a/docs/root/version_history/version_history.rst +++ b/docs/root/version_history/version_history.rst @@ -7,6 +7,7 @@ Version history :titlesonly: current + v1.16.1 v1.16.0 v1.15.2 v1.15.1 diff --git a/source/common/network/io_socket_handle_impl.cc b/source/common/network/io_socket_handle_impl.cc index 5a43368f86b7..3f9f0800d9ea 100644 --- a/source/common/network/io_socket_handle_impl.cc +++ b/source/common/network/io_socket_handle_impl.cc @@ -48,6 +48,16 @@ in_addr addressFromMessage(const cmsghdr& cmsg) { #endif } +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 { @@ -296,7 +306,8 @@ Api::IoCallUint64Result IoSocketHandleImpl::recvmsg(Buffer::RawSlice* slices, hdr.msg_flags = 0; hdr.msg_control = cbuf.begin(); hdr.msg_controllen = cmsg_space_; - Api::SysCallSizeResult result = Api::OsSysCallsSingleton::get().recvmsg(fd_, &hdr, MSG_TRUNC); + Api::SysCallSizeResult result = + Api::OsSysCallsSingleton::get().recvmsg(fd_, &hdr, messageTruncatedOption()); if (result.rc_ < 0) { return sysCallResultToIoCallResult(result); } @@ -384,8 +395,9 @@ Api::IoCallUint64Result IoSocketHandleImpl::recvmmsg(RawSliceArrays& slices, uin // Set MSG_WAITFORONE so that recvmmsg will not waiting for // |num_packets_per_mmsg_call| packets to arrive before returning when the // socket is a blocking socket. - const Api::SysCallIntResult result = Api::OsSysCallsSingleton::get().recvmmsg( - fd_, mmsg_hdr.data(), num_packets_per_mmsg_call, MSG_TRUNC | MSG_WAITFORONE, nullptr); + const Api::SysCallIntResult result = + Api::OsSysCallsSingleton::get().recvmmsg(fd_, mmsg_hdr.data(), num_packets_per_mmsg_call, + messageTruncatedOption() | MSG_WAITFORONE, nullptr); if (result.rc_ <= 0) { return sysCallResultToIoCallResult(result); From 4853822c45a6a2b4096d0fe48b96cfb02f7a1161 Mon Sep 17 00:00:00 2001 From: Christoph Pakulski Date: Fri, 20 Nov 2020 18:26:28 +0000 Subject: [PATCH 7/8] Small modifications to docs. Signed-off-by: Christoph Pakulski --- docs/root/version_history/current.rst | 1 + docs/root/version_history/v1.16.1.rst | 6 ------ docs/root/version_history/version_history.rst | 1 - 3 files changed, 1 insertion(+), 7 deletions(-) delete mode 100644 docs/root/version_history/v1.16.1.rst diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index c0ac992bb143..ad3249ccf4fa 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -38,6 +38,7 @@ Bug Fixes * tls: fix detection of the upstream connection close event. * tls: fix read resumption after triggering buffer high-watermark and all remaining request/response bytes are stored in the SSL connection's internal buffers. * watchdog: touch the watchdog before most event loop operations to avoid misses when handling bursts of callbacks. +* udp: fixed issue in which receiving truncated UDP datagrams would cause Envoy to crash. Removed Config or Runtime ------------------------- diff --git a/docs/root/version_history/v1.16.1.rst b/docs/root/version_history/v1.16.1.rst deleted file mode 100644 index 4522f107f518..000000000000 --- a/docs/root/version_history/v1.16.1.rst +++ /dev/null @@ -1,6 +0,0 @@ -1.16.1 (TBD) -============ - -Changes -------- -* udp: fixed issue in which receiving truncated UDP datagrams would cause Envoy to crash. \ No newline at end of file diff --git a/docs/root/version_history/version_history.rst b/docs/root/version_history/version_history.rst index c01875afafcc..453bda753f1f 100644 --- a/docs/root/version_history/version_history.rst +++ b/docs/root/version_history/version_history.rst @@ -7,7 +7,6 @@ Version history :titlesonly: current - v1.16.1 v1.16.0 v1.15.2 v1.15.1 From 7c1ba69889cb76523c973a4508af71f9b1410d09 Mon Sep 17 00:00:00 2001 From: Christoph Pakulski Date: Fri, 20 Nov 2020 19:01:17 +0000 Subject: [PATCH 8/8] Corrected release notes to be in alphabetical order. Signed-off-by: Christoph Pakulski --- docs/root/version_history/current.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index ad3249ccf4fa..460757b11d7d 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -37,8 +37,8 @@ Bug Fixes * proxy_proto: fixed a bug where the wrong downstream address got sent to upstream connections. * tls: fix detection of the upstream connection close event. * tls: fix read resumption after triggering buffer high-watermark and all remaining request/response bytes are stored in the SSL connection's internal buffers. -* watchdog: touch the watchdog before most event loop operations to avoid misses when handling bursts of callbacks. * udp: fixed issue in which receiving truncated UDP datagrams would cause Envoy to crash. +* watchdog: touch the watchdog before most event loop operations to avoid misses when handling bursts of callbacks. Removed Config or Runtime -------------------------