From cfb1b7e793a4a7f6c5e2dcf49027b980c05a3b0b Mon Sep 17 00:00:00 2001 From: danzh Date: Fri, 20 Sep 2024 14:10:18 -0400 Subject: [PATCH] quic: fix connection close error when blocked socket gets unblocked (#36238) Today the connection will be closed with QUIC_INTERNAL_ERROR because of https://github.com/google/quiche/blob/4249f8025caed1e3d71d04d9cadf42251acb7cac/quiche/quic/core/quic_connection.cc#L2703 if the socket becomes write blocked and then unblocked. This change fixes it by calling OnBlockedWriterCanWrite()(https://github.com/google/quiche/blob/4249f8025caed1e3d71d04d9cadf42251acb7cac/quiche/quic/core/quic_connection.cc#L2692) instead which set the writer to be unblocked before OnCanWrite(). Risk Level: low, fix existing issue Testing: added unit tests Docs Changes: N/A Release Notes: N/A Platform Specific Features: N/A Signed-off-by: Dan Zhang --- .../quic/envoy_quic_client_connection.cc | 15 +++--- .../quic/envoy_quic_client_connection.h | 2 + test/common/quic/BUILD | 1 + .../quic/envoy_quic_client_session_test.cc | 46 +++++++++++++++++++ 4 files changed, 57 insertions(+), 7 deletions(-) diff --git a/source/common/quic/envoy_quic_client_connection.cc b/source/common/quic/envoy_quic_client_connection.cc index d3321085458a..a2b817dadd1f 100644 --- a/source/common/quic/envoy_quic_client_connection.cc +++ b/source/common/quic/envoy_quic_client_connection.cc @@ -242,7 +242,7 @@ void EnvoyQuicClientConnection::onFileEvent(uint32_t events, ASSERT(events & (Event::FileReadyType::Read | Event::FileReadyType::Write)); if (events & Event::FileReadyType::Write) { - OnCanWrite(); + OnBlockedWriterCanWrite(); } bool is_probing_socket = @@ -261,14 +261,15 @@ void EnvoyQuicClientConnection::onFileEvent(uint32_t events, connection_socket.ioHandle(), *connection_socket.connectionInfoProvider().localAddress(), *this, dispatcher_.timeSource(), prefer_gro_, !disallow_mmsg_, packets_dropped_); if (err == nullptr) { - // In the case where the path validation fails, the probing socket will be closed and its IO - // events are no longer interesting. - if (!is_probing_socket || HasPendingPathValidation() || - connectionSocket().get() == &connection_socket) { + // If this READ event is on the probing socket and any packet read failed the path validation + // (i.e. via STATELESS_RESET), the probing socket should have been closed and the default + // socket remained unchanged. In this case any remaining unread packets are no longer + // interesting. Only re-register READ event to continue reading the remaining packets in the + // next loop if this is not the case. + if (!(is_probing_socket && !HasPendingPathValidation() && + connectionSocket().get() != &connection_socket)) { connection_socket.ioHandle().activateFileEvents(Event::FileReadyType::Read); - return; } - } else if (err->getErrorCode() != Api::IoError::IoErrorCode::Again) { ENVOY_CONN_LOG(error, "recvmsg result {}: {}", *this, static_cast(err->getErrorCode()), err->getErrorDetails()); diff --git a/source/common/quic/envoy_quic_client_connection.h b/source/common/quic/envoy_quic_client_connection.h index 197f12ebf726..d673033ed9f3 100644 --- a/source/common/quic/envoy_quic_client_connection.h +++ b/source/common/quic/envoy_quic_client_connection.h @@ -121,6 +121,8 @@ class EnvoyQuicClientConnection : public quic::QuicConnection, probeAndMigrateToServerPreferredAddress(const quic::QuicSocketAddress& server_preferred_address); private: + friend class EnvoyQuicClientConnectionPeer; + // Receives notifications from the Quiche layer on path validation results. class EnvoyPathValidationResultDelegate : public quic::QuicPathValidator::ResultDelegate { public: diff --git a/test/common/quic/BUILD b/test/common/quic/BUILD index 71fa7de6818a..0a4e8ad774c0 100644 --- a/test/common/quic/BUILD +++ b/test/common/quic/BUILD @@ -200,6 +200,7 @@ envoy_cc_test( "//source/common/quic:envoy_quic_client_session_lib", "//source/common/quic:envoy_quic_connection_helper_lib", "//source/extensions/quic/crypto_stream:envoy_quic_crypto_client_stream_lib", + "//test/mocks/api:api_mocks", "//test/mocks/http:http_mocks", "//test/mocks/http:stream_decoder_mock", "//test/mocks/network:network_mocks", diff --git a/test/common/quic/envoy_quic_client_session_test.cc b/test/common/quic/envoy_quic_client_session_test.cc index 592b7782dd27..d757f03a0b7f 100644 --- a/test/common/quic/envoy_quic_client_session_test.cc +++ b/test/common/quic/envoy_quic_client_session_test.cc @@ -2,6 +2,7 @@ #include "source/common/api/os_sys_calls_impl.h" #include "source/common/network/transport_socket_options_impl.h" +#include "source/common/network/udp_packet_writer_handler_impl.h" #include "source/common/quic/client_codec_impl.h" #include "source/common/quic/envoy_quic_alarm_factory.h" #include "source/common/quic/envoy_quic_client_connection.h" @@ -11,6 +12,7 @@ #include "source/extensions/quic/crypto_stream/envoy_quic_crypto_client_stream.h" #include "test/common/quic/test_utils.h" +#include "test/mocks/api/mocks.h" #include "test/mocks/event/mocks.h" #include "test/mocks/http/mocks.h" #include "test/mocks/http/stream_decoder.h" @@ -37,6 +39,14 @@ using testing::Return; namespace Envoy { namespace Quic { +class EnvoyQuicClientConnectionPeer { +public: + static void onFileEvent(EnvoyQuicClientConnection& connection, uint32_t events, + Network::ConnectionSocket& connection_socket) { + connection.onFileEvent(events, connection_socket); + } +}; + class TestEnvoyQuicClientConnection : public EnvoyQuicClientConnection { public: TestEnvoyQuicClientConnection(const quic::QuicConnectionId& server_connection_id, @@ -636,6 +646,42 @@ TEST_P(EnvoyQuicClientSessionTest, UseSocketAddressCache) { EXPECT_EQ(last_peer_address.get(), quic_connection_->getLastPeerAddress().get()); } +TEST_P(EnvoyQuicClientSessionTest, WriteBlockedAndUnblock) { + Api::MockOsSysCalls os_sys_calls; + TestThreadsafeSingletonInjector singleton_injector{&os_sys_calls}; + + // Switch to a real writer, and synthesize a write block on it. + quic_connection_->SetQuicPacketWriter( + new EnvoyQuicPacketWriter(std::make_unique( + quic_connection_->connectionSocket()->ioHandle())), + true); + if (quic_connection_->connectionSocket()->ioHandle().wasConnected()) { + EXPECT_CALL(os_sys_calls, send(_, _, _, _)) + .Times(2) + .WillOnce(Return(Api::SysCallSizeResult{-1, SOCKET_ERROR_AGAIN})); + } else { + EXPECT_CALL(os_sys_calls, sendmsg(_, _, _)) + .Times(2) + .WillOnce(Return(Api::SysCallSizeResult{-1, SOCKET_ERROR_AGAIN})); + } + // OnCanWrite() would close the connection if the underlying writer is not unblocked. + EXPECT_CALL(*quic_connection_, SendConnectionClosePacket(quic::QUIC_INTERNAL_ERROR, _, _)) + .Times(0); + Http::MockResponseDecoder response_decoder; + Http::MockStreamCallbacks stream_callbacks; + EnvoyQuicClientStream& stream = sendGetRequest(response_decoder, stream_callbacks); + EXPECT_TRUE(quic_connection_->writer()->IsWriteBlocked()); + + // Synthesize a WRITE event. + EnvoyQuicClientConnectionPeer::onFileEvent(*quic_connection_, Event::FileReadyType::Write, + *quic_connection_->connectionSocket()); + EXPECT_FALSE(quic_connection_->writer()->IsWriteBlocked()); + EXPECT_CALL(stream_callbacks, + onResetStream(Http::StreamResetReason::LocalReset, "QUIC_STREAM_REQUEST_REJECTED")); + EXPECT_CALL(*quic_connection_, SendControlFrame(_)); + stream.resetStream(Http::StreamResetReason::LocalReset); +} + class MockOsSysCallsImpl : public Api::OsSysCallsImpl { public: MOCK_METHOD(Api::SysCallSizeResult, recvmsg, (os_fd_t socket, msghdr* msg, int flags),