You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@trafficserver.apache.org by cm...@apache.org on 2024/03/01 15:25:34 UTC
(trafficserver) 06/17: http3: Fix use-after-free (#11088)
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 <ma...@apache.org>
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;