quiche: use max header size configured in HCM (#15912)

Commit Message: use max header size in HCM config to initialize quic session.
Additional Description:
change QuicSession::Initialize() and filter chain creation order to set max header list size for each session before Initialize(). Move Initialize() of Http3 connection from CodecClient to CodecClientProd.
Added CodecClient::connect() interface.
Change EnvoyQuicConnection not to directly inherit from QuicConnection. And renamed it.
Fix a use-after-free bug in FakeUpstream which causes ASAN failure.

Risk Level: low
Testing: more protocol H3 tests

Part of #2557 #12930 #14829

Signed-off-by: Dan Zhang <[email protected]>

source/common/http/codec_client.cc

@@ -34,16 +34,6 @@ CodecClient::CodecClient(Type type, Network::ClientConnectionPtr&& connection,
   connection_->addConnectionCallbacks(*this);
   connection_->addReadFilter(Network::ReadFilterSharedPtr{new CodecReadFilter(*this)});
 
-  // In general, codecs are handed new not-yet-connected connections, but in the
-  // case of ALPN, the codec may be handed an already connected connection.
-  if (!connection_->connecting()) {
-    ASSERT(connection_->state() == Network::Connection::State::Open);
-    connected_ = true;
-  } else {
-    ENVOY_CONN_LOG(debug, "connecting", *connection_);
-    connection_->connect();
-  }
-
   if (idle_timeout_) {
     idle_timer_ = dispatcher.createTimer([this]() -> void { onIdleTimeout(); });
     enableIdleTimer();
@@ -54,7 +44,23 @@ CodecClient::CodecClient(Type type, Network::ClientConnectionPtr&& connection,
   connection_->noDelay(true);
 }
 
-CodecClient::~CodecClient() = default;
+CodecClient::~CodecClient() {
+  ASSERT(connect_called_, "CodecClient::connect() is not called through out the life time.");
+}
+
+void CodecClient::connect() {
+  connect_called_ = true;
+  ASSERT(codec_ != nullptr);
+  // In general, codecs are handed new not-yet-connected connections, but in the
+  // case of ALPN, the codec may be handed an already connected connection.
+  if (!connection_->connecting()) {
+    ASSERT(connection_->state() == Network::Connection::State::Open);
+    connected_ = true;
+  } else {
+    ENVOY_CONN_LOG(debug, "connecting", *connection_);
+    connection_->connect();
+  }
+}
 
 void CodecClient::close() { connection_->close(Network::ConnectionCloseType::NoFlush); }
 
@@ -168,7 +174,6 @@ CodecClientProd::CodecClientProd(Type type, Network::ClientConnectionPtr&& conne
                                  Event::Dispatcher& dispatcher,
                                  Random::RandomGenerator& random_generator)
     : CodecClient(type, std::move(connection), host, dispatcher) {
-
   switch (type) {
   case Type::HTTP1: {
     codec_ = std::make_unique<Http1::ClientConnectionImpl>(
@@ -185,17 +190,21 @@ CodecClientProd::CodecClientProd(Type type, Network::ClientConnectionPtr&& conne
   }
   case Type::HTTP3: {
 #ifdef ENVOY_ENABLE_QUIC
+    auto& quic_session = dynamic_cast<Quic::EnvoyQuicClientSession&>(*connection_);
     codec_ = std::make_unique<Quic::QuicHttpClientConnectionImpl>(
-        dynamic_cast<Quic::EnvoyQuicClientSession&>(*connection_), *this,
-        host->cluster().http3CodecStats(), host->cluster().http3Options(),
+        quic_session, *this, host->cluster().http3CodecStats(), host->cluster().http3Options(),
         Http::DEFAULT_MAX_REQUEST_HEADERS_KB);
+    // Initialize the session after max request header size is changed in above http client
+    // connection creation.
+    quic_session.Initialize();
     break;
 #else
     // Should be blocked by configuration checking at an earlier point.
     NOT_REACHED_GCOVR_EXCL_LINE;
 #endif
   }
   }
+  connect();
 }
 
 } // namespace Http

source/common/http/codec_client.h

@@ -47,7 +47,7 @@ class CodecClientCallbacks {
  * This is an HTTP client that multiple stream management and underlying connection management
  * across multiple HTTP codec types.
  */
-class CodecClient : Logger::Loggable<Logger::Id::client>,
+class CodecClient : protected Logger::Loggable<Logger::Id::client>,
                     public Http::ConnectionCallbacks,
                     public Network::ConnectionCallbacks,
                     public Event::DeferredDeletable {
@@ -140,6 +140,12 @@ class CodecClient : Logger::Loggable<Logger::Id::client>,
   CodecClient(Type type, Network::ClientConnectionPtr&& connection,
               Upstream::HostDescriptionConstSharedPtr host, Event::Dispatcher& dispatcher);
 
+  /**
+   * Connect to the host.
+   * Needs to be called after codec_ is instantiated.
+   */
+  void connect();
+
   // Http::ConnectionCallbacks
   void onGoAway(GoAwayErrorCode error_code) override {
     if (codec_callbacks_) {
@@ -257,6 +263,7 @@ class CodecClient : Logger::Loggable<Logger::Id::client>,
   bool connected_{};
   bool remote_closed_{};
   bool protocol_error_{false};
+  bool connect_called_{false};
 };
 
 using CodecClientPtr = std::unique_ptr<CodecClient>;

source/common/quic/BUILD

@@ -180,8 +180,8 @@ envoy_cc_library(
     hdrs = ["quic_filter_manager_connection_impl.h"],
     tags = ["nofips"],
     deps = [
-        ":envoy_quic_connection_lib",
         ":envoy_quic_simulated_watermark_buffer_lib",
+        ":quic_network_connection_lib",
         ":send_buffer_monitor_lib",
         "//include/envoy/event:dispatcher_interface",
         "//include/envoy/network:connection_interface",
@@ -192,6 +192,7 @@ envoy_cc_library(
         "//source/common/http/http3:codec_stats_lib",
         "//source/common/network:connection_base_lib",
         "//source/common/stream_info:stream_info_lib",
+        "@com_googlesource_quiche//:quic_core_connection_lib",
     ],
 )
 
@@ -208,6 +209,7 @@ envoy_cc_library(
     tags = ["nofips"],
     deps = [
         ":envoy_quic_proof_source_lib",
+        ":envoy_quic_server_connection_lib",
         ":envoy_quic_stream_lib",
         ":envoy_quic_utils_lib",
         ":quic_filter_manager_connection_lib",
@@ -255,17 +257,13 @@ envoy_cc_library(
 )
 
 envoy_cc_library(
-    name = "envoy_quic_connection_lib",
-    srcs = ["envoy_quic_connection.cc"],
-    hdrs = ["envoy_quic_connection.h"],
+    name = "quic_network_connection_lib",
+    srcs = ["quic_network_connection.cc"],
+    hdrs = ["quic_network_connection.h"],
     tags = ["nofips"],
     deps = [
-        ":quic_io_handle_wrapper_lib",
         "//include/envoy/network:connection_interface",
         "//source/common/network:listen_socket_lib",
-        "//source/common/quic:envoy_quic_utils_lib",
-        "//source/extensions/transport_sockets:well_known_names",
-        "@com_googlesource_quiche//:quic_core_connection_lib",
     ],
 )
 
@@ -275,8 +273,12 @@ envoy_cc_library(
     hdrs = ["envoy_quic_server_connection.h"],
     tags = ["nofips"],
     deps = [
-        ":envoy_quic_connection_lib",
+        ":quic_io_handle_wrapper_lib",
+        ":quic_network_connection_lib",
+        "//source/common/quic:envoy_quic_utils_lib",
+        "//source/extensions/transport_sockets:well_known_names",
         "//source/server:connection_handler_lib",
+        "@com_googlesource_quiche//:quic_core_connection_lib",
     ],
 )
 
@@ -286,11 +288,12 @@ envoy_cc_library(
     hdrs = ["envoy_quic_client_connection.h"],
     tags = ["nofips"],
     deps = [
-        ":envoy_quic_connection_lib",
         ":envoy_quic_packet_writer_lib",
+        ":quic_network_connection_lib",
         "//include/envoy/event:dispatcher_interface",
         "//source/common/network:socket_option_factory_lib",
         "//source/common/network:udp_packet_writer_handler_lib",
+        "@com_googlesource_quiche//:quic_core_connection_lib",
         "@envoy_api//envoy/config/core/v3:pkg_cc_proto",
     ],
 )
@@ -354,6 +357,8 @@ envoy_cc_library(
         "//source/common/network:address_lib",
         "//source/common/network:listen_socket_lib",
         "//source/common/network:socket_option_factory_lib",
+        "//source/common/quic:quic_io_handle_wrapper_lib",
+        "//source/extensions/transport_sockets:well_known_names",
         "@com_googlesource_quiche//:quic_core_http_header_list_lib",
         "@envoy_api//envoy/config/core/v3:pkg_cc_proto",
     ],

source/common/quic/active_quic_listener.cc

@@ -46,6 +46,9 @@ ActiveQuicListener::ActiveQuicListener(
       kernel_worker_routing_(kernel_worker_routing) {
   // This flag fix a QUICHE issue which may crash Envoy during connection close.
   SetQuicReloadableFlag(quic_single_ack_in_packet2, true);
+  // Do not include 32-byte per-entry overhead while counting header size.
+  quiche::FlagRegistry::getInstance();
+  ASSERT(!GetQuicFlag(FLAGS_quic_header_size_limit_includes_overhead));
 
   if (Runtime::LoaderSingleton::getExisting()) {
     enabled_.emplace(Runtime::FeatureFlag(enabled, Runtime::LoaderSingleton::get()));

source/common/quic/client_connection_factory_impl.cc

@@ -22,7 +22,9 @@ PersistentQuicInfoImpl::PersistentQuicInfoImpl(
                  static_cast<uint16_t>(server_addr->ip()->port()), false},
       crypto_config_(
           std::make_unique<quic::QuicCryptoClientConfig>(std::make_unique<EnvoyQuicProofVerifier>(
-              stats_scope, getConfig(transport_socket_factory), time_source))) {}
+              stats_scope, getConfig(transport_socket_factory), time_source))) {
+  quiche::FlagRegistry::getInstance();
+}
 
 namespace {
 // TODO(alyssawilk, danzh2010): This is mutable static info that is required for the QUICHE code.
@@ -53,7 +55,6 @@ createQuicNetworkConnection(Http::PersistentQuicInfo& info, Event::Dispatcher& d
       static_info.quic_config_, info_impl->supported_versions_, std::move(connection),
       info_impl->server_id_, info_impl->crypto_config_.get(), &static_info.push_promise_index_,
       dispatcher, 0);
-  ret->Initialize();
   return ret;
 }
 

source/common/quic/codec_impl.cc

@@ -22,14 +22,15 @@ QuicHttpServerConnectionImpl::QuicHttpServerConnectionImpl(
     EnvoyQuicServerSession& quic_session, Http::ServerConnectionCallbacks& callbacks,
     Http::Http3::CodecStats& stats,
     const envoy::config::core::v3::Http3ProtocolOptions& http3_options,
-    const uint32_t /*max_request_headers_kb*/,
+    const uint32_t max_request_headers_kb,
     envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction
         headers_with_underscores_action)
     : QuicHttpConnectionImplBase(quic_session, stats), quic_server_session_(quic_session) {
   quic_session.setCodecStats(stats);
   quic_session.setHttp3Options(http3_options);
   quic_session.setHeadersWithUnderscoreAction(headers_with_underscores_action);
   quic_session.setHttpConnectionCallbacks(callbacks);
+  quic_session.set_max_inbound_header_list_size(max_request_headers_kb * 1024u);
 }
 
 void QuicHttpServerConnectionImpl::onUnderlyingConnectionAboveWriteBufferHighWatermark() {
@@ -68,11 +69,12 @@ QuicHttpClientConnectionImpl::QuicHttpClientConnectionImpl(
     EnvoyQuicClientSession& session, Http::ConnectionCallbacks& callbacks,
     Http::Http3::CodecStats& stats,
     const envoy::config::core::v3::Http3ProtocolOptions& http3_options,
-    const uint32_t /*max_request_headers_kb*/)
+    const uint32_t max_request_headers_kb)
     : QuicHttpConnectionImplBase(session, stats), quic_client_session_(session) {
   session.setCodecStats(stats);
   session.setHttp3Options(http3_options);
   session.setHttpConnectionCallbacks(callbacks);
+  session.set_max_inbound_header_list_size(max_request_headers_kb * 1024);
 }
 
 Http::RequestEncoder&

source/common/quic/envoy_quic_client_connection.cc

@@ -42,12 +42,12 @@ EnvoyQuicClientConnection::EnvoyQuicClientConnection(
     quic::QuicAlarmFactory& alarm_factory, quic::QuicPacketWriter* writer, bool owns_writer,
     const quic::ParsedQuicVersionVector& supported_versions, Event::Dispatcher& dispatcher,
     Network::ConnectionSocketPtr&& connection_socket)
-    : EnvoyQuicConnection(server_connection_id, quic::QuicSocketAddress(),
-                          envoyIpAddressToQuicSocketAddress(
-                              connection_socket->addressProvider().remoteAddress()->ip()),
-                          helper, alarm_factory, writer, owns_writer, quic::Perspective::IS_CLIENT,
-                          supported_versions, std::move(connection_socket)),
-      dispatcher_(dispatcher) {}
+    : quic::QuicConnection(server_connection_id, quic::QuicSocketAddress(),
+                           envoyIpAddressToQuicSocketAddress(
+                               connection_socket->addressProvider().remoteAddress()->ip()),
+                           &helper, &alarm_factory, writer, owns_writer,
+                           quic::Perspective::IS_CLIENT, supported_versions),
+      QuicNetworkConnection(std::move(connection_socket)), dispatcher_(dispatcher) {}
 
 void EnvoyQuicClientConnection::processPacket(
     Network::Address::InstanceConstSharedPtr local_address,

source/common/quic/envoy_quic_client_connection.h

@@ -3,13 +3,28 @@
 #include "envoy/event/dispatcher.h"
 
 #include "common/network/utility.h"
-#include "common/quic/envoy_quic_connection.h"
+#include "common/quic/envoy_quic_utils.h"
+#include "common/quic/quic_network_connection.h"
+
+#if defined(__GNUC__)
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wunused-parameter"
+#pragma GCC diagnostic ignored "-Winvalid-offsetof"
+#endif
+
+#include "quiche/quic/core/quic_connection.h"
+
+#if defined(__GNUC__)
+#pragma GCC diagnostic pop
+#endif
 
 namespace Envoy {
 namespace Quic {
 
 // A client QuicConnection instance managing its own file events.
-class EnvoyQuicClientConnection : public EnvoyQuicConnection, public Network::UdpPacketProcessor {
+class EnvoyQuicClientConnection : public quic::QuicConnection,
+                                  public QuicNetworkConnection,
+                                  public Network::UdpPacketProcessor {
 public:
   // A connection socket will be created with given |local_addr|. If binding
   // port not provided in |local_addr|, pick up a random port.

source/common/quic/envoy_quic_client_session.cc

@@ -11,23 +11,21 @@ EnvoyQuicClientSession::EnvoyQuicClientSession(
     quic::QuicCryptoClientConfig* crypto_config,
     quic::QuicClientPushPromiseIndex* push_promise_index, Event::Dispatcher& dispatcher,
     uint32_t send_buffer_limit)
-    : QuicFilterManagerConnectionImpl(*connection, dispatcher, send_buffer_limit),
+    : QuicFilterManagerConnectionImpl(*connection, connection->connection_id(), dispatcher,
+                                      send_buffer_limit),
       quic::QuicSpdyClientSession(config, supported_versions, connection.release(), server_id,
                                   crypto_config, push_promise_index),
-      host_name_(server_id.host()) {
-  // HTTP/3 header limits should be configurable, but for now hard-code to Envoy defaults.
-  set_max_inbound_header_list_size(Http::DEFAULT_MAX_REQUEST_HEADERS_KB * 1000);
-}
+      host_name_(server_id.host()) {}
 
 EnvoyQuicClientSession::~EnvoyQuicClientSession() {
   ASSERT(!connection()->connected());
-  quic_connection_ = nullptr;
+  network_connection_ = nullptr;
 }
 
 absl::string_view EnvoyQuicClientSession::requestedServerName() const { return host_name_; }
 
 void EnvoyQuicClientSession::connect() {
-  dynamic_cast<EnvoyQuicClientConnection*>(quic_connection_)->setUpConnectionSocket();
+  dynamic_cast<EnvoyQuicClientConnection*>(network_connection_)->setUpConnectionSocket();
   // Start version negotiation and crypto handshake during which the connection may fail if server
   // doesn't support the one and only supported version.
   CryptoConnect();
@@ -41,7 +39,8 @@ void EnvoyQuicClientSession::OnConnectionClosed(const quic::QuicConnectionCloseF
 
 void EnvoyQuicClientSession::Initialize() {
   quic::QuicSpdyClientSession::Initialize();
-  quic_connection_->setEnvoyConnection(*this);
+  initialized_ = true;
+  network_connection_->setEnvoyConnection(*this);
 }
 
 void EnvoyQuicClientSession::OnCanWrite() {
@@ -102,6 +101,14 @@ EnvoyQuicClientSession::CreateIncomingStream(quic::PendingStream* /*pending*/) {
 
 bool EnvoyQuicClientSession::hasDataToWrite() { return HasDataToWrite(); }
 
+const quic::QuicConnection* EnvoyQuicClientSession::quicConnection() const {
+  return initialized_ ? connection() : nullptr;
+}
+
+quic::QuicConnection* EnvoyQuicClientSession::quicConnection() {
+  return initialized_ ? connection() : nullptr;
+}
+
 void EnvoyQuicClientSession::OnTlsHandshakeComplete() {
   raiseConnectionEvent(Network::ConnectionEvent::Connected);
 }

source/common/quic/envoy_quic_client_session.h

@@ -77,6 +77,9 @@ class EnvoyQuicClientSession : public QuicFilterManagerConnectionImpl,
 
   // QuicFilterManagerConnectionImpl
   bool hasDataToWrite() override;
+  // Used by base class to access quic connection after initialization.
+  const quic::QuicConnection* quicConnection() const override;
+  quic::QuicConnection* quicConnection() override;
 
 private:
   // These callbacks are owned by network filters and quic session should outlive