From 301e4984a82b2fa03fa62faaf77cfbae1074d402 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 0da251fa22..44a03e8c05 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -2,6 +2,9 @@ date: June 30, 2024 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 c0c5f9a159..fbe6c72ba3 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -1366,6 +1366,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 b59e1931a7..587b94b39d 100644 --- a/source/common/http/http1/codec_impl.h +++ b/source/common/http/http1/codec_impl.h @@ -541,7 +541,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 57c4b744c2..37bc1ae247 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -2422,6 +2422,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));