You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@trafficserver.apache.org by sh...@apache.org on 2015/02/26 18:14:38 UTC
trafficserver git commit: TS-3405: Memory use after free in HTTP/2.
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 <ro...@yahoo-corp.jp>
Authored: Thu Feb 26 11:12:04 2015 -0600
Committer: shinrich <sh...@yahoo-inc.com>
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