Skip to content

Fix 408 timeout caused by ignoring DATA frames after GOAWAY #12936

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

Open
yknoya wants to merge 1 commit into
base: master
Choose a base branch
from
+106 −23
Open

Fix 408 timeout caused by ignoring DATA frames after GOAWAY #12936

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
11 changes: 11 additions & 0 deletions include/proxy/http2/Http2ConnectionState.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,7 @@ class Http2ConnectionState : public Continuation
Http2StreamId get_latest_stream_id_in() const;
Http2StreamId get_latest_stream_id_out() const;
int get_stream_requests() const;
bool get_goaway_sent() const;
void increment_stream_requests();
bool is_peer_concurrent_stream_ub() const;
bool is_peer_concurrent_stream_lb() const;
Expand Down Expand Up @@ -281,6 +282,10 @@ class Http2ConnectionState : public Continuation
Http2StreamId latest_streamid_out = 0;
std::atomic<int> stream_requests = 0;

// The last stream identifier in the GOAWAY frame
Http2StreamId last_stream_id_tx = 0;
bool goaway_sent = false;

// Counter for current active streams which are started by the client.
std::atomic<uint32_t> peer_streams_count_in = 0;

Expand Down Expand Up @@ -442,6 +447,12 @@ Http2ConnectionState::get_stream_requests() const
return stream_requests;
}

inline bool
Http2ConnectionState::get_goaway_sent() const
{
return goaway_sent;
}

inline void
Http2ConnectionState::increment_stream_requests()
{
Expand Down
3 changes: 3 additions & 0 deletions include/proxy/http2/Http2Stream.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,9 @@ class Http2Stream : public ProxyTransaction
bool parsing_header_done = false;
bool is_first_transaction_flag = false;

bool reset_header_after_decoding = false;
bool free_stream_after_decoding = false;

HTTPHdr _send_header;
IOBufferReader *_send_reader = nullptr;
Http2DependencyTree::Node *priority_node = nullptr;
Expand Down
19 changes: 12 additions & 7 deletions src/proxy/http2/Http2CommonSession.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -362,25 +362,30 @@ Http2CommonSession::do_process_frame_read(int /* event ATS_UNUSED */, VIO *vio,

while (this->_read_buffer_reader->read_avail() >= static_cast<int64_t>(HTTP2_FRAME_HEADER_LEN)) {
// Cancel reading if there was an error or connection is closed
if (connection_state.tx_error_code.code != static_cast<uint32_t>(Http2ErrorCode::HTTP2_ERROR_NO_ERROR) ||
connection_state.is_state_closed()) {
const auto has_fatal_error_code =
(connection_state.tx_error_code.code != static_cast<uint32_t>(Http2ErrorCode::HTTP2_ERROR_NO_ERROR) &&
connection_state.tx_error_code.code != static_cast<uint32_t>(Http2ErrorCode::HTTP2_ERROR_ENHANCE_YOUR_CALM));
if (has_fatal_error_code || connection_state.is_state_closed()) {
Http2SsnDebug("reading a frame has been canceled (%u)", connection_state.tx_error_code.code);
break;
return 0;
}

Http2ErrorCode err = Http2ErrorCode::HTTP2_ERROR_NO_ERROR;
if (this->connection_state.get_stream_error_rate() > std::min(1.0, Http2::stream_error_rate_threshold * 2.0)) {
if (this->connection_state.get_stream_error_rate() > std::min(1.0, Http2::stream_error_rate_threshold * 2.0) &&
!this->connection_state.get_goaway_sent()) {
ip_port_text_buffer ipb;
const char *peer_ip = ats_ip_ntop(this->get_proxy_session()->get_remote_addr(), ipb, sizeof(ipb));
SiteThrottledWarning("HTTP/2 session error peer_ip=%s session_id=%" PRId64
" closing a connection, because its stream error rate (%f) exceeded the threshold (%f)",
peer_ip, this->get_connection_id(), this->connection_state.get_stream_error_rate(),
Http2::stream_error_rate_threshold);
err = Http2ErrorCode::HTTP2_ERROR_ENHANCE_YOUR_CALM;
this->connection_state.send_goaway_frame(this->connection_state.get_latest_stream_id_in(),
Http2ErrorCode::HTTP2_ERROR_ENHANCE_YOUR_CALM);
this->set_half_close_local_flag(true);
}

// Return if there was an error
if (err > Http2ErrorCode::HTTP2_ERROR_NO_ERROR || do_start_frame_read(err) < 0) {
auto err = Http2ErrorCode::HTTP2_ERROR_NO_ERROR;
if (do_start_frame_read(err) < 0) {
// send an error if specified. Otherwise, just go away
this->connection_state.restart_receiving(nullptr);
if (err > Http2ErrorCode::HTTP2_ERROR_NO_ERROR) {
Expand Down
96 changes: 80 additions & 16 deletions src/proxy/http2/Http2ConnectionState.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,14 @@ Http2ConnectionState::rcv_data_frame(const Http2Frame &frame)
"recv data bad frame client id");
}

// After sending a GOAWAY frame, the sender can discard frames for streams initiated by the receiver with
// identifiers higher than the identified last stream. However, DATA frames MUST be counted toward
// the connection flow-control window. (Details in [RFC 9113] 6.8.)
if (this->goaway_sent && id > this->last_stream_id_tx) {
return Http2Error(Http2ErrorClass::HTTP2_ERROR_CLASS_STREAM, Http2ErrorCode::HTTP2_ERROR_REFUSED_STREAM,
"recv data with id higher than last stream id");
}

Http2Stream *stream = this->find_stream(id);
if (stream == nullptr) {
if (this->is_valid_streamid(id)) {
Expand Down Expand Up @@ -330,15 +338,25 @@ Http2ConnectionState::rcv_headers_frame(const Http2Frame &frame)
"recv headers bad client id");
}

// After sending a GOAWAY frame, the sender can discard frames for streams initiated by the receiver with
// identifiers higher than the identified last stream. However, HEADERS, PUSH_PROMISE, and CONTINUATION
// frames MUST be minimally processed to ensure that the state maintained for field section compression is
// consistent (Details in [RFC 9113] 6.8.)
if (this->goaway_sent && stream_id > this->last_stream_id_tx) {
reset_header_after_decoding = true;
}

if (!stream) {
if (reset_header_after_decoding) {
free_stream_after_decoding = true;
uint32_t const initial_local_stream_window = this->acknowledged_local_settings.get(HTTP2_SETTINGS_INITIAL_WINDOW_SIZE);
ink_assert(dynamic_cast<Http2CommonSession *>(this->session->get_proxy_session()));
ink_assert(this->session->is_outbound() == true);
stream = THREAD_ALLOC_INIT(http2StreamAllocator, this_ethread(), this->session->get_proxy_session(), stream_id,
this->peer_settings.get(HTTP2_SETTINGS_INITIAL_WINDOW_SIZE), initial_local_stream_window,
!STREAM_IS_REGISTERED);
stream = THREAD_ALLOC_INIT(http2StreamAllocator, this_ethread(), this->session->get_proxy_session(), stream_id,
this->peer_settings.get(HTTP2_SETTINGS_INITIAL_WINDOW_SIZE), initial_local_stream_window,
!STREAM_IS_REGISTERED);
stream->mutex = new_ProxyMutex();
SCOPED_MUTEX_LOCK(lock, this->mutex, this_ethread());
this->stream_list.enqueue(stream);
} else {
// Create new stream
Http2Error error(Http2ErrorClass::HTTP2_ERROR_CLASS_NONE);
Expand Down Expand Up @@ -370,6 +388,9 @@ Http2ConnectionState::rcv_headers_frame(const Http2Frame &frame)
}
}

stream->reset_header_after_decoding = reset_header_after_decoding;
stream->free_stream_after_decoding = free_stream_after_decoding;

Http2HeadersParameter params;
uint32_t header_block_fragment_offset = 0;
uint32_t header_block_fragment_length = payload_length;
Expand Down Expand Up @@ -477,13 +498,19 @@ Http2ConnectionState::rcv_headers_frame(const Http2Frame &frame)
Http2ErrorCode result = stream->decode_header_blocks(*this->local_hpack_handle,
this->acknowledged_local_settings.get(HTTP2_SETTINGS_HEADER_TABLE_SIZE));

// If this was an outbound connection and the state was already closed, just clear the
// headers after processing. We just processed the heaer blocks to keep the dynamic table in
// We just processed the heaer blocks to keep the dynamic table in
// sync with peer to avoid future HPACK compression errors
if (reset_header_after_decoding) {
stream->reset_receive_headers();
SCOPED_MUTEX_LOCK(lock, this->mutex, this_ethread());
this->stream_list.remove(stream);
if (free_stream_after_decoding) {
THREAD_FREE(stream, http2StreamAllocator, this_ethread());
stream->initiating_close();
}

if (this->goaway_sent && stream_id > this->last_stream_id_tx) {
return Http2Error(Http2ErrorClass::HTTP2_ERROR_CLASS_STREAM, Http2ErrorCode::HTTP2_ERROR_REFUSED_STREAM,
"recv headers with id higher than last stream id");
}
return Http2Error(Http2ErrorClass::HTTP2_ERROR_CLASS_NONE);
}
Expand Down Expand Up @@ -1100,6 +1127,27 @@ Http2ConnectionState::rcv_continuation_frame(const Http2Frame &frame)
Http2ErrorCode result = stream->decode_header_blocks(*this->local_hpack_handle,
this->acknowledged_local_settings.get(HTTP2_SETTINGS_HEADER_TABLE_SIZE));

// We just processed the heaer blocks to keep the dynamic table in
// sync with peer to avoid future HPACK compression errors
if (stream->reset_header_after_decoding) {
stream->reset_receive_headers();
SCOPED_MUTEX_LOCK(lock, this->mutex, this_ethread());
this->stream_list.remove(stream);
if (stream->free_stream_after_decoding) {
stream->initiating_close();
}

// After sending a GOAWAY frame, the sender can discard frames for streams initiated by the receiver with
// identifiers higher than the identified last stream. However, HEADERS, PUSH_PROMISE, and CONTINUATION
// frames MUST be minimally processed to ensure that the state maintained for field section compression is
// consistent (Details in [RFC 9113] 6.8.)
if (this->goaway_sent && stream_id > this->last_stream_id_tx) {
return Http2Error(Http2ErrorClass::HTTP2_ERROR_CLASS_STREAM, Http2ErrorCode::HTTP2_ERROR_REFUSED_STREAM,
"recv continuation with id higher than last stream id");
}
return Http2Error(Http2ErrorClass::HTTP2_ERROR_CLASS_NONE);
}

if (result != Http2ErrorCode::HTTP2_ERROR_NO_ERROR) {
if (result == Http2ErrorCode::HTTP2_ERROR_COMPRESSION_ERROR) {
return Http2Error(Http2ErrorClass::HTTP2_ERROR_CLASS_CONNECTION, Http2ErrorCode::HTTP2_ERROR_COMPRESSION_ERROR,
Expand Down Expand Up @@ -1437,12 +1485,13 @@ Http2ConnectionState::rcv_frame(const Http2Frame *frame)
{
REMEMBER(NO_EVENT, this->recursion);
const Http2StreamId stream_id = frame->header().streamid;
const auto type = frame->header().type;
Http2Error error;

// [RFC 7540] 5.5. Extending HTTP/2
// Implementations MUST discard frames that have unknown or unsupported types.
if (frame->header().type >= HTTP2_FRAME_TYPE_MAX) {
Http2StreamDebug(session, stream_id, "Discard a frame which has unknown type, type=%x", frame->header().type);
if (type >= HTTP2_FRAME_TYPE_MAX) {
Http2StreamDebug(session, stream_id, "Discard a frame which has unknown type, type=%x", type);
return;
}

Expand All @@ -1457,15 +1506,28 @@ Http2ConnectionState::rcv_frame(const Http2Frame *frame)
// GOAWAY: NO
// WINDOW_UPDATE: YES
// CONTINUATION: YES (safe http methods only, same as HEADERS frame).
if (frame->is_from_early_data() &&
(frame->header().type == HTTP2_FRAME_TYPE_DATA || frame->header().type == HTTP2_FRAME_TYPE_RST_STREAM ||
frame->header().type == HTTP2_FRAME_TYPE_PUSH_PROMISE || frame->header().type == HTTP2_FRAME_TYPE_GOAWAY)) {
Http2StreamDebug(session, stream_id, "Discard a frame which is received from early data and has type=%x", frame->header().type);
if (frame->is_from_early_data() && (type == HTTP2_FRAME_TYPE_DATA || type == HTTP2_FRAME_TYPE_RST_STREAM ||
type == HTTP2_FRAME_TYPE_PUSH_PROMISE || type == HTTP2_FRAME_TYPE_GOAWAY)) {
Http2StreamDebug(session, stream_id, "Discard a frame which is received from early data and has type=%x", type);
return;
}

if (this->_frame_handlers[frame->header().type]) {
error = (this->*_frame_handlers[frame->header().type])(*frame);
// After sending a GOAWAY frame, the sender can discard frames for streams initiated by the receiver with
// identifiers higher than the identified last stream. However, HEADERS, PUSH_PROMISE, and CONTINUATION
// frames MUST be minimally processed to ensure that the state maintained for field section compression is
// consistent; similarly, DATA frames MUST be counted toward the connection flow-control window.
// (Details in [RFC 9113] 6.8.)
if (this->goaway_sent && stream_id > this->last_stream_id_tx) {
const auto is_discardable = (type != HTTP2_FRAME_TYPE_HEADERS && type != HTTP2_FRAME_TYPE_PUSH_PROMISE &&
type != HTTP2_FRAME_TYPE_CONTINUATION && type != HTTP2_FRAME_TYPE_DATA);
if (is_discardable) {
Http2StreamDebug(session, stream_id, "Discard a frame which is received after sending a GOAWAY and has type=%x", type);
return;
}
}

if (this->_frame_handlers[type]) {
error = (this->*_frame_handlers[type])(*frame);
} else {
error = Http2Error(Http2ErrorClass::HTTP2_ERROR_CLASS_CONNECTION, Http2ErrorCode::HTTP2_ERROR_INTERNAL_ERROR, "no handler");
}
Expand Down Expand Up @@ -2744,7 +2806,9 @@ Http2ConnectionState::send_goaway_frame(Http2StreamId id, Http2ErrorCode ec)
Metrics::Counter::increment(http2_rsb.connection_errors_count);
}

this->tx_error_code = {ProxyErrorClass::SSN, static_cast<uint32_t>(ec)};
this->tx_error_code = {ProxyErrorClass::SSN, static_cast<uint32_t>(ec)};
this->last_stream_id_tx = id;
this->goaway_sent = true;

Http2Goaway goaway;
goaway.last_streamid = id;
Expand Down