diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index ef6ece4380c2..ac1826a534b4 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -894,7 +894,7 @@ void ClientConnectionImpl::onMessageComplete() { // 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); } diff --git a/source/common/network/connection_impl.cc b/source/common/network/connection_impl.cc index 15e76a9128e0..d7baebb26ef2 100644 --- a/source/common/network/connection_impl.cc +++ b/source/common/network/connection_impl.cc @@ -291,13 +291,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(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 @@ -314,6 +313,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_) { @@ -328,6 +332,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); @@ -358,7 +368,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::addConnectionCallbacks(ConnectionCallbacks& cb) { callbacks_.push_back(&cb); } diff --git a/test/common/network/connection_impl_test.cc b/test/common/network/connection_impl_test.cc index 272884e3f518..5f9a6dadcc47 100644 --- a/test/common/network/connection_impl_test.cc +++ b/test/common/network/connection_impl_test.cc @@ -570,15 +570,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) { diff --git a/test/integration/integration_test.cc b/test/integration/integration_test.cc index 91f38de55137..19f5b172edec 100644 --- a/test/integration/integration_test.cc +++ b/test/integration/integration_test.cc @@ -219,6 +219,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(); }