From dc4cc8f096c9ffce40008cc0a385100349a3f1c9 Mon Sep 17 00:00:00 2001 From: Boteng Yao Date: Tue, 5 Nov 2024 21:20:29 +0000 Subject: [PATCH] http/1: fix sending overload crash when request is reset Signed-off-by: Boteng Yao Signed-off-by: Ryan Northey --- changelogs/current.yaml | 3 +++ source/common/http/http1/codec_impl.cc | 4 +++ source/common/http/http1/codec_impl.h | 1 - test/common/http/http1/codec_impl_test.cc | 33 +++++++++++++++++++++++ 4 files changed, 40 insertions(+), 1 deletion(-) diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 9ecf0d6e48ce..4e0f2e4d9092 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -8,6 +8,9 @@ minor_behavior_changes: bug_fixes: # *Changes expected to improve the state of the world and are unlikely to have negative effects* +- area: http/1 + change: | + Fixes sending overload crashes when HTTP/1 request is reset. removed_config_or_runtime: # *Normally occurs at the end of the* :ref:`deprecation period ` diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index be3445209e80..786556c38756 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -1363,6 +1363,10 @@ Status ServerConnectionImpl::sendProtocolError(absl::string_view details) { if (active_request_ == nullptr) { RETURN_IF_ERROR(onMessageBeginImpl()); } + + if (resetStreamCalled()) { + return okStatus(); + } ASSERT(active_request_); active_request_->response_encoder_.setDetails(details); diff --git a/source/common/http/http1/codec_impl.h b/source/common/http/http1/codec_impl.h index 59ba6bfd8e99..241f1e4a63ba 100644 --- a/source/common/http/http1/codec_impl.h +++ b/source/common/http/http1/codec_impl.h @@ -542,7 +542,6 @@ class ServerConnectionImpl : public ServerConnection, public ConnectionImpl { return *absl::get(headers_or_trailers_); } void allocHeaders(StatefulHeaderKeyFormatterPtr&& formatter) override { - ASSERT(nullptr == absl::get(headers_or_trailers_)); ASSERT(!processing_trailers_); auto headers = RequestHeaderMapImpl::create(max_headers_kb_, max_headers_count_); headers->setFormatter(std::move(formatter)); diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index 89c649961247..ec5f9e678bb0 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -2428,6 +2428,39 @@ TEST_P(Http1ServerConnectionImplTest, LoadShedPointCanCloseConnectionOnDispatchO EXPECT_TRUE(isEnvoyOverloadError(status)); } +TEST_P(Http1ServerConnectionImplTest, LoadShedPointForAlreadyResetStream) { + InSequence sequence; + + Server::MockLoadShedPoint mock_abort_dispatch; + EXPECT_CALL(overload_manager_, getLoadShedPoint(_)).WillOnce(Return(&mock_abort_dispatch)); + initialize(); + + EXPECT_CALL(mock_abort_dispatch, shouldShedLoad()).WillOnce(Return(false)); + + NiceMock decoder; + Http::ResponseEncoder* response_encoder = nullptr; + EXPECT_CALL(callbacks_, newStream(_, _)) + .WillOnce(Invoke([&](ResponseEncoder& encoder, bool) -> RequestDecoder& { + response_encoder = &encoder; + return decoder; + })); + + Buffer::OwnedImpl request_line_buffer("GET / HTTP/1.1\r\n"); + auto status = codec_->dispatch(request_line_buffer); + EXPECT_EQ(0, request_line_buffer.length()); + + EXPECT_CALL(mock_abort_dispatch, shouldShedLoad()).WillRepeatedly(Return(true)); + Buffer::OwnedImpl headers_buffer("final-header: value\r\n\r\n"); + + // The reset stream can be triggered in the middle of dispatching. + connection_.dispatcher_.post( + [&] { response_encoder->getStream().resetStream(StreamResetReason::LocalReset); }); + + status = codec_->dispatch(headers_buffer); + EXPECT_FALSE(status.ok()); + EXPECT_TRUE(isEnvoyOverloadError(status)); +} + TEST_P(Http1ServerConnectionImplTest, LoadShedPointCanCloseConnectionOnDispatchOfContinuingStream) { Server::MockLoadShedPoint mock_abort_dispatch; EXPECT_CALL(overload_manager_, getLoadShedPoint(_)).WillOnce(Return(&mock_abort_dispatch));