This is an automated email from the ASF dual-hosted git repository.
sorber pushed a commit to branch 6.2.x
in repository https://git-dual.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/6.2.x by this push:
new 06700bf TS-4717: Http2 stack explosion.
06700bf is described below
commit 06700bf7c22eb823f683244b8d77c7c54d23d8b1
Author: Susan Hinrichs <[email protected]>
AuthorDate: Mon Aug 8 18:36:41 2016 +0000
TS-4717: Http2 stack explosion.
(cherry picked from commit 8a7c8fce6a70467e097289ad8e654118b2122293)
---
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 c98662f..9d290e2 100644
--- a/iocore/net/UnixNetVConnection.cc
+++ b/iocore/net/UnixNetVConnection.cc
@@ -797,7 +797,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);
@@ -911,7 +911,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 ed5b1d6..b32511e 100644
--- a/proxy/ProxyClientSession.h
+++ b/proxy/ProxyClientSession.h
@@ -180,6 +180,12 @@ public:
void set_session_active();
void clear_session_active();
+ 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 a849e05..7de8c9d 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
@@ -385,71 +385,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;
}
@@ -457,30 +443,66 @@ 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 58d3859..38f681a 100644
--- a/proxy/http2/Http2ClientSession.h
+++ b/proxy/http2/Http2ClientSession.h
@@ -252,7 +252,13 @@ private:
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;
--
To stop receiving notification emails like this one, please contact
['"[email protected]" <[email protected]>'].
