response decoder: Roll forward ResponseDecoderHandle

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

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<ResponseDecoder> get() PURE;
+};
+using ResponseDecoderHandlePtr = std::unique_ptr<ResponseDecoderHandle>;
+
 /**
  * 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;
 };
 
 /**

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(

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<ActiveRequest>(*this, std::move(response_decoder_handle)));
+}
+
 RequestEncoder& CodecClient::newStream(ResponseDecoder& response_decoder) {
-  ActiveRequestPtr request(new ActiveRequest(*this, response_decoder));
+  return enlistAndCreateEncoder(std::make_unique<ActiveRequest>(*this, response_decoder));
+}
+
+RequestEncoder& CodecClient::enlistAndCreateEncoder(ActiveRequestPtr request) {
   request->setEncoder(codec_->newStream(*request));
   LinkedList::moveIntoList(std::move(request), active_requests_);
 

source/common/http/codec_client.h

@@ -118,6 +118,16 @@ class CodecClient : protected Logger::Loggable<Logger::Id::client>,
    */
   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<Logger::Id::client>,
       }
     }
 
+    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<Logger::Id::client>,
   void onBelowWriteBufferLowWatermark() override {
     codec_->onUnderlyingConnectionBelowWriteBufferLowWatermark();
   }
+  RequestEncoder& enlistAndCreateEncoder(ActiveRequestPtr request);
 
   std::list<ActiveRequestPtr> active_requests_;
   Http::ConnectionCallbacks* codec_callbacks_{};

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,28 +40,51 @@ 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();
     }
   }
 
   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<ResponseDecoder> 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);
+  }
 };
 
 /**

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<ActiveClient*>(&client);
   auto& http_context = typedContext<HttpAttachContext>(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

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<Http::Protocol> 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;

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<ResponseDecoder> 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;
   }

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.