http2: add specific error code for custom frames

As suggested in #37849 (comment) 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: #37936 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
nodejs · MylesBorins · Apr 6, 2021 · Mar 24, 2021 · Mar 28, 2021 · Mar 23, 2021
commit 062541aae5b49eac463ee3b3151521bff0963752
diff --git 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.
 
+<a id="ERR_HTTP2_TOO_MANY_INVALID_FRAMES"></a>
+### `ERR_HTTP2_TOO_MANY_INVALID_FRAMES`
+<!--
+added: REPLACEME
+-->
+
+The limit of acceptable invalid HTTP/2 protocol frames sent by the peer,
+as specified through the `maxSessionInvalidFrames` option, has been exceeded.
+
 <a id="ERR_HTTP2_TRAILERS_ALREADY_SENT"></a>
 ### `ERR_HTTP2_TRAILERS_ALREADY_SENT`
 

diff --git a/lib/internal/errors.js b/lib/internal/errors.js
@@ -971,6 +971,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
@@ -777,9 +777,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
@@ -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
@@ -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;
@@ -732,7 +733,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_;
@@ -742,12 +743,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<uint8_t*>(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());
@@ -759,7 +762,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.
@@ -769,14 +772,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<Value> args[] = {
+      Integer::New(isolate, static_cast<int32_t>(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);
+  }
 }
 
 
@@ -900,14 +923,17 @@ int Http2Session::OnInvalidFrame(nghttp2_session* handle,
                                  int lib_error_code,
                                  void* user_data) {
   Http2Session* session = static_cast<Http2Session*>(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) ||
@@ -1286,6 +1312,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;
@@ -1470,7 +1497,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();
   }
@@ -1798,21 +1825,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<unsigned int>(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<Value> arg = Integer::New(isolate, static_cast<int32_t>(ret));
-    MakeCallback(env()->http2session_on_error_function(), 1, &arg);
-    return;
-  }
+  ConsumeHTTP2Data();
 
   MaybeStopReading();
 }

diff --git a/src/node_http2.h b/src/node_http2.h
@@ -660,8 +660,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)
@@ -895,6 +896,9 @@ class Http2Session : public AsyncWrap,
   v8::Global<v8::ArrayBuffer> 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<BaseObjectPtr<Http2Ping>> outstanding_pings_;

diff --git 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');