From 95fbfad7c47441060d0de87558d614ac6e063e6f Mon Sep 17 00:00:00 2001 From: Bryan Call Date: Tue, 31 Jan 2023 10:08:39 -0800 Subject: [PATCH 1/2] Fixed memory leaks with http/3 - Leak with not freeing packets - Leak in buffers for QPACK - Leak with the http/3 session --- iocore/net/QUICNet.cc | 1 + iocore/net/quic/QUICApplicationMap.h | 7 +++++++ proxy/http3/QPACK.cc | 6 +++++- 3 files changed, 13 insertions(+), 1 deletion(-) diff --git a/iocore/net/QUICNet.cc b/iocore/net/QUICNet.cc index da073dd498a..b14cda3ae75 100644 --- a/iocore/net/QUICNet.cc +++ b/iocore/net/QUICNet.cc @@ -77,6 +77,7 @@ QUICPollCont::_process_packet(QUICPollEvent *e, NetHandler *nh) } // Note: We should free QUICPollEvent here since vc could be freed from other thread. + p->free(); e->free(); } #else diff --git a/iocore/net/quic/QUICApplicationMap.h b/iocore/net/quic/QUICApplicationMap.h index a657adf6e41..5a3c1fae4c2 100644 --- a/iocore/net/quic/QUICApplicationMap.h +++ b/iocore/net/quic/QUICApplicationMap.h @@ -33,6 +33,13 @@ class QUICApplicationMap void set(QUICStreamId id, QUICApplication *app); void set_default(QUICApplication *app); QUICApplication *get(QUICStreamId id); + ~QUICApplicationMap() + { + for (auto it = _map.begin(); it != _map.end(); ++it) { + delete it->second; + } + delete _default_app; + } private: std::map _map; diff --git a/proxy/http3/QPACK.cc b/proxy/http3/QPACK.cc index 092008de28b..2e570756a3b 100644 --- a/proxy/http3/QPACK.cc +++ b/proxy/http3/QPACK.cc @@ -148,7 +148,11 @@ QPACK::QPACK(QUICConnection *qc, uint32_t max_header_list_size, uint16_t max_tab this->_decoder_stream_sending_instructions_reader = this->_decoder_stream_sending_instructions->alloc_reader(); } -QPACK::~QPACK() {} +QPACK::~QPACK() +{ + free_MIOBuffer(_encoder_stream_sending_instructions); + free_MIOBuffer(_decoder_stream_sending_instructions); +} void QPACK::on_new_stream(QUICStream &stream) From 387d3e8a047b081f049b48a23d2df8a4da5962e4 Mon Sep 17 00:00:00 2001 From: Bryan Call Date: Wed, 1 Feb 2023 10:29:51 -0800 Subject: [PATCH 2/2] Give ownership of the application to the quic stream manager Removed unused variable --- iocore/net/QUICNetVConnection.cc | 2 -- .../net/quic/test/test_QUICStreamManager.cc | 20 ++++++++----------- 2 files changed, 8 insertions(+), 14 deletions(-) diff --git a/iocore/net/QUICNetVConnection.cc b/iocore/net/QUICNetVConnection.cc index 2b3f3bbdeb9..5d8290208d9 100644 --- a/iocore/net/QUICNetVConnection.cc +++ b/iocore/net/QUICNetVConnection.cc @@ -1649,7 +1649,6 @@ QUICNetVConnection::_packetize_frames(uint8_t *packet_buf, QUICEncryptionLevel l max_frame_size = std::min(max_frame_size, this->_maximum_stream_frame_data_size()); bool probing = false; - int frame_count = 0; size_t len = 0; Ptr first_block = make_ptr(new_IOBufferBlock()); Ptr last_block = first_block; @@ -1688,7 +1687,6 @@ QUICNetVConnection::_packetize_frames(uint8_t *packet_buf, QUICEncryptionLevel l break; } - ++frame_count; probing |= frame->is_probing_frame(); if (frame->is_flow_controlled()) { int ret = this->_remote_flow_controller->update(this->_stream_manager->total_offset_sent()); diff --git a/iocore/net/quic/test/test_QUICStreamManager.cc b/iocore/net/quic/test/test_QUICStreamManager.cc index 130454f9ddc..9849af3b7ea 100644 --- a/iocore/net/quic/test/test_QUICStreamManager.cc +++ b/iocore/net/quic/test/test_QUICStreamManager.cc @@ -37,8 +37,7 @@ TEST_CASE("QUICStreamManager_NewStream", "[quic]") QUICEncryptionLevel level = QUICEncryptionLevel::ONE_RTT; QUICApplicationMap app_map; MockQUICConnection connection; - MockQUICApplication mock_app(&connection); - app_map.set_default(&mock_app); + app_map.set_default(new MockQUICApplication(&connection)); MockQUICConnectionInfoProvider cinfo_provider; QUICStreamManagerImpl sm(&context, &app_map); @@ -101,8 +100,7 @@ TEST_CASE("QUICStreamManager_first_initial_map", "[quic]") QUICEncryptionLevel level = QUICEncryptionLevel::ONE_RTT; QUICApplicationMap app_map; MockQUICConnection connection; - MockQUICApplication mock_app(&connection); - app_map.set_default(&mock_app); + app_map.set_default(new MockQUICApplication(&connection)); MockQUICConnectionInfoProvider cinfo_provider; QUICStreamManagerImpl sm(&context, &app_map); std::shared_ptr local_tp = std::make_shared(); @@ -127,8 +125,7 @@ TEST_CASE("QUICStreamManager_total_offset_received", "[quic]") QUICEncryptionLevel level = QUICEncryptionLevel::ONE_RTT; QUICApplicationMap app_map; MockQUICConnection connection; - MockQUICApplication mock_app(&connection); - app_map.set_default(&mock_app); + app_map.set_default(new MockQUICApplication(&connection)); QUICStreamManagerImpl sm(&context, &app_map); uint8_t local_tp_buf[] = { @@ -182,8 +179,8 @@ TEST_CASE("QUICStreamManager_total_offset_sent", "[quic]") QUICEncryptionLevel level = QUICEncryptionLevel::ONE_RTT; QUICApplicationMap app_map; MockQUICConnection connection; - MockQUICApplication mock_app(&connection); - app_map.set_default(&mock_app); + auto mock_app = new MockQUICApplication(&connection); + app_map.set_default(mock_app); QUICStreamManagerImpl sm(&context, &app_map); uint8_t local_tp_buf[] = { @@ -232,12 +229,12 @@ TEST_CASE("QUICStreamManager_total_offset_sent", "[quic]") // total_offset should be a integer in unit of octets uint8_t frame_buf[4096]; - mock_app.send(reinterpret_cast(block_1024->buf()), 1024, 0); + mock_app->send(reinterpret_cast(block_1024->buf()), 1024, 0); sm.generate_frame(frame_buf, QUICEncryptionLevel::ONE_RTT, 16384, 16384, 0, 0, nullptr); CHECK(sm.total_offset_sent() == 1024); // total_offset should be a integer in unit of octets - mock_app.send(reinterpret_cast(block_1024->buf()), 1024, 4); + mock_app->send(reinterpret_cast(block_1024->buf()), 1024, 4); sm.generate_frame(frame_buf, QUICEncryptionLevel::ONE_RTT, 16384, 16384, 0, 0, nullptr); CHECK(sm.total_offset_sent() == 2048); @@ -250,8 +247,7 @@ TEST_CASE("QUICStreamManager_max_streams", "[quic]") QUICEncryptionLevel level = QUICEncryptionLevel::ONE_RTT; QUICApplicationMap app_map; MockQUICConnection connection; - MockQUICApplication mock_app(&connection); - app_map.set_default(&mock_app); + app_map.set_default(new MockQUICApplication(&connection)); QUICStreamManagerImpl sm(&context, &app_map); uint8_t local_tp_buf[] = {