diff --git a/source/common/http/conn_pool_base.cc b/source/common/http/conn_pool_base.cc index 19c057a53edb..3ffa8fb9bb45 100644 --- a/source/common/http/conn_pool_base.cc +++ b/source/common/http/conn_pool_base.cc @@ -55,7 +55,8 @@ HttpConnPoolImplBase::HttpConnPoolImplBase( : Envoy::ConnectionPool::ConnPoolImplBase( host, priority, dispatcher, options, wrapTransportSocketOptions(transport_socket_options, protocols), state), - random_generator_(random_generator) { + random_generator_(random_generator), + upstream_ready_cb_(dispatcher_.createSchedulableCallback([this]() { onUpstreamReady(); })) { ASSERT(!protocols.empty()); } diff --git a/source/common/http/conn_pool_base.h b/source/common/http/conn_pool_base.h index 152dc6df86d5..7968bee68ebe 100644 --- a/source/common/http/conn_pool_base.h +++ b/source/common/http/conn_pool_base.h @@ -80,10 +80,14 @@ class HttpConnPoolImplBase : public Envoy::ConnectionPool::ConnPoolImplBase, virtual CodecClientPtr createCodecClient(Upstream::Host::CreateConnectionData& data) PURE; Random::RandomGenerator& randomGenerator() { return random_generator_; } + Event::SchedulableCallback* upstreamReadyCallback() { return upstream_ready_cb_.get(); } protected: friend class ActiveClient; Random::RandomGenerator& random_generator_; + + // onUpstreamReady callback only used by HTTP/1.1 implementation. + Event::SchedulableCallbackPtr upstream_ready_cb_; }; // An implementation of Envoy::ConnectionPool::ActiveClient for HTTP/1.1 and HTTP/2 diff --git a/source/common/http/http1/conn_pool.cc b/source/common/http/http1/conn_pool.cc index cc69ab976c0e..a6afd859be09 100644 --- a/source/common/http/http1/conn_pool.cc +++ b/source/common/http/http1/conn_pool.cc @@ -76,7 +76,10 @@ void ActiveClient::StreamWrapper::onDecodeComplete() { parent_.codec_client_->close(); } else { auto* pool = &parent_.parent(); - pool->dispatcher().post([pool]() -> void { pool->onUpstreamReady(); }); + if (pool->hasPendingStreams()) { + // SchedulableCallback guarantees callback will only be scheduled if not already enabled. + pool->upstreamReadyCallback()->scheduleCallbackCurrentIteration(); + } parent_.stream_wrapper_.reset(); pool->checkForDrained(); diff --git a/test/common/http/http1/conn_pool_test.cc b/test/common/http/http1/conn_pool_test.cc index b86e9c426a31..1e20cd7b7999 100644 --- a/test/common/http/http1/conn_pool_test.cc +++ b/test/common/http/http1/conn_pool_test.cc @@ -52,7 +52,8 @@ class ConnPoolImplForTest : public Event::TestUsingSimulatedTime, public FixedHt public: ConnPoolImplForTest(Event::MockDispatcher& dispatcher, Upstream::ClusterInfoConstSharedPtr cluster, - Random::RandomGenerator& random_generator) + Random::RandomGenerator& random_generator, + NiceMock* upstream_ready_cb) : FixedHttpConnPoolImpl( Upstream::makeTestHost(cluster, "tcp://127.0.0.1:9000", dispatcher.timeSource()), Upstream::ResourcePriority::Default, dispatcher, nullptr, nullptr, random_generator, @@ -62,7 +63,8 @@ class ConnPoolImplForTest : public Event::TestUsingSimulatedTime, public FixedHt return nullptr; // Not used: createCodecClient overloaded. }, std::vector{Protocol::Http11}), - api_(Api::createApiForTest()), mock_dispatcher_(dispatcher) {} + api_(Api::createApiForTest()), mock_dispatcher_(dispatcher), + mock_upstream_ready_cb_(upstream_ready_cb) {} ~ConnPoolImplForTest() override { EXPECT_EQ(0U, ready_clients_.size()); @@ -118,15 +120,17 @@ class ConnPoolImplForTest : public Event::TestUsingSimulatedTime, public FixedHt } void expectEnableUpstreamReady() { - EXPECT_CALL(mock_dispatcher_, post(_)).WillOnce(SaveArg<0>(&post_cb_)); + EXPECT_CALL(*mock_upstream_ready_cb_, scheduleCallbackCurrentIteration()) + .Times(1) + .RetiresOnSaturation(); } - void expectAndRunUpstreamReady() { post_cb_(); } + void expectAndRunUpstreamReady() { mock_upstream_ready_cb_->invokeCallback(); } Upstream::ClusterConnectivityState state_; Api::ApiPtr api_; Event::MockDispatcher& mock_dispatcher_; - Event::PostCb post_cb_; + NiceMock* mock_upstream_ready_cb_; std::vector test_clients_; }; @@ -136,7 +140,9 @@ class ConnPoolImplForTest : public Event::TestUsingSimulatedTime, public FixedHt class Http1ConnPoolImplTest : public testing::Test { public: Http1ConnPoolImplTest() - : conn_pool_(std::make_unique(dispatcher_, cluster_, random_)) {} + : upstream_ready_cb_(new NiceMock(&dispatcher_)), + conn_pool_(std::make_unique(dispatcher_, cluster_, random_, + upstream_ready_cb_)) {} ~Http1ConnPoolImplTest() override { EXPECT_EQ("", TestUtility::nonZeroedGauges(cluster_->stats_store_.gauges())); @@ -145,6 +151,7 @@ class Http1ConnPoolImplTest : public testing::Test { NiceMock random_; NiceMock dispatcher_; std::shared_ptr cluster_{new NiceMock()}; + NiceMock* upstream_ready_cb_; std::unique_ptr conn_pool_; NiceMock runtime_; }; @@ -290,13 +297,16 @@ TEST_F(Http1ConnPoolImplTest, VerifyAlpnFallback) { })); cluster_->transport_socket_matcher_ = std::make_unique>(std::move(factory)); + new NiceMock(&dispatcher_); // Recreate the conn pool so that the host re-evaluates the transport socket match, arriving at // our test transport socket factory. - conn_pool_ = std::make_unique(dispatcher_, cluster_, random_); + conn_pool_ = + std::make_unique(dispatcher_, cluster_, random_, upstream_ready_cb_); NiceMock outer_decoder; ConnPoolCallbacks callbacks; conn_pool_->expectClientCreate(Protocol::Http11); + Http::ConnectionPool::Cancellable* handle = conn_pool_->newStream(outer_decoder, callbacks); EXPECT_NE(nullptr, handle); @@ -646,7 +656,6 @@ TEST_F(Http1ConnPoolImplTest, MaxConnections) { inner_decoder->decodeHeaders(std::move(response_headers), true); conn_pool_->expectAndRunUpstreamReady(); - conn_pool_->expectEnableUpstreamReady(); EXPECT_TRUE( callbacks2.outer_encoder_ ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true) @@ -716,7 +725,6 @@ TEST_F(Http1ConnPoolImplTest, ConnectionCloseWithoutHeader) { EXPECT_CALL(callbacks2.pool_ready_, ready()); conn_pool_->test_clients_[0].connection_->raiseEvent(Network::ConnectionEvent::Connected); - conn_pool_->expectEnableUpstreamReady(); EXPECT_TRUE( callbacks2.outer_encoder_ ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true) @@ -978,9 +986,7 @@ TEST_F(Http1ConnPoolImplTest, ConcurrentConnections) { r3.startRequest(); EXPECT_EQ(3U, cluster_->stats_.upstream_rq_total_.value()); - conn_pool_->expectEnableUpstreamReady(); r2.completeResponse(false); - conn_pool_->expectEnableUpstreamReady(); r3.completeResponse(false); // Disconnect both clients. @@ -1125,6 +1131,56 @@ TEST_F(Http1ConnPoolImplTest, PendingRequestIsConsideredActive) { EXPECT_EQ(1U, cluster_->stats_.upstream_cx_destroy_local_.value()); } +class ConnPoolImplNoDestructForTest : public ConnPoolImplForTest { +public: + ConnPoolImplNoDestructForTest(Event::MockDispatcher& dispatcher, + Upstream::ClusterInfoConstSharedPtr cluster, + Random::RandomGenerator& random_generator, + NiceMock* upstream_ready_cb) + : ConnPoolImplForTest(dispatcher, cluster, random_generator, upstream_ready_cb) {} + + ~ConnPoolImplNoDestructForTest() override { destructAllConnections(); } +}; + +// Regression test for connection pool use after free when dispatcher runs posted callback +// conn_pool->onUpstreamReady() after connection pool is destroyed. +TEST_F(Http1ConnPoolImplTest, UseAfterFreeConnPool) { + // Recreate conn pool without destructor checks. + new NiceMock(&dispatcher_); + conn_pool_ = std::make_unique(dispatcher_, cluster_, random_, + upstream_ready_cb_); + + InSequence s; + + NiceMock outer_decoder; + ConnPoolCallbacks callbacks; + conn_pool_->expectClientCreate(); + Http::ConnectionPool::Cancellable* handle = conn_pool_->newStream(outer_decoder, callbacks); + EXPECT_NE(nullptr, handle); + + NiceMock request_encoder; + ResponseDecoder* inner_decoder; + EXPECT_CALL(*conn_pool_->test_clients_[0].codec_, newStream(_)) + .WillOnce(DoAll(SaveArgAddress(&inner_decoder), ReturnRef(request_encoder))); + EXPECT_CALL(callbacks.pool_ready_, ready()); + EXPECT_CALL(*conn_pool_->test_clients_[0].connect_timer_, disableTimer()); + conn_pool_->test_clients_[0].connection_->raiseEvent(Network::ConnectionEvent::Connected); + + EXPECT_TRUE( + callbacks.outer_encoder_ + ->encodeHeaders(TestRequestHeaderMapImpl{{":path", "/"}, {":method", "GET"}}, true) + .ok()); + + EXPECT_CALL(*conn_pool_, onClientDestroy()); + dispatcher_.deferredDelete(std::move(conn_pool_)); + inner_decoder->decodeHeaders( + ResponseHeaderMapPtr{new TestResponseHeaderMapImpl{{":status", "200"}}}, true); + dispatcher_.clearDeferredDeleteList(); + + // TODO(asraa): Clean this leak. + delete upstream_ready_cb_; +} + } // namespace } // namespace Http1 } // namespace Http