From 9d07c0a20b96d468343f3e87b98911711b0e6856 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 26 Mar 2021 21:48:39 +0100 Subject: [PATCH] http2: add specific error code for custom frames As suggested in https://github.com/nodejs/node/issues/37849#issuecomment-805049586 improve the error presented when encountering a large number of invalid frames by giving this situation a specific error code (which we should have had from the beginning). PR-URL: https://github.com/nodejs/node/pull/37936 Reviewed-By: James M Snell Reviewed-By: Yongsheng Zhang --- doc/api/errors.md | 9 +++ lib/internal/errors.js | 1 + lib/internal/http2/core.js | 4 +- lib/internal/http2/util.js | 13 +++-- src/node_http2.cc | 58 ++++++++++++------- src/node_http2.h | 8 ++- .../test-http2-empty-frame-without-eof.js | 8 ++- 7 files changed, 70 insertions(+), 31 deletions(-) diff --git a/doc/api/errors.md b/doc/api/errors.md index f0d78854703a41..805aa49776093c 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -1364,6 +1364,15 @@ When setting the priority for an HTTP/2 stream, the stream may be marked as a dependency for a parent stream. This error code is used when an attempt is made to mark a stream and dependent of itself. + +### `ERR_HTTP2_TOO_MANY_INVALID_FRAMES` + + +The limit of acceptable invalid HTTP/2 protocol frames sent by the peer, +as specified through the `maxSessionInvalidFrames` option, has been exceeded. + ### `ERR_HTTP2_TRAILERS_ALREADY_SENT` diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 34c168ce38ab91..a2a337730a011f 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -992,6 +992,7 @@ E('ERR_HTTP2_STREAM_CANCEL', function(error) { E('ERR_HTTP2_STREAM_ERROR', 'Stream closed with error code %s', Error); E('ERR_HTTP2_STREAM_SELF_DEPENDENCY', 'A stream cannot depend on itself', Error); +E('ERR_HTTP2_TOO_MANY_INVALID_FRAMES', 'Too many invalid HTTP/2 frames', Error); E('ERR_HTTP2_TRAILERS_ALREADY_SENT', 'Trailing headers have already been sent', Error); E('ERR_HTTP2_TRAILERS_NOT_READY', diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 620e28a1524aa8..5427f74a5f2b59 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -778,9 +778,9 @@ const setAndValidatePriorityOptions = hideStackFrames((options) => { // When an error occurs internally at the binding level, immediately // destroy the session. -function onSessionInternalError(code) { +function onSessionInternalError(integerCode, customErrorCode) { if (this[kOwner] !== undefined) - this[kOwner].destroy(new NghttpError(code)); + this[kOwner].destroy(new NghttpError(integerCode, customErrorCode)); } function settingsCallback(cb, ack, duration) { diff --git a/lib/internal/http2/util.js b/lib/internal/http2/util.js index bde89f8a3d1869..f8252fffba65f5 100644 --- a/lib/internal/http2/util.js +++ b/lib/internal/http2/util.js @@ -29,6 +29,7 @@ const { ERR_INVALID_HTTP_TOKEN }, addCodeToName, + getMessage, hideStackFrames } = require('internal/errors'); @@ -543,11 +544,13 @@ function mapToHeaders(map, } class NghttpError extends Error { - constructor(ret) { - super(binding.nghttp2ErrorString(ret)); - this.code = 'ERR_HTTP2_ERROR'; - this.errno = ret; - addCodeToName(this, super.name, 'ERR_HTTP2_ERROR'); + constructor(integerCode, customErrorCode) { + super(customErrorCode ? + getMessage(customErrorCode, [], null) : + binding.nghttp2ErrorString(integerCode)); + this.code = customErrorCode || 'ERR_HTTP2_ERROR'; + this.errno = integerCode; + addCodeToName(this, super.name, this.code); } toString() { diff --git a/src/node_http2.cc b/src/node_http2.cc index 275284a147d583..2565b086384e69 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -32,6 +32,7 @@ using v8::Integer; using v8::Isolate; using v8::Local; using v8::MaybeLocal; +using v8::NewStringType; using v8::Number; using v8::Object; using v8::ObjectTemplate; @@ -782,7 +783,7 @@ ssize_t Http2Session::OnMaxFrameSizePadding(size_t frameLen, // various callback functions. Each of these will typically result in a call // out to JavaScript so this particular function is rather hot and can be // quite expensive. This is a potential performance optimization target later. -ssize_t Http2Session::ConsumeHTTP2Data() { +void Http2Session::ConsumeHTTP2Data() { CHECK_NOT_NULL(stream_buf_.base); CHECK_LE(stream_buf_offset_, stream_buf_.len); size_t read_len = stream_buf_.len - stream_buf_offset_; @@ -792,12 +793,14 @@ ssize_t Http2Session::ConsumeHTTP2Data() { read_len, nghttp2_session_want_read(session_.get())); set_receive_paused(false); + custom_recv_error_code_ = nullptr; ssize_t ret = nghttp2_session_mem_recv(session_.get(), reinterpret_cast(stream_buf_.base) + stream_buf_offset_, read_len); CHECK_NE(ret, NGHTTP2_ERR_NOMEM); + CHECK_IMPLIES(custom_recv_error_code_ != nullptr, ret < 0); if (is_receive_paused()) { CHECK(is_reading_stopped()); @@ -809,7 +812,7 @@ ssize_t Http2Session::ConsumeHTTP2Data() { // Even if all bytes were received, a paused stream may delay the // nghttp2_on_frame_recv_callback which may have an END_STREAM flag. stream_buf_offset_ += ret; - return ret; + goto done; } // We are done processing the current input chunk. @@ -819,14 +822,34 @@ ssize_t Http2Session::ConsumeHTTP2Data() { stream_buf_allocation_.clear(); stream_buf_ = uv_buf_init(nullptr, 0); - if (ret < 0) - return ret; - // Send any data that was queued up while processing the received data. - if (!is_destroyed()) { + if (ret >= 0 && !is_destroyed()) { SendPendingData(); } - return ret; + +done: + if (UNLIKELY(ret < 0)) { + Isolate* isolate = env()->isolate(); + Debug(this, + "fatal error receiving data: %d (%s)", + ret, + custom_recv_error_code_ != nullptr ? + custom_recv_error_code_ : "(no custom error code)"); + Local args[] = { + Integer::New(isolate, static_cast(ret)), + Null(isolate) + }; + if (custom_recv_error_code_ != nullptr) { + args[1] = String::NewFromUtf8( + isolate, + custom_recv_error_code_, + NewStringType::kInternalized).ToLocalChecked(); + } + MakeCallback( + env()->http2session_on_error_function(), + arraysize(args), + args); + } } @@ -950,14 +973,17 @@ int Http2Session::OnInvalidFrame(nghttp2_session* handle, int lib_error_code, void* user_data) { Http2Session* session = static_cast(user_data); + const uint32_t max_invalid_frames = session->js_fields_->max_invalid_frames; Debug(session, "invalid frame received (%u/%u), code: %d", session->invalid_frame_count_, - session->js_fields_->max_invalid_frames, + max_invalid_frames, lib_error_code); - if (session->invalid_frame_count_++ > session->js_fields_->max_invalid_frames) + if (session->invalid_frame_count_++ > max_invalid_frames) { + session->custom_recv_error_code_ = "ERR_HTTP2_TOO_MANY_INVALID_FRAMES"; return 1; + } // If the error is fatal or if error code is ERR_STREAM_CLOSED... emit error if (nghttp2_is_fatal(lib_error_code) || @@ -1336,6 +1362,7 @@ int Http2Session::HandleDataFrame(const nghttp2_frame* frame) { stream->EmitRead(UV_EOF); } else if (frame->hd.length == 0) { if (invalid_frame_count_++ > js_fields_->max_invalid_frames) { + custom_recv_error_code_ = "ERR_HTTP2_TOO_MANY_INVALID_FRAMES"; Debug(this, "rejecting empty-frame-without-END_STREAM flood\n"); // Consider a flood of 0-length frames without END_STREAM an error. return 1; @@ -1520,7 +1547,7 @@ void Http2Session::OnStreamAfterWrite(WriteWrap* w, int status) { ConsumeHTTP2Data(); } - if (!is_write_scheduled()) { + if (!is_write_scheduled() && !is_destroyed()) { // Schedule a new write if nghttp2 wants to send data. MaybeScheduleWrite(); } @@ -1848,21 +1875,12 @@ void Http2Session::OnStreamRead(ssize_t nread, const uv_buf_t& buf_) { // offset of a DATA frame's data into the socket read buffer. stream_buf_ = uv_buf_init(buf.data(), static_cast(nread)); - Isolate* isolate = env()->isolate(); - // Store this so we can create an ArrayBuffer for read data from it. // DATA frames will be emitted as slices of that ArrayBuffer to avoid having // to copy memory. stream_buf_allocation_ = std::move(buf); - ssize_t ret = ConsumeHTTP2Data(); - - if (UNLIKELY(ret < 0)) { - Debug(this, "fatal error receiving data: %d", ret); - Local arg = Integer::New(isolate, static_cast(ret)); - MakeCallback(env()->http2session_on_error_function(), 1, &arg); - return; - } + ConsumeHTTP2Data(); MaybeStopReading(); } diff --git a/src/node_http2.h b/src/node_http2.h index 4fa2dc446c05af..4d267e647d3494 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -662,8 +662,9 @@ class Http2Session : public AsyncWrap, // Indicates whether there currently exist outgoing buffers for this stream. bool HasWritesOnSocketForStream(Http2Stream* stream); - // Write data from stream_buf_ to the session - ssize_t ConsumeHTTP2Data(); + // Write data from stream_buf_ to the session. + // This will call the error callback if an error occurs. + void ConsumeHTTP2Data(); void MemoryInfo(MemoryTracker* tracker) const override; SET_MEMORY_INFO_NAME(Http2Session) @@ -898,6 +899,9 @@ class Http2Session : public AsyncWrap, v8::Global stream_buf_ab_; AllocatedBuffer stream_buf_allocation_; size_t stream_buf_offset_ = 0; + // Custom error code for errors that originated inside one of the callbacks + // called by nghttp2_session_mem_recv. + const char* custom_recv_error_code_ = nullptr; size_t max_outstanding_pings_ = kDefaultMaxPings; std::queue> outstanding_pings_; diff --git a/test/parallel/test-http2-empty-frame-without-eof.js b/test/parallel/test-http2-empty-frame-without-eof.js index 02da78d940a92d..fe65f26bb31d49 100644 --- a/test/parallel/test-http2-empty-frame-without-eof.js +++ b/test/parallel/test-http2-empty-frame-without-eof.js @@ -26,8 +26,12 @@ async function main() { stream.on('error', common.mustNotCall()); client.on('error', common.mustNotCall()); } else { - stream.on('error', common.mustCall()); - client.on('error', common.mustCall()); + const expected = { + code: 'ERR_HTTP2_TOO_MANY_INVALID_FRAMES', + message: 'Too many invalid HTTP/2 frames' + }; + stream.on('error', common.expectsError(expected)); + client.on('error', common.expectsError(expected)); } stream.resume(); await once(stream, 'end');