diff --git a/source/common/http/http2/BUILD b/source/common/http/http2/BUILD index d2e4ed0113115..ac83f7b49884e 100644 --- a/source/common/http/http2/BUILD +++ b/source/common/http/http2/BUILD @@ -58,7 +58,9 @@ envoy_cc_library( "abseil_inlined_vector", "abseil_algorithm", ], - deps = CODEC_LIB_DEPS, + deps = CODEC_LIB_DEPS + [ + "//source/common/common:statusor_lib", + ], ) envoy_cc_library( diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index 0405ec12a5c8a..205c9d2d18abc 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -159,7 +159,16 @@ void ConnectionImpl::StreamImpl::encodeHeadersBase(const std::vector local_end_stream_ = end_stream; submitHeaders(final_headers, end_stream ? nullptr : &provider); - parent_.sendPendingFrames(); + auto status = parent_.sendPendingFrames(); + // The RELEASE_ASSERT below does not change the existing behavior of `sendPendingFrames()`. + // The `sendPendingFrames()` used to throw on errors and the only method that was catching + // these exceptions was the `dispatch()`. The `dispatch()` method still checks and handles + // errors returned by the `sendPendingFrames()`. + // Other callers of `sendPendingFrames()` do not catch exceptions from this method and + // would cause abnormal process termination in error cases. This change replaces abnormal + // process termination from unhandled exception with the RELEASE_ASSERT. + // Further work will replace this RELEASE_ASSERT with proper error handling. + RELEASE_ASSERT(status.ok(), "sendPendingFrames() failure in non dispatching context"); } void ConnectionImpl::ClientStreamImpl::encodeHeaders(const RequestHeaderMap& headers, @@ -222,7 +231,9 @@ void ConnectionImpl::StreamImpl::encodeTrailersBase(const HeaderMap& trailers) { createPendingFlushTimer(); } else { submitTrailers(trailers); - parent_.sendPendingFrames(); + auto status = parent_.sendPendingFrames(); + // See comment in the `encodeHeadersBase()` method about this RELEASE_ASSERT. + RELEASE_ASSERT(status.ok(), "sendPendingFrames() failure in non dispatching context"); } } @@ -235,7 +246,9 @@ void ConnectionImpl::StreamImpl::encodeMetadata(const MetadataMapVector& metadat for (uint8_t flags : metadata_encoder.payloadFrameFlagBytes()) { submitMetadata(flags); } - parent_.sendPendingFrames(); + auto status = parent_.sendPendingFrames(); + // See comment in the `encodeHeadersBase()` method about this RELEASE_ASSERT. + RELEASE_ASSERT(status.ok(), "sendPendingFrames() failure in non dispatching context"); } void ConnectionImpl::StreamImpl::readDisable(bool disable) { @@ -250,7 +263,9 @@ void ConnectionImpl::StreamImpl::readDisable(bool disable) { if (!buffersOverrun()) { nghttp2_session_consume(parent_.session_, stream_id_, unconsumed_bytes_); unconsumed_bytes_ = 0; - parent_.sendPendingFrames(); + auto status = parent_.sendPendingFrames(); + // See comment in the `encodeHeadersBase()` method about this RELEASE_ASSERT. + RELEASE_ASSERT(status.ok(), "sendPendingFrames() failure in non dispatching context"); } } } @@ -364,7 +379,7 @@ ssize_t ConnectionImpl::StreamImpl::onDataSourceRead(uint64_t length, uint32_t* } } -int ConnectionImpl::StreamImpl::onDataSourceSend(const uint8_t* framehd, size_t length) { +Status ConnectionImpl::StreamImpl::onDataSourceSend(const uint8_t* framehd, size_t length) { // In this callback we are writing out a raw DATA frame without copying. nghttp2 assumes that we // "just know" that the frame header is 9 bytes. // https://nghttp2.org/documentation/types.html#c.nghttp2_send_data_callback @@ -373,17 +388,18 @@ int ConnectionImpl::StreamImpl::onDataSourceSend(const uint8_t* framehd, size_t parent_.outbound_data_frames_++; Buffer::OwnedImpl output; - if (!parent_.addOutboundFrameFragment(output, framehd, FRAME_HEADER_SIZE)) { + auto status = parent_.addOutboundFrameFragment(output, framehd, FRAME_HEADER_SIZE); + if (!status.ok()) { ENVOY_CONN_LOG(debug, "error sending data frame: Too many frames in the outbound queue", parent_.connection_); setDetails(Http2ResponseCodeDetails::get().outbound_frame_flood); - return NGHTTP2_ERR_FLOODED; + return status; } parent_.stats_.pending_send_bytes_.sub(length); output.move(pending_send_data_, length); parent_.connection_.write(output, false); - return 0; + return status; } void ConnectionImpl::ClientStreamImpl::submitHeaders(const std::vector& final_headers, @@ -419,7 +435,9 @@ void ConnectionImpl::StreamImpl::onPendingFlushTimer() { // This will emit a reset frame for this stream and close the stream locally. No reset callbacks // will be run because higher layers think the stream is already finished. resetStreamWorker(StreamResetReason::LocalReset); - parent_.sendPendingFrames(); + auto status = parent_.sendPendingFrames(); + // See comment in the `encodeHeadersBase()` method about this RELEASE_ASSERT. + RELEASE_ASSERT(status.ok(), "sendPendingFrames() failure in non dispatching context"); } void ConnectionImpl::StreamImpl::encodeData(Buffer::Instance& data, bool end_stream) { @@ -434,7 +452,9 @@ void ConnectionImpl::StreamImpl::encodeData(Buffer::Instance& data, bool end_str data_deferred_ = false; } - parent_.sendPendingFrames(); + auto status = parent_.sendPendingFrames(); + // See comment in the `encodeHeadersBase()` method about this RELEASE_ASSERT. + RELEASE_ASSERT(status.ok(), "sendPendingFrames() failure in non dispatching context"); if (local_end_stream_ && pending_send_data_.length() > 0) { createPendingFlushTimer(); } @@ -458,7 +478,9 @@ void ConnectionImpl::StreamImpl::resetStream(StreamResetReason reason) { // We must still call sendPendingFrames() in both the deferred and not deferred path. This forces // the cleanup logic to run which will reset the stream in all cases if all data frames could not // be sent. - parent_.sendPendingFrames(); + auto status = parent_.sendPendingFrames(); + // See comment in the `encodeHeadersBase()` method about this RELEASE_ASSERT. + RELEASE_ASSERT(status.ok(), "sendPendingFrames() failure in non dispatching context"); } void ConnectionImpl::StreamImpl::resetStreamWorker(StreamResetReason reason) { @@ -498,7 +520,7 @@ ConnectionImpl::ConnectionImpl(Network::Connection& connection, CodecStats& stat per_stream_buffer_limit_(http2_options.initial_stream_window_size().value()), stream_error_on_invalid_http_messaging_( http2_options.override_stream_error_on_invalid_http_message().value()), - flood_detected_(false), max_outbound_frames_(http2_options.max_outbound_frames().value()), + max_outbound_frames_(http2_options.max_outbound_frames().value()), frame_buffer_releasor_([this]() { releaseOutboundFrame(); }), max_outbound_control_frames_(http2_options.max_outbound_control_frames().value()), control_frame_buffer_releasor_([this]() { releaseOutboundControlFrame(); }), @@ -535,12 +557,19 @@ Http::Status ConnectionImpl::innerDispatch(Buffer::Instance& data) { dispatching_ = true; ssize_t rc = nghttp2_session_mem_recv(session_, static_cast(slice.mem_), slice.len_); - if (rc == NGHTTP2_ERR_FLOODED || flood_detected_) { - throw FrameFloodException( + if (!nghttp2_callback_status_.ok()) { + return nghttp2_callback_status_; + } + // This error is returned when nghttp2 library detected a frame flood by one of its + // internal mechanisms. Most flood protection is done by Envoy's codec and this error + // should never be returned. However it is handled here in case nghttp2 has some flood + // protections that Envoy's codec does not have. + if (rc == NGHTTP2_ERR_FLOODED) { + return bufferFloodError( "Flooding was detected in this HTTP/2 session, and it must be closed"); } if (rc != static_cast(slice.len_)) { - throw CodecProtocolException(fmt::format("{}", nghttp2_strerror(rc))); + return codecProtocolError(nghttp2_strerror(rc)); } dispatching_ = false; @@ -550,8 +579,7 @@ Http::Status ConnectionImpl::innerDispatch(Buffer::Instance& data) { data.drain(data.length()); // Decoding incoming frames can generate outbound frames so flush pending. - sendPendingFrames(); - return Http::okStatus(); + return sendPendingFrames(); } ConnectionImpl::StreamImpl* ConnectionImpl::getStream(int32_t stream_id) { @@ -579,30 +607,33 @@ void ConnectionImpl::goAway() { NGHTTP2_NO_ERROR, nullptr, 0); ASSERT(rc == 0); - sendPendingFrames(); + auto status = sendPendingFrames(); + // See comment in the `encodeHeadersBase()` method about this RELEASE_ASSERT. + RELEASE_ASSERT(status.ok(), "sendPendingFrames() failure in non dispatching context"); } void ConnectionImpl::shutdownNotice() { int rc = nghttp2_submit_shutdown_notice(session_); ASSERT(rc == 0); - sendPendingFrames(); + auto status = sendPendingFrames(); + // See comment in the `encodeHeadersBase()` method about this RELEASE_ASSERT. + RELEASE_ASSERT(status.ok(), "sendPendingFrames() failure in non dispatching context"); } -int ConnectionImpl::onBeforeFrameReceived(const nghttp2_frame_hd* hd) { +Status ConnectionImpl::onBeforeFrameReceived(const nghttp2_frame_hd* hd) { ENVOY_CONN_LOG(trace, "about to recv frame type={}, flags={}", connection_, static_cast(hd->type), static_cast(hd->flags)); // Track all the frames without padding here, since this is the only callback we receive // for some of them (e.g. CONTINUATION frame, frames sent on closed streams, etc.). // HEADERS frame is tracked in onBeginHeaders(), DATA frame is tracked in onFrameReceived(). + auto status = okStatus(); if (hd->type != NGHTTP2_HEADERS && hd->type != NGHTTP2_DATA) { - if (!trackInboundFrames(hd, 0)) { - return NGHTTP2_ERR_FLOODED; - } + status = trackInboundFrames(hd, 0); } - return 0; + return status; } ABSL_MUST_USE_RESULT @@ -615,7 +646,7 @@ enum GoAwayErrorCode ngHttp2ErrorCodeToErrorCode(uint32_t code) noexcept { } } -int ConnectionImpl::onFrameReceived(const nghttp2_frame* frame) { +Status ConnectionImpl::onFrameReceived(const nghttp2_frame* frame) { ENVOY_CONN_LOG(trace, "recv frame type={}", connection_, static_cast(frame->hd.type)); // onFrameReceived() is called with a complete HEADERS frame assembled from all the HEADERS @@ -624,9 +655,7 @@ int ConnectionImpl::onFrameReceived(const nghttp2_frame* frame) { ASSERT(frame->hd.type != NGHTTP2_CONTINUATION); if (frame->hd.type == NGHTTP2_DATA) { - if (!trackInboundFrames(&frame->hd, frame->data.padlen)) { - return NGHTTP2_ERR_FLOODED; - } + RETURN_IF_ERROR(trackInboundFrames(&frame->hd, frame->data.padlen)); } // Only raise GOAWAY once, since we don't currently expose stream information. Shutdown @@ -636,7 +665,7 @@ int ConnectionImpl::onFrameReceived(const nghttp2_frame* frame) { ASSERT(frame->hd.stream_id == 0); raised_goaway_ = true; callbacks().onGoAway(ngHttp2ErrorCodeToErrorCode(frame->goaway.error_code)); - return 0; + return okStatus(); } if (frame->hd.type == NGHTTP2_SETTINGS && frame->hd.flags == NGHTTP2_FLAG_NONE) { @@ -645,7 +674,7 @@ int ConnectionImpl::onFrameReceived(const nghttp2_frame* frame) { StreamImpl* stream = getStream(frame->hd.stream_id); if (!stream) { - return 0; + return okStatus(); } switch (frame->hd.type) { @@ -705,7 +734,7 @@ int ConnectionImpl::onFrameReceived(const nghttp2_frame* frame) { } } - return 0; + return okStatus(); } int ConnectionImpl::onFrameSend(const nghttp2_frame* frame) { @@ -794,30 +823,26 @@ int ConnectionImpl::onBeforeFrameSend(const nghttp2_frame* frame) { return 0; } -void ConnectionImpl::incrementOutboundFrameCount(bool is_outbound_flood_monitored_control_frame) { +Status ConnectionImpl::incrementOutboundFrameCount(bool is_outbound_flood_monitored_control_frame) { ++outbound_frames_; if (is_outbound_flood_monitored_control_frame) { ++outbound_control_frames_; } - checkOutboundQueueLimits(); + return checkOutboundQueueLimits(); } -bool ConnectionImpl::addOutboundFrameFragment(Buffer::OwnedImpl& output, const uint8_t* data, - size_t length) { +Status ConnectionImpl::addOutboundFrameFragment(Buffer::OwnedImpl& output, const uint8_t* data, + size_t length) { // Reset the outbound frame type (set in the onBeforeFrameSend callback) since the // onBeforeFrameSend callback is not called for DATA frames. bool is_outbound_flood_monitored_control_frame = false; std::swap(is_outbound_flood_monitored_control_frame, is_outbound_flood_monitored_control_frame_); - try { - incrementOutboundFrameCount(is_outbound_flood_monitored_control_frame); - } catch (const FrameFloodException&) { - return false; - } + RETURN_IF_ERROR(incrementOutboundFrameCount(is_outbound_flood_monitored_control_frame)); output.add(data, length); output.addDrainTracker(is_outbound_flood_monitored_control_frame ? control_frame_buffer_releasor_ : frame_buffer_releasor_); - return true; + return okStatus(); } void ConnectionImpl::releaseOutboundFrame() { @@ -831,13 +856,14 @@ void ConnectionImpl::releaseOutboundControlFrame() { releaseOutboundFrame(); } -ssize_t ConnectionImpl::onSend(const uint8_t* data, size_t length) { +StatusOr ConnectionImpl::onSend(const uint8_t* data, size_t length) { ENVOY_CONN_LOG(trace, "send data: bytes={}", connection_, length); Buffer::OwnedImpl buffer; - if (!addOutboundFrameFragment(buffer, data, length)) { + auto status = addOutboundFrameFragment(buffer, data, length); + if (!status.ok()) { ENVOY_CONN_LOG(debug, "error sending frame: Too many frames in the outbound queue.", connection_); - return NGHTTP2_ERR_FLOODED; + return status; } // While the buffer is transient the fragment it contains will be moved into the @@ -959,24 +985,25 @@ int ConnectionImpl::saveHeader(const nghttp2_frame* frame, HeaderString&& name, } } -void ConnectionImpl::sendPendingFrames() { +Status ConnectionImpl::sendPendingFrames() { if (dispatching_ || connection_.state() == Network::Connection::State::Closed) { - return; + return okStatus(); } const int rc = nghttp2_session_send(session_); if (rc != 0) { ASSERT(rc == NGHTTP2_ERR_CALLBACK_FAILURE); - // For errors caused by the pending outbound frame flood the FrameFloodException has - // to be thrown. However the nghttp2 library returns only the generic error code for - // all failure types. Check queue limits and throw FrameFloodException if they were - // exceeded. - if (outbound_frames_ > max_outbound_frames_ || - outbound_control_frames_ > max_outbound_control_frames_) { - throw FrameFloodException("Too many frames in the outbound queue."); + + if (!nghttp2_callback_status_.ok()) { + return nghttp2_callback_status_; } - throw CodecProtocolException(std::string(nghttp2_strerror(rc))); + // The frame flood error should set the nghttp2_callback_status_ error, and return at the + // statement above. + ASSERT(outbound_frames_ <= max_outbound_frames_ && + outbound_control_frames_ <= max_outbound_control_frames_); + + return codecProtocolError(nghttp2_strerror(rc)); } // See ConnectionImpl::StreamImpl::resetStream() for why we do this. This is an uncommon event, @@ -998,8 +1025,9 @@ void ConnectionImpl::sendPendingFrames() { stream->resetStreamWorker(stream->deferred_reset_.value()); } } - sendPendingFrames(); + RETURN_IF_ERROR(sendPendingFrames()); } + return okStatus(); } void ConnectionImpl::sendSettings( @@ -1065,12 +1093,25 @@ void ConnectionImpl::sendSettings( } } +int ConnectionImpl::setAndCheckNghttp2CallbackStatus(Status&& status) { + // Keep the error status that caused the original failure. Subsequent + // error statuses are silently discarded. + nghttp2_callback_status_.Update(std::move(status)); + return nghttp2_callback_status_.ok() ? 0 : NGHTTP2_ERR_CALLBACK_FAILURE; +} + ConnectionImpl::Http2Callbacks::Http2Callbacks() { nghttp2_session_callbacks_new(&callbacks_); nghttp2_session_callbacks_set_send_callback( callbacks_, [](nghttp2_session*, const uint8_t* data, size_t length, int, void* user_data) -> ssize_t { - return static_cast(user_data)->onSend(data, length); + auto status_or_len = static_cast(user_data)->onSend(data, length); + if (status_or_len.ok()) { + return status_or_len.value(); + } + auto status = status_or_len.status(); + return static_cast(user_data)->setAndCheckNghttp2CallbackStatus( + std::move(status)); }); nghttp2_session_callbacks_set_send_data_callback( @@ -1078,12 +1119,16 @@ ConnectionImpl::Http2Callbacks::Http2Callbacks() { [](nghttp2_session*, nghttp2_frame* frame, const uint8_t* framehd, size_t length, nghttp2_data_source* source, void*) -> int { ASSERT(frame->data.padlen == 0); - return static_cast(source->ptr)->onDataSourceSend(framehd, length); + auto status = static_cast(source->ptr)->onDataSourceSend(framehd, length); + return static_cast(source->ptr) + ->parent_.setAndCheckNghttp2CallbackStatus(std::move(status)); }); nghttp2_session_callbacks_set_on_begin_headers_callback( callbacks_, [](nghttp2_session*, const nghttp2_frame* frame, void* user_data) -> int { - return static_cast(user_data)->onBeginHeaders(frame); + auto status = static_cast(user_data)->onBeginHeaders(frame); + return static_cast(user_data)->setAndCheckNghttp2CallbackStatus( + std::move(status)); }); nghttp2_session_callbacks_set_on_header_callback( @@ -1108,12 +1153,16 @@ ConnectionImpl::Http2Callbacks::Http2Callbacks() { nghttp2_session_callbacks_set_on_begin_frame_callback( callbacks_, [](nghttp2_session*, const nghttp2_frame_hd* hd, void* user_data) -> int { - return static_cast(user_data)->onBeforeFrameReceived(hd); + auto status = static_cast(user_data)->onBeforeFrameReceived(hd); + return static_cast(user_data)->setAndCheckNghttp2CallbackStatus( + std::move(status)); }); nghttp2_session_callbacks_set_on_frame_recv_callback( callbacks_, [](nghttp2_session*, const nghttp2_frame* frame, void* user_data) -> int { - return static_cast(user_data)->onFrameReceived(frame); + auto status = static_cast(user_data)->onFrameReceived(frame); + return static_cast(user_data)->setAndCheckNghttp2CallbackStatus( + std::move(status)); }); nghttp2_session_callbacks_set_on_stream_close_callback( @@ -1251,7 +1300,7 @@ RequestEncoder& ClientConnectionImpl::newStream(ResponseDecoder& decoder) { return stream_ref; } -int ClientConnectionImpl::onBeginHeaders(const nghttp2_frame* frame) { +Status ClientConnectionImpl::onBeginHeaders(const nghttp2_frame* frame) { // The client code explicitly does not currently support push promise. RELEASE_ASSERT(frame->hd.type == NGHTTP2_HEADERS, ""); RELEASE_ASSERT(frame->headers.cat == NGHTTP2_HCAT_RESPONSE || @@ -1262,7 +1311,7 @@ int ClientConnectionImpl::onBeginHeaders(const nghttp2_frame* frame) { stream->allocTrailers(); } - return 0; + return okStatus(); } int ClientConnectionImpl::onHeader(const nghttp2_frame* frame, HeaderString&& name, @@ -1290,13 +1339,11 @@ ServerConnectionImpl::ServerConnectionImpl( allow_metadata_ = http2_options.allow_metadata(); } -int ServerConnectionImpl::onBeginHeaders(const nghttp2_frame* frame) { +Status ServerConnectionImpl::onBeginHeaders(const nghttp2_frame* frame) { // For a server connection, we should never get push promise frames. ASSERT(frame->hd.type == NGHTTP2_HEADERS); - if (!trackInboundFrames(&frame->hd, frame->headers.padlen)) { - return NGHTTP2_ERR_FLOODED; - } + RETURN_IF_ERROR(trackInboundFrames(&frame->hd, frame->headers.padlen)); if (frame->headers.cat != NGHTTP2_HCAT_REQUEST) { stats_.trailers_.inc(); @@ -1304,7 +1351,7 @@ int ServerConnectionImpl::onBeginHeaders(const nghttp2_frame* frame) { StreamImpl* stream = getStream(frame->hd.stream_id); stream->allocTrailers(); - return 0; + return okStatus(); } ServerStreamImplPtr stream(new ServerStreamImpl(*this, per_stream_buffer_limit_)); @@ -1316,7 +1363,7 @@ int ServerConnectionImpl::onBeginHeaders(const nghttp2_frame* frame) { LinkedList::moveIntoList(std::move(stream), active_streams_); nghttp2_session_set_stream_user_data(session_, frame->hd.stream_id, active_streams_.front().get()); - return 0; + return okStatus(); } int ServerConnectionImpl::onHeader(const nghttp2_frame* frame, HeaderString&& name, @@ -1327,7 +1374,8 @@ int ServerConnectionImpl::onHeader(const nghttp2_frame* frame, HeaderString&& na return saveHeader(frame, std::move(name), std::move(value)); } -bool ServerConnectionImpl::trackInboundFrames(const nghttp2_frame_hd* hd, uint32_t padding_length) { +Status ServerConnectionImpl::trackInboundFrames(const nghttp2_frame_hd* hd, + uint32_t padding_length) { ENVOY_CONN_LOG(trace, "track inbound frame type={} flags={} length={} padding_length={}", connection_, static_cast(hd->type), static_cast(hd->flags), static_cast(hd->length), padding_length); @@ -1358,17 +1406,10 @@ bool ServerConnectionImpl::trackInboundFrames(const nghttp2_frame_hd* hd, uint32 break; } - if (!checkInboundFrameLimits(hd->stream_id)) { - // NGHTTP2_ERR_FLOODED is overridden within nghttp2 library and it doesn't propagate - // all the way to nghttp2_session_mem_recv() where we need it. - flood_detected_ = true; - return false; - } - - return true; + return checkInboundFrameLimits(hd->stream_id); } -bool ServerConnectionImpl::checkInboundFrameLimits(int32_t stream_id) { +Status ServerConnectionImpl::checkInboundFrameLimits(int32_t stream_id) { ASSERT(dispatching_downstream_data_); ConnectionImpl::StreamImpl* stream = getStream(stream_id); @@ -1382,7 +1423,7 @@ bool ServerConnectionImpl::checkInboundFrameLimits(int32_t stream_id) { stream->setDetails(Http2ResponseCodeDetails::get().inbound_empty_frame_flood); } stats_.inbound_empty_frames_flood_.inc(); - return false; + return bufferFloodError("Too many consecutive frames with an empty payload"); } if (inbound_priority_frames_ > max_inbound_priority_frames_per_stream_ * (1 + inbound_streams_)) { @@ -1390,7 +1431,7 @@ bool ServerConnectionImpl::checkInboundFrameLimits(int32_t stream_id) { "error reading frame: Too many PRIORITY frames received in this HTTP/2 session.", connection_); stats_.inbound_priority_frames_flood_.inc(); - return false; + return bufferFloodError("Too many PRIORITY frames"); } if (inbound_window_update_frames_ > @@ -1401,21 +1442,22 @@ bool ServerConnectionImpl::checkInboundFrameLimits(int32_t stream_id) { "error reading frame: Too many WINDOW_UPDATE frames received in this HTTP/2 session.", connection_); stats_.inbound_window_update_frames_flood_.inc(); - return false; + return bufferFloodError("Too many WINDOW_UPDATE frames"); } - return true; + return okStatus(); } -void ServerConnectionImpl::checkOutboundQueueLimits() { +Status ServerConnectionImpl::checkOutboundQueueLimits() { if (outbound_frames_ > max_outbound_frames_ && dispatching_downstream_data_) { stats_.outbound_flood_.inc(); - throw FrameFloodException("Too many frames in the outbound queue."); + return bufferFloodError("Too many frames in the outbound queue."); } if (outbound_control_frames_ > max_outbound_control_frames_ && dispatching_downstream_data_) { stats_.outbound_control_flood_.inc(); - throw FrameFloodException("Too many control frames in the outbound queue."); + return bufferFloodError("Too many control frames in the outbound queue."); } + return okStatus(); } Http::Status ServerConnectionImpl::dispatch(Buffer::Instance& data) { @@ -1430,13 +1472,11 @@ Http::Status ServerConnectionImpl::innerDispatch(Buffer::Instance& data) { ASSERT(!dispatching_downstream_data_); dispatching_downstream_data_ = true; - // Make sure the dispatching_downstream_data_ is set to false even - // when ConnectionImpl::dispatch throws an exception. + // Make sure the dispatching_downstream_data_ is set to false when innerDispatch ends. Cleanup cleanup([this]() { dispatching_downstream_data_ = false; }); // Make sure downstream outbound queue was not flooded by the upstream frames. - checkOutboundQueueLimits(); - + RETURN_IF_ERROR(checkOutboundQueueLimits()); return ConnectionImpl::innerDispatch(data); } diff --git a/source/common/http/http2/codec_impl.h b/source/common/http/http2/codec_impl.h index b19f9880a3dbf..ee3b712535029 100644 --- a/source/common/http/http2/codec_impl.h +++ b/source/common/http/http2/codec_impl.h @@ -16,6 +16,7 @@ #include "common/buffer/watermark_buffer.h" #include "common/common/linked_object.h" #include "common/common/logger.h" +#include "common/common/statusor.h" #include "common/common/thread.h" #include "common/http/codec_helper.h" #include "common/http/header_map_impl.h" @@ -185,7 +186,7 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable& final_headers, const HeaderMap& headers); void saveHeader(HeaderString&& name, HeaderString&& value); @@ -391,7 +392,7 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable active_streams_; @@ -421,7 +428,12 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable onSend(const uint8_t* data, size_t length); private: virtual ConnectionCallbacks& callbacks() PURE; - virtual int onBeginHeaders(const nghttp2_frame* frame) PURE; + virtual Status onBeginHeaders(const nghttp2_frame* frame) PURE; int onData(int32_t stream_id, const uint8_t* data, size_t len); - int onBeforeFrameReceived(const nghttp2_frame_hd* hd); - int onFrameReceived(const nghttp2_frame* frame); + Status onBeforeFrameReceived(const nghttp2_frame_hd* hd); + Status onFrameReceived(const nghttp2_frame* frame); int onBeforeFrameSend(const nghttp2_frame* frame); int onFrameSend(const nghttp2_frame* frame); int onError(absl::string_view error); @@ -501,12 +513,12 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable checkHeaderNameForUnderscores(absl::string_view header_name) override; // Http::Connection diff --git a/test/common/http/http2/BUILD b/test/common/http/http2/BUILD index 646345fd13734..62ef3d05c34cb 100644 --- a/test/common/http/http2/BUILD +++ b/test/common/http/http2/BUILD @@ -106,7 +106,6 @@ envoy_cc_test_library( "//source/common/http:utility_lib", "//source/common/http/http2:codec_lib", "//test/common/http:common_lib", - "//test/common/http/http2:codec_impl_test_util", "//test/mocks/http:http_mocks", "//test/mocks/network:network_mocks", "//test/test_common:environment_lib", @@ -124,7 +123,10 @@ envoy_cc_test( "response_header_corpus/simple_example_plain", ], tags = ["fails_on_windows"], - deps = [":frame_replay_lib"], + deps = [ + ":frame_replay_lib", + "//test/common/http/http2:codec_impl_test_util", + ], ) envoy_cc_test( @@ -147,12 +149,18 @@ envoy_cc_fuzz_test( name = "response_header_fuzz_test", srcs = ["response_header_fuzz_test.cc"], corpus = "response_header_corpus", - deps = [":frame_replay_lib"], + deps = [ + ":frame_replay_lib", + "//test/common/http/http2:codec_impl_test_util", + ], ) envoy_cc_fuzz_test( name = "request_header_fuzz_test", srcs = ["request_header_fuzz_test.cc"], corpus = "request_header_corpus", - deps = [":frame_replay_lib"], + deps = [ + ":frame_replay_lib", + "//test/common/http/http2:codec_impl_test_util", + ], ) diff --git a/test/common/http/http2/codec_impl_test.cc b/test/common/http/http2/codec_impl_test.cc index c3c56839244bf..15696671724d1 100644 --- a/test/common/http/http2/codec_impl_test.cc +++ b/test/common/http/http2/codec_impl_test.cc @@ -1801,7 +1801,12 @@ TEST_P(Http2CodecImplTest, PingFlood) { buffer.move(frame); })); - EXPECT_THROW(client_->sendPendingFrames(), ServerCodecError); + // Legacy codec does not propagate error details and uses generic error message + EXPECT_THROW_WITH_MESSAGE( + client_->sendPendingFrames().IgnoreError(), ServerCodecError, + Runtime::runtimeFeatureEnabled("envoy.reloadable_features.new_codec_behavior") + ? "Too many control frames in the outbound queue." + : "Too many frames in the outbound queue."); EXPECT_EQ(ack_count, CommonUtility::OptionsLimits::DEFAULT_MAX_OUTBOUND_CONTROL_FRAMES); EXPECT_EQ(1, server_stats_store_.counter("http2.outbound_control_flood").value()); } @@ -1824,7 +1829,7 @@ TEST_P(Http2CodecImplTest, PingFloodMitigationDisabled) { EXPECT_CALL(server_connection_, write(_, _)) .Times(CommonUtility::OptionsLimits::DEFAULT_MAX_OUTBOUND_CONTROL_FRAMES + 1); - EXPECT_NO_THROW(client_->sendPendingFrames()); + EXPECT_NO_THROW(client_->sendPendingFrames().IgnoreError()); } // Verify that outbound control frame counter decreases when send buffer is drained @@ -1854,7 +1859,7 @@ TEST_P(Http2CodecImplTest, PingFloodCounterReset) { })); // We should be 1 frame under the control frame flood mitigation threshold. - EXPECT_NO_THROW(client_->sendPendingFrames()); + EXPECT_NO_THROW(client_->sendPendingFrames().IgnoreError()); EXPECT_EQ(ack_count, kMaxOutboundControlFrames); // Drain floor(kMaxOutboundFrames / 2) slices from the send buffer @@ -1866,12 +1871,17 @@ TEST_P(Http2CodecImplTest, PingFloodCounterReset) { } // The number of outbound frames should be half of max so the connection should not be // terminated. - EXPECT_NO_THROW(client_->sendPendingFrames()); + EXPECT_NO_THROW(client_->sendPendingFrames().IgnoreError()); EXPECT_EQ(ack_count, kMaxOutboundControlFrames + kMaxOutboundControlFrames / 2); // 1 more ping frame should overflow the outbound frame limit. EXPECT_EQ(0, nghttp2_submit_ping(client_->session(), NGHTTP2_FLAG_NONE, nullptr)); - EXPECT_THROW(client_->sendPendingFrames(), ServerCodecError); + // Legacy codec does not propagate error details and uses generic error message + EXPECT_THROW_WITH_MESSAGE( + client_->sendPendingFrames().IgnoreError(), ServerCodecError, + Runtime::runtimeFeatureEnabled("envoy.reloadable_features.new_codec_behavior") + ? "Too many control frames in the outbound queue." + : "Too many frames in the outbound queue."); } // Verify that codec detects flood of outbound HEADER frames @@ -1898,7 +1908,8 @@ TEST_P(Http2CodecImplTest, ResponseHeadersFlood) { // Presently flood mitigation is done only when processing downstream data // So we need to send stream from downstream client to trigger mitigation EXPECT_EQ(0, nghttp2_submit_ping(client_->session(), NGHTTP2_FLAG_NONE, nullptr)); - EXPECT_THROW(client_->sendPendingFrames(), ServerCodecError); + EXPECT_THROW_WITH_MESSAGE(client_->sendPendingFrames().IgnoreError(), ServerCodecError, + "Too many frames in the outbound queue."); EXPECT_EQ(frame_count, CommonUtility::OptionsLimits::DEFAULT_MAX_OUTBOUND_FRAMES + 1); EXPECT_EQ(1, server_stats_store_.counter("http2.outbound_flood").value()); @@ -1931,7 +1942,8 @@ TEST_P(Http2CodecImplTest, ResponseDataFlood) { // Presently flood mitigation is done only when processing downstream data // So we need to send stream from downstream client to trigger mitigation EXPECT_EQ(0, nghttp2_submit_ping(client_->session(), NGHTTP2_FLAG_NONE, nullptr)); - EXPECT_THROW(client_->sendPendingFrames(), ServerCodecError); + EXPECT_THROW_WITH_MESSAGE(client_->sendPendingFrames().IgnoreError(), ServerCodecError, + "Too many frames in the outbound queue."); EXPECT_EQ(frame_count, CommonUtility::OptionsLimits::DEFAULT_MAX_OUTBOUND_FRAMES + 1); EXPECT_EQ(1, server_stats_store_.counter("http2.outbound_flood").value()); @@ -1963,7 +1975,7 @@ TEST_P(Http2CodecImplTest, ResponseDataFloodMitigationDisabled) { // Presently flood mitigation is done only when processing downstream data // So we need to send stream from downstream client to trigger mitigation EXPECT_EQ(0, nghttp2_submit_ping(client_->session(), NGHTTP2_FLAG_NONE, nullptr)); - EXPECT_NO_THROW(client_->sendPendingFrames()); + EXPECT_NO_THROW(client_->sendPendingFrames().IgnoreError()); } // Verify that outbound frame counter decreases when send buffer is drained @@ -2005,7 +2017,8 @@ TEST_P(Http2CodecImplTest, ResponseDataFloodCounterReset) { // Presently flood mitigation is done only when processing downstream data // So we need to send a frame from downstream client to trigger mitigation EXPECT_EQ(0, nghttp2_submit_ping(client_->session(), NGHTTP2_FLAG_NONE, nullptr)); - EXPECT_THROW(client_->sendPendingFrames(), ServerCodecError); + EXPECT_THROW_WITH_MESSAGE(client_->sendPendingFrames().IgnoreError(), ServerCodecError, + "Too many frames in the outbound queue."); } // Verify that control frames are added to the counter of outbound frames of all types. @@ -2034,7 +2047,8 @@ TEST_P(Http2CodecImplTest, PingStacksWithDataFlood) { } // Send one PING frame above the outbound queue size limit EXPECT_EQ(0, nghttp2_submit_ping(client_->session(), NGHTTP2_FLAG_NONE, nullptr)); - EXPECT_THROW(client_->sendPendingFrames(), ServerCodecError); + EXPECT_THROW_WITH_MESSAGE(client_->sendPendingFrames().IgnoreError(), ServerCodecError, + "Too many frames in the outbound queue."); EXPECT_EQ(frame_count, CommonUtility::OptionsLimits::DEFAULT_MAX_OUTBOUND_FRAMES); EXPECT_EQ(1, server_stats_store_.counter("http2.outbound_flood").value()); @@ -2042,25 +2056,35 @@ TEST_P(Http2CodecImplTest, PingStacksWithDataFlood) { TEST_P(Http2CodecImplTest, PriorityFlood) { priorityFlood(); - EXPECT_THROW(client_->sendPendingFrames(), ServerCodecError); + // Legacy codec does not propagate error details and uses generic error message + EXPECT_THROW_WITH_MESSAGE( + client_->sendPendingFrames().IgnoreError(), ServerCodecError, + Runtime::runtimeFeatureEnabled("envoy.reloadable_features.new_codec_behavior") + ? "Too many PRIORITY frames" + : "Flooding was detected in this HTTP/2 session, and it must be closed"); } TEST_P(Http2CodecImplTest, PriorityFloodOverride) { max_inbound_priority_frames_per_stream_ = 2147483647; priorityFlood(); - EXPECT_NO_THROW(client_->sendPendingFrames()); + EXPECT_NO_THROW(client_->sendPendingFrames().IgnoreError()); } TEST_P(Http2CodecImplTest, WindowUpdateFlood) { windowUpdateFlood(); - EXPECT_THROW(client_->sendPendingFrames(), ServerCodecError); + // Legacy codec does not propagate error details and uses generic error message + EXPECT_THROW_WITH_MESSAGE( + client_->sendPendingFrames().IgnoreError(), ServerCodecError, + Runtime::runtimeFeatureEnabled("envoy.reloadable_features.new_codec_behavior") + ? "Too many WINDOW_UPDATE frames" + : "Flooding was detected in this HTTP/2 session, and it must be closed"); } TEST_P(Http2CodecImplTest, WindowUpdateFloodOverride) { max_inbound_window_update_frames_per_data_frame_sent_ = 2147483647; windowUpdateFlood(); - EXPECT_NO_THROW(client_->sendPendingFrames()); + EXPECT_NO_THROW(client_->sendPendingFrames().IgnoreError()); } TEST_P(Http2CodecImplTest, EmptyDataFlood) { @@ -2070,6 +2094,11 @@ TEST_P(Http2CodecImplTest, EmptyDataFlood) { auto status = server_wrapper_.dispatch(data, *server_); EXPECT_FALSE(status.ok()); EXPECT_TRUE(isBufferFloodError(status)); + // Legacy codec does not propagate error details and uses generic error message + EXPECT_EQ(Runtime::runtimeFeatureEnabled("envoy.reloadable_features.new_codec_behavior") + ? "Too many consecutive frames with an empty payload" + : "Flooding was detected in this HTTP/2 session, and it must be closed", + status.message()); } TEST_P(Http2CodecImplTest, EmptyDataFloodOverride) { @@ -2162,34 +2191,7 @@ class TestNghttp2SessionFactory : public Nghttp2SessionFactoryType, nghttp2_session* create(const nghttp2_session_callbacks*, typename Nghttp2SessionFactoryType::ConnectionImplType* connection, - const nghttp2_option*) override { - // Only need to provide callbacks required to send METADATA frames. - nghttp2_session_callbacks_new(&callbacks_); - nghttp2_session_callbacks_set_pack_extension_callback( - callbacks_, - [](nghttp2_session*, uint8_t* data, size_t length, const nghttp2_frame*, - void* user_data) -> ssize_t { - // Double cast required due to multiple inheritance. - return static_cast*>( - static_cast( - user_data)) - ->encoder_.packNextFramePayload(data, length); - }); - nghttp2_session_callbacks_set_send_callback( - callbacks_, - [](nghttp2_session*, const uint8_t* data, size_t length, int, void* user_data) -> ssize_t { - // Cast down to MetadataTestClientConnectionImpl to leverage friendship. - return static_cast*>( - static_cast( - user_data)) - ->onSend(data, length); - }); - nghttp2_option_new(&options_); - nghttp2_option_set_user_recv_extension_type(options_, METADATA_FRAME_TYPE); - nghttp2_session* session; - nghttp2_session_client_new2(&session, callbacks_, connection, options_); - return session; - } + const nghttp2_option*) override; void init(nghttp2_session*, typename Nghttp2SessionFactoryType::ConnectionImplType*, const envoy::config::core::v3::Http2ProtocolOptions&) override {} @@ -2199,6 +2201,77 @@ class TestNghttp2SessionFactory : public Nghttp2SessionFactoryType, nghttp2_option* options_; }; +template +nghttp2_session* +TestNghttp2SessionFactory::create( + const nghttp2_session_callbacks*, + typename Nghttp2SessionFactoryType::ConnectionImplType* connection, const nghttp2_option*) { + // Only need to provide callbacks required to send METADATA frames. + nghttp2_session_callbacks_new(&callbacks_); + nghttp2_session_callbacks_set_pack_extension_callback( + callbacks_, + [](nghttp2_session*, uint8_t* data, size_t length, const nghttp2_frame*, + void* user_data) -> ssize_t { + // Double cast required due to multiple inheritance. + return static_cast*>( + static_cast(user_data)) + ->encoder_.packNextFramePayload(data, length); + }); + nghttp2_session_callbacks_set_send_callback( + callbacks_, + [](nghttp2_session*, const uint8_t* data, size_t length, int, void* user_data) -> ssize_t { + // Cast down to MetadataTestClientConnectionImpl to leverage friendship. + auto status_or_len = + static_cast*>( + static_cast(user_data)) + ->onSend(data, length); + if (status_or_len.ok()) { + return status_or_len.value(); + } + return NGHTTP2_ERR_CALLBACK_FAILURE; + }); + nghttp2_option_new(&options_); + nghttp2_option_set_user_recv_extension_type(options_, METADATA_FRAME_TYPE); + nghttp2_session* session; + nghttp2_session_client_new2(&session, callbacks_, connection, options_); + return session; +} + +template <> +nghttp2_session* TestNghttp2SessionFactory:: + create(const nghttp2_session_callbacks*, + Envoy::Http::Legacy::Http2::ProdNghttp2SessionFactory::ConnectionImplType* connection, + const nghttp2_option*) { + // Only need to provide callbacks required to send METADATA frames. + nghttp2_session_callbacks_new(&callbacks_); + nghttp2_session_callbacks_set_pack_extension_callback( + callbacks_, + [](nghttp2_session*, uint8_t* data, size_t length, const nghttp2_frame*, + void* user_data) -> ssize_t { + // Double cast required due to multiple inheritance. + return static_cast*>( + static_cast< + Envoy::Http::Legacy::Http2::ProdNghttp2SessionFactory::ConnectionImplType*>( + user_data)) + ->encoder_.packNextFramePayload(data, length); + }); + nghttp2_session_callbacks_set_send_callback( + callbacks_, + [](nghttp2_session*, const uint8_t* data, size_t length, int, void* user_data) -> ssize_t { + // Cast down to MetadataTestClientConnectionImpl to leverage friendship. + return static_cast*>( + static_cast(user_data)) + ->onSend(data, length); + }); + nghttp2_option_new(&options_); + nghttp2_option_set_user_recv_extension_type(options_, METADATA_FRAME_TYPE); + nghttp2_session* session; + nghttp2_session_client_new2(&session, callbacks_, connection, options_); + return session; +} + using TestNghttp2SessionFactoryNew = TestNghttp2SessionFactory; using TestNghttp2SessionFactoryLegacy = diff --git a/test/common/http/http2/codec_impl_test_util.h b/test/common/http/http2/codec_impl_test_util.h index 339481d6d4085..6049876ef8446 100644 --- a/test/common/http/http2/codec_impl_test_util.h +++ b/test/common/http/http2/codec_impl_test_util.h @@ -114,7 +114,7 @@ struct ClientCodecFacade : public ClientConnection { virtual nghttp2_session* session() PURE; virtual Http::Stream* getStream(int32_t stream_id) PURE; virtual uint64_t getStreamPendingSendDataLength(int32_t stream_id) PURE; - virtual void sendPendingFrames() PURE; + virtual Status sendPendingFrames() PURE; virtual bool submitMetadata(const MetadataMapVector& mm_vector, int32_t stream_id) PURE; }; @@ -148,7 +148,7 @@ class TestClientConnectionImpl : public TestClientConnection, public CodecImplTy uint64_t getStreamPendingSendDataLength(int32_t stream_id) override { return CodecImplType::getStream(stream_id)->pending_send_data_.length(); } - void sendPendingFrames() override { CodecImplType::sendPendingFrames(); } + Status sendPendingFrames() override; // Submits an H/2 METADATA frame to the peer. // Returns true on success, false otherwise. bool submitMetadata(const MetadataMapVector& mm_vector, int32_t stream_id) override { @@ -162,6 +162,18 @@ class TestClientConnectionImpl : public TestClientConnection, public CodecImplTy void onSettingsForTest(const nghttp2_settings& settings) override { onSettingsFrame(settings); } }; +template +Status TestClientConnectionImpl::sendPendingFrames() { + return CodecImplType::sendPendingFrames(); +} + +template <> +Status +TestClientConnectionImpl::sendPendingFrames() { + Envoy::Http::Legacy::Http2::ClientConnectionImpl::sendPendingFrames(); + return okStatus(); +} + using TestClientConnectionImplLegacy = TestClientConnectionImpl; using TestClientConnectionImplNew = diff --git a/test/common/http/http2/frame_replay.h b/test/common/http/http2/frame_replay.h index 3a6e89c6ca5b8..2922d6a191105 100644 --- a/test/common/http/http2/frame_replay.h +++ b/test/common/http/http2/frame_replay.h @@ -4,7 +4,6 @@ #include "common/stats/isolated_store_impl.h" -#include "test/common/http/http2/codec_impl_test_util.h" #include "test/mocks/http/mocks.h" #include "test/mocks/network/mocks.h" #include "test/test_common/utility.h" diff --git a/test/common/http/http2/frame_replay_test.cc b/test/common/http/http2/frame_replay_test.cc index aadda98c8b3d7..b1931d350bb85 100644 --- a/test/common/http/http2/frame_replay_test.cc +++ b/test/common/http/http2/frame_replay_test.cc @@ -1,6 +1,7 @@ #include "common/http/exception.h" #include "test/common/http/common.h" +#include "test/common/http/http2/codec_impl_test_util.h" #include "test/common/http/http2/frame_replay.h" #include "gtest/gtest.h" diff --git a/test/common/http/http2/request_header_fuzz_test.cc b/test/common/http/http2/request_header_fuzz_test.cc index d925ed1bb0021..3af7f5c594cea 100644 --- a/test/common/http/http2/request_header_fuzz_test.cc +++ b/test/common/http/http2/request_header_fuzz_test.cc @@ -4,6 +4,7 @@ #include "common/http/exception.h" +#include "test/common/http/http2/codec_impl_test_util.h" #include "test/common/http/http2/frame_replay.h" #include "test/fuzz/fuzz_runner.h" diff --git a/test/common/http/http2/response_header_fuzz_test.cc b/test/common/http/http2/response_header_fuzz_test.cc index e73b88ab954d4..4559aa06419b8 100644 --- a/test/common/http/http2/response_header_fuzz_test.cc +++ b/test/common/http/http2/response_header_fuzz_test.cc @@ -5,6 +5,7 @@ #include "common/http/exception.h" #include "test/common/http/common.h" +#include "test/common/http/http2/codec_impl_test_util.h" #include "test/common/http/http2/frame_replay.h" #include "test/fuzz/fuzz_runner.h"