Skip to content

Fix H3 transaction leak #9714

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
maskit merged 2 commits into from
May 17, 2023
Merged

Fix H3 transaction leak #9714

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e68edeb
Fix H3 transaction leak
maskit May 16, 2023
a1fce0a
Add fallthrough
maskit May 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion proxy/http3/Http3App.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -353,7 +353,6 @@ 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);
delete txn;
}

//
Expand Down
42 changes: 38 additions & 4 deletions proxy/http3/Http3Transaction.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ HQTransaction::do_io_read(Continuation *c, int64_t nbytes, MIOBuffer *buf)

if (buf) {
this->_process_read_vio();
this->_send_tracked_event(this->_read_event, VC_EVENT_READ_READY, &this->_read_vio);
this->_read_event = this->_send_tracked_event(this->_read_event, VC_EVENT_READ_READY, &this->_read_vio);
}

return &this->_read_vio;
Expand All @@ -145,7 +145,7 @@ HQTransaction::do_io_write(Continuation *c, int64_t nbytes, IOBufferReader *buf,
if (c != nullptr && nbytes > 0) {
// TODO Return nullptr if the stream is not on writable state
this->_process_write_vio();
this->_send_tracked_event(this->_write_event, VC_EVENT_WRITE_READY, &this->_write_vio);
this->_write_event = this->_send_tracked_event(this->_write_event, VC_EVENT_WRITE_READY, &this->_write_vio);
}

return &this->_write_vio;
Expand Down Expand Up @@ -173,8 +173,6 @@ HQTransaction::do_io_close(int lerrno)
this->_write_vio.nbytes = 0;
this->_write_vio.op = VIO::NONE;
this->_write_vio.cont = nullptr;

this->_proxy_ssn->do_io_close(lerrno);
}

void
Expand Down Expand Up @@ -208,6 +206,7 @@ HQTransaction::transaction_done()
{
// TODO: start closing transaction
super::transaction_done();
delete this;
Copy link
Contributor

@brbzull0 brbzull0 May 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am always scare about this

Copy link
Member Author

@maskit maskit May 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a big fun of this pattern too, but this is probably the right place to delete/free/release transaction instance if we follow the design. H2 tries to free transactions here and there and it's a mess, but If you look at H1ClientTransaction, it's released here in transaction_done().

return;
}

Expand Down Expand Up @@ -327,10 +326,19 @@ Http3Transaction::Http3Transaction(Http3Session *session, QUICStreamVCAdapter::I

Http3Transaction::~Http3Transaction()
{
Http3TransDebug("Delete transaction");

// This should have already been called but call it here just incase.
do_io_close();

delete this->_header_framer;
this->_header_framer = nullptr;
delete this->_data_framer;
this->_data_framer = nullptr;
delete this->_header_handler;
this->_header_handler = nullptr;
delete this->_data_handler;
this->_data_handler = nullptr;
}

int
Expand All @@ -355,6 +363,9 @@ Http3Transaction::state_stream_open(int event, void *edata)
switch (event) {
case VC_EVENT_READ_READY:
Http3TransVDebug("%s (%d)", get_vc_event_name(event), event);
if (this->_read_event == edata) {
this->_read_event = nullptr;
}
// if no progress, don't need to signal
if (this->_process_read_vio() > 0) {
this->_signal_read_event();
Expand All @@ -375,6 +386,9 @@ Http3Transaction::state_stream_open(int event, void *edata)
this->_info.read_vio->reenable();
break;
case VC_EVENT_WRITE_READY:
if (this->_write_event == edata) {
this->_write_event = nullptr;
}
Http3TransVDebug("%s (%d)", get_vc_event_name(event), event);
// if no progress, don't need to signal
if (this->_process_write_vio() > 0) {
Expand Down Expand Up @@ -410,11 +424,17 @@ Http3Transaction::state_stream_closed(int event, void *data)

switch (event) {
case VC_EVENT_READ_READY:
if (this->_read_event == data) {
this->_read_event = nullptr;
}
case VC_EVENT_READ_COMPLETE: {
// ignore
break;
}
case VC_EVENT_WRITE_READY:
if (this->_write_event == data) {
this->_write_event = nullptr;
}
case VC_EVENT_WRITE_COMPLETE: {
// ignore
break;
Expand Down Expand Up @@ -547,6 +567,10 @@ Http09Transaction::state_stream_open(int event, void *edata)

switch (event) {
case VC_EVENT_READ_READY:
if (this->_read_event == edata) {
this->_read_event = nullptr;
}
[[fallthrough]];
case VC_EVENT_READ_COMPLETE: {
int64_t len = this->_process_read_vio();
// if no progress, don't need to signal
Expand All @@ -558,6 +582,10 @@ Http09Transaction::state_stream_open(int event, void *edata)
break;
}
case VC_EVENT_WRITE_READY:
if (this->_write_event == edata) {
this->_write_event = nullptr;
}
[[fallthrough]];
case VC_EVENT_WRITE_COMPLETE: {
int64_t len = this->_process_write_vio();
if (len > 0) {
Expand Down Expand Up @@ -595,11 +623,17 @@ Http09Transaction::state_stream_closed(int event, void *data)

switch (event) {
case VC_EVENT_READ_READY:
if (this->_read_event == data) {
this->_read_event = nullptr;
}
Copy link
Contributor

@brbzull0 brbzull0 May 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

funny the way complier complains about [[fallthrough]] as here seems not to care (probably because it just breaks)

case VC_EVENT_READ_COMPLETE: {
// ignore
break;
}
case VC_EVENT_WRITE_READY:
if (this->_write_event == data) {
this->_write_event = nullptr;
}
case VC_EVENT_WRITE_COMPLETE: {
// ignore
break;
Expand Down
2 changes: 1 addition & 1 deletion proxy/http3/Http3Transaction.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ class Http3Transaction : public HQTransaction
using super = HQTransaction;

Http3Transaction(Http3Session *session, QUICStreamVCAdapter::IOInfo &info);
~Http3Transaction();
virtual ~Http3Transaction();

int state_stream_open(int event, void *data) override;
int state_stream_closed(int event, void *data) override;
Expand Down