diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 01606a2cad772..4ec1e0a0aaa0e 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/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_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 7b98413e1a5a1..418c593ac5933 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,39 @@ 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_(&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 @@ -59,7 +93,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 (inner_handle_ == nullptr) { + 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/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 99aea9e453b2d..a5261109c8be2 100644 --- a/source/common/http/http1/conn_pool.cc +++ b/source/common/http/http1/conn_pool.cc @@ -25,8 +25,15 @@ 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); +} + +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); } @@ -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 7bdeb288e6aa4..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. @@ -36,13 +37,15 @@ 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 { 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/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..6228d625b6b12 100644 --- a/source/common/quic/envoy_quic_client_stream.cc +++ b/source/common/quic/envoy_quic_client_stream.cc @@ -36,6 +36,13 @@ EnvoyQuicClientStream::EnvoyQuicClientStream( RegisterMetadataVisitor(this); } +void EnvoyQuicClientStream::setResponseDecoder(Http::ResponseDecoder& decoder) { + if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.use_response_decoder_handle")) { + 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 +214,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 +235,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 +320,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 +371,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 +459,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 +498,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 +507,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 (response_decoder_handle_ == nullptr) { + 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/router/BUILD b/source/common/router/BUILD index ba6ce85acb9b3..e367925495cb3 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/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 9504ba40cd1a7..132f335726905 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -82,6 +82,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); @@ -179,6 +180,8 @@ FALSE_RUNTIME_GUARD(envoy_reloadable_features_remove_legacy_route_formatter); 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); // TODO(pradeepcrao): Create a config option to enable this instead after // testing. FALSE_RUNTIME_GUARD(envoy_restart_features_use_cached_grpc_client_for_xds); 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 1b452ae99f941..20a7a822c4d0b 100644 --- a/source/extensions/bootstrap/reverse_tunnel/downstream_socket_interface/BUILD +++ b/source/extensions/bootstrap/reverse_tunnel/downstream_socket_interface/BUILD @@ -78,6 +78,7 @@ envoy_cc_library( "//source/common/event:real_time_system_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 8230a8da470e3..f0b57076a5953 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..1a9b571d307a2 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,56 @@ namespace Http { class MockResponseDecoderWrapper : public ResponseDecoderWrapper { public: - MockResponseDecoderWrapper() : ResponseDecoderWrapper(inner_decoder_) {} - MockResponseDecoder& innerEncoder() { return inner_decoder_; } + explicit MockResponseDecoderWrapper(MockResponseDecoder& inner_decoder) + : ResponseDecoderWrapper(inner_decoder) {} + explicit MockResponseDecoderWrapper(ResponseDecoderHandlePtr handle) + : ResponseDecoderWrapper(std::move(handle)) {} 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->createResponseDecoderHandle()); + + 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/http/http1/conn_pool_test.cc b/test/common/http/http1/conn_pool_test.cc index 7f500aee6fe6f..863bf34fb8ab0 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.use_response_decoder_handle", "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/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/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 diff --git a/test/integration/BUILD b/test/integration/BUILD index 529ad6fecbc35..106fecce2256e 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/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/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); } 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/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)); 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