Skip to content

Commit 40970bd

Browse files
Raúl Gutiérrez Segalészuercher
Raúl Gutiérrez Segalés
authored and
zuercher
committed
thrift-proxy: fix crash (#9089)
After Router::onEvent() handles a local or remote close, do not attempt to close the connection a second time during Router::onDestroy(). Risk Level: low Testing: unit/integration tests Doc Changes: n/a Release Notes: n/a Fixes: #9037 Signed-off-by: Raul Gutierrez Segales <[email protected]>
1 parent fb82359 commit 40970bd

File tree

4 files changed

+75
-7
lines changed

4 files changed

+75
-7
lines changed

‎source/extensions/filters/network/thrift_proxy/router/router_impl.cc‎

Lines changed: 21 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -184,8 +184,8 @@ RouteConstSharedPtr RouteMatcher::route(const MessageMetadata& metadata,
184184
void Router::onDestroy() {
185185
if (upstream_request_ != nullptr) {
186186
upstream_request_->resetStream();
187+
cleanup();
187188
}
188-
cleanup();
189189
}
190190

191191
void Router::setDecoderFilterCallbacks(ThriftFilters::DecoderFilterCallbacks& callbacks) {
@@ -354,17 +354,21 @@ void Router::onEvent(Network::ConnectionEvent event) {
354354

355355
switch (event) {
356356
case Network::ConnectionEvent::RemoteClose:
357+
ENVOY_STREAM_LOG(debug, "upstream remote close", *callbacks_);
357358
upstream_request_->onResetStream(
358359
Tcp::ConnectionPool::PoolFailureReason::RemoteConnectionFailure);
359360
break;
360361
case Network::ConnectionEvent::LocalClose:
362+
ENVOY_STREAM_LOG(debug, "upstream local close", *callbacks_);
361363
upstream_request_->onResetStream(
362364
Tcp::ConnectionPool::PoolFailureReason::LocalConnectionFailure);
363365
break;
364366
default:
365367
// Connected is consumed by the connection pool.
366368
NOT_REACHED_GCOVR_EXCL_LINE;
367369
}
370+
371+
upstream_request_->releaseConnection(false);
368372
}
369373

370374
const Network::Connection* Router::downstreamConnection() const {
@@ -389,7 +393,11 @@ Router::UpstreamRequest::UpstreamRequest(Router& parent, Tcp::ConnectionPool::In
389393
protocol_(NamedProtocolConfigFactory::getFactory(protocol_type).createProtocol()),
390394
request_complete_(false), response_started_(false), response_complete_(false) {}
391395

392-
Router::UpstreamRequest::~UpstreamRequest() = default;
396+
Router::UpstreamRequest::~UpstreamRequest() {
397+
if (conn_pool_handle_) {
398+
conn_pool_handle_->cancel(Tcp::ConnectionPool::CancelPolicy::Default);
399+
}
400+
}
393401

394402
FilterStatus Router::UpstreamRequest::start() {
395403
Tcp::ConnectionPool::Cancellable* handle = conn_pool_.newConnection(*this);
@@ -407,18 +415,24 @@ FilterStatus Router::UpstreamRequest::start() {
407415
return FilterStatus::Continue;
408416
}
409417

410-
void Router::UpstreamRequest::resetStream() {
418+
void Router::UpstreamRequest::releaseConnection(const bool close) {
411419
if (conn_pool_handle_) {
412420
conn_pool_handle_->cancel(Tcp::ConnectionPool::CancelPolicy::Default);
421+
conn_pool_handle_ = nullptr;
413422
}
414423

415-
if (conn_data_ != nullptr) {
416-
conn_state_ = nullptr;
417-
conn_data_->connection().close(Network::ConnectionCloseType::NoFlush);
418-
conn_data_.reset();
424+
conn_state_ = nullptr;
425+
426+
// The event triggered by close will also release this connection so clear conn_data_ before
427+
// closing.
428+
auto conn_data = std::move(conn_data_);
429+
if (close && conn_data != nullptr) {
430+
conn_data->connection().close(Network::ConnectionCloseType::NoFlush);
419431
}
420432
}
421433

434+
void Router::UpstreamRequest::resetStream() { releaseConnection(true); }
435+
422436
void Router::UpstreamRequest::onPoolFailure(Tcp::ConnectionPool::PoolFailureReason reason,
423437
Upstream::HostDescriptionConstSharedPtr host) {
424438
conn_pool_handle_ = nullptr;

‎source/extensions/filters/network/thrift_proxy/router/router_impl.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -206,6 +206,7 @@ class Router : public Tcp::ConnectionPool::UpstreamCallbacks,
206206

207207
FilterStatus start();
208208
void resetStream();
209+
void releaseConnection(bool close);
209210

210211
// Tcp::ConnectionPool::Callbacks
211212
void onPoolFailure(Tcp::ConnectionPool::PoolFailureReason reason,

‎test/extensions/filters/network/thrift_proxy/integration_test.cc‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
#include "gtest/gtest.h"
88

99
using testing::Combine;
10+
using testing::HasSubstr;
1011
using ::testing::TestParamInfo;
1112
using testing::Values;
1213

@@ -301,6 +302,37 @@ TEST_P(ThriftConnManagerIntegrationTest, EarlyCloseWithUpstream) {
301302
EXPECT_EQ(1U, counter->value());
302303
}
303304

305+
// Regression test for https://github.com/envoyproxy/envoy/issues/9037.
306+
TEST_P(ThriftConnManagerIntegrationTest, EarlyUpstreamClose) {
307+
initializeCall(DriverMode::Success);
308+
309+
const std::string partial_request =
310+
request_bytes_.toString().substr(0, request_bytes_.length() - 5);
311+
312+
IntegrationTcpClientPtr tcp_client = makeTcpConnection(lookupPort("listener_0"));
313+
tcp_client->write(request_bytes_.toString());
314+
315+
FakeUpstream* expected_upstream = getExpectedUpstream(false);
316+
FakeRawConnectionPtr fake_upstream_connection;
317+
ASSERT_TRUE(expected_upstream->waitForRawConnection(fake_upstream_connection));
318+
319+
std::string data;
320+
ASSERT_TRUE(fake_upstream_connection->waitForData(request_bytes_.length(), &data));
321+
Buffer::OwnedImpl upstream_request(data);
322+
EXPECT_EQ(request_bytes_.toString(), upstream_request.toString());
323+
324+
ASSERT_TRUE(fake_upstream_connection->close());
325+
326+
tcp_client->waitForDisconnect();
327+
328+
EXPECT_THAT(tcp_client->data(), HasSubstr("connection failure"));
329+
330+
Stats::CounterSharedPtr counter = test_server_->counter("thrift.thrift_stats.request_call");
331+
EXPECT_EQ(1U, counter->value());
332+
counter = test_server_->counter("thrift.thrift_stats.response_exception");
333+
EXPECT_EQ(1U, counter->value());
334+
}
335+
304336
TEST_P(ThriftConnManagerIntegrationTest, Oneway) {
305337
initializeOneway();
306338

‎test/extensions/filters/network/thrift_proxy/router_test.cc‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -603,6 +603,27 @@ TEST_F(ThriftRouterTest, UnexpectedUpstreamLocalClose) {
603603
router_->onEvent(Network::ConnectionEvent::RemoteClose);
604604
}
605605

606+
// Regression test for https://github.com/envoyproxy/envoy/issues/9037.
607+
TEST_F(ThriftRouterTest, DontCloseConnectionTwice) {
608+
initializeRouter();
609+
startRequest(MessageType::Call);
610+
connectUpstream();
611+
sendTrivialStruct(FieldType::String);
612+
613+
EXPECT_CALL(callbacks_, sendLocalReply(_, _))
614+
.WillOnce(Invoke([&](const DirectResponse& response, bool end_stream) -> void {
615+
auto& app_ex = dynamic_cast<const AppException&>(response);
616+
EXPECT_EQ(AppExceptionType::InternalError, app_ex.type_);
617+
EXPECT_THAT(app_ex.what(), ContainsRegex(".*connection failure.*"));
618+
EXPECT_TRUE(end_stream);
619+
}));
620+
router_->onEvent(Network::ConnectionEvent::RemoteClose);
621+
622+
// Connection close shouldn't happen in onDestroy(), since it's been handled.
623+
EXPECT_CALL(upstream_connection_, close(Network::ConnectionCloseType::NoFlush)).Times(0);
624+
destroyRouter();
625+
}
626+
606627
TEST_F(ThriftRouterTest, UnexpectedRouterDestroyBeforeUpstreamConnect) {
607628
initializeRouter();
608629
startRequest(MessageType::Call);

0 commit comments

Comments
 (0)