Revert "response decoder: add new interfaces for life time tracking o…

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

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

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",

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.

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(

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,45 +29,28 @@ 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();
     }
   }
 
   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<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);
-  }
+  ResponseDecoder& inner_;
 };
 
 /**

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

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<Logger::Id::pool> {