http2: properly handle already closed stream error

Backport-PR-URL: #18050
Backport-PR-URL: #20456
PR-URL: #17942
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

src/node_http2.cc

@@ -457,6 +457,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(
@@ -881,6 +883,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

@@ -992,6 +992,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/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()));
+});