response decoder: add new interfaces for life time tracking of response decoder

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;
 };
 
 /**

envoy/router/BUILD

@@ -95,6 +95,7 @@ envoy_cc_library(
         "//envoy/tracing:tracer_interface",
         "//envoy/upstream:resource_manager_interface",
         "//envoy/upstream:retry_interface",
+        "//source/common/http:response_decoder_impl_base",
         "//source/common/protobuf",
         "//source/common/protobuf:utility_lib",
         "@com_google_absl//absl/types:optional",

envoy/router/router.h

@@ -27,6 +27,7 @@
 #include "envoy/upstream/resource_manager.h"
 #include "envoy/upstream/retry.h"
 
+#include "source/common/http/response_decoder_impl_base.h"
 #include "source/common/protobuf/protobuf.h"
 #include "source/common/protobuf/utility.h"
 
@@ -1490,7 +1491,7 @@ class GenericConnPool {
  * An API for the interactions the upstream stream needs to have with the downstream stream
  * and/or router components
  */
-class UpstreamToDownstream : public Http::ResponseDecoder, public Http::StreamCallbacks {
+class UpstreamToDownstream : public Http::ResponseDecoderImplBase, public Http::StreamCallbacks {
 public:
   /**
    * @return return the route for the downstream stream.

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_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,45 @@ 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_handle_(inner.createResponseDecoderHandle()), inner_(&inner) {}
 
   /**
    * Consumers of the wrapper generally want to know when a decode is complete. This is called
@@ -59,7 +87,29 @@ class ResponseDecoderWrapper : public ResponseDecoder {
   virtual void onPreDecodeComplete() PURE;
   virtual void onDecodeComplete() PURE;
 
-  ResponseDecoder& inner_;
+  ResponseDecoderHandlePtr inner_handle_;
+  Http::ResponseDecoder* inner_ = nullptr;
+
+private:
+  Http::ResponseDecoder* getInnerDecoder() const {
+    if (!Runtime::runtimeFeatureEnabled("envoy.reloadable_features.use_response_decoder_handle")) {
+      return inner_;
+    }
+    if (inner_handle_) {
+      if (OptRef<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/http1/conn_pool.cc

@@ -25,8 +25,8 @@ namespace Http {
 namespace Http1 {
 
 ActiveClient::StreamWrapper::StreamWrapper(ResponseDecoder& response_decoder, ActiveClient& parent)
-    : RequestEncoderWrapper(&parent.codec_client_->newStream(*this)),
-      ResponseDecoderWrapper(response_decoder), parent_(parent) {
+    : ResponseDecoderWrapper(response_decoder),
+      RequestEncoderWrapper(&parent.codec_client_->newStream(*this)), parent_(parent) {
   RequestEncoderWrapper::inner_encoder_->getStream().addCallbacks(*this);
 }
 

source/common/http/http1/conn_pool.h

@@ -36,8 +36,8 @@ class ActiveClient : public Envoy::Http::ActiveClient {
     Envoy::Http::ActiveClient::releaseResources();
   }
 
-  struct StreamWrapper : public RequestEncoderWrapper,
-                         public ResponseDecoderWrapper,
+  struct StreamWrapper : public ResponseDecoderWrapper,
+                         public RequestEncoderWrapper,
                          public StreamCallbacks,
                          public Event::DeferredDeletable,
                          protected Logger::Loggable<Logger::Id::pool> {

source/common/http/response_decoder_impl_base.h

@@ -0,0 +1,40 @@
+#pragma once
+
+#include <memory>
+
+#include "envoy/http/codec.h"
+
+namespace Envoy {
+namespace Http {
+
+class ResponseDecoderHandleImpl : public ResponseDecoderHandle {
+public:
+  ResponseDecoderHandleImpl(std::weak_ptr<bool> live_trackable, ResponseDecoder& decoder)
+      : live_trackable_(live_trackable), decoder_(decoder) {}
+
+  OptRef<ResponseDecoder> get() override {
+    if (live_trackable_.lock()) {
+      return decoder_;
+    }
+    return {};
+  }
+
+private:
+  std::weak_ptr<bool> live_trackable_;
+  ResponseDecoder& decoder_;
+};
+
+class ResponseDecoderImplBase : public ResponseDecoder {
+public:
+  ResponseDecoderImplBase() : live_trackable_(std::make_shared<bool>(true)) {}
+
+  ResponseDecoderHandlePtr createResponseDecoderHandle() override {
+    return std::make_unique<ResponseDecoderHandleImpl>(live_trackable_, *this);
+  }
+
+private:
+  std::shared_ptr<bool> live_trackable_;
+};
+
+} // namespace Http
+} // namespace Envoy