This is an automated email from the ASF dual-hosted git repository. cmcfarlen pushed a commit to branch 10.0.x in repository https://gitbox.apache.org/repos/asf/trafficserver.git
commit b081247894c36279a725c61f40ef7903d38f74ed Author: Masakazu Kitajo <[email protected]> AuthorDate: Thu Feb 22 15:24:55 2024 -0700 http3: Fix use-after-free (#11088) * http3: Don't delete streams on app layer until QUIC layer let it do * http3: Clear events tracked when they are processed Events remained in QUICStreamVCAdapter after the events were processed, and that caused use-after-free in the destructor of QUICStreamVCAdapter. (cherry picked from commit 1b9e15594443058030b955363e99ab91f00c1d2a) --- include/iocore/net/quic/Mock.h | 7 ++- include/iocore/net/quic/QUICApplication.h | 3 +- include/iocore/net/quic/QUICStreamVCAdapter.h | 14 ++++- include/proxy/http3/Http09App.h | 3 +- include/proxy/http3/Http3App.h | 3 +- include/proxy/http3/QPACK.h | 3 +- src/iocore/net/quic/QUICStreamManager.cc | 7 ++- src/iocore/net/quic/QUICStreamVCAdapter.cc | 82 ++++++++++++++++++++++----- src/proxy/http3/Http09App.cc | 7 ++- src/proxy/http3/Http3App.cc | 21 ++++++- src/proxy/http3/QPACK.cc | 7 ++- src/proxy/http3/test/test_QPACK.cc | 6 +- 12 files changed, 134 insertions(+), 29 deletions(-) diff --git a/include/iocore/net/quic/Mock.h b/include/iocore/net/quic/Mock.h index 1fee0d8bd0..001a1d1e30 100644 --- a/include/iocore/net/quic/Mock.h +++ b/include/iocore/net/quic/Mock.h @@ -548,13 +548,18 @@ public: } void - on_new_stream(QUICStream &stream) override + on_stream_open(QUICStream &stream) override { auto ite = this->_streams.emplace(stream.id(), stream); QUICStreamAdapter &adapter = ite.first->second; stream.set_io_adapter(&adapter); } + void + on_stream_close(QUICStream &stream) override + { + } + void send(const uint8_t *data, size_t size, QUICStreamId stream_id) { diff --git a/include/iocore/net/quic/QUICApplication.h b/include/iocore/net/quic/QUICApplication.h index 0a87d8b1c9..008621c689 100644 --- a/include/iocore/net/quic/QUICApplication.h +++ b/include/iocore/net/quic/QUICApplication.h @@ -41,7 +41,8 @@ public: QUICApplication(QUICConnection *qc); virtual ~QUICApplication(); - virtual void on_new_stream(QUICStream &stream) = 0; + virtual void on_stream_open(QUICStream &stream) = 0; + virtual void on_stream_close(QUICStream &stream) = 0; protected: QUICConnection *_qc = nullptr; diff --git a/include/iocore/net/quic/QUICStreamVCAdapter.h b/include/iocore/net/quic/QUICStreamVCAdapter.h index 0e3b296e20..b787896548 100644 --- a/include/iocore/net/quic/QUICStreamVCAdapter.h +++ b/include/iocore/net/quic/QUICStreamVCAdapter.h @@ -50,6 +50,12 @@ public: void do_io_shutdown(ShutdownHowTo_t howto) override; void reenable(VIO *vio) override; + void clear_read_ready_event(Event *e); + void clear_read_complete_event(Event *e); + void clear_write_ready_event(Event *e); + void clear_write_complete_event(Event *e); + void clear_eos_event(Event *e); + int state_stream_open(int event, void *data); int state_stream_closed(int event, void *data); @@ -59,9 +65,11 @@ protected: VIO _read_vio; VIO _write_vio; - Event *_read_event = nullptr; - Event *_write_event = nullptr; - Event *_eos_event = nullptr; + Event *_read_ready_event = nullptr; + Event *_read_complete_event = nullptr; + Event *_write_ready_event = nullptr; + Event *_write_complete_event = nullptr; + Event *_eos_event = nullptr; }; class QUICStreamVCAdapter::IOInfo diff --git a/include/proxy/http3/Http09App.h b/include/proxy/http3/Http09App.h index 9221d48b05..a801431de7 100644 --- a/include/proxy/http3/Http09App.h +++ b/include/proxy/http3/Http09App.h @@ -44,7 +44,8 @@ public: Http09App(NetVConnection *client_vc, QUICConnection *qc, IpAllow::ACL &&session_acl, const HttpSessionAccept::Options &options); ~Http09App(); - void on_new_stream(QUICStream &stream) override; + void on_stream_open(QUICStream &stream) override; + void on_stream_close(QUICStream &stream) override; int main_event_handler(int event, Event *data); diff --git a/include/proxy/http3/Http3App.h b/include/proxy/http3/Http3App.h index 50c05137a2..26e6d51985 100644 --- a/include/proxy/http3/Http3App.h +++ b/include/proxy/http3/Http3App.h @@ -50,7 +50,8 @@ public: Http3App(NetVConnection *client_vc, QUICConnection *qc, IpAllow::ACL &&session_acl, const HttpSessionAccept::Options &options); virtual ~Http3App(); - void on_new_stream(QUICStream &stream) override; + void on_stream_open(QUICStream &stream) override; + void on_stream_close(QUICStream &stream) override; virtual void start(); virtual int main_event_handler(int event, Event *data); diff --git a/include/proxy/http3/QPACK.h b/include/proxy/http3/QPACK.h index bae08bb7cf..77555e3cdf 100644 --- a/include/proxy/http3/QPACK.h +++ b/include/proxy/http3/QPACK.h @@ -51,7 +51,8 @@ public: QPACK(QUICConnection *qc, uint32_t max_field_section_size, uint16_t max_table_size, uint16_t max_blocking_streams); virtual ~QPACK(); - void on_new_stream(QUICStream &stream) override; + void on_stream_open(QUICStream &stream) override; + void on_stream_close(QUICStream &stream) override; int event_handler(int event, Event *data); diff --git a/src/iocore/net/quic/QUICStreamManager.cc b/src/iocore/net/quic/QUICStreamManager.cc index c18e54c955..e524795005 100644 --- a/src/iocore/net/quic/QUICStreamManager.cc +++ b/src/iocore/net/quic/QUICStreamManager.cc @@ -96,7 +96,7 @@ QUICStreamManager::create_stream(QUICStreamId stream_id) this->stream_list.push(stream); QUICApplication *application = this->_app_map->get(stream_id); - application->on_new_stream(*stream); + application->on_stream_open(*stream); return nullptr; } @@ -116,7 +116,12 @@ QUICConnectionErrorUPtr QUICStreamManager::delete_stream(QUICStreamId &stream_id) { QUICStream *stream = static_cast<QUICStream *>(this->find_stream(stream_id)); + + QUICApplication *application = this->_app_map->get(stream_id); + application->on_stream_close(*stream); + stream_list.remove(stream); + delete stream; return nullptr; diff --git a/src/iocore/net/quic/QUICStreamVCAdapter.cc b/src/iocore/net/quic/QUICStreamVCAdapter.cc index b427410df1..e742572f7d 100644 --- a/src/iocore/net/quic/QUICStreamVCAdapter.cc +++ b/src/iocore/net/quic/QUICStreamVCAdapter.cc @@ -31,14 +31,24 @@ QUICStreamVCAdapter::QUICStreamVCAdapter(QUICStream &stream) : VConnection(new_P QUICStreamVCAdapter::~QUICStreamVCAdapter() { - if (this->_read_event) { - this->_read_event->cancel(); - this->_read_event = nullptr; + if (this->_read_ready_event) { + this->_read_ready_event->cancel(); + this->_read_ready_event = nullptr; } - if (this->_write_event) { - this->_write_event->cancel(); - this->_write_event = nullptr; + if (this->_read_complete_event) { + this->_read_complete_event->cancel(); + this->_read_complete_event = nullptr; + } + + if (this->_write_ready_event) { + this->_write_ready_event->cancel(); + this->_write_ready_event = nullptr; + } + + if (this->_write_complete_event) { + this->_write_complete_event->cancel(); + this->_write_complete_event = nullptr; } if (this->_eos_event) { @@ -150,9 +160,14 @@ QUICStreamVCAdapter::encourge_read() return; } - int event = this->_read_vio.nbytes == INT64_MAX ? VC_EVENT_READ_READY : VC_EVENT_READ_COMPLETE; - if (!(this->_read_event && this->_read_event->callback_event == event)) { - this->_read_event = this_ethread()->schedule_imm(this->_read_vio.cont, event, &this->_read_vio); + if (this->_read_vio.nbytes == INT64_MAX) { + if (!this->_read_ready_event) { + this->_read_ready_event = this_ethread()->schedule_imm(this->_read_vio.cont, VC_EVENT_READ_READY, &this->_read_vio); + } + } else { + if (!this->_read_complete_event) { + this->_read_complete_event = this_ethread()->schedule_imm(this->_read_vio.cont, VC_EVENT_READ_COMPLETE, &this->_read_vio); + } } } } @@ -170,9 +185,15 @@ QUICStreamVCAdapter::encourge_write() return; } - int event = this->_write_vio.ntodo() ? VC_EVENT_WRITE_READY : VC_EVENT_WRITE_COMPLETE; - if (!(this->_write_event && this->_write_event->callback_event == event)) { - this->_write_event = this_ethread()->schedule_imm(this->_write_vio.cont, event, &this->_write_vio); + if (this->_write_vio.ntodo()) { + if (!this->_write_ready_event) { + this->_write_ready_event = this_ethread()->schedule_imm(this->_write_vio.cont, VC_EVENT_WRITE_READY, &this->_write_vio); + } + } else { + if (!this->_write_complete_event) { + this->_write_complete_event = + this_ethread()->schedule_imm(this->_write_vio.cont, VC_EVENT_WRITE_COMPLETE, &this->_write_vio); + } } } } @@ -193,13 +214,48 @@ QUICStreamVCAdapter::notify_eos() if (lock.is_locked()) { this->_read_vio.cont->handleEvent(event, &this->_read_vio); } else { - if (!(this->_eos_event && this->_eos_event->callback_event == event)) { + if (!this->_eos_event) { this->_eos_event = this_ethread()->schedule_imm(this->_read_vio.cont, event, &this->_read_vio); } } } } +void +QUICStreamVCAdapter::clear_read_ready_event(Event *e) +{ + ink_assert(e == this->_read_ready_event); + this->_read_ready_event = nullptr; +} + +void +QUICStreamVCAdapter::clear_read_complete_event(Event *e) +{ + ink_assert(e == this->_read_complete_event); + this->_read_complete_event = nullptr; +} + +void +QUICStreamVCAdapter::clear_write_ready_event(Event *e) +{ + ink_assert(e == this->_write_ready_event); + this->_write_ready_event = nullptr; +} + +void +QUICStreamVCAdapter::clear_write_complete_event(Event *e) +{ + ink_assert(e == this->_write_complete_event); + this->_write_complete_event = nullptr; +} + +void +QUICStreamVCAdapter::clear_eos_event(Event *e) +{ + ink_assert(e == this->_eos_event); + this->_eos_event = nullptr; +} + // this->_read_vio.nbytes should be INT64_MAX until receive FIN flag VIO * QUICStreamVCAdapter::do_io_read(Continuation *c, int64_t nbytes, MIOBuffer *buf) diff --git a/src/proxy/http3/Http09App.cc b/src/proxy/http3/Http09App.cc index e5f7954e9d..384f9515a6 100644 --- a/src/proxy/http3/Http09App.cc +++ b/src/proxy/http3/Http09App.cc @@ -57,7 +57,7 @@ Http09App::~Http09App() } void -Http09App::on_new_stream(QUICStream &stream) +Http09App::on_stream_open(QUICStream &stream) { auto ret = this->_streams.emplace(stream.id(), stream); auto &info = ret.first->second; @@ -81,6 +81,11 @@ Http09App::on_new_stream(QUICStream &stream) stream.set_io_adapter(&info.adapter); } +void +Http09App::on_stream_close(QUICStream &stream) +{ +} + int Http09App::main_event_handler(int event, Event *data) { diff --git a/src/proxy/http3/Http3App.cc b/src/proxy/http3/Http3App.cc index 579b55eaef..97ec43e833 100644 --- a/src/proxy/http3/Http3App.cc +++ b/src/proxy/http3/Http3App.cc @@ -97,7 +97,7 @@ Http3App::start() } void -Http3App::on_new_stream(QUICStream &stream) +Http3App::on_stream_open(QUICStream &stream) { auto ret = this->_streams.emplace(stream.id(), stream); auto &info = ret.first->second; @@ -121,6 +121,12 @@ Http3App::on_new_stream(QUICStream &stream) stream.set_io_adapter(&info.adapter); } +void +Http3App::on_stream_close(QUICStream &stream) +{ + this->_streams.erase(stream.id()); +} + int Http3App::main_event_handler(int event, Event *data) { @@ -138,7 +144,16 @@ Http3App::main_event_handler(int event, Event *data) switch (event) { case VC_EVENT_READ_READY: + adapter->clear_read_ready_event(data); + if (is_bidirectional) { + this->_handle_bidi_stream_on_read_ready(event, vio); + } else { + this->_handle_uni_stream_on_read_ready(event, vio); + } + break; case VC_EVENT_READ_COMPLETE: + adapter->clear_read_complete_event(data); + // Calling read_ready handlers because there's no need to do different things if (is_bidirectional) { this->_handle_bidi_stream_on_read_ready(event, vio); } else { @@ -146,6 +161,7 @@ Http3App::main_event_handler(int event, Event *data) } break; case VC_EVENT_WRITE_READY: + adapter->clear_write_ready_event(data); if (is_bidirectional) { this->_handle_bidi_stream_on_write_ready(event, vio); } else { @@ -153,6 +169,7 @@ Http3App::main_event_handler(int event, Event *data) } break; case VC_EVENT_WRITE_COMPLETE: + adapter->clear_write_complete_event(data); if (is_bidirectional) { this->_handle_bidi_stream_on_write_complete(event, vio); } else { @@ -160,6 +177,7 @@ Http3App::main_event_handler(int event, Event *data) } break; case VC_EVENT_EOS: + adapter->clear_eos_event(data); if (is_bidirectional) { this->_handle_bidi_stream_on_eos(event, vio); } else { @@ -423,5 +441,4 @@ Http3App::_handle_bidi_stream_on_write_complete(int event, VIO *vio) } // FIXME There may be data to read this->_qc->stream_manager()->delete_stream(stream_id); - this->_streams.erase(stream_id); } diff --git a/src/proxy/http3/QPACK.cc b/src/proxy/http3/QPACK.cc index dd2c802a68..00f02e77a6 100644 --- a/src/proxy/http3/QPACK.cc +++ b/src/proxy/http3/QPACK.cc @@ -155,7 +155,7 @@ QPACK::~QPACK() } void -QPACK::on_new_stream(QUICStream &stream) +QPACK::on_stream_open(QUICStream &stream) { auto *info = new QUICStreamVCAdapter::IOInfo(stream); @@ -180,6 +180,11 @@ QPACK::on_new_stream(QUICStream &stream) stream.set_io_adapter(&info->adapter); } +void +QPACK::on_stream_close(QUICStream &stream) +{ +} + int QPACK::event_handler(int event, Event *data) { diff --git a/src/proxy/http3/test/test_QPACK.cc b/src/proxy/http3/test/test_QPACK.cc index 9b92851091..da6b5d1db7 100644 --- a/src/proxy/http3/test/test_QPACK.cc +++ b/src/proxy/http3/test/test_QPACK.cc @@ -295,8 +295,8 @@ test_encode(const char *qif_file, const char *out_file, int dts, int mbs, int am QPACK *qpack = new QPACK(driver.get_connection(), UINT32_MAX, dts, mbs); TestQUICStream *encoder_stream = new TestQUICStream(0); TestQUICStream *decoder_stream = new TestQUICStream(10); - qpack->on_new_stream(*encoder_stream); - qpack->on_new_stream(*decoder_stream); + qpack->on_stream_open(*encoder_stream); + qpack->on_stream_open(*decoder_stream); qpack->set_encoder_stream(encoder_stream->id()); qpack->set_decoder_stream(decoder_stream->id()); @@ -355,7 +355,7 @@ test_decode(const char *enc_file, const char *out_file, int dts, int mbs, int am QUICApplicationDriver driver; QPACK *qpack = new QPACK(driver.get_connection(), UINT32_MAX, dts, mbs); TestQUICStream *encoder_stream = new TestQUICStream(0); - qpack->on_new_stream(*encoder_stream); + qpack->on_stream_open(*encoder_stream); int offset = 0; uint8_t *block = nullptr;
