Repository: trafficserver Updated Branches: refs/heads/master c94f87216 -> 4ab0ea32b
TS-3405: Memory use after free in HTTP/2. Project: http://git-wip-us.apache.org/repos/asf/trafficserver/repo Commit: http://git-wip-us.apache.org/repos/asf/trafficserver/commit/4ab0ea32 Tree: http://git-wip-us.apache.org/repos/asf/trafficserver/tree/4ab0ea32 Diff: http://git-wip-us.apache.org/repos/asf/trafficserver/diff/4ab0ea32 Branch: refs/heads/master Commit: 4ab0ea32bd19a0c76cfe4ca5ae183bd3457e2251 Parents: c94f872 Author: Ryo Okubo <[email protected]> Authored: Thu Feb 26 11:12:04 2015 -0600 Committer: shinrich <[email protected]> Committed: Thu Feb 26 11:12:04 2015 -0600 ---------------------------------------------------------------------- CHANGES | 2 ++ proxy/http2/Http2ClientSession.cc | 27 ++++++++++++++++----------- proxy/http2/Http2ClientSession.h | 3 --- proxy/http2/Http2ConnectionState.cc | 6 +++++- proxy/http2/Http2ConnectionState.h | 20 ++++++++++++-------- 5 files changed, 35 insertions(+), 23 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/trafficserver/blob/4ab0ea32/CHANGES ---------------------------------------------------------------------- diff --git a/CHANGES b/CHANGES index 7ff6686..442c079 100644 --- a/CHANGES +++ b/CHANGES @@ -1,6 +1,8 @@ -*- coding: utf-8 -*- Changes with Apache Traffic Server 5.3.0 + *) [TS-3405] Memory use after free in HTTP/2. + *) [TS-3409] Add metric to track number of SSL connections from ATS to Origin Servers. *) [TS-3410] Remove the traffic_ctl "bounce" command. http://git-wip-us.apache.org/repos/asf/trafficserver/blob/4ab0ea32/proxy/http2/Http2ClientSession.cc ---------------------------------------------------------------------- diff --git a/proxy/http2/Http2ClientSession.cc b/proxy/http2/Http2ClientSession.cc index bf763d5..73be007 100644 --- a/proxy/http2/Http2ClientSession.cc +++ b/proxy/http2/Http2ClientSession.cc @@ -59,7 +59,7 @@ send_connection_event(Continuation * cont, int event, void * edata) } Http2ClientSession::Http2ClientSession() - : con_id(0), client_vc(NULL), read_buffer(NULL), sm_reader(NULL), write_buffer(NULL), sm_writer(NULL), upgrade_context(), is_sending_goaway(false) + : con_id(0), client_vc(NULL), read_buffer(NULL), sm_reader(NULL), write_buffer(NULL), sm_writer(NULL), upgrade_context() { } @@ -70,6 +70,8 @@ Http2ClientSession::destroy() ink_release_assert(this->client_vc == NULL); + this->connection_state.destroy(); + free_MIOBuffer(this->read_buffer); ProxyClientSession::cleanup(); http2ClientSessionAllocator.free(this); @@ -213,12 +215,6 @@ Http2ClientSession::main_event_handler(int event, void * edata) case HTTP2_SESSION_EVENT_XMIT: { Http2Frame * frame = (Http2Frame *)edata; frame->xmit(this->write_buffer); - - // Mark to wait until sending GOAWAY is complete - if (frame->header().type == HTTP2_FRAME_TYPE_GOAWAY) { - is_sending_goaway = true; - } - write_reenable(); return 0; } @@ -233,7 +229,7 @@ Http2ClientSession::main_event_handler(int event, void * edata) case VC_EVENT_WRITE_COMPLETE: case VC_EVENT_WRITE_READY: // After sending GOAWAY, close the connection - if (is_sending_goaway && write_vio->ntodo() <= 0) { + if (this->connection_state.is_state_closed() && write_vio->ntodo() <= 0) { this->do_io_close(); } return 0; @@ -328,21 +324,30 @@ Http2ClientSession::state_start_frame_read(int event, void * edata) // If we know up front that the payload is too long, nuke this connection. if (this->current_hdr.length > this->connection_state.client_settings.get(HTTP2_SETTINGS_MAX_FRAME_SIZE)) { - this->connection_state.send_goaway_frame(this->current_hdr.streamid, HTTP2_ERROR_FRAME_SIZE_ERROR); + 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; } // Allow only stream id = 0 or streams started by client. if (this->current_hdr.streamid != 0 && !http2_is_client_streamid(this->current_hdr.streamid)) { - this->connection_state.send_goaway_frame(this->current_hdr.streamid, HTTP2_ERROR_PROTOCOL_ERROR); + 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 if (this->connection_state.get_continued_id() != 0 && this->current_hdr.type != HTTP2_FRAME_TYPE_CONTINUATION) { - this->connection_state.send_goaway_frame(this->current_hdr.streamid, HTTP2_ERROR_PROTOCOL_ERROR); + 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; } http://git-wip-us.apache.org/repos/asf/trafficserver/blob/4ab0ea32/proxy/http2/Http2ClientSession.h ---------------------------------------------------------------------- diff --git a/proxy/http2/Http2ClientSession.h b/proxy/http2/Http2ClientSession.h index e785dbb..feea5b2 100644 --- a/proxy/http2/Http2ClientSession.h +++ b/proxy/http2/Http2ClientSession.h @@ -182,9 +182,6 @@ private: Http2UpgradeContext upgrade_context; VIO * write_vio; - - // Mark whether ATS is sending GOAWAY - bool is_sending_goaway; }; extern ClassAllocator<Http2ClientSession> http2ClientSessionAllocator; http://git-wip-us.apache.org/repos/asf/trafficserver/blob/4ab0ea32/proxy/http2/Http2ConnectionState.cc ---------------------------------------------------------------------- diff --git a/proxy/http2/Http2ConnectionState.cc b/proxy/http2/Http2ConnectionState.cc index 63000a1..42e7933 100644 --- a/proxy/http2/Http2ConnectionState.cc +++ b/proxy/http2/Http2ConnectionState.cc @@ -412,7 +412,7 @@ rcv_ping_frame(Http2ClientSession& cs, Http2ConnectionState& cstate, const Http2 } static Http2ErrorCode -rcv_goaway_frame(Http2ClientSession& cs, Http2ConnectionState& /*cstate*/, const Http2Frame& frame) +rcv_goaway_frame(Http2ClientSession& cs, Http2ConnectionState& cstate, const Http2Frame& frame) { Http2Goaway goaway; char buf[HTTP2_GOAWAY_LEN]; @@ -437,6 +437,7 @@ rcv_goaway_frame(Http2ClientSession& cs, Http2ConnectionState& /*cstate*/, const DebugSsn(&cs, "http2_cs", "[%" PRId64 "] GOAWAY: last stream id=%d, error code=%d.", cs.connection_id(), goaway.last_streamid, goaway.error_code); + cstate.handleEvent(HTTP2_SESSION_EVENT_FINI, NULL); // eventProcessor.schedule_imm(&cs, ET_NET, VC_EVENT_ERROR); return HTTP2_ERROR_NO_ERROR; @@ -785,6 +786,7 @@ Http2ConnectionState::finish_continued_headers() { continued_id = 0; ats_free(continued_buffer.iov_base); + continued_buffer.iov_base = NULL; continued_buffer.iov_len = 0; } @@ -958,6 +960,8 @@ Http2ConnectionState::send_goaway_frame(Http2StreamId id, Http2ErrorCode ec) // xmit event MUTEX_LOCK(lock, this->ua_session->mutex, this_ethread()); this->ua_session->handleEvent(HTTP2_SESSION_EVENT_XMIT, &frame); + + handleEvent(HTTP2_SESSION_EVENT_FINI, NULL); } void http://git-wip-us.apache.org/repos/asf/trafficserver/blob/4ab0ea32/proxy/http2/Http2ConnectionState.h ---------------------------------------------------------------------- diff --git a/proxy/http2/Http2ConnectionState.h b/proxy/http2/Http2ConnectionState.h index fdb7ead..b4667df 100644 --- a/proxy/http2/Http2ConnectionState.h +++ b/proxy/http2/Http2ConnectionState.h @@ -151,14 +151,6 @@ public: SET_HANDLER(&Http2ConnectionState::main_event_handler); } - ~Http2ConnectionState() - { - delete local_dynamic_table; - delete remote_dynamic_table; - - ats_free(continued_buffer.iov_base); - } - Http2ClientSession * ua_session; Http2DynamicTable * local_dynamic_table; Http2DynamicTable * remote_dynamic_table; @@ -176,6 +168,16 @@ public: continued_buffer.iov_len = 0; } + void destroy() + { + cleanup_streams(); + + delete local_dynamic_table; + delete remote_dynamic_table; + + ats_free(continued_buffer.iov_base); + } + // Event handlers int main_event_handler(int, void *); int state_closed(int, void *); @@ -206,6 +208,8 @@ public: void send_goaway_frame(Http2StreamId id, Http2ErrorCode ec); void send_window_update_frame(Http2StreamId id, uint32_t size); + bool is_state_closed() const { return ua_session == NULL; } + private: Http2ConnectionState(const Http2ConnectionState&); // noncopyable Http2ConnectionState& operator=(const Http2ConnectionState&); // noncopyable
