http: Fix ASSERT failure and infinite loop when attempting to unset r…

…eadDisable state on a closed connection. (#9509)

Description: Handle calls to ConnectionImpl::readDisable on Closed connections more gracefully. There are several places under source/common, including ClientConnectionImpl::onMessageComplete, where Connection::readDisable(false) is called on a loop to clear the readDisable state as part of the process of preparing upstream connections for reuse. It is possible to end up in an infinite loop if proxying an HTTP/1 response is framed by connection close, the call to onData triggers readDisable(true), and there is a pending PostIoAction::Close action. The closeSocket call on the upstreamConnection triggers the call to ClientConnectionImpl::onMessageComplete
Risk Level: medium
Testing: unit and integration tests
Docs Changes: n/a
Release Notes: n/a
Fixes #9508

Signed-off-by: Antonio Vicente <avd@google.com>

source/common/http/http1/codec_impl.cc

@@ -944,7 +944,7 @@ void ClientConnectionImpl::onMessageComplete(HeaderMapImplPtr&& trailers) {
     // reused, unwind any outstanding readDisable() calls here. Only do this if there are no
     // pipelined responses remaining. Also do this before we dispatch end_stream in case the caller
     // immediately reuses the connection.
-    if (pending_responses_.empty()) {
+    if (connection_.state() == Network::Connection::State::Open && pending_responses_.empty()) {
       while (!connection_.readEnabled()) {
         connection_.readDisable(false);
       }

source/common/network/connection_impl.cc

@@ -279,13 +279,12 @@ void ConnectionImpl::enableHalfClose(bool enabled) {
 }
 
 void ConnectionImpl::readDisable(bool disable) {
+  // Calls to readEnabled on a closed socket are considered to be an error.
   ASSERT(state() == State::Open);
-  if (state() != State::Open || file_event_ == nullptr) {
-    // If readDisable is called on a closed connection in error, do not crash.
-    return;
-  }
+  ASSERT(file_event_ != nullptr);
 
-  ENVOY_CONN_LOG(trace, "readDisable: enabled={} disable={}", *this, read_enabled_, disable);
+  ENVOY_CONN_LOG(trace, "readDisable: enabled={} disable={} state={}", *this, read_enabled_,
+                 disable, static_cast<int>(state()));
 
   // When we disable reads, we still allow for early close notifications (the equivalent of
   // EPOLLRDHUP for an epoll backend). For backends that support it, this allows us to apply
@@ -302,6 +301,11 @@ void ConnectionImpl::readDisable(bool disable) {
     ASSERT(read_enabled_);
     read_enabled_ = false;
 
+    if (state() != State::Open || file_event_ == nullptr) {
+      // If readDisable is called on a closed connection, do not crash.
+      return;
+    }
+
     // If half-close semantics are enabled, we never want early close notifications; we
     // always want to read all available data, even if the other side has closed.
     if (detect_early_close_ && !enable_half_close_) {
@@ -316,6 +320,12 @@ void ConnectionImpl::readDisable(bool disable) {
     }
     ASSERT(!read_enabled_);
     read_enabled_ = true;
+
+    if (state() != State::Open || file_event_ == nullptr) {
+      // If readDisable is called on a closed connection, do not crash.
+      return;
+    }
+
     // We never ask for both early close and read at the same time. If we are reading, we want to
     // consume all available data.
     file_event_->setEnabled(Event::FileReadyType::Read | Event::FileReadyType::Write);
@@ -342,7 +352,12 @@ void ConnectionImpl::raiseEvent(ConnectionEvent event) {
   }
 }
 
-bool ConnectionImpl::readEnabled() const { return read_enabled_; }
+bool ConnectionImpl::readEnabled() const {
+  // Calls to readEnabled on a closed socket are considered to be an error.
+  ASSERT(state() == State::Open);
+  ASSERT(file_event_ != nullptr);
+  return read_enabled_;
+}
 
 void ConnectionImpl::addBytesSentCallback(BytesSentCb cb) {
   bytes_sent_callbacks_.emplace_back(cb);

test/common/network/connection_impl_test.cc

@@ -577,15 +577,38 @@ TEST_P(ConnectionImplTest, KickUndone) {
   disconnect(true);
 }
 
-// Regression test for (at least one failure mode of)
-// https://github.com/envoyproxy/envoy/issues/3639 where readDisable on a close
-// connection caused a crash.
-TEST_P(ConnectionImplTest, ReadDisableAfterClose) {
+// Ensure that calls to readDisable on a closed connection are handled gracefully. Known past issues
+// include a crash on https://github.com/envoyproxy/envoy/issues/3639, and ASSERT failure followed
+// by infinite loop in https://github.com/envoyproxy/envoy/issues/9508
+TEST_P(ConnectionImplTest, ReadDisableAfterCloseHandledGracefully) {
   setUpBasicConnection();
-  disconnect(false);
 
+  client_connection_->readDisable(true);
+  client_connection_->readDisable(false);
+
+  client_connection_->readDisable(true);
+  client_connection_->readDisable(true);
+  client_connection_->readDisable(false);
+  client_connection_->readDisable(false);
+
+  client_connection_->readDisable(true);
+  client_connection_->readDisable(true);
+  disconnect(false);
+#ifndef NDEBUG
+  // When running in debug mode, verify that calls to readDisable and readEnabled on a closed socket
+  // trigger ASSERT failures.
+  EXPECT_DEBUG_DEATH(client_connection_->readEnabled(), "");
   EXPECT_DEBUG_DEATH(client_connection_->readDisable(true), "");
   EXPECT_DEBUG_DEATH(client_connection_->readDisable(false), "");
+#else
+  // When running in release mode, verify that calls to readDisable change the readEnabled state.
+  client_connection_->readDisable(false);
+  client_connection_->readDisable(true);
+  client_connection_->readDisable(false);
+  EXPECT_FALSE(client_connection_->readEnabled());
+  client_connection_->readDisable(false);
+  EXPECT_TRUE(client_connection_->readEnabled());
+#endif
 }
 
 TEST_P(ConnectionImplTest, EarlyCloseOnReadDisabledConnection) {

test/integration/integration_test.cc

@@ -224,6 +224,31 @@ TEST_P(IntegrationTest, RouterUpstreamDisconnectBeforeResponseComplete) {
   testRouterUpstreamDisconnectBeforeResponseComplete();
 }
 
+// Regression test for https://github.com/envoyproxy/envoy/issues/9508
+TEST_P(IntegrationTest, ResponseFramedByConnectionCloseWithReadLimits) {
+  // Set a small buffer limit on the downstream in order to trigger a call to trigger readDisable on
+  // the upstream when proxying the response. Upstream limit needs to be larger so that
+  // RawBufferSocket::doRead reads the response body and detects the upstream close in the same call
+  // stack.
+  config_helper_.setBufferLimits(100000, 1);
+  initialize();
+
+  codec_client_ = makeHttpConnection(makeClientConnection((lookupPort("http"))));
+  fake_upstreams_[0]->set_allow_unexpected_disconnects(true);
+
+  auto response = codec_client_->makeHeaderOnlyRequest(default_request_headers_);
+  waitForNextUpstreamRequest();
+  // Disable chunk encoding to trigger framing by connection close.
+  upstream_request_->encodeHeaders(Http::TestHeaderMapImpl{{":status", "200"}, {":no-chunks", "1"}},
+                                   false);
+  upstream_request_->encodeData(512, true);
+  ASSERT_TRUE(fake_upstream_connection_->close());
+
+  response->waitForEndStream();
+
+  EXPECT_TRUE(response->complete());
+}
+
 TEST_P(IntegrationTest, RouterDownstreamDisconnectBeforeRequestComplete) {
   testRouterDownstreamDisconnectBeforeRequestComplete();
 }