diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 1af734193d2cc..ba8eb47d72b9c 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -1,11 +1,6 @@ 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 bc7c45cacf0c3..3c23ea86b2636 100644 --- a/envoy/http/codec.h +++ b/envoy/http/codec.h @@ -198,23 +198,6 @@ 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. @@ -321,16 +304,9 @@ 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 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 ef26a58dec6ca..d37a0bd61968d 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" @@ -1491,7 +1490,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/BUILD b/source/common/http/BUILD index e2385f30ccb58..7e1002a8080d4 100644 --- a/source/common/http/BUILD +++ b/source/common/http/BUILD @@ -132,18 +132,7 @@ envoy_cc_library( envoy_cc_library( name = "codec_wrappers_lib", hdrs = ["codec_wrappers.h"], - 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", - ], + 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 1494df392b06e..7b98413e1a5a1 100644 --- a/source/common/http/codec_wrappers.h +++ b/source/common/http/codec_wrappers.h @@ -2,35 +2,24 @@ #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 ResponseDecoderImplBase { +class ResponseDecoderWrapper : public ResponseDecoder { public: // ResponseDecoder void decode1xxHeaders(ResponseHeaderMapPtr&& headers) override { - if (Http::ResponseDecoder* inner = getInnerDecoder()) { - inner->decode1xxHeaders(std::move(headers)); - } else { - onInnerDecoderDead(); - } + inner_.decode1xxHeaders(std::move(headers)); } void decodeHeaders(ResponseHeaderMapPtr&& headers, bool end_stream) override { if (end_stream) { onPreDecodeComplete(); } - if (Http::ResponseDecoder* inner = getInnerDecoder()) { - inner->decodeHeaders(std::move(headers), end_stream); - } else { - onInnerDecoderDead(); - } + inner_.decodeHeaders(std::move(headers), end_stream); if (end_stream) { onDecodeComplete(); } @@ -40,11 +29,7 @@ class ResponseDecoderWrapper : public ResponseDecoderImplBase { if (end_stream) { onPreDecodeComplete(); } - if (Http::ResponseDecoder* inner = getInnerDecoder()) { - inner->decodeData(data, end_stream); - } else { - onInnerDecoderDead(); - } + inner_.decodeData(data, end_stream); if (end_stream) { onDecodeComplete(); } @@ -52,33 +37,20 @@ class ResponseDecoderWrapper : public ResponseDecoderImplBase { void decodeTrailers(ResponseTrailerMapPtr&& trailers) override { onPreDecodeComplete(); - if (Http::ResponseDecoder* inner = getInnerDecoder()) { - inner->decodeTrailers(std::move(trailers)); - } else { - onInnerDecoderDead(); - } + inner_.decodeTrailers(std::move(trailers)); onDecodeComplete(); } void decodeMetadata(MetadataMapPtr&& metadata_map) override { - if (Http::ResponseDecoder* inner = getInnerDecoder()) { - inner->decodeMetadata(std::move(metadata_map)); - } else { - onInnerDecoderDead(); - } + inner_.decodeMetadata(std::move(metadata_map)); } void dumpState(std::ostream& os, int indent_level) const override { - if (Http::ResponseDecoder* inner = getInnerDecoder()) { - inner->dumpState(os, indent_level); - } else { - onInnerDecoderDead(); - } + inner_.dumpState(os, indent_level); } protected: - ResponseDecoderWrapper(ResponseDecoder& inner) - : inner_handle_(inner.createResponseDecoderHandle()), inner_(&inner) {} + ResponseDecoderWrapper(ResponseDecoder& inner) : inner_(inner) {} /** * Consumers of the wrapper generally want to know when a decode is complete. This is called @@ -87,29 +59,7 @@ class ResponseDecoderWrapper : public ResponseDecoderImplBase { virtual void onPreDecodeComplete() PURE; virtual void onDecodeComplete() PURE; - 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); - } + ResponseDecoder& inner_; }; /** diff --git a/source/common/http/http1/conn_pool.cc b/source/common/http/http1/conn_pool.cc index 5877a035f041d..99aea9e453b2d 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) - : ResponseDecoderWrapper(response_decoder), - RequestEncoderWrapper(&parent.codec_client_->newStream(*this)), parent_(parent) { + : RequestEncoderWrapper(&parent.codec_client_->newStream(*this)), + ResponseDecoderWrapper(response_decoder), 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 b8b1fa803dfca..7bdeb288e6aa4 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 ResponseDecoderWrapper, - public RequestEncoderWrapper, + struct StreamWrapper : public RequestEncoderWrapper, + public ResponseDecoderWrapper, 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 deleted file mode 100644 index 694c2b7a6af24..0000000000000 --- a/source/common/http/response_decoder_impl_base.h +++ /dev/null @@ -1,40 +0,0 @@ -#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 f6506a7fb33e1..53773d3e33107 100644 --- a/source/common/quic/envoy_quic_client_stream.cc +++ b/source/common/quic/envoy_quic_client_stream.cc @@ -36,11 +36,6 @@ 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); @@ -212,11 +207,7 @@ 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 - if (Http::ResponseDecoder* decoder = getResponseDecoder()) { - decoder->decodeHeaders(std::move(headers), fin); - } else { - onResponseDecoderDead(); - } + response_decoder_->decodeHeaders(std::move(headers), fin); ConsumeHeaderList(); return; } @@ -233,18 +224,10 @@ 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; - if (Http::ResponseDecoder* decoder = getResponseDecoder()) { - decoder->decode1xxHeaders(std::move(headers)); - } else { - onResponseDecoderDead(); - } + response_decoder_->decode1xxHeaders(std::move(headers)); } else if (!is_special_1xx) { - if (Http::ResponseDecoder* decoder = getResponseDecoder()) { - decoder->decodeHeaders(std::move(headers), - /*end_stream=*/fin); - } else { - onResponseDecoderDead(); - } + response_decoder_->decodeHeaders(std::move(headers), + /*end_stream=*/fin); if (status == enumToInt(Http::Code::NotModified)) { got_304_response_ = true; } @@ -318,11 +301,7 @@ void EnvoyQuicClientStream::OnBodyAvailable() { // A stream error has occurred, stop processing. return; } - if (Http::ResponseDecoder* decoder = getResponseDecoder()) { - decoder->decodeData(*buffer, fin_read_and_no_trailers); - } else { - onResponseDecoderDead(); - } + response_decoder_->decodeData(*buffer, fin_read_and_no_trailers); } if (!sequencer()->IsClosed() || read_side_closed()) { @@ -369,11 +348,7 @@ void EnvoyQuicClientStream::maybeDecodeTrailers() { onStreamError(close_connection_upon_invalid_header_, transform_rst); return; } - if (Http::ResponseDecoder* decoder = getResponseDecoder()) { - decoder->decodeTrailers(std::move(trailers)); - } else { - onResponseDecoderDead(); - } + response_decoder_->decodeTrailers(std::move(trailers)); MarkTrailersConsumed(); } } @@ -457,15 +432,10 @@ 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()) { - if (Http::ResponseDecoder* decoder = getResponseDecoder()) { - decoder->decodeMetadata(metadataMapFromHeaderList(header_list)); - } else { - onResponseDecoderDead(); - } + response_decoder_->decodeMetadata(metadataMapFromHeaderList(header_list)); } } @@ -496,7 +466,7 @@ bool EnvoyQuicClientStream::hasPendingData() { return BufferedDataBytes() > 0; } // connect-udp". void EnvoyQuicClientStream::useCapsuleProtocol() { http_datagram_handler_ = std::make_unique(*this); - http_datagram_handler_->setStreamDecoder(getResponseDecoder()); + http_datagram_handler_->setStreamDecoder(response_decoder_); RegisterHttp3DatagramVisitor(http_datagram_handler_.get()); } #endif @@ -505,26 +475,5 @@ 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 5352f2b92bea5..67214eb9741f2 100644 --- a/source/common/quic/envoy_quic_client_stream.h +++ b/source/common/quic/envoy_quic_client_stream.h @@ -3,7 +3,6 @@ #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" @@ -26,7 +25,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); + void setResponseDecoder(Http::ResponseDecoder& decoder) { response_decoder_ = &decoder; } // Http::StreamEncoder Http::Http1StreamEncoderOptionsOptRef http1StreamEncoderOptions() override { @@ -93,12 +92,6 @@ 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 f6285de260684..19d559393d3b5 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -81,7 +81,6 @@ 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,8 +173,7 @@ 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 914382ab15f81..c6b30b08d03d1 100644 --- a/source/common/tcp_proxy/BUILD +++ b/source/common/tcp_proxy/BUILD @@ -29,7 +29,6 @@ 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 86a770a8517bd..661797dc65de0 100644 --- a/source/common/tcp_proxy/upstream.h +++ b/source/common/tcp_proxy/upstream.h @@ -17,7 +17,6 @@ #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" @@ -231,7 +230,8 @@ class HttpUpstream : public GenericUpstream, protected Http::StreamCallbacks { std::unique_ptr downstream_headers_; private: - class DecoderShim : public Http::ResponseDecoderImplBase { + Upstream::ClusterInfoConstSharedPtr cluster_; + class DecoderShim : public Http::ResponseDecoder { public: DecoderShim(HttpUpstream& parent) : parent_(parent) {} void decode1xxHeaders(Http::ResponseHeaderMapPtr&&) override {} @@ -351,7 +351,7 @@ class CombinedUpstream : public GenericUpstream, public Envoy::Router::RouterFil private: Http::StreamDecoderFilterCallbacks& decoder_filter_callbacks_; - class DecoderShim : public Http::ResponseDecoderImplBase { + class DecoderShim : public Http::ResponseDecoder { 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 62257e221fcc2..cef2da998723f 100644 --- a/source/extensions/bootstrap/reverse_tunnel/downstream_socket_interface/BUILD +++ b/source/extensions/bootstrap/reverse_tunnel/downstream_socket_interface/BUILD @@ -77,7 +77,6 @@ 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 23dbacb1db6c0..68828336a43d7 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,7 +12,6 @@ #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 { @@ -50,7 +49,7 @@ class SimpleConnReadFilter : public Network::ReadFilterBaseImpl, class RCConnectionWrapper : public Network::ConnectionCallbacks, public Event::DeferredDeletable, public Logger::Loggable, - public Http::ResponseDecoderImplBase, + public Http::ResponseDecoder, 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 9e60d1f00f3f0..a9e79751a3afc 100644 --- a/source/extensions/filters/udp/udp_proxy/BUILD +++ b/source/extensions/filters/udp/udp_proxy/BUILD @@ -42,7 +42,6 @@ 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 c1c921375f269..f2b6f64606fa9 100644 --- a/source/extensions/filters/udp/udp_proxy/udp_proxy_filter.h +++ b/source/extensions/filters/udp/udp_proxy/udp_proxy_filter.h @@ -21,7 +21,6 @@ #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" @@ -312,7 +311,7 @@ class HttpUpstreamImpl : public HttpUpstream, protected Http::StreamCallbacks { } private: - class ResponseDecoder : public Http::ResponseDecoderImplBase { + class ResponseDecoder : public Http::ResponseDecoder { public: ResponseDecoder(HttpUpstreamImpl& parent) : parent_(parent) {} diff --git a/source/extensions/health_checkers/grpc/BUILD b/source/extensions/health_checkers/grpc/BUILD index b3688169d1f2d..c06822d4e75c9 100644 --- a/source/extensions/health_checkers/grpc/BUILD +++ b/source/extensions/health_checkers/grpc/BUILD @@ -20,7 +20,6 @@ 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 a3e54bb454792..326035eac59b1 100644 --- a/source/extensions/health_checkers/grpc/health_checker_impl.h +++ b/source/extensions/health_checkers/grpc/health_checker_impl.h @@ -17,7 +17,6 @@ #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" @@ -53,7 +52,7 @@ class GrpcHealthCheckerImpl : public HealthCheckerImplBase { private: struct GrpcActiveHealthCheckSession : public ActiveHealthCheckSession, - public Http::ResponseDecoderImplBase, + public Http::ResponseDecoder, 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 b759a789d7568..6ad5ed7773564 100644 --- a/source/extensions/health_checkers/http/BUILD +++ b/source/extensions/health_checkers/http/BUILD @@ -18,7 +18,6 @@ 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 68fd6214f9166..81fae53f73330 100644 --- a/source/extensions/health_checkers/http/health_checker_impl.h +++ b/source/extensions/health_checkers/http/health_checker_impl.h @@ -17,7 +17,6 @@ #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" @@ -78,7 +77,7 @@ class HttpHealthCheckerImpl : public HealthCheckerImplBase { private: struct HttpActiveHealthCheckSession : public ActiveHealthCheckSession, - public Http::ResponseDecoderImplBase, + public Http::ResponseDecoder, public Http::StreamCallbacks { HttpActiveHealthCheckSession(HttpHealthCheckerImpl& parent, const HostSharedPtr& host); ~HttpActiveHealthCheckSession() override; diff --git a/test/common/http/BUILD b/test/common/http/BUILD index 97f77b1eb390e..3c522bcda3803 100644 --- a/test/common/http/BUILD +++ b/test/common/http/BUILD @@ -162,7 +162,6 @@ 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 e5ffb0acdd615..35bf742a03f02 100644 --- a/test/common/http/codec_wrappers_test.cc +++ b/test/common/http/codec_wrappers_test.cc @@ -1,7 +1,6 @@ #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::_; @@ -11,54 +10,23 @@ namespace Http { class MockResponseDecoderWrapper : public ResponseDecoderWrapper { public: - explicit MockResponseDecoderWrapper(MockResponseDecoder& inner_decoder) - : ResponseDecoderWrapper(inner_decoder) {} + MockResponseDecoderWrapper() : ResponseDecoderWrapper(inner_decoder_) {} + MockResponseDecoder& innerEncoder() { return inner_decoder_; } void onDecodeComplete() override {} void onPreDecodeComplete() override {} + +private: + MockResponseDecoder inner_decoder_; }; TEST(MockResponseDecoderWrapper, dumpState) { - MockResponseDecoder inner_decoder; - MockResponseDecoderWrapper wrapper(inner_decoder); + MockResponseDecoderWrapper wrapper; std::stringstream os; - EXPECT_CALL(inner_decoder, dumpState(_, _)); + EXPECT_CALL(wrapper.innerEncoder(), 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 5a2202a9921a8..bc61e050ff7d6 100644 --- a/test/common/quic/BUILD +++ b/test/common/quic/BUILD @@ -152,7 +152,6 @@ 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 4522e5d788ff0..473dd22af5e05 100644 --- a/test/common/quic/envoy_quic_client_stream_test.cc +++ b/test/common/quic/envoy_quic_client_stream_test.cc @@ -8,7 +8,6 @@ #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" @@ -742,115 +741,6 @@ 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 ce83293aed3a0..9654ce9e824ee 100644 --- a/test/integration/integration_stream_decoder.h +++ b/test/integration/integration_stream_decoder.h @@ -10,7 +10,6 @@ #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" @@ -22,8 +21,7 @@ namespace Envoy { /** * Stream decoder wrapper used during integration testing. */ -class IntegrationStreamDecoder : public Http::ResponseDecoderImplBase, - public Http::StreamCallbacks { +class IntegrationStreamDecoder : public Http::ResponseDecoder, public Http::StreamCallbacks { public: IntegrationStreamDecoder(Event::Dispatcher& dispatcher); ~IntegrationStreamDecoder() override; diff --git a/test/integration/utility.h b/test/integration/utility.h index 9db2c3d6d0bea..39c2c664a3d0a 100644 --- a/test/integration/utility.h +++ b/test/integration/utility.h @@ -17,7 +17,6 @@ #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" @@ -30,7 +29,7 @@ namespace Envoy { /** * A buffering response decoder used for testing. */ -class BufferingStreamDecoder : public Http::ResponseDecoderImplBase, public Http::StreamCallbacks { +class BufferingStreamDecoder : public Http::ResponseDecoder, 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 af75f948d3445..416d71fe5a2c3 100644 --- a/test/mocks/http/BUILD +++ b/test/mocks/http/BUILD @@ -104,7 +104,6 @@ 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 2f265b158b82e..e1c8585a49a3c 100644 --- a/test/mocks/http/stream_decoder.h +++ b/test/mocks/http/stream_decoder.h @@ -1,8 +1,6 @@ #pragma once #include "envoy/http/codec.h" -#include "source/common/http/response_decoder_impl_base.h" - #include "gmock/gmock.h" namespace Envoy { @@ -45,7 +43,7 @@ class MockRequestDecoder : public RequestDecoder { MOCK_METHOD(RequestDecoderHandlePtr, getRequestDecoderHandle, ()); }; -class MockResponseDecoder : public ResponseDecoderImplBase { +class MockResponseDecoder : public ResponseDecoder { public: MockResponseDecoder(); ~MockResponseDecoder() override; diff --git a/tools/spelling/spelling_dictionary.txt b/tools/spelling/spelling_dictionary.txt index d2bd26af0b031..2fbc45cc57e1c 100644 --- a/tools/spelling/spelling_dictionary.txt +++ b/tools/spelling/spelling_dictionary.txt @@ -493,7 +493,6 @@ TTLs TX TXT UA -UAF UBSAN UDP UDS @@ -974,7 +973,6 @@ linkability linkable linux livelock -liveness llvm loc localhost