From 9fc68128185a52dbbe43a5cc47f0893aa398618f Mon Sep 17 00:00:00 2001 From: Dan Zhang Date: Fri, 19 Sep 2025 15:27:23 +0000 Subject: [PATCH 1/5] Reapply "response decoder: add new interfaces for life time tracking of response decoder (#41048)" This reverts commit bfd5b101b1e21c296743cba4783f5c23ace03846. Signed-off-by: Dan Zhang --- changelogs/current.yaml | 5 + envoy/http/codec.h | 24 ++++ envoy/router/BUILD | 1 + envoy/router/router.h | 3 +- source/common/http/BUILD | 13 ++- source/common/http/codec_wrappers.h | 68 +++++++++-- source/common/http/http1/conn_pool.cc | 4 +- source/common/http/http1/conn_pool.h | 4 +- .../common/http/response_decoder_impl_base.h | 40 +++++++ .../common/quic/envoy_quic_client_stream.cc | 67 +++++++++-- source/common/quic/envoy_quic_client_stream.h | 9 +- source/common/runtime/runtime_features.cc | 4 +- source/common/tcp_proxy/BUILD | 1 + source/common/tcp_proxy/upstream.h | 6 +- .../downstream_socket_interface/BUILD | 1 + .../rc_connection_wrapper.h | 3 +- source/extensions/filters/udp/udp_proxy/BUILD | 1 + .../filters/udp/udp_proxy/udp_proxy_filter.h | 3 +- source/extensions/health_checkers/grpc/BUILD | 1 + .../grpc/health_checker_impl.h | 3 +- source/extensions/health_checkers/http/BUILD | 1 + .../http/health_checker_impl.h | 3 +- test/common/http/BUILD | 1 + test/common/http/codec_wrappers_test.cc | 46 ++++++-- test/common/quic/BUILD | 1 + .../quic/envoy_quic_client_stream_test.cc | 110 ++++++++++++++++++ test/integration/integration_stream_decoder.h | 4 +- test/integration/utility.h | 3 +- test/mocks/http/BUILD | 1 + test/mocks/http/stream_decoder.h | 4 +- tools/spelling/spelling_dictionary.txt | 2 + 31 files changed, 395 insertions(+), 42 deletions(-) create mode 100644 source/common/http/response_decoder_impl_base.h diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 542b2f0e5e726..a43406043daa9 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -1,6 +1,11 @@ date: Pending behavior_changes: +- area: response_decoder + change: | + Updated EnvoyQuicClientStream and ResponseDecoderWrapper to use a handle to access the response decoder + to prevent use-after-free errors by ensuring the decoder instance is still live before calling its methods. + This change is guarded by the runtime flag ``envoy.reloadable_features.use_response_decoder_handle``. - area: http change: | A route refresh will now result in a tracing refresh. The trace sampling decision and decoration diff --git a/envoy/http/codec.h b/envoy/http/codec.h index 3c23ea86b2636..bc7c45cacf0c3 100644 --- a/envoy/http/codec.h +++ b/envoy/http/codec.h @@ -198,6 +198,23 @@ class ResponseEncoder : public virtual StreamEncoder { StreamInfo::StreamInfo& stream_info) PURE; }; +class ResponseDecoder; + +/** + * A handle to a ResponseDecoder. This handle can be used to check if the underlying decoder is + * still valid and to get a reference to it. + */ +class ResponseDecoderHandle { +public: + virtual ~ResponseDecoderHandle() = default; + + /** + * @return a reference to the underlying decoder if it is still valid. + */ + virtual OptRef get() PURE; +}; +using ResponseDecoderHandlePtr = std::unique_ptr; + /** * Decodes an HTTP stream. These are callbacks fired into a sink. This interface contains methods * common to both the request and response path. @@ -304,9 +321,16 @@ class ResponseDecoder : public virtual StreamDecoder { * @param os the ostream to dump state to * @param indent_level the depth, for pretty-printing. * + * This function is called on Envoy fatal errors so should avoid memory allocation. */ virtual void dumpState(std::ostream& os, int indent_level = 0) const PURE; + + /** + * @return A handle to the response decoder. Caller can check the response decoder's liveness via + * the handle. + */ + virtual ResponseDecoderHandlePtr createResponseDecoderHandle() PURE; }; /** diff --git a/envoy/router/BUILD b/envoy/router/BUILD index 33af45f78fe55..47dc4cbebcbb4 100644 --- a/envoy/router/BUILD +++ b/envoy/router/BUILD @@ -95,6 +95,7 @@ envoy_cc_library( "//envoy/tracing:tracer_interface", "//envoy/upstream:resource_manager_interface", "//envoy/upstream:retry_interface", + "//source/common/http:response_decoder_impl_base", "//source/common/protobuf", "//source/common/protobuf:utility_lib", "@com_google_absl//absl/types:optional", diff --git a/envoy/router/router.h b/envoy/router/router.h index ea2b1fae6a6fd..6cb2aff882035 100644 --- a/envoy/router/router.h +++ b/envoy/router/router.h @@ -27,6 +27,7 @@ #include "envoy/upstream/resource_manager.h" #include "envoy/upstream/retry.h" +#include "source/common/http/response_decoder_impl_base.h" #include "source/common/protobuf/protobuf.h" #include "source/common/protobuf/utility.h" @@ -1493,7 +1494,7 @@ class GenericConnPool { * An API for the interactions the upstream stream needs to have with the downstream stream * and/or router components */ -class UpstreamToDownstream : public Http::ResponseDecoder, public Http::StreamCallbacks { +class UpstreamToDownstream : public Http::ResponseDecoderImplBase, public Http::StreamCallbacks { public: /** * @return return the route for the downstream stream. diff --git a/source/common/http/BUILD b/source/common/http/BUILD index 7e1002a8080d4..e2385f30ccb58 100644 --- a/source/common/http/BUILD +++ b/source/common/http/BUILD @@ -132,7 +132,18 @@ envoy_cc_library( envoy_cc_library( name = "codec_wrappers_lib", hdrs = ["codec_wrappers.h"], - deps = ["//envoy/http:codec_interface"], + deps = [ + ":response_decoder_impl_base", + "//envoy/http:codec_interface", + ], +) + +envoy_cc_library( + name = "response_decoder_impl_base", + hdrs = ["response_decoder_impl_base.h"], + deps = [ + "//envoy/http:codec_interface", + ], ) envoy_cc_library( diff --git a/source/common/http/codec_wrappers.h b/source/common/http/codec_wrappers.h index 7b98413e1a5a1..1494df392b06e 100644 --- a/source/common/http/codec_wrappers.h +++ b/source/common/http/codec_wrappers.h @@ -2,24 +2,35 @@ #include "envoy/http/codec.h" +#include "source/common/http/response_decoder_impl_base.h" +#include "source/common/runtime/runtime_features.h" + namespace Envoy { namespace Http { /** * Wrapper for ResponseDecoder that just forwards to an "inner" decoder. */ -class ResponseDecoderWrapper : public ResponseDecoder { +class ResponseDecoderWrapper : public ResponseDecoderImplBase { public: // ResponseDecoder void decode1xxHeaders(ResponseHeaderMapPtr&& headers) override { - inner_.decode1xxHeaders(std::move(headers)); + if (Http::ResponseDecoder* inner = getInnerDecoder()) { + inner->decode1xxHeaders(std::move(headers)); + } else { + onInnerDecoderDead(); + } } void decodeHeaders(ResponseHeaderMapPtr&& headers, bool end_stream) override { if (end_stream) { onPreDecodeComplete(); } - inner_.decodeHeaders(std::move(headers), end_stream); + if (Http::ResponseDecoder* inner = getInnerDecoder()) { + inner->decodeHeaders(std::move(headers), end_stream); + } else { + onInnerDecoderDead(); + } if (end_stream) { onDecodeComplete(); } @@ -29,7 +40,11 @@ class ResponseDecoderWrapper : public ResponseDecoder { if (end_stream) { onPreDecodeComplete(); } - inner_.decodeData(data, end_stream); + if (Http::ResponseDecoder* inner = getInnerDecoder()) { + inner->decodeData(data, end_stream); + } else { + onInnerDecoderDead(); + } if (end_stream) { onDecodeComplete(); } @@ -37,20 +52,33 @@ class ResponseDecoderWrapper : public ResponseDecoder { void decodeTrailers(ResponseTrailerMapPtr&& trailers) override { onPreDecodeComplete(); - inner_.decodeTrailers(std::move(trailers)); + if (Http::ResponseDecoder* inner = getInnerDecoder()) { + inner->decodeTrailers(std::move(trailers)); + } else { + onInnerDecoderDead(); + } onDecodeComplete(); } void decodeMetadata(MetadataMapPtr&& metadata_map) override { - inner_.decodeMetadata(std::move(metadata_map)); + if (Http::ResponseDecoder* inner = getInnerDecoder()) { + inner->decodeMetadata(std::move(metadata_map)); + } else { + onInnerDecoderDead(); + } } void dumpState(std::ostream& os, int indent_level) const override { - inner_.dumpState(os, indent_level); + if (Http::ResponseDecoder* inner = getInnerDecoder()) { + inner->dumpState(os, indent_level); + } else { + onInnerDecoderDead(); + } } protected: - ResponseDecoderWrapper(ResponseDecoder& inner) : inner_(inner) {} + ResponseDecoderWrapper(ResponseDecoder& inner) + : inner_handle_(inner.createResponseDecoderHandle()), inner_(&inner) {} /** * Consumers of the wrapper generally want to know when a decode is complete. This is called @@ -59,7 +87,29 @@ class ResponseDecoderWrapper : public ResponseDecoder { virtual void onPreDecodeComplete() PURE; virtual void onDecodeComplete() PURE; - ResponseDecoder& inner_; + ResponseDecoderHandlePtr inner_handle_; + Http::ResponseDecoder* inner_ = nullptr; + +private: + Http::ResponseDecoder* getInnerDecoder() const { + if (!Runtime::runtimeFeatureEnabled("envoy.reloadable_features.use_response_decoder_handle")) { + return inner_; + } + if (inner_handle_) { + if (OptRef inner = inner_handle_->get(); inner.has_value()) { + return &inner.value().get(); + } + } + return nullptr; + } + + void onInnerDecoderDead() const { + const std::string error_msg = "Wrapped decoder use after free detected."; + IS_ENVOY_BUG(error_msg); + RELEASE_ASSERT(!Runtime::runtimeFeatureEnabled( + "envoy.reloadable_features.abort_when_accessing_dead_decoder"), + error_msg); + } }; /** diff --git a/source/common/http/http1/conn_pool.cc b/source/common/http/http1/conn_pool.cc index 99aea9e453b2d..5877a035f041d 100644 --- a/source/common/http/http1/conn_pool.cc +++ b/source/common/http/http1/conn_pool.cc @@ -25,8 +25,8 @@ namespace Http { namespace Http1 { ActiveClient::StreamWrapper::StreamWrapper(ResponseDecoder& response_decoder, ActiveClient& parent) - : RequestEncoderWrapper(&parent.codec_client_->newStream(*this)), - ResponseDecoderWrapper(response_decoder), parent_(parent) { + : ResponseDecoderWrapper(response_decoder), + RequestEncoderWrapper(&parent.codec_client_->newStream(*this)), parent_(parent) { RequestEncoderWrapper::inner_encoder_->getStream().addCallbacks(*this); } diff --git a/source/common/http/http1/conn_pool.h b/source/common/http/http1/conn_pool.h index 7bdeb288e6aa4..b8b1fa803dfca 100644 --- a/source/common/http/http1/conn_pool.h +++ b/source/common/http/http1/conn_pool.h @@ -36,8 +36,8 @@ class ActiveClient : public Envoy::Http::ActiveClient { Envoy::Http::ActiveClient::releaseResources(); } - struct StreamWrapper : public RequestEncoderWrapper, - public ResponseDecoderWrapper, + struct StreamWrapper : public ResponseDecoderWrapper, + public RequestEncoderWrapper, public StreamCallbacks, public Event::DeferredDeletable, protected Logger::Loggable { diff --git a/source/common/http/response_decoder_impl_base.h b/source/common/http/response_decoder_impl_base.h new file mode 100644 index 0000000000000..694c2b7a6af24 --- /dev/null +++ b/source/common/http/response_decoder_impl_base.h @@ -0,0 +1,40 @@ +#pragma once + +#include + +#include "envoy/http/codec.h" + +namespace Envoy { +namespace Http { + +class ResponseDecoderHandleImpl : public ResponseDecoderHandle { +public: + ResponseDecoderHandleImpl(std::weak_ptr live_trackable, ResponseDecoder& decoder) + : live_trackable_(live_trackable), decoder_(decoder) {} + + OptRef get() override { + if (live_trackable_.lock()) { + return decoder_; + } + return {}; + } + +private: + std::weak_ptr live_trackable_; + ResponseDecoder& decoder_; +}; + +class ResponseDecoderImplBase : public ResponseDecoder { +public: + ResponseDecoderImplBase() : live_trackable_(std::make_shared(true)) {} + + ResponseDecoderHandlePtr createResponseDecoderHandle() override { + return std::make_unique(live_trackable_, *this); + } + +private: + std::shared_ptr live_trackable_; +}; + +} // namespace Http +} // namespace Envoy diff --git a/source/common/quic/envoy_quic_client_stream.cc b/source/common/quic/envoy_quic_client_stream.cc index 53773d3e33107..f6506a7fb33e1 100644 --- a/source/common/quic/envoy_quic_client_stream.cc +++ b/source/common/quic/envoy_quic_client_stream.cc @@ -36,6 +36,11 @@ EnvoyQuicClientStream::EnvoyQuicClientStream( RegisterMetadataVisitor(this); } +void EnvoyQuicClientStream::setResponseDecoder(Http::ResponseDecoder& decoder) { + response_decoder_handle_ = decoder.createResponseDecoderHandle(); + response_decoder_ = &decoder; +} + Http::Status EnvoyQuicClientStream::encodeHeaders(const Http::RequestHeaderMap& headers, bool end_stream) { ENVOY_STREAM_LOG(debug, "encodeHeaders: (end_stream={}) {}.", *this, end_stream, headers); @@ -207,7 +212,11 @@ void EnvoyQuicClientStream::OnInitialHeadersComplete(bool fin, size_t frame_len, if (!optional_status.has_value()) { // In case the status is invalid or missing, the response_decoder_.decodeHeaders() will fail the // request - response_decoder_->decodeHeaders(std::move(headers), fin); + if (Http::ResponseDecoder* decoder = getResponseDecoder()) { + decoder->decodeHeaders(std::move(headers), fin); + } else { + onResponseDecoderDead(); + } ConsumeHeaderList(); return; } @@ -224,10 +233,18 @@ void EnvoyQuicClientStream::OnInitialHeadersComplete(bool fin, size_t frame_len, if (is_special_1xx && !decoded_1xx_) { // This is 100 Continue, only decode it once to support Expect:100-Continue header. decoded_1xx_ = true; - response_decoder_->decode1xxHeaders(std::move(headers)); + if (Http::ResponseDecoder* decoder = getResponseDecoder()) { + decoder->decode1xxHeaders(std::move(headers)); + } else { + onResponseDecoderDead(); + } } else if (!is_special_1xx) { - response_decoder_->decodeHeaders(std::move(headers), - /*end_stream=*/fin); + if (Http::ResponseDecoder* decoder = getResponseDecoder()) { + decoder->decodeHeaders(std::move(headers), + /*end_stream=*/fin); + } else { + onResponseDecoderDead(); + } if (status == enumToInt(Http::Code::NotModified)) { got_304_response_ = true; } @@ -301,7 +318,11 @@ void EnvoyQuicClientStream::OnBodyAvailable() { // A stream error has occurred, stop processing. return; } - response_decoder_->decodeData(*buffer, fin_read_and_no_trailers); + if (Http::ResponseDecoder* decoder = getResponseDecoder()) { + decoder->decodeData(*buffer, fin_read_and_no_trailers); + } else { + onResponseDecoderDead(); + } } if (!sequencer()->IsClosed() || read_side_closed()) { @@ -348,7 +369,11 @@ void EnvoyQuicClientStream::maybeDecodeTrailers() { onStreamError(close_connection_upon_invalid_header_, transform_rst); return; } - response_decoder_->decodeTrailers(std::move(trailers)); + if (Http::ResponseDecoder* decoder = getResponseDecoder()) { + decoder->decodeTrailers(std::move(trailers)); + } else { + onResponseDecoderDead(); + } MarkTrailersConsumed(); } } @@ -432,10 +457,15 @@ void EnvoyQuicClientStream::OnMetadataComplete(size_t /*frame_len*/, const quic::QuicHeaderList& header_list) { if (mustRejectMetadata(header_list.uncompressed_header_bytes())) { onStreamError(true, quic::QUIC_HEADERS_TOO_LARGE); + return; } if (!header_list.empty()) { - response_decoder_->decodeMetadata(metadataMapFromHeaderList(header_list)); + if (Http::ResponseDecoder* decoder = getResponseDecoder()) { + decoder->decodeMetadata(metadataMapFromHeaderList(header_list)); + } else { + onResponseDecoderDead(); + } } } @@ -466,7 +496,7 @@ bool EnvoyQuicClientStream::hasPendingData() { return BufferedDataBytes() > 0; } // connect-udp". void EnvoyQuicClientStream::useCapsuleProtocol() { http_datagram_handler_ = std::make_unique(*this); - http_datagram_handler_->setStreamDecoder(response_decoder_); + http_datagram_handler_->setStreamDecoder(getResponseDecoder()); RegisterHttp3DatagramVisitor(http_datagram_handler_.get()); } #endif @@ -475,5 +505,26 @@ void EnvoyQuicClientStream::OnInvalidHeaders() { onStreamError(absl::nullopt, quic::QUIC_BAD_APPLICATION_PAYLOAD); } +void EnvoyQuicClientStream::onResponseDecoderDead() const { + const std::string error_msg = "response_decoder_ use after free detected."; + IS_ENVOY_BUG(error_msg); + RELEASE_ASSERT(!Runtime::runtimeFeatureEnabled( + "envoy.reloadable_features.abort_when_accessing_dead_decoder"), + error_msg); +} + +Http::ResponseDecoder* EnvoyQuicClientStream::getResponseDecoder() { + if (!Runtime::runtimeFeatureEnabled("envoy.reloadable_features.use_response_decoder_handle")) { + return response_decoder_; + } + if (response_decoder_handle_) { + if (OptRef decoder = response_decoder_handle_->get(); + decoder.has_value()) { + return &decoder.value().get(); + } + } + return nullptr; +} + } // namespace Quic } // namespace Envoy diff --git a/source/common/quic/envoy_quic_client_stream.h b/source/common/quic/envoy_quic_client_stream.h index 67214eb9741f2..5352f2b92bea5 100644 --- a/source/common/quic/envoy_quic_client_stream.h +++ b/source/common/quic/envoy_quic_client_stream.h @@ -3,6 +3,7 @@ #include "envoy/buffer/buffer.h" #include "source/common/quic/envoy_quic_stream.h" +#include "source/common/runtime/runtime_features.h" #ifdef ENVOY_ENABLE_HTTP_DATAGRAMS #include "source/common/quic/http_datagram_handler.h" @@ -25,7 +26,7 @@ class EnvoyQuicClientStream : public quic::QuicSpdyClientStream, quic::StreamType type, Http::Http3::CodecStats& stats, const envoy::config::core::v3::Http3ProtocolOptions& http3_options); - void setResponseDecoder(Http::ResponseDecoder& decoder) { response_decoder_ = &decoder; } + void setResponseDecoder(Http::ResponseDecoder& decoder); // Http::StreamEncoder Http::Http1StreamEncoderOptionsOptRef http1StreamEncoderOptions() override { @@ -92,6 +93,12 @@ class EnvoyQuicClientStream : public quic::QuicSpdyClientStream, void useCapsuleProtocol(); #endif + // Returns nullptr if the response decoder has already been destructed. + Http::ResponseDecoder* getResponseDecoder(); + + void onResponseDecoderDead() const; + + Http::ResponseDecoderHandlePtr response_decoder_handle_; Http::ResponseDecoder* response_decoder_{nullptr}; bool decoded_1xx_{false}; diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index aa71150452972..038d29d2198c4 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -81,6 +81,7 @@ RUNTIME_GUARD(envoy_reloadable_features_trace_refresh_after_route_refresh); RUNTIME_GUARD(envoy_reloadable_features_udp_set_do_not_fragment); RUNTIME_GUARD(envoy_reloadable_features_uhv_allow_malformed_url_encoding); RUNTIME_GUARD(envoy_reloadable_features_uri_template_match_on_asterisk); +RUNTIME_GUARD(envoy_reloadable_features_use_response_decoder_handle); RUNTIME_GUARD(envoy_reloadable_features_validate_connect); RUNTIME_GUARD(envoy_reloadable_features_validate_upstream_headers); RUNTIME_GUARD(envoy_reloadable_features_websocket_allow_4xx_5xx_through_filter_chain); @@ -174,7 +175,8 @@ FALSE_RUNTIME_GUARD(envoy_reloadable_features_getaddrinfo_no_ai_flags); FALSE_RUNTIME_GUARD(envoy_reloadable_features_enable_new_dns_implementation); // Force a local reply from upstream envoy for reverse connections. FALSE_RUNTIME_GUARD(envoy_reloadable_features_reverse_conn_force_local_reply); - +// RELEASE_ASSERT when upstream stream detects UAF of downstream response decoder instance. +FALSE_RUNTIME_GUARD(envoy_reloadable_features_abort_when_accessing_dead_decoder); // Block of non-boolean flags. Use of int flags is deprecated. Do not add more. ABSL_FLAG(uint64_t, re2_max_program_size_error_level, 100, ""); // NOLINT ABSL_FLAG(uint64_t, re2_max_program_size_warn_level, // NOLINT diff --git a/source/common/tcp_proxy/BUILD b/source/common/tcp_proxy/BUILD index c6b30b08d03d1..914382ab15f81 100644 --- a/source/common/tcp_proxy/BUILD +++ b/source/common/tcp_proxy/BUILD @@ -29,6 +29,7 @@ envoy_cc_library( "//source/common/http:hash_policy_lib", "//source/common/http:header_map_lib", "//source/common/http:headers_lib", + "//source/common/http:response_decoder_impl_base", "//source/common/http:utility_lib", "//source/common/network:utility_lib", "//source/common/router:header_parser_lib", diff --git a/source/common/tcp_proxy/upstream.h b/source/common/tcp_proxy/upstream.h index e6932574b3abf..2fdf1bd373f56 100644 --- a/source/common/tcp_proxy/upstream.h +++ b/source/common/tcp_proxy/upstream.h @@ -17,6 +17,7 @@ #include "source/common/http/codec_client.h" #include "source/common/http/hash_policy.h" #include "source/common/http/null_route_impl.h" +#include "source/common/http/response_decoder_impl_base.h" #include "source/common/network/utility.h" #include "source/common/router/config_impl.h" #include "source/common/router/header_parser.h" @@ -230,8 +231,7 @@ class HttpUpstream : public GenericUpstream, protected Http::StreamCallbacks { std::unique_ptr downstream_headers_; private: - Upstream::ClusterInfoConstSharedPtr cluster_; - class DecoderShim : public Http::ResponseDecoder { + class DecoderShim : public Http::ResponseDecoderImplBase { public: DecoderShim(HttpUpstream& parent) : parent_(parent) {} void decode1xxHeaders(Http::ResponseHeaderMapPtr&&) override {} @@ -353,7 +353,7 @@ class CombinedUpstream : public GenericUpstream, public Envoy::Router::RouterFil private: Http::StreamDecoderFilterCallbacks& decoder_filter_callbacks_; - class DecoderShim : public Http::ResponseDecoder { + class DecoderShim : public Http::ResponseDecoderImplBase { public: DecoderShim(CombinedUpstream& parent) : parent_(parent) {} // Http::ResponseDecoder diff --git a/source/extensions/bootstrap/reverse_tunnel/downstream_socket_interface/BUILD b/source/extensions/bootstrap/reverse_tunnel/downstream_socket_interface/BUILD index cef2da998723f..62257e221fcc2 100644 --- a/source/extensions/bootstrap/reverse_tunnel/downstream_socket_interface/BUILD +++ b/source/extensions/bootstrap/reverse_tunnel/downstream_socket_interface/BUILD @@ -77,6 +77,7 @@ envoy_cc_library( "//source/common/common:logger_lib", "//source/common/grpc:typed_async_client_lib", "//source/common/http:codec_client_lib", + "//source/common/http:response_decoder_impl_base", "//source/common/http/http1:codec_lib", "//source/common/http/http1:codec_stats_lib", "//source/common/network:address_lib", diff --git a/source/extensions/bootstrap/reverse_tunnel/downstream_socket_interface/rc_connection_wrapper.h b/source/extensions/bootstrap/reverse_tunnel/downstream_socket_interface/rc_connection_wrapper.h index 68828336a43d7..23dbacb1db6c0 100644 --- a/source/extensions/bootstrap/reverse_tunnel/downstream_socket_interface/rc_connection_wrapper.h +++ b/source/extensions/bootstrap/reverse_tunnel/downstream_socket_interface/rc_connection_wrapper.h @@ -12,6 +12,7 @@ #include "source/common/common/logger.h" #include "source/common/http/http1/codec_impl.h" +#include "source/common/http/response_decoder_impl_base.h" #include "source/common/network/filter_impl.h" namespace Envoy { @@ -49,7 +50,7 @@ class SimpleConnReadFilter : public Network::ReadFilterBaseImpl, class RCConnectionWrapper : public Network::ConnectionCallbacks, public Event::DeferredDeletable, public Logger::Loggable, - public Http::ResponseDecoder, + public Http::ResponseDecoderImplBase, public Http::ConnectionCallbacks { friend class SimpleConnReadFilterTest; diff --git a/source/extensions/filters/udp/udp_proxy/BUILD b/source/extensions/filters/udp/udp_proxy/BUILD index a9e79751a3afc..9e60d1f00f3f0 100644 --- a/source/extensions/filters/udp/udp_proxy/BUILD +++ b/source/extensions/filters/udp/udp_proxy/BUILD @@ -42,6 +42,7 @@ envoy_cc_library( "//source/common/common:empty_string", "//source/common/common:linked_object", "//source/common/common:random_generator_lib", + "//source/common/http:response_decoder_impl_base", "//source/common/network:socket_lib", "//source/common/network:socket_option_factory_lib", "//source/common/network:utility_lib", diff --git a/source/extensions/filters/udp/udp_proxy/udp_proxy_filter.h b/source/extensions/filters/udp/udp_proxy/udp_proxy_filter.h index f2b6f64606fa9..c1c921375f269 100644 --- a/source/extensions/filters/udp/udp_proxy/udp_proxy_filter.h +++ b/source/extensions/filters/udp/udp_proxy/udp_proxy_filter.h @@ -21,6 +21,7 @@ #include "source/common/http/codes.h" #include "source/common/http/header_map_impl.h" #include "source/common/http/headers.h" +#include "source/common/http/response_decoder_impl_base.h" #include "source/common/http/utility.h" #include "source/common/network/socket_impl.h" #include "source/common/network/socket_interface.h" @@ -311,7 +312,7 @@ class HttpUpstreamImpl : public HttpUpstream, protected Http::StreamCallbacks { } private: - class ResponseDecoder : public Http::ResponseDecoder { + class ResponseDecoder : public Http::ResponseDecoderImplBase { public: ResponseDecoder(HttpUpstreamImpl& parent) : parent_(parent) {} diff --git a/source/extensions/health_checkers/grpc/BUILD b/source/extensions/health_checkers/grpc/BUILD index c06822d4e75c9..b3688169d1f2d 100644 --- a/source/extensions/health_checkers/grpc/BUILD +++ b/source/extensions/health_checkers/grpc/BUILD @@ -20,6 +20,7 @@ envoy_cc_extension( deps = [ "//source/common/grpc:codec_lib", "//source/common/http:codec_client_lib", + "//source/common/http:response_decoder_impl_base", "//source/common/upstream:health_checker_lib", "//source/common/upstream:host_utility_lib", "//source/extensions/health_checkers/common:health_checker_base_lib", diff --git a/source/extensions/health_checkers/grpc/health_checker_impl.h b/source/extensions/health_checkers/grpc/health_checker_impl.h index 326035eac59b1..a3e54bb454792 100644 --- a/source/extensions/health_checkers/grpc/health_checker_impl.h +++ b/source/extensions/health_checkers/grpc/health_checker_impl.h @@ -17,6 +17,7 @@ #include "source/common/common/logger.h" #include "source/common/grpc/codec.h" #include "source/common/http/codec_client.h" +#include "source/common/http/response_decoder_impl_base.h" #include "source/common/router/header_parser.h" #include "source/common/stream_info/stream_info_impl.h" #include "source/common/upstream/health_checker_impl.h" @@ -52,7 +53,7 @@ class GrpcHealthCheckerImpl : public HealthCheckerImplBase { private: struct GrpcActiveHealthCheckSession : public ActiveHealthCheckSession, - public Http::ResponseDecoder, + public Http::ResponseDecoderImplBase, public Http::StreamCallbacks { GrpcActiveHealthCheckSession(GrpcHealthCheckerImpl& parent, const HostSharedPtr& host); ~GrpcActiveHealthCheckSession() override; diff --git a/source/extensions/health_checkers/http/BUILD b/source/extensions/health_checkers/http/BUILD index 6ad5ed7773564..b759a789d7568 100644 --- a/source/extensions/health_checkers/http/BUILD +++ b/source/extensions/health_checkers/http/BUILD @@ -18,6 +18,7 @@ envoy_cc_extension( ], deps = [ "//source/common/http:codec_client_lib", + "//source/common/http:response_decoder_impl_base", "//source/common/upstream:health_checker_lib", "//source/common/upstream:host_utility_lib", "//source/extensions/health_checkers/common:health_checker_base_lib", diff --git a/source/extensions/health_checkers/http/health_checker_impl.h b/source/extensions/health_checkers/http/health_checker_impl.h index 81fae53f73330..68fd6214f9166 100644 --- a/source/extensions/health_checkers/http/health_checker_impl.h +++ b/source/extensions/health_checkers/http/health_checker_impl.h @@ -17,6 +17,7 @@ #include "source/common/common/logger.h" #include "source/common/grpc/codec.h" #include "source/common/http/codec_client.h" +#include "source/common/http/response_decoder_impl_base.h" #include "source/common/router/header_parser.h" #include "source/common/stream_info/stream_info_impl.h" #include "source/common/upstream/health_checker_impl.h" @@ -77,7 +78,7 @@ class HttpHealthCheckerImpl : public HealthCheckerImplBase { private: struct HttpActiveHealthCheckSession : public ActiveHealthCheckSession, - public Http::ResponseDecoder, + public Http::ResponseDecoderImplBase, public Http::StreamCallbacks { HttpActiveHealthCheckSession(HttpHealthCheckerImpl& parent, const HostSharedPtr& host); ~HttpActiveHealthCheckSession() override; diff --git a/test/common/http/BUILD b/test/common/http/BUILD index 3c522bcda3803..97f77b1eb390e 100644 --- a/test/common/http/BUILD +++ b/test/common/http/BUILD @@ -162,6 +162,7 @@ envoy_cc_test( deps = [ "//source/common/http:codec_wrappers_lib", "//test/mocks/http:http_mocks", + "//test/test_common:test_runtime_lib", "//test/test_common:utility_lib", ], ) diff --git a/test/common/http/codec_wrappers_test.cc b/test/common/http/codec_wrappers_test.cc index 35bf742a03f02..e5ffb0acdd615 100644 --- a/test/common/http/codec_wrappers_test.cc +++ b/test/common/http/codec_wrappers_test.cc @@ -1,6 +1,7 @@ #include "source/common/http/codec_wrappers.h" #include "test/mocks/http/mocks.h" +#include "test/test_common/test_runtime.h" #include "test/test_common/utility.h" using testing::_; @@ -10,23 +11,54 @@ namespace Http { class MockResponseDecoderWrapper : public ResponseDecoderWrapper { public: - MockResponseDecoderWrapper() : ResponseDecoderWrapper(inner_decoder_) {} - MockResponseDecoder& innerEncoder() { return inner_decoder_; } + explicit MockResponseDecoderWrapper(MockResponseDecoder& inner_decoder) + : ResponseDecoderWrapper(inner_decoder) {} void onDecodeComplete() override {} void onPreDecodeComplete() override {} - -private: - MockResponseDecoder inner_decoder_; }; TEST(MockResponseDecoderWrapper, dumpState) { - MockResponseDecoderWrapper wrapper; + MockResponseDecoder inner_decoder; + MockResponseDecoderWrapper wrapper(inner_decoder); std::stringstream os; - EXPECT_CALL(wrapper.innerEncoder(), dumpState(_, _)); + EXPECT_CALL(inner_decoder, dumpState(_, _)); wrapper.dumpState(os, 0); } +TEST(MockResponseDecoderWrapper, decoderDestroyedBeforeDecoding) { + TestScopedRuntime runtime; + runtime.mergeValues({{"envoy.reloadable_features.abort_when_accessing_dead_decoder", "false"}}); + auto inner_decoder = std::make_unique(); + MockResponseDecoderWrapper wrapper(*inner_decoder); + + inner_decoder.reset(); + + EXPECT_ENVOY_BUG( + wrapper.decodeHeaders(ResponseHeaderMapPtr{new TestResponseHeaderMapImpl{{":status", "200"}}}, + true), + "Wrapped decoder use after free detected"); + + EXPECT_ENVOY_BUG(wrapper.decode1xxHeaders( + ResponseHeaderMapPtr{new TestResponseHeaderMapImpl{{":status", "100"}}}), + "Wrapped decoder use after free detected"); + + Buffer::OwnedImpl data("foo"); + EXPECT_ENVOY_BUG(wrapper.decodeData(data, true), "Wrapped decoder use after free detected"); + + EXPECT_ENVOY_BUG(wrapper.decodeTrailers( + ResponseTrailerMapPtr{new TestResponseTrailerMapImpl{{"key", "value"}}}), + "Wrapped decoder use after free detected"); + + MetadataMapPtr metadata = std::make_unique(); + (*metadata)["key1"] = "value1"; + EXPECT_ENVOY_BUG(wrapper.decodeMetadata(std::move(metadata)), + "Wrapped decoder use after free detected"); + + std::stringstream os; + EXPECT_ENVOY_BUG(wrapper.dumpState(os, 0), "Wrapped decoder use after free detected"); +} + class MockRequestEncoderWrapper : public RequestEncoderWrapper { public: MockRequestEncoderWrapper() : RequestEncoderWrapper(&inner_encoder_) {} diff --git a/test/common/quic/BUILD b/test/common/quic/BUILD index bc61e050ff7d6..5a2202a9921a8 100644 --- a/test/common/quic/BUILD +++ b/test/common/quic/BUILD @@ -152,6 +152,7 @@ envoy_cc_test( "//test/mocks/http:http_mocks", "//test/mocks/http:stream_decoder_mock", "//test/mocks/network:network_mocks", + "//test/test_common:test_runtime_lib", "//test/test_common:utility_lib", "@com_github_google_quiche//:quic_core_http_spdy_session_lib", "@com_github_google_quiche//:quic_test_tools_qpack_qpack_test_utils_lib", diff --git a/test/common/quic/envoy_quic_client_stream_test.cc b/test/common/quic/envoy_quic_client_stream_test.cc index 473dd22af5e05..4522e5d788ff0 100644 --- a/test/common/quic/envoy_quic_client_stream_test.cc +++ b/test/common/quic/envoy_quic_client_stream_test.cc @@ -8,6 +8,7 @@ #include "test/mocks/http/mocks.h" #include "test/mocks/http/stream_decoder.h" #include "test/mocks/network/mocks.h" +#include "test/test_common/test_runtime.h" #include "test/test_common/utility.h" #include "gmock/gmock.h" @@ -741,6 +742,115 @@ TEST_F(EnvoyQuicClientStreamTest, EncodeTrailersOnClosedStream) { EXPECT_EQ(0u, quic_session_.bytesToSend()); } +TEST_F(EnvoyQuicClientStreamTest, DecoderDestroyedBeforeDecoding1xxHeader) { + TestScopedRuntime runtime; + runtime.mergeValues({{"envoy.reloadable_features.abort_when_accessing_dead_decoder", "false"}}); + auto stream_decoder = std::make_unique(); + quic_stream_->setResponseDecoder(*stream_decoder); + + auto result = quic_stream_->encodeHeaders(request_headers_, true); + EXPECT_TRUE(result.ok()); + + // Destroy the mock decoder. + stream_decoder.reset(); + + quiche::HttpHeaderBlock continue_header; + continue_header[":status"] = "100"; + std::string headers = spdyHeaderToHttp3StreamPayload(continue_header); + quic::QuicStreamFrame frame1(stream_id_, /*fin*/ false, /*offset*/ 0, headers); + EXPECT_ENVOY_BUG(quic_stream_->OnStreamFrame(frame1), + "response_decoder_ use after free detected"); + + EXPECT_CALL(stream_callbacks_, + onResetStream(Http::StreamResetReason::LocalRefusedStreamReset, _)); + quic_stream_->resetStream(Http::StreamResetReason::LocalRefusedStreamReset); +} + +TEST_F(EnvoyQuicClientStreamTest, DecoderDestroyedBeforeDecodingHeader) { + TestScopedRuntime runtime; + runtime.mergeValues({{"envoy.reloadable_features.abort_when_accessing_dead_decoder", "false"}}); + auto stream_decoder = std::make_unique(); + quic_stream_->setResponseDecoder(*stream_decoder); + + auto result = quic_stream_->encodeHeaders(request_headers_, true); + EXPECT_TRUE(result.ok()); + + // Destroy the mock decoder. + stream_decoder.reset(); + + std::string headers = spdyHeaderToHttp3StreamPayload(spdy_response_headers_); + quic::QuicStreamFrame frame1(stream_id_, /*fin*/ false, /*offset*/ 0, headers); + EXPECT_ENVOY_BUG(quic_stream_->OnStreamFrame(frame1), + "response_decoder_ use after free detected"); + + EXPECT_CALL(stream_callbacks_, + onResetStream(Http::StreamResetReason::LocalRefusedStreamReset, _)); + quic_stream_->resetStream(Http::StreamResetReason::LocalRefusedStreamReset); +} + +TEST_F(EnvoyQuicClientStreamTest, DecoderDestroyedBeforeDecodingBody) { + TestScopedRuntime runtime; + runtime.mergeValues({{"envoy.reloadable_features.abort_when_accessing_dead_decoder", "false"}}); + auto stream_decoder = std::make_unique(); + quic_stream_->setResponseDecoder(*stream_decoder); + + auto result = quic_stream_->encodeHeaders(request_headers_, true); + EXPECT_TRUE(result.ok()); + + EXPECT_CALL(*stream_decoder, decodeHeaders_(_, /*end_stream=*/false)); + std::string headers = spdyHeaderToHttp3StreamPayload(spdy_response_headers_); + quic::QuicStreamFrame frame1(stream_id_, /*fin*/ false, /*offset*/ 0, headers); + quic_stream_->OnStreamFrame(frame1); + + // Destroy the mock decoder. + stream_decoder.reset(); + + std::string body = bodyToHttp3StreamPayload("body"); + quic::QuicStreamFrame frame2(stream_id_, /*fin*/ false, headers.length(), body); + EXPECT_ENVOY_BUG(quic_stream_->OnStreamFrame(frame2), + "response_decoder_ use after free detected"); + + std::string trailers = spdyHeaderToHttp3StreamPayload(spdy_trailers_); + quic::QuicStreamFrame frame3(stream_id_, true, (headers.length() + body.length()), trailers); + quic_stream_->OnStreamFrame(frame3); + + EXPECT_CALL(stream_callbacks_, + onResetStream(Http::StreamResetReason::LocalRefusedStreamReset, _)); + quic_stream_->resetStream(Http::StreamResetReason::LocalRefusedStreamReset); +} + +TEST_F(EnvoyQuicClientStreamTest, DecoderDestroyedBeforeDecodingTrailer) { + TestScopedRuntime runtime; + runtime.mergeValues({{"envoy.reloadable_features.abort_when_accessing_dead_decoder", "false"}}); + auto stream_decoder = std::make_unique(); + quic_stream_->setResponseDecoder(*stream_decoder); + + auto result = quic_stream_->encodeHeaders(request_headers_, true); + EXPECT_TRUE(result.ok()); + + EXPECT_CALL(*stream_decoder, decodeHeaders_(_, /*end_stream=*/false)); + std::string headers = spdyHeaderToHttp3StreamPayload(spdy_response_headers_); + quic::QuicStreamFrame frame1(stream_id_, /*fin*/ false, /*offset*/ 0, headers); + quic_stream_->OnStreamFrame(frame1); + + EXPECT_CALL(*stream_decoder, decodeData(_, /*end_stream=*/false)); + std::string body = bodyToHttp3StreamPayload("body"); + quic::QuicStreamFrame frame2(stream_id_, /*fin*/ false, headers.length(), body); + quic_stream_->OnStreamFrame(frame2); + + // Destroy the mock decoder. + stream_decoder.reset(); + + std::string trailers = spdyHeaderToHttp3StreamPayload(spdy_trailers_); + quic::QuicStreamFrame frame3(stream_id_, true, (headers.length() + body.length()), trailers); + EXPECT_ENVOY_BUG(quic_stream_->OnStreamFrame(frame3), + "response_decoder_ use after free detected"); + + EXPECT_CALL(stream_callbacks_, + onResetStream(Http::StreamResetReason::LocalRefusedStreamReset, _)); + quic_stream_->resetStream(Http::StreamResetReason::LocalRefusedStreamReset); +} + #ifdef ENVOY_ENABLE_HTTP_DATAGRAMS TEST_F(EnvoyQuicClientStreamTest, EncodeCapsule) { setUpCapsuleProtocol(false, true); diff --git a/test/integration/integration_stream_decoder.h b/test/integration/integration_stream_decoder.h index 9654ce9e824ee..ce83293aed3a0 100644 --- a/test/integration/integration_stream_decoder.h +++ b/test/integration/integration_stream_decoder.h @@ -10,6 +10,7 @@ #include "envoy/http/metadata_interface.h" #include "source/common/common/dump_state_utils.h" +#include "source/common/http/response_decoder_impl_base.h" #include "test/test_common/utility.h" @@ -21,7 +22,8 @@ namespace Envoy { /** * Stream decoder wrapper used during integration testing. */ -class IntegrationStreamDecoder : public Http::ResponseDecoder, public Http::StreamCallbacks { +class IntegrationStreamDecoder : public Http::ResponseDecoderImplBase, + public Http::StreamCallbacks { public: IntegrationStreamDecoder(Event::Dispatcher& dispatcher); ~IntegrationStreamDecoder() override; diff --git a/test/integration/utility.h b/test/integration/utility.h index 39c2c664a3d0a..9db2c3d6d0bea 100644 --- a/test/integration/utility.h +++ b/test/integration/utility.h @@ -17,6 +17,7 @@ #include "source/common/common/dump_state_utils.h" #include "source/common/common/utility.h" #include "source/common/http/codec_client.h" +#include "source/common/http/response_decoder_impl_base.h" #include "source/common/stats/isolated_store_impl.h" #include "test/test_common/printers.h" @@ -29,7 +30,7 @@ namespace Envoy { /** * A buffering response decoder used for testing. */ -class BufferingStreamDecoder : public Http::ResponseDecoder, public Http::StreamCallbacks { +class BufferingStreamDecoder : public Http::ResponseDecoderImplBase, public Http::StreamCallbacks { public: BufferingStreamDecoder(std::function on_complete_cb) : on_complete_cb_(on_complete_cb) {} diff --git a/test/mocks/http/BUILD b/test/mocks/http/BUILD index 416d71fe5a2c3..af75f948d3445 100644 --- a/test/mocks/http/BUILD +++ b/test/mocks/http/BUILD @@ -104,6 +104,7 @@ envoy_cc_mock( hdrs = ["stream_decoder.h"], deps = [ "//envoy/http:codec_interface", + "//source/common/http:response_decoder_impl_base", ], ) diff --git a/test/mocks/http/stream_decoder.h b/test/mocks/http/stream_decoder.h index e1c8585a49a3c..2f265b158b82e 100644 --- a/test/mocks/http/stream_decoder.h +++ b/test/mocks/http/stream_decoder.h @@ -1,6 +1,8 @@ #pragma once #include "envoy/http/codec.h" +#include "source/common/http/response_decoder_impl_base.h" + #include "gmock/gmock.h" namespace Envoy { @@ -43,7 +45,7 @@ class MockRequestDecoder : public RequestDecoder { MOCK_METHOD(RequestDecoderHandlePtr, getRequestDecoderHandle, ()); }; -class MockResponseDecoder : public ResponseDecoder { +class MockResponseDecoder : public ResponseDecoderImplBase { public: MockResponseDecoder(); ~MockResponseDecoder() override; diff --git a/tools/spelling/spelling_dictionary.txt b/tools/spelling/spelling_dictionary.txt index 2fbc45cc57e1c..d2bd26af0b031 100644 --- a/tools/spelling/spelling_dictionary.txt +++ b/tools/spelling/spelling_dictionary.txt @@ -493,6 +493,7 @@ TTLs TX TXT UA +UAF UBSAN UDP UDS @@ -973,6 +974,7 @@ linkability linkable linux livelock +liveness llvm loc localhost From 9a0637372afe7cb7cd03075ff73331973e9ad347 Mon Sep 17 00:00:00 2001 From: Dan Zhang Date: Mon, 22 Sep 2025 21:34:08 +0000 Subject: [PATCH 2/5] extend usage of handle Signed-off-by: Dan Zhang --- envoy/router/BUILD | 1 - envoy/router/router.h | 3 +- source/common/http/codec_client.cc | 10 ++++++- source/common/http/codec_client.h | 28 +++++++++++++++++++ source/common/http/codec_wrappers.h | 12 ++++++-- source/common/http/conn_pool_base.cc | 16 +++++++++-- source/common/http/conn_pool_base.h | 12 +++++++- source/common/http/conn_pool_grid.cc | 20 ++++++++++++- source/common/http/conn_pool_grid.h | 2 ++ source/common/http/http1/conn_pool.cc | 13 +++++++++ source/common/http/http1/conn_pool.h | 3 ++ source/common/http/http3/conn_pool.h | 11 +++++++- .../common/quic/envoy_quic_client_stream.cc | 6 ++-- source/common/router/BUILD | 13 +++++++++ source/common/router/upstream_codec_filter.h | 3 +- source/common/router/upstream_request.h | 3 +- .../router/upstream_to_downstream_impl_base.h | 25 +++++++++++++++++ test/common/http/codec_wrappers_test.cc | 4 ++- test/integration/BUILD | 2 ++ test/mocks/router/BUILD | 1 + test/mocks/router/mocks.h | 3 +- 21 files changed, 173 insertions(+), 18 deletions(-) create mode 100644 source/common/router/upstream_to_downstream_impl_base.h diff --git a/envoy/router/BUILD b/envoy/router/BUILD index 47dc4cbebcbb4..33af45f78fe55 100644 --- a/envoy/router/BUILD +++ b/envoy/router/BUILD @@ -95,7 +95,6 @@ envoy_cc_library( "//envoy/tracing:tracer_interface", "//envoy/upstream:resource_manager_interface", "//envoy/upstream:retry_interface", - "//source/common/http:response_decoder_impl_base", "//source/common/protobuf", "//source/common/protobuf:utility_lib", "@com_google_absl//absl/types:optional", diff --git a/envoy/router/router.h b/envoy/router/router.h index 6cb2aff882035..ea2b1fae6a6fd 100644 --- a/envoy/router/router.h +++ b/envoy/router/router.h @@ -27,7 +27,6 @@ #include "envoy/upstream/resource_manager.h" #include "envoy/upstream/retry.h" -#include "source/common/http/response_decoder_impl_base.h" #include "source/common/protobuf/protobuf.h" #include "source/common/protobuf/utility.h" @@ -1494,7 +1493,7 @@ class GenericConnPool { * An API for the interactions the upstream stream needs to have with the downstream stream * and/or router components */ -class UpstreamToDownstream : public Http::ResponseDecoderImplBase, public Http::StreamCallbacks { +class UpstreamToDownstream : public Http::ResponseDecoder, public Http::StreamCallbacks { public: /** * @return return the route for the downstream stream. diff --git a/source/common/http/codec_client.cc b/source/common/http/codec_client.cc index 1dbedf1634990..c169a2fada2f4 100644 --- a/source/common/http/codec_client.cc +++ b/source/common/http/codec_client.cc @@ -71,8 +71,16 @@ void CodecClient::deleteRequest(ActiveRequest& request) { } } +RequestEncoder& CodecClient::newStream(ResponseDecoderHandlePtr response_decoder_handle) { + return enlistAndCreateEncoder( + std::make_unique(*this, std::move(response_decoder_handle))); +} + RequestEncoder& CodecClient::newStream(ResponseDecoder& response_decoder) { - ActiveRequestPtr request(new ActiveRequest(*this, response_decoder)); + return enlistAndCreateEncoder(std::make_unique(*this, response_decoder)); +} + +RequestEncoder& CodecClient::enlistAndCreateEncoder(ActiveRequestPtr request) { request->setEncoder(codec_->newStream(*request)); LinkedList::moveIntoList(std::move(request), active_requests_); diff --git a/source/common/http/codec_client.h b/source/common/http/codec_client.h index af59af2d15829..4638c839d5ea5 100644 --- a/source/common/http/codec_client.h +++ b/source/common/http/codec_client.h @@ -118,6 +118,16 @@ class CodecClient : protected Logger::Loggable, */ RequestEncoder& newStream(ResponseDecoder& response_decoder); + /** + * Create a new stream. Note: The CodecClient will NOT buffer multiple requests for HTTP1 + * connections. Thus, calling newStream() before the previous request has been fully encoded + * is an error. Pipelining is supported however. + * @param response_decoder_handle supplies the decoder to use for response callbacks if it's still + * alive. + * @return StreamEncoder& the encoder to use for encoding the request. + */ + RequestEncoder& newStream(ResponseDecoderHandlePtr response_decoder_handle); + void setConnectionStats(const Network::Connection::ConnectionStats& stats) { connection_->setConnectionStats(stats); } @@ -248,6 +258,23 @@ class CodecClient : protected Logger::Loggable, } } + ActiveRequest(CodecClient& parent, ResponseDecoderHandlePtr inner_handle) + : ResponseDecoderWrapper(std::move(inner_handle)), RequestEncoderWrapper(nullptr), + parent_(parent), header_validator_(parent.host_->cluster().makeHeaderValidator( + parent.codec_->protocol())) { + switch (parent.protocol()) { + case Protocol::Http10: + case Protocol::Http11: + // HTTP/1.1 codec does not support half-close on the response completion. + wait_encode_complete_ = false; + break; + case Protocol::Http2: + case Protocol::Http3: + wait_encode_complete_ = true; + break; + } + } + void decodeHeaders(ResponseHeaderMapPtr&& headers, bool end_stream) override; // StreamCallbacks @@ -305,6 +332,7 @@ class CodecClient : protected Logger::Loggable, void onBelowWriteBufferLowWatermark() override { codec_->onUnderlyingConnectionBelowWriteBufferLowWatermark(); } + RequestEncoder& enlistAndCreateEncoder(ActiveRequestPtr request); std::list active_requests_; Http::ConnectionCallbacks* codec_callbacks_{}; diff --git a/source/common/http/codec_wrappers.h b/source/common/http/codec_wrappers.h index 1494df392b06e..418c593ac5933 100644 --- a/source/common/http/codec_wrappers.h +++ b/source/common/http/codec_wrappers.h @@ -77,8 +77,14 @@ class ResponseDecoderWrapper : public ResponseDecoderImplBase { } protected: - ResponseDecoderWrapper(ResponseDecoder& inner) - : inner_handle_(inner.createResponseDecoderHandle()), inner_(&inner) {} + ResponseDecoderWrapper(ResponseDecoder& inner) : inner_(&inner) {} + + /** + * @param inner_handle refers a response decoder which may have already died at + * this point. Following access to the decoder will check its liveliness. + */ + ResponseDecoderWrapper(ResponseDecoderHandlePtr inner_handle) + : inner_handle_(std::move(inner_handle)) {} /** * Consumers of the wrapper generally want to know when a decode is complete. This is called @@ -92,7 +98,7 @@ class ResponseDecoderWrapper : public ResponseDecoderImplBase { private: Http::ResponseDecoder* getInnerDecoder() const { - if (!Runtime::runtimeFeatureEnabled("envoy.reloadable_features.use_response_decoder_handle")) { + if (inner_handle_ == nullptr) { return inner_; } if (inner_handle_) { diff --git a/source/common/http/conn_pool_base.cc b/source/common/http/conn_pool_base.cc index e3733f97e97dc..4de1405a92d1a 100644 --- a/source/common/http/conn_pool_base.cc +++ b/source/common/http/conn_pool_base.cc @@ -85,14 +85,21 @@ void HttpConnPoolImplBase::onPoolReady(Envoy::ConnectionPool::ActiveClient& clie Envoy::ConnectionPool::AttachContext& context) { ActiveClient* http_client = static_cast(&client); auto& http_context = typedContext(context); + // This decoder might have already died if ConnectivityGrid is in use and TCP + // win over QUIC. Http::ResponseDecoder& response_decoder = *http_context.decoder_; Http::ConnectionPool::Callbacks& callbacks = *http_context.callbacks_; // Track this request on the connection http_client->request_count_++; - Http::RequestEncoder& new_encoder = http_client->newStreamEncoder(response_decoder); - callbacks.onPoolReady(new_encoder, client.real_host_description_, + Http::RequestEncoder* new_encoder = nullptr; + if (http_context.decoder_handle_ == nullptr) { + new_encoder = &http_client->newStreamEncoder(response_decoder); + } else { + new_encoder = &http_client->newStreamEncoder(std::move(http_context.decoder_handle_)); + } + callbacks.onPoolReady(*new_encoder, client.real_host_description_, http_client->codec_client_->streamInfo(), http_client->codec_client_->protocol()); } @@ -210,5 +217,10 @@ RequestEncoder& MultiplexedActiveClientBase::newStreamEncoder(ResponseDecoder& r return codec_client_->newStream(response_decoder); } +RequestEncoder& +MultiplexedActiveClientBase::newStreamEncoder(ResponseDecoderHandlePtr response_decoder_handle) { + return codec_client_->newStream(std::move(response_decoder_handle)); +} + } // namespace Http } // namespace Envoy diff --git a/source/common/http/conn_pool_base.h b/source/common/http/conn_pool_base.h index 4b0b0fd3674ea..29ce6086b8800 100644 --- a/source/common/http/conn_pool_base.h +++ b/source/common/http/conn_pool_base.h @@ -9,6 +9,7 @@ #include "source/common/conn_pool/conn_pool_base.h" #include "source/common/http/codec_client.h" #include "source/common/http/http_server_properties_cache_impl.h" +#include "source/common/http/response_decoder_impl_base.h" #include "source/common/http/utility.h" #include "absl/strings/string_view.h" @@ -18,9 +19,15 @@ namespace Http { struct HttpAttachContext : public Envoy::ConnectionPool::AttachContext { HttpAttachContext(Http::ResponseDecoder* d, Http::ConnectionPool::Callbacks* c) - : decoder_(d), callbacks_(c) {} + : decoder_(d), callbacks_(c) { + if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.use_response_decoder_handle")) { + decoder_handle_ = d->createResponseDecoderHandle(); + } + } + Http::ResponseDecoder* decoder_; Http::ConnectionPool::Callbacks* callbacks_; + ResponseDecoderHandlePtr decoder_handle_; }; // An implementation of Envoy::ConnectionPool::PendingStream for HTTP/1.1 and HTTP/2 @@ -139,6 +146,8 @@ class ActiveClient : public Envoy::ConnectionPool::ActiveClient { absl::optional protocol() const override { return codec_client_->protocol(); } void close() override { codec_client_->close(); } virtual Http::RequestEncoder& newStreamEncoder(Http::ResponseDecoder& response_decoder) PURE; + virtual Http::RequestEncoder& + newStreamEncoder(Http::ResponseDecoderHandlePtr response_decoder_handle) PURE; void onEvent(Network::ConnectionEvent event) override { // Record request metrics only for successfully connected connections that handled requests if ((event == Network::ConnectionEvent::LocalClose || @@ -221,6 +230,7 @@ class MultiplexedActiveClientBase : public CodecClientCallbacks, // ConnPoolImpl::ActiveClient bool closingWithIncompleteStream() const override; RequestEncoder& newStreamEncoder(ResponseDecoder& response_decoder) override; + RequestEncoder& newStreamEncoder(ResponseDecoderHandlePtr response_decoder_handle) override; // CodecClientCallbacks void onStreamDestroy() override; diff --git a/source/common/http/conn_pool_grid.cc b/source/common/http/conn_pool_grid.cc index efd45066c9068..7193de2f5bf2f 100644 --- a/source/common/http/conn_pool_grid.cc +++ b/source/common/http/conn_pool_grid.cc @@ -50,6 +50,10 @@ ConnectivityGrid::WrapperCallbacks::WrapperCallbacks(ConnectivityGrid& grid, next_attempt_timer_( grid_.dispatcher_.createTimer([this]() -> void { onNextAttemptTimer(); })), stream_options_(options) { + if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.use_response_decoder_handle")) { + decoder_handle_ = decoder.createResponseDecoderHandle(); + } + if (!stream_options_.can_use_http3_) { // If alternate protocols are explicitly disabled, there must have been a failed request over // HTTP/3 and the failure must be post-handshake. So disable HTTP/3 for this request. @@ -70,7 +74,21 @@ ConnectivityGrid::WrapperCallbacks::ConnectionAttemptCallbacks::~ConnectionAttem ConnectivityGrid::StreamCreationResult ConnectivityGrid::WrapperCallbacks::ConnectionAttemptCallbacks::newStream() { ASSERT(!parent_.grid_.isPoolHttp3(pool()) || parent_.stream_options_.can_use_http3_); - auto* cancellable = pool().newStream(parent_.decoder_, *this, parent_.stream_options_); + Http::ResponseDecoder& decoder = parent_.decoder_; + if (parent_.decoder_handle_ != nullptr) { + if (OptRef opt_ref = parent_.decoder_handle_->get(); opt_ref.has_value()) { + decoder = opt_ref.value().get(); + } else { + const std::string error_msg = "parent_.decoder_ use after free detected."; + IS_ENVOY_BUG(error_msg); + RELEASE_ASSERT(!Runtime::runtimeFeatureEnabled( + "envoy.reloadable_features.abort_when_accessing_dead_decoder"), + error_msg); + return StreamCreationResult::ImmediateResult; + } + } + + auto* cancellable = pool().newStream(decoder, *this, parent_.stream_options_); if (cancellable == nullptr) { return StreamCreationResult::ImmediateResult; } diff --git a/source/common/http/conn_pool_grid.h b/source/common/http/conn_pool_grid.h index 22e0172866e1d..feb49dbba53e7 100644 --- a/source/common/http/conn_pool_grid.h +++ b/source/common/http/conn_pool_grid.h @@ -156,6 +156,8 @@ class ConnectivityGrid : public ConnectionPool::Instance, ConnectivityGrid& grid_; // The decoder for the original newStream, needed to create streams on subsequent pools. Http::ResponseDecoder& decoder_; + Http::ResponseDecoderHandlePtr decoder_handle_; + // The callbacks from the original caller, which must get onPoolFailure or // onPoolReady unless there is call to cancel(). Will be nullptr if the caller // has been notified while attempts are still pending. diff --git a/source/common/http/http1/conn_pool.cc b/source/common/http/http1/conn_pool.cc index 5877a035f041d..a5261109c8be2 100644 --- a/source/common/http/http1/conn_pool.cc +++ b/source/common/http/http1/conn_pool.cc @@ -30,6 +30,13 @@ ActiveClient::StreamWrapper::StreamWrapper(ResponseDecoder& response_decoder, Ac RequestEncoderWrapper::inner_encoder_->getStream().addCallbacks(*this); } +ActiveClient::StreamWrapper::StreamWrapper(ResponseDecoderHandlePtr response_decoder_handle, + ActiveClient& parent) + : ResponseDecoderWrapper(std::move(response_decoder_handle)), + RequestEncoderWrapper(&parent.codec_client_->newStream(*this)), parent_(parent) { + RequestEncoderWrapper::inner_encoder_->getStream().addCallbacks(*this); +} + ActiveClient::StreamWrapper::~StreamWrapper() { // Upstream connection might be closed right after response is complete. Setting delay=true // here to attach pending requests in next dispatcher loop to handle that case. @@ -92,6 +99,12 @@ RequestEncoder& ActiveClient::newStreamEncoder(ResponseDecoder& response_decoder return *stream_wrapper_; } +RequestEncoder& ActiveClient::newStreamEncoder(ResponseDecoderHandlePtr response_decoder_handle) { + ASSERT(!stream_wrapper_); + stream_wrapper_ = std::make_unique(std::move(response_decoder_handle), *this); + return *stream_wrapper_; +} + ConnectionPool::InstancePtr allocateConnPool(Event::Dispatcher& dispatcher, Random::RandomGenerator& random_generator, Upstream::HostConstSharedPtr host, Upstream::ResourcePriority priority, diff --git a/source/common/http/http1/conn_pool.h b/source/common/http/http1/conn_pool.h index b8b1fa803dfca..f7b4ccf7a32b4 100644 --- a/source/common/http/http1/conn_pool.h +++ b/source/common/http/http1/conn_pool.h @@ -23,6 +23,7 @@ class ActiveClient : public Envoy::Http::ActiveClient { // ConnPoolImplBase::ActiveClient bool closingWithIncompleteStream() const override; RequestEncoder& newStreamEncoder(ResponseDecoder& response_decoder) override; + RequestEncoder& newStreamEncoder(ResponseDecoderHandlePtr response_decoder_handle) override; uint32_t numActiveStreams() const override { // Override the parent class using the codec for numActiveStreams. @@ -43,6 +44,8 @@ class ActiveClient : public Envoy::Http::ActiveClient { protected Logger::Loggable { public: StreamWrapper(ResponseDecoder& response_decoder, ActiveClient& parent); + StreamWrapper(ResponseDecoderHandlePtr response_decoder_handle, ActiveClient& parent); + ~StreamWrapper() override; // StreamEncoderWrapper diff --git a/source/common/http/http3/conn_pool.h b/source/common/http/http3/conn_pool.h index e27b723106eef..4f67b6efcc6ed 100644 --- a/source/common/http/http3/conn_pool.h +++ b/source/common/http/http3/conn_pool.h @@ -39,15 +39,24 @@ class ActiveClient : public MultiplexedActiveClientBase { // Http::ConnectionCallbacks void onMaxStreamsChanged(uint32_t num_streams) override; - RequestEncoder& newStreamEncoder(ResponseDecoder& response_decoder) override { + void updateQuicheCapacity() { ASSERT(quiche_capacity_ != 0); has_created_stream_ = true; // Each time a quic stream is allocated the quic capacity needs to get // decremented. See comments by quiche_capacity_. updateCapacity(quiche_capacity_ - 1); + } + + RequestEncoder& newStreamEncoder(ResponseDecoder& response_decoder) override { + updateQuicheCapacity(); return MultiplexedActiveClientBase::newStreamEncoder(response_decoder); } + RequestEncoder& newStreamEncoder(ResponseDecoderHandlePtr response_decoder_handle) override { + updateQuicheCapacity(); + return MultiplexedActiveClientBase::newStreamEncoder(std::move(response_decoder_handle)); + } + uint32_t effectiveConcurrentStreamLimit() const override { return std::min(MultiplexedActiveClientBase::effectiveConcurrentStreamLimit(), quiche_capacity_); diff --git a/source/common/quic/envoy_quic_client_stream.cc b/source/common/quic/envoy_quic_client_stream.cc index f6506a7fb33e1..6228d625b6b12 100644 --- a/source/common/quic/envoy_quic_client_stream.cc +++ b/source/common/quic/envoy_quic_client_stream.cc @@ -37,7 +37,9 @@ EnvoyQuicClientStream::EnvoyQuicClientStream( } void EnvoyQuicClientStream::setResponseDecoder(Http::ResponseDecoder& decoder) { - response_decoder_handle_ = decoder.createResponseDecoderHandle(); + if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.use_response_decoder_handle")) { + response_decoder_handle_ = decoder.createResponseDecoderHandle(); + } response_decoder_ = &decoder; } @@ -514,7 +516,7 @@ void EnvoyQuicClientStream::onResponseDecoderDead() const { } Http::ResponseDecoder* EnvoyQuicClientStream::getResponseDecoder() { - if (!Runtime::runtimeFeatureEnabled("envoy.reloadable_features.use_response_decoder_handle")) { + if (response_decoder_handle_ == nullptr) { return response_decoder_; } if (response_decoder_handle_) { diff --git a/source/common/router/BUILD b/source/common/router/BUILD index fb5d8979aa5f3..356a6fa34d791 100644 --- a/source/common/router/BUILD +++ b/source/common/router/BUILD @@ -347,6 +347,17 @@ envoy_cc_library( ], ) +envoy_cc_library( + name = "upstream_to_downstream_impl_base", + srcs = [ + "upstream_to_downstream_impl_base.h", + ], + deps = [ + "//envoy/router:router_interface", + "//source/common/http:response_decoder_impl_base", + ], +) + envoy_cc_library( name = "router_lib", srcs = [ @@ -364,6 +375,7 @@ envoy_cc_library( ":metadatamatchcriteria_lib", ":retry_state_lib", ":upstream_codec_filter_lib", + ":upstream_to_downstream_impl_base", "//envoy/event:dispatcher_interface", "//envoy/event:timer_interface", "//envoy/grpc:status", @@ -422,6 +434,7 @@ envoy_cc_library( ], deps = [ ":config_lib", + ":upstream_to_downstream_impl_base", "//envoy/event:dispatcher_interface", "//envoy/http:codec_interface", "//envoy/http:filter_interface", diff --git a/source/common/router/upstream_codec_filter.h b/source/common/router/upstream_codec_filter.h index ff8b4328c09d6..2e88017475095 100644 --- a/source/common/router/upstream_codec_filter.h +++ b/source/common/router/upstream_codec_filter.h @@ -13,6 +13,7 @@ #include "source/common/common/logger.h" #include "source/common/config/well_known_names.h" +#include "source/common/router/upstream_to_downstream_impl_base.h" #include "source/common/runtime/runtime_features.h" #include "source/extensions/filters/http/common/factory_base.h" @@ -51,7 +52,7 @@ class UpstreamCodecFilter : public Http::StreamDecoderFilter, void setDecoderFilterCallbacks(Http::StreamDecoderFilterCallbacks& callbacks) override; // This bridge connects the upstream stream to the filter manager. - class CodecBridge : public UpstreamToDownstream { + class CodecBridge : public UpstreamToDownstreamImplBase { public: CodecBridge(UpstreamCodecFilter& filter) : filter_(filter) {} void decode1xxHeaders(Http::ResponseHeaderMapPtr&& headers) override; diff --git a/source/common/router/upstream_request.h b/source/common/router/upstream_request.h index 0f2b1a063f94f..2fbb90e66ebb7 100644 --- a/source/common/router/upstream_request.h +++ b/source/common/router/upstream_request.h @@ -23,6 +23,7 @@ #include "source/common/common/logger.h" #include "source/common/config/well_known_names.h" #include "source/common/http/filter_manager.h" +#include "source/common/router/upstream_to_downstream_impl_base.h" #include "source/common/stream_info/stream_info_impl.h" #include "source/common/tracing/null_span_impl.h" #include "source/extensions/filters/http/common/factory_base.h" @@ -64,7 +65,7 @@ class UpstreamCodecFilter; * */ class UpstreamRequest : public Logger::Loggable, - public UpstreamToDownstream, + public UpstreamToDownstreamImplBase, public LinkedObject, public GenericConnectionPoolCallbacks, public Event::DeferredDeletable { diff --git a/source/common/router/upstream_to_downstream_impl_base.h b/source/common/router/upstream_to_downstream_impl_base.h new file mode 100644 index 0000000000000..cd81d09cb31ff --- /dev/null +++ b/source/common/router/upstream_to_downstream_impl_base.h @@ -0,0 +1,25 @@ +#pragma once + +#include + +#include "envoy/router/router.h" + +#include "source/common/http/response_decoder_impl_base.h" + +namespace Envoy { +namespace Router { + +class UpstreamToDownstreamImplBase : public UpstreamToDownstream { +public: + UpstreamToDownstreamImplBase() : live_trackable_(std::make_shared(true)) {} + + Http::ResponseDecoderHandlePtr createResponseDecoderHandle() override { + return std::make_unique(live_trackable_, *this); + } + +private: + std::shared_ptr live_trackable_; +}; + +} // namespace Router +} // namespace Envoy diff --git a/test/common/http/codec_wrappers_test.cc b/test/common/http/codec_wrappers_test.cc index e5ffb0acdd615..1a9b571d307a2 100644 --- a/test/common/http/codec_wrappers_test.cc +++ b/test/common/http/codec_wrappers_test.cc @@ -13,6 +13,8 @@ class MockResponseDecoderWrapper : public ResponseDecoderWrapper { public: explicit MockResponseDecoderWrapper(MockResponseDecoder& inner_decoder) : ResponseDecoderWrapper(inner_decoder) {} + explicit MockResponseDecoderWrapper(ResponseDecoderHandlePtr handle) + : ResponseDecoderWrapper(std::move(handle)) {} void onDecodeComplete() override {} void onPreDecodeComplete() override {} }; @@ -30,7 +32,7 @@ TEST(MockResponseDecoderWrapper, decoderDestroyedBeforeDecoding) { TestScopedRuntime runtime; runtime.mergeValues({{"envoy.reloadable_features.abort_when_accessing_dead_decoder", "false"}}); auto inner_decoder = std::make_unique(); - MockResponseDecoderWrapper wrapper(*inner_decoder); + MockResponseDecoderWrapper wrapper(inner_decoder->createResponseDecoderHandle()); inner_decoder.reset(); diff --git a/test/integration/BUILD b/test/integration/BUILD index 86e56f222bc9f..0d67611312554 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -1196,6 +1196,7 @@ envoy_cc_test_library( "//envoy/http:header_map_interface", "//envoy/http:metadata_interface", "//source/common/common:dump_state_utils", + "//source/common/http:response_decoder_impl_base", "//test/test_common:utility_lib", ], ) @@ -1391,6 +1392,7 @@ envoy_cc_test_library( "//source/common/common:thread_lib", "//source/common/common:utility_lib", "//source/common/http:codec_client_lib", + "//source/common/http:response_decoder_impl_base", "//source/common/json:json_loader_lib", "//source/common/network:utility_lib", "//source/common/quic:quic_stat_names_lib", diff --git a/test/mocks/router/BUILD b/test/mocks/router/BUILD index 1ae14c11f649e..da34f6414267b 100644 --- a/test/mocks/router/BUILD +++ b/test/mocks/router/BUILD @@ -29,6 +29,7 @@ envoy_cc_mock( "//envoy/stream_info:stream_info_interface", "//envoy/thread_local:thread_local_interface", "//envoy/upstream:cluster_manager_interface", + "//source/common/router:upstream_to_downstream_impl_base", "//test/mocks:common_lib", "//test/mocks/stats:stats_mocks", "//test/mocks/upstream:host_mocks", diff --git a/test/mocks/router/mocks.h b/test/mocks/router/mocks.h index 47584ab4b4571..be1b169e9bb60 100644 --- a/test/mocks/router/mocks.h +++ b/test/mocks/router/mocks.h @@ -30,6 +30,7 @@ #include "envoy/type/v3/percent.pb.h" #include "envoy/upstream/cluster_manager.h" +#include "source/common/router/upstream_to_downstream_impl_base.h" #include "source/common/stats/symbol_table.h" #include "test/mocks/stats/mocks.h" @@ -714,7 +715,7 @@ class MockGenericConnPool : public GenericConnPool { new NiceMock()}; }; -class MockUpstreamToDownstream : public UpstreamToDownstream { +class MockUpstreamToDownstream : public UpstreamToDownstreamImplBase { public: MOCK_METHOD(const Route&, route, (), (const)); MOCK_METHOD(OptRef, connection, (), (const)); From 63e21335d094f2644d8a926da6444a3e3cbe3f41 Mon Sep 17 00:00:00 2001 From: Dan Zhang Date: Tue, 23 Sep 2025 14:14:12 +0000 Subject: [PATCH 3/5] regression test Signed-off-by: Dan Zhang --- .../filter_integration_test.cc | 55 +++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/test/extensions/filters/http/alternate_protocols_cache/filter_integration_test.cc b/test/extensions/filters/http/alternate_protocols_cache/filter_integration_test.cc index 2c7cff266f837..05a9ee608b1d6 100644 --- a/test/extensions/filters/http/alternate_protocols_cache/filter_integration_test.cc +++ b/test/extensions/filters/http/alternate_protocols_cache/filter_integration_test.cc @@ -247,6 +247,61 @@ TEST_P(FilterIntegrationTest, AltSvcCachedH3Slow) { 100); } +TEST_P(FilterIntegrationTest, AltSvcCachedH3SlowTillH2Finishes) { +#ifdef WIN32 + GTEST_SKIP() << "Skipping on Windows"; +#endif + // Start with the alt-svc header in the cache. + write_alt_svc_to_file_ = true; + + const uint64_t request_size = 0; + const uint64_t response_size = 0; + const std::chrono::milliseconds timeout = TestUtility::DefaultTimeout; + + initialize(); + codec_client_ = makeHttpConnection(makeClientConnection(lookupPort("http"))); + + absl::Notification block_until_notify; + // Block the H3 upstream so it can't process packets. + fake_upstreams_[1]->runOnDispatcherThread([&] { block_until_notify.WaitForNotification(); }); + + ASSERT(codec_client_ != nullptr); + // Send the request to Envoy. + auto response = codec_client_->makeHeaderOnlyRequest(default_request_headers_); + // The request should fail over to the HTTP/2 upstream (index 0) as the H3 upstream is wedged. + waitForNextUpstreamRequest(0); + // Finish the response over HTTP/2. + upstream_request_->encodeHeaders(default_response_headers_, true); + // Wait for the response to be read by the codec client. + ASSERT_TRUE(response->waitForEndStream(timeout)); + checkSimpleRequestSuccess(request_size, response_size, response.get()); + + // Now unblock the HTTP/3 server and wait for the connection to be established. + block_until_notify.Notify(); + FakeHttpConnectionPtr h3_connection; + waitForNextUpstreamConnection(std::vector{1}, TestUtility::DefaultTimeout, + h3_connection); + // Of the 100 connection pools configured, the grid registers as taking up one. + test_server_->waitForGaugeEq("cluster.cluster_0.circuit_breakers.default.remaining_cx_pools", 99); + test_server_->waitForCounterEq("cluster.cluster_0.upstream_cx_http3_total", 1); + + // An upstream HTTP/3 stream should be created without crash, and the created stream will be + // reset. + FakeStreamPtr upstream_request2; + ASSERT_TRUE(h3_connection->waitForNewStream(*dispatcher_, upstream_request2)); + ASSERT_TRUE(upstream_request2->waitForReset()); + + // Now close the HTTP/3 connection to make sure it doesn't cause problems for the + // downstream stream. + ASSERT_TRUE(h3_connection->close()); + test_server_->waitForCounterEq("cluster.cluster_0.upstream_cx_destroy", 1); + + cleanupUpstreamAndDownstream(); + // Wait for the grid to be torn down to make sure it is not problematic. + test_server_->waitForGaugeEq("cluster.cluster_0.circuit_breakers.default.remaining_cx_pools", + 100); +} + // TODO(32151): Figure out why it's flaky and re-enable. TEST_P(FilterIntegrationTest, DISABLED_AltSvcCachedH2Slow) { #ifdef WIN32 From aefb778e5811ef8f5db1a81ffc5cb5acc1845574 Mon Sep 17 00:00:00 2001 From: Dan Zhang Date: Wed, 24 Sep 2025 17:48:27 +0000 Subject: [PATCH 4/5] test coverage Signed-off-by: Dan Zhang --- test/common/http/http1/conn_pool_test.cc | 21 +++++++++++++++++++++ test/common/http/http3/BUILD | 1 + test/common/http/http3/conn_pool_test.cc | 13 ++++++++++++- 3 files changed, 34 insertions(+), 1 deletion(-) diff --git a/test/common/http/http1/conn_pool_test.cc b/test/common/http/http1/conn_pool_test.cc index 7f500aee6fe6f..908fa2c23f71c 100644 --- a/test/common/http/http1/conn_pool_test.cc +++ b/test/common/http/http1/conn_pool_test.cc @@ -1,4 +1,5 @@ #include +#include #include #include "envoy/http/codec.h" @@ -387,6 +388,26 @@ TEST_F(Http1ConnPoolImplTest, VerifyCancelInCallback) { dispatcher_.clearDeferredDeleteList(); } +/** + * Added for code coverage when envoy.reloadable_features.abort_when_accessing_dead_decoder is false + */ +TEST_F(Http1ConnPoolImplTest, RequestAndResponseWithoutDecoderHandle) { + TestScopedRuntime runtime; + runtime.mergeValues({{"envoy.reloadable_features.abort_when_accessing_dead_decoder", "false"}}); + + InSequence s; + ActiveTestRequest r1(*this, 0, ActiveTestRequest::Type::CreateConnection); + r1.startRequest(); + conn_pool_->expectEnableUpstreamReady(); + r1.completeResponse(false); + + // Cause the connection to go away. + EXPECT_CALL(*conn_pool_, onClientDestroy()); + conn_pool_->expectAndRunUpstreamReady(); + conn_pool_->test_clients_[0].connection_->raiseEvent(Network::ConnectionEvent::RemoteClose); + dispatcher_.clearDeferredDeleteList(); +} + /** * Tests a request that generates a new connection, completes, and then a second request that uses * the same connection. diff --git a/test/common/http/http3/BUILD b/test/common/http/http3/BUILD index e34a486509e59..65a2e8d671401 100644 --- a/test/common/http/http3/BUILD +++ b/test/common/http/http3/BUILD @@ -14,6 +14,7 @@ envoy_cc_test( srcs = envoy_select_enable_http3(["conn_pool_test.cc"]), rbe_pool = "6gig", deps = envoy_select_enable_http3([ + "//test/test_common:test_runtime_lib", "//source/common/event:dispatcher_lib", "//source/common/http/http3:conn_pool_lib", "//source/common/network:utility_lib", diff --git a/test/common/http/http3/conn_pool_test.cc b/test/common/http/http3/conn_pool_test.cc index 67f12a2e500f1..a2672e6369961 100644 --- a/test/common/http/http3/conn_pool_test.cc +++ b/test/common/http/http3/conn_pool_test.cc @@ -15,6 +15,7 @@ #include "test/mocks/upstream/cluster_info.h" #include "test/mocks/upstream/host.h" #include "test/test_common/simulated_time_system.h" +#include "test/test_common/test_runtime.h" using testing::NiceMock; using testing::Return; @@ -219,7 +220,17 @@ void Http3ConnPoolImplTest::createNewStream() { pool_->onConnectionEvent(*clients.front(), "", Network::ConnectionEvent::Connected); } -TEST_F(Http3ConnPoolImplTest, CreationAndNewStream) { createNewStream(); } +TEST_F(Http3ConnPoolImplTest, CreationAndNewStreamWithDecoderHandle) { + TestScopedRuntime runtime; + runtime.mergeValues({{"envoy.reloadable_features.use_response_decoder_handle", "true"}}); + createNewStream(); +} + +TEST_F(Http3ConnPoolImplTest, CreationAndNewStreamWithoutDecoderHandle) { + TestScopedRuntime runtime; + runtime.mergeValues({{"envoy.reloadable_features.use_response_decoder_handle", "false"}}); + createNewStream(); +} TEST_F(Http3ConnPoolImplTest, CreationAndNewHappyEyeballsStream) { happy_eyeballs_ = true; From 301e736baf8c4d8d030acb0fb6927891c8fad914 Mon Sep 17 00:00:00 2001 From: Dan Zhang Date: Thu, 25 Sep 2025 16:26:42 +0000 Subject: [PATCH 5/5] real coverage Signed-off-by: Dan Zhang --- test/common/http/http1/conn_pool_test.cc | 2 +- test/common/http/http3/BUILD | 1 - test/common/http/http3/conn_pool_test.cc | 13 +------------ .../multiplexed_upstream_integration_test.cc | 10 ++++++++++ 4 files changed, 12 insertions(+), 14 deletions(-) diff --git a/test/common/http/http1/conn_pool_test.cc b/test/common/http/http1/conn_pool_test.cc index 908fa2c23f71c..863bf34fb8ab0 100644 --- a/test/common/http/http1/conn_pool_test.cc +++ b/test/common/http/http1/conn_pool_test.cc @@ -393,7 +393,7 @@ TEST_F(Http1ConnPoolImplTest, VerifyCancelInCallback) { */ TEST_F(Http1ConnPoolImplTest, RequestAndResponseWithoutDecoderHandle) { TestScopedRuntime runtime; - runtime.mergeValues({{"envoy.reloadable_features.abort_when_accessing_dead_decoder", "false"}}); + runtime.mergeValues({{"envoy.reloadable_features.use_response_decoder_handle", "false"}}); InSequence s; ActiveTestRequest r1(*this, 0, ActiveTestRequest::Type::CreateConnection); diff --git a/test/common/http/http3/BUILD b/test/common/http/http3/BUILD index 65a2e8d671401..e34a486509e59 100644 --- a/test/common/http/http3/BUILD +++ b/test/common/http/http3/BUILD @@ -14,7 +14,6 @@ envoy_cc_test( srcs = envoy_select_enable_http3(["conn_pool_test.cc"]), rbe_pool = "6gig", deps = envoy_select_enable_http3([ - "//test/test_common:test_runtime_lib", "//source/common/event:dispatcher_lib", "//source/common/http/http3:conn_pool_lib", "//source/common/network:utility_lib", diff --git a/test/common/http/http3/conn_pool_test.cc b/test/common/http/http3/conn_pool_test.cc index a2672e6369961..67f12a2e500f1 100644 --- a/test/common/http/http3/conn_pool_test.cc +++ b/test/common/http/http3/conn_pool_test.cc @@ -15,7 +15,6 @@ #include "test/mocks/upstream/cluster_info.h" #include "test/mocks/upstream/host.h" #include "test/test_common/simulated_time_system.h" -#include "test/test_common/test_runtime.h" using testing::NiceMock; using testing::Return; @@ -220,17 +219,7 @@ void Http3ConnPoolImplTest::createNewStream() { pool_->onConnectionEvent(*clients.front(), "", Network::ConnectionEvent::Connected); } -TEST_F(Http3ConnPoolImplTest, CreationAndNewStreamWithDecoderHandle) { - TestScopedRuntime runtime; - runtime.mergeValues({{"envoy.reloadable_features.use_response_decoder_handle", "true"}}); - createNewStream(); -} - -TEST_F(Http3ConnPoolImplTest, CreationAndNewStreamWithoutDecoderHandle) { - TestScopedRuntime runtime; - runtime.mergeValues({{"envoy.reloadable_features.use_response_decoder_handle", "false"}}); - createNewStream(); -} +TEST_F(Http3ConnPoolImplTest, CreationAndNewStream) { createNewStream(); } TEST_F(Http3ConnPoolImplTest, CreationAndNewHappyEyeballsStream) { happy_eyeballs_ = true; diff --git a/test/integration/multiplexed_upstream_integration_test.cc b/test/integration/multiplexed_upstream_integration_test.cc index a4848e9a179da..4f3e26b440875 100644 --- a/test/integration/multiplexed_upstream_integration_test.cc +++ b/test/integration/multiplexed_upstream_integration_test.cc @@ -43,6 +43,16 @@ INSTANTIATE_TEST_SUITE_P(Protocols, MultiplexedUpstreamIntegrationTest, HttpProtocolIntegrationTest::protocolTestParamsToString); TEST_P(MultiplexedUpstreamIntegrationTest, RouterRequestAndResponseWithBodyNoBuffer) { + config_helper_.addRuntimeOverride("envoy.reloadable_features.use_response_decoder_handle", + "true"); + testRouterRequestAndResponseWithBody(1024, 512, false); +} + +// Needed for test coverage. +TEST_P(MultiplexedUpstreamIntegrationTest, + RouterRequestAndResponseWithBodyNoBufferWithoutDecoderHandle) { + config_helper_.addRuntimeOverride("envoy.reloadable_features.use_response_decoder_handle", + "false"); testRouterRequestAndResponseWithBody(1024, 512, false); }