http2: properly handle already closed stream error

src/node_http2.cc

@@ -452,6 +452,8 @@ Http2Session::Callbacks::Callbacks(bool kHasGetPaddingCallback) {
     callbacks, OnNghttpError);
   nghttp2_session_callbacks_set_send_data_callback(
     callbacks, OnSendData);
+  nghttp2_session_callbacks_set_on_invalid_frame_recv_callback(
+    callbacks, OnInvalidFrame);
 
   if (kHasGetPaddingCallback) {
     nghttp2_session_callbacks_set_select_padding_callback(
@@ -836,6 +838,31 @@ inline int Http2Session::OnFrameReceive(nghttp2_session* handle,
   return 0;
 }
 
+inline int Http2Session::OnInvalidFrame(nghttp2_session* handle,
+                                        const nghttp2_frame *frame,
+                                        int lib_error_code,
+                                        void* user_data) {
+  Http2Session* session = static_cast<Http2Session*>(user_data);
+
+  DEBUG_HTTP2SESSION2(session, "invalid frame received, code: %d",
+                      lib_error_code);
+
+  // If the error is fatal or if error code is ERR_STREAM_CLOSED... emit error
+  if (nghttp2_is_fatal(lib_error_code) ||
+      lib_error_code == NGHTTP2_ERR_STREAM_CLOSED) {
+    Environment* env = session->env();
+    Isolate* isolate = env->isolate();
+    HandleScope scope(isolate);
+    Local<Context> context = env->context();
+    Context::Scope context_scope(context);
+
+    Local<Value> argv[1] = {
+      Integer::New(isolate, lib_error_code),
+    };
+    session->MakeCallback(env->error_string(), arraysize(argv), argv);
+  }
+  return 0;
+}
 
 // If nghttp2 is unable to send a queued up frame, it will call this callback
 // to let us know. If the failure occurred because we are in the process of

src/node_http2.h

@@ -979,6 +979,11 @@ class Http2Session : public AsyncWrap {
       size_t length,
       nghttp2_data_source* source,
       void* user_data);
+  static inline int OnInvalidFrame(
+      nghttp2_session* session,
+      const nghttp2_frame *frame,
+      int lib_error_code,
+      void* user_data);
 
 
   static inline ssize_t OnStreamReadFD(

test/common/README.md

@@ -12,6 +12,7 @@ This directory contains modules used to test the Node.js implementation.
 * [Fixtures module](#fixtures-module)
 * [Internet module](#internet-module)
 * [WPT module](#wpt-module)
+* [HTTP2 module](#http2-module)
 
 ## Benchmark Module
 
@@ -551,6 +552,143 @@ Node.js
 implementation with tests from
 [W3C Web Platform Tests](https://github.com/w3c/web-platform-tests).
 
+## HTTP/2 Module
+
+The http2.js module provides a handful of utilities for creating mock HTTP/2
+frames for testing of HTTP/2 endpoints
+
+<!-- eslint-disable strict -->
+<!-- eslint-disable required-modules -->
+<!-- eslint-disable no-unused-vars -->
+<!-- eslint-disable no-undef -->
+```js
+const http2 = require('../common/http2');
+```
+
+### Class: Frame
+
+The `http2.Frame` is a base class that creates a `Buffer` containing a
+serialized HTTP/2 frame header.
+
+<!-- eslint-disable strict -->
+<!-- eslint-disable required-modules -->
+<!-- eslint-disable no-unused-vars -->
+<!-- eslint-disable no-undef -->
+```js
+// length is a 24-bit unsigned integer
+// type is an 8-bit unsigned integer identifying the frame type
+// flags is an 8-bit unsigned integer containing the flag bits
+// id is the 32-bit stream identifier, if any.
+const frame = new http2.Frame(length, type, flags, id);
+
+// Write the frame data to a socket
+socket.write(frame.data);
+```
+
+The serialized `Buffer` may be retrieved using the `frame.data` property.
+
+### Class: DataFrame extends Frame
+
+The `http2.DataFrame` is a subclass of `http2.Frame` that serializes a `DATA`
+frame.
+
+<!-- eslint-disable strict -->
+<!-- eslint-disable required-modules -->
+<!-- eslint-disable no-unused-vars -->
+<!-- eslint-disable no-undef -->
+```js
+// id is the 32-bit stream identifier
+// payload is a Buffer containing the DATA payload
+// padlen is an 8-bit integer giving the number of padding bytes to include
+// final is a boolean indicating whether the End-of-stream flag should be set,
+// defaults to false.
+const data = new http2.DataFrame(id, payload, padlen, final);
+
+socket.write(frame.data);
+```
+
+### Class: HeadersFrame
+
+The `http2.HeadersFrame` is a subclass of `http2.Frame` that serializes a
+`HEADERS` frame.
+
+<!-- eslint-disable strict -->
+<!-- eslint-disable required-modules -->
+<!-- eslint-disable no-unused-vars -->
+<!-- eslint-disable no-undef -->
+```js
+// id is the 32-bit stream identifier
+// payload is a Buffer containing the HEADERS payload (see either
+// http2.kFakeRequestHeaders or http2.kFakeResponseHeaders).
+// padlen is an 8-bit integer giving the number of padding bytes to include
+// final is a boolean indicating whether the End-of-stream flag should be set,
+// defaults to false.
+const data = new http2.HeadersFrame(id, http2.kFakeRequestHeaders,
+                                    padlen, final);
+
+socket.write(frame.data);
+```
+
+### Class: SettingsFrame
+
+The `http2.SettingsFrame` is a subclass of `http2.Frame` that serializes an
+empty `SETTINGS` frame.
+
+<!-- eslint-disable strict -->
+<!-- eslint-disable required-modules -->
+<!-- eslint-disable no-unused-vars -->
+<!-- eslint-disable no-undef -->
+```js
+// ack is a boolean indicating whether or not to set the ACK flag.
+const frame = new http2.SettingsFrame(ack);
+
+socket.write(frame.data);
+```
+
+### http2.kFakeRequestHeaders
+
+Set to a `Buffer` instance that contains a minimal set of serialized HTTP/2
+request headers to be used as the payload of a `http2.HeadersFrame`.
+
+<!-- eslint-disable strict -->
+<!-- eslint-disable required-modules -->
+<!-- eslint-disable no-unused-vars -->
+<!-- eslint-disable no-undef -->
+```js
+const frame = new http2.HeadersFrame(1, http2.kFakeRequestHeaders, 0, true);
+
+socket.write(frame.data);
+```
+
+### http2.kFakeResponseHeaders
+
+Set to a `Buffer` instance that contains a minimal set of serialized HTTP/2
+response headers to be used as the payload a `http2.HeadersFrame`.
+
+<!-- eslint-disable strict -->
+<!-- eslint-disable required-modules -->
+<!-- eslint-disable no-unused-vars -->
+<!-- eslint-disable no-undef -->
+```js
+const frame = new http2.HeadersFrame(1, http2.kFakeResponseHeaders, 0, true);
+
+socket.write(frame.data);
+```
+
+### http2.kClientMagic
+
+Set to a `Buffer` containing the preamble bytes an HTTP/2 client must send
+upon initial establishment of a connection.
+
+<!-- eslint-disable strict -->
+<!-- eslint-disable required-modules -->
+<!-- eslint-disable no-unused-vars -->
+<!-- eslint-disable no-undef -->
+```js
+socket.write(http2.kClientMagic);
+```
+
+
 [&lt;Array>]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array
 [&lt;ArrayBufferView&#91;&#93;>]: https://developer.mozilla.org/en-US/docs/Web/API/ArrayBufferView
 [&lt;Boolean>]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Data_structures#Boolean_type

test/common/http2.js

@@ -0,0 +1,129 @@
+/* eslint-disable required-modules */
+'use strict';
+
+// An HTTP/2 testing tool used to create mock frames for direct testing
+// of HTTP/2 endpoints.
+
+const kFrameData = Symbol('frame-data');
+const FLAG_EOS = 0x1;
+const FLAG_ACK = 0x1;
+const FLAG_EOH = 0x4;
+const FLAG_PADDED = 0x8;
+const PADDING = Buffer.alloc(255);
+
+const kClientMagic = Buffer.from('505249202a20485454502f322' +
+                                 'e300d0a0d0a534d0d0a0d0a', 'hex');
+
+const kFakeRequestHeaders = Buffer.from('828684410f7777772e65' +
+                                        '78616d706c652e636f6d', 'hex');
+
+
+const kFakeResponseHeaders = Buffer.from('4803333032580770726976617465611d' +
+                                         '4d6f6e2c203231204f63742032303133' +
+                                         '2032303a31333a323120474d546e1768' +
+                                         '747470733a2f2f7777772e6578616d70' +
+                                         '6c652e636f6d', 'hex');
+
+function isUint32(val) {
+  return val >>> 0 === val;
+}
+
+function isUint24(val) {
+  return val >>> 0 === val && val <= 0xFFFFFF;
+}
+
+function isUint8(val) {
+  return val >>> 0 === val && val <= 0xFF;
+}
+
+function write32BE(array, pos, val) {
+  if (!isUint32(val))
+    throw new RangeError('val is not a 32-bit number');
+  array[pos++] = (val >> 24) & 0xff;
+  array[pos++] = (val >> 16) & 0xff;
+  array[pos++] = (val >> 8) & 0xff;
+  array[pos++] = val & 0xff;
+}
+
+function write24BE(array, pos, val) {
+  if (!isUint24(val))
+    throw new RangeError('val is not a 24-bit number');
+  array[pos++] = (val >> 16) & 0xff;
+  array[pos++] = (val >> 8) & 0xff;
+  array[pos++] = val & 0xff;
+}
+
+function write8(array, pos, val) {
+  if (!isUint8(val))
+    throw new RangeError('val is not an 8-bit number');
+  array[pos] = val;
+}
+
+class Frame {
+  constructor(length, type, flags, id) {
+    this[kFrameData] = Buffer.alloc(9);
+    write24BE(this[kFrameData], 0, length);
+    write8(this[kFrameData], 3, type);
+    write8(this[kFrameData], 4, flags);
+    write32BE(this[kFrameData], 5, id);
+  }
+
+  get data() {
+    return this[kFrameData];
+  }
+}
+
+class SettingsFrame extends Frame {
+  constructor(ack = false) {
+    let flags = 0;
+    if (ack)
+      flags |= FLAG_ACK;
+    super(0, 4, flags, 0);
+  }
+}
+
+class DataFrame extends Frame {
+  constructor(id, payload, padlen = 0, final = false) {
+    let len = payload.length;
+    let flags = 0;
+    if (final) flags |= FLAG_EOS;
+    const buffers = [payload];
+    if (padlen > 0) {
+      buffers.unshift(Buffer.from([padlen]));
+      buffers.push(PADDING.slice(0, padlen));
+      len += padlen + 1;
+      flags |= FLAG_PADDED;
+    }
+    super(len, 0, flags, id);
+    buffers.unshift(this[kFrameData]);
+    this[kFrameData] = Buffer.concat(buffers);
+  }
+}
+
+class HeadersFrame extends Frame {
+  constructor(id, payload, padlen = 0, final = false) {
+    let len = payload.length;
+    let flags = FLAG_EOH;
+    if (final) flags |= FLAG_EOS;
+    const buffers = [payload];
+    if (padlen > 0) {
+      buffers.unshift(Buffer.from([padlen]));
+      buffers.push(PADDING.slice(0, padlen));
+      len += padlen + 1;
+      flags |= FLAG_PADDED;
+    }
+    super(len, 1, flags, id);
+    buffers.unshift(this[kFrameData]);
+    this[kFrameData] = Buffer.concat(buffers);
+  }
+}
+
+module.exports = {
+  Frame,
+  DataFrame,
+  HeadersFrame,
+  SettingsFrame,
+  kFakeRequestHeaders,
+  kFakeResponseHeaders,
+  kClientMagic
+};

test/parallel/test-http2-misbehaving-multiplex.js

@@ -0,0 +1,59 @@
+'use strict';
+
+const common = require('../common');
+
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+
+const h2 = require('http2');
+const net = require('net');
+const h2test = require('../common/http2');
+let client;
+
+const server = h2.createServer();
+server.on('stream', common.mustCall((stream) => {
+  stream.respond();
+  stream.end('ok');
+}, 2));
+server.on('session', common.mustCall((session) => {
+  session.on('error', common.expectsError({
+    code: 'ERR_HTTP2_ERROR',
+    type: Error,
+    message: 'Stream was already closed or invalid'
+  }));
+}));
+
+const settings = new h2test.SettingsFrame();
+const settingsAck = new h2test.SettingsFrame(true);
+const head1 = new h2test.HeadersFrame(1, h2test.kFakeRequestHeaders, 0, true);
+const head2 = new h2test.HeadersFrame(3, h2test.kFakeRequestHeaders, 0, true);
+const head3 = new h2test.HeadersFrame(1, h2test.kFakeRequestHeaders, 0, true);
+const head4 = new h2test.HeadersFrame(5, h2test.kFakeRequestHeaders, 0, true);
+
+server.listen(0, () => {
+  client = net.connect(server.address().port, () => {
+    client.write(h2test.kClientMagic, () => {
+      client.write(settings.data, () => {
+        client.write(settingsAck.data);
+        // This will make it ok.
+        client.write(head1.data, () => {
+          // This will make it ok.
+          client.write(head2.data, () => {
+            // This will cause an error to occur because the client is
+            // attempting to reuse an already closed stream. This must
+            // cause the server session to be torn down.
+            client.write(head3.data, () => {
+              // This won't ever make it to the server
+              client.write(head4.data);
+            });
+          });
+        });
+      });
+    });
+  });
+
+  // An error may or may not be emitted on the client side, we don't care
+  // either way if it is, but we don't want to die if it is.
+  client.on('error', () => {});
+  client.on('close', common.mustCall(() => server.close()));
+});