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 <[email protected]>
envoyproxy · Sep 20, 2024 · cfb1b7e · cfb1b7e
1 parent 135a6a2
commit cfb1b7e
Show file tree

Hide file tree

Showing 4 changed files with 57 additions and 7 deletions.
diff --git 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<int>(err->getErrorCode()),
                      err->getErrorDetails());

diff --git 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
@@ -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
@@ -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<Api::OsSysCallsImpl> 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<Network::UdpDefaultWriter>(
+          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),