From 8a7c8fce6a70467e097289ad8e654118b2122293 Mon Sep 17 00:00:00 2001 From: Susan Hinrichs Date: Mon, 8 Aug 2016 18:36:41 +0000 Subject: [PATCH] TS-4717: Http2 stack explosion. --- iocore/net/UnixNetVConnection.cc | 4 +- proxy/ProxyClientSession.h | 6 ++ proxy/http2/Http2ClientSession.cc | 148 +++++++++++++++++------------- proxy/http2/Http2ClientSession.h | 6 ++ 4 files changed, 99 insertions(+), 65 deletions(-) diff --git a/iocore/net/UnixNetVConnection.cc b/iocore/net/UnixNetVConnection.cc index 28794bfdbc7..adf9e005965 100644 --- a/iocore/net/UnixNetVConnection.cc +++ b/iocore/net/UnixNetVConnection.cc @@ -801,7 +801,7 @@ UnixNetVConnection::reenable(VIO *vio) return; EThread *t = vio->mutex->thread_holding; ink_assert(t == this_ethread()); - ink_assert(!closed); + ink_release_assert(!closed); if (nh->mutex->thread_holding == t) { if (vio == &read.vio) { ep.modify(EVENTIO_READ); @@ -915,7 +915,7 @@ void UnixNetVConnection::set_enabled(VIO *vio) { ink_assert(vio->mutex->thread_holding == this_ethread() && thread); - ink_assert(!closed); + ink_release_assert(!closed); STATE_FROM_VIO(vio)->enabled = 1; #ifdef INACTIVITY_TIMEOUT if (!inactivity_timeout && inactivity_timeout_in) { diff --git a/proxy/ProxyClientSession.h b/proxy/ProxyClientSession.h index d6feef70e44..e235f71bdb9 100644 --- a/proxy/ProxyClientSession.h +++ b/proxy/ProxyClientSession.h @@ -178,6 +178,12 @@ class ProxyClientSession : public VConnection } virtual const char *get_protocol_string() const = 0; + bool + is_client_closed() const + { + return get_netvc() == NULL; + } + protected: // XXX Consider using a bitwise flags variable for the following flags, so that we can make the best // use of internal alignment padding. diff --git a/proxy/http2/Http2ClientSession.cc b/proxy/http2/Http2ClientSession.cc index 891516c246d..dd9602d8c6d 100644 --- a/proxy/http2/Http2ClientSession.cc +++ b/proxy/http2/Http2ClientSession.cc @@ -86,8 +86,6 @@ Http2ClientSession::destroy() void Http2ClientSession::free() { - DebugHttp2Ssn("session free"); - if (client_vc) { release_netvc(); client_vc->do_io_close(); @@ -102,6 +100,8 @@ Http2ClientSession::free() return; } + DebugHttp2Ssn("session free"); + HTTP2_DECREMENT_THREAD_DYN_STAT(HTTP2_STAT_CURRENT_CLIENT_SESSION_COUNT, this->mutex->thread_holding); // Update stats on how we died. May want to eliminate this. Was useful for @@ -390,71 +390,57 @@ Http2ClientSession::state_start_frame_read(int event, void *edata) STATE_ENTER(&Http2ClientSession::state_start_frame_read, event); ink_assert(event == VC_EVENT_READ_COMPLETE || event == VC_EVENT_READ_READY); + return state_process_frame_read(event, vio, false); +} - if (this->sm_reader->read_avail() >= (int64_t)HTTP2_FRAME_HEADER_LEN) { - uint8_t buf[HTTP2_FRAME_HEADER_LEN]; - unsigned nbytes; +int +Http2ClientSession::do_start_frame_read(Http2ErrorCode &ret_error) +{ + ret_error = HTTP2_ERROR_NO_ERROR; + ink_release_assert(this->sm_reader->read_avail() >= (int64_t)HTTP2_FRAME_HEADER_LEN); - DebugHttp2Ssn("receiving frame header"); - nbytes = copy_from_buffer_reader(buf, this->sm_reader, sizeof(buf)); + uint8_t buf[HTTP2_FRAME_HEADER_LEN]; + unsigned nbytes; - if (!http2_parse_frame_header(make_iovec(buf), this->current_hdr)) { - DebugHttp2Ssn("frame header parse failure"); - this->do_io_close(); - return 0; - } + DebugHttp2Ssn("receiving frame header"); + nbytes = copy_from_buffer_reader(buf, this->sm_reader, sizeof(buf)); - DebugHttp2Ssn("frame header length=%u, type=%u, flags=0x%x, streamid=%u", (unsigned)this->current_hdr.length, - (unsigned)this->current_hdr.type, (unsigned)this->current_hdr.flags, this->current_hdr.streamid); + if (!http2_parse_frame_header(make_iovec(buf), this->current_hdr)) { + DebugHttp2Ssn("frame header parse failure"); + this->do_io_close(); + return -1; + } - this->sm_reader->consume(nbytes); + DebugHttp2Ssn("frame header length=%u, type=%u, flags=0x%x, streamid=%u", (unsigned)this->current_hdr.length, + (unsigned)this->current_hdr.type, (unsigned)this->current_hdr.flags, this->current_hdr.streamid); - if (!http2_frame_header_is_valid(this->current_hdr, - this->connection_state.server_settings.get(HTTP2_SETTINGS_MAX_FRAME_SIZE))) { - SCOPED_MUTEX_LOCK(lock, this->connection_state.mutex, this_ethread()); - if (!this->connection_state.is_state_closed()) { - this->connection_state.send_goaway_frame(this->current_hdr.streamid, HTTP2_ERROR_PROTOCOL_ERROR); - } - return 0; - } + this->sm_reader->consume(nbytes); - // If we know up front that the payload is too long, nuke this connection. - if (this->current_hdr.length > this->connection_state.server_settings.get(HTTP2_SETTINGS_MAX_FRAME_SIZE)) { - SCOPED_MUTEX_LOCK(lock, this->connection_state.mutex, this_ethread()); - if (!this->connection_state.is_state_closed()) { - this->connection_state.send_goaway_frame(this->current_hdr.streamid, HTTP2_ERROR_FRAME_SIZE_ERROR); - } - return 0; - } + if (!http2_frame_header_is_valid(this->current_hdr, this->connection_state.server_settings.get(HTTP2_SETTINGS_MAX_FRAME_SIZE))) { + ret_error = HTTP2_ERROR_PROTOCOL_ERROR; + return -1; + } - // Allow only stream id = 0 or streams started by client. - if (this->current_hdr.streamid != 0 && !http2_is_client_streamid(this->current_hdr.streamid)) { - SCOPED_MUTEX_LOCK(lock, this->connection_state.mutex, this_ethread()); - if (!this->connection_state.is_state_closed()) { - this->connection_state.send_goaway_frame(this->current_hdr.streamid, HTTP2_ERROR_PROTOCOL_ERROR); - } - return 0; - } + // If we know up front that the payload is too long, nuke this connection. + if (this->current_hdr.length > this->connection_state.server_settings.get(HTTP2_SETTINGS_MAX_FRAME_SIZE)) { + ret_error = HTTP2_ERROR_FRAME_SIZE_ERROR; + return -1; + } - // CONTINUATIONs MUST follow behind HEADERS which doesn't have END_HEADERS - Http2StreamId continued_stream_id = this->connection_state.get_continued_stream_id(); + // Allow only stream id = 0 or streams started by client. + if (this->current_hdr.streamid != 0 && !http2_is_client_streamid(this->current_hdr.streamid)) { + ret_error = HTTP2_ERROR_PROTOCOL_ERROR; + return -1; + } - if (continued_stream_id != 0 && - (continued_stream_id != this->current_hdr.streamid || this->current_hdr.type != HTTP2_FRAME_TYPE_CONTINUATION)) { - SCOPED_MUTEX_LOCK(lock, this->connection_state.mutex, this_ethread()); - if (!this->connection_state.is_state_closed()) { - this->connection_state.send_goaway_frame(this->current_hdr.streamid, HTTP2_ERROR_PROTOCOL_ERROR); - } - return 0; - } + // CONTINUATIONs MUST follow behind HEADERS which doesn't have END_HEADERS + Http2StreamId continued_stream_id = this->connection_state.get_continued_stream_id(); - HTTP2_SET_SESSION_HANDLER(&Http2ClientSession::state_complete_frame_read); - if (this->sm_reader->read_avail() >= this->current_hdr.length) { - return this->handleEvent(VC_EVENT_READ_READY, vio); - } + if (continued_stream_id != 0 && + (continued_stream_id != this->current_hdr.streamid || this->current_hdr.type != HTTP2_FRAME_TYPE_CONTINUATION)) { + ret_error = HTTP2_ERROR_PROTOCOL_ERROR; + return -1; } - - vio->reenable(); return 0; } @@ -462,29 +448,65 @@ int Http2ClientSession::state_complete_frame_read(int event, void *edata) { VIO *vio = (VIO *)edata; - STATE_ENTER(&Http2ClientSession::state_complete_frame_read, event); ink_assert(event == VC_EVENT_READ_COMPLETE || event == VC_EVENT_READ_READY); - if (this->sm_reader->read_avail() < this->current_hdr.length) { vio->reenable(); return 0; } - DebugHttp2Ssn("completed frame read, %" PRId64 " bytes available", this->sm_reader->read_avail()); + return state_process_frame_read(event, vio, true); +} + +int +Http2ClientSession::do_complete_frame_read() +{ // XXX parse the frame and handle it ... + ink_release_assert(this->sm_reader->read_avail() >= this->current_hdr.length); Http2Frame frame(this->current_hdr, this->sm_reader); - send_connection_event(&this->connection_state, HTTP2_SESSION_EVENT_RECV, &frame); this->sm_reader->consume(this->current_hdr.length); - HTTP2_SET_SESSION_HANDLER(&Http2ClientSession::state_start_frame_read); - if (this->sm_reader->is_read_avail_more_than(0)) { - return this->handleEvent(VC_EVENT_READ_READY, vio); + return 0; +} + +int +Http2ClientSession::state_process_frame_read(int event, VIO *vio, bool inside_frame) +{ + if (inside_frame) { + do_complete_frame_read(); } - vio->reenable(); + while (this->sm_reader->read_avail() >= (int64_t)HTTP2_FRAME_HEADER_LEN) { + // Return if there was an error + Http2ErrorCode err; + if (do_start_frame_read(err) < 0) { + // send an error if specified. Otherwise, just go away + if (err > HTTP2_ERROR_NO_ERROR) { + SCOPED_MUTEX_LOCK(lock, this->connection_state.mutex, this_ethread()); + if (!this->connection_state.is_state_closed()) { + this->connection_state.send_goaway_frame(this->current_hdr.streamid, err); + } + } + return 0; + } + + // If there is no more data to finish the frame, set up the event handler and reenable + if (this->sm_reader->read_avail() < this->current_hdr.length) { + HTTP2_SET_SESSION_HANDLER(&Http2ClientSession::state_complete_frame_read); + break; + } + do_complete_frame_read(); + + // Set the event handler if there is no more data to process a new frame + HTTP2_SET_SESSION_HANDLER(&Http2ClientSession::state_start_frame_read); + } + + // If the client hasn't shut us down, reenable + if (!this->is_client_closed()) { + vio->reenable(); + } return 0; } diff --git a/proxy/http2/Http2ClientSession.h b/proxy/http2/Http2ClientSession.h index e8ce529bd99..efefad97e9f 100644 --- a/proxy/http2/Http2ClientSession.h +++ b/proxy/http2/Http2ClientSession.h @@ -251,7 +251,13 @@ class Http2ClientSession : public ProxyClientSession int state_read_connection_preface(int, void *); int state_start_frame_read(int, void *); + int do_start_frame_read(Http2ErrorCode &ret_error); int state_complete_frame_read(int, void *); + int do_complete_frame_read(); + // state_start_frame_read and state_complete_frame_read are set up as + // event handler. Both feed into state_process_frame_read which may iterate + // if there are multiple frames ready on the wire + int state_process_frame_read(int event, VIO *vio, bool inside_frame); int64_t con_id; int64_t total_write_len;