From b3341d4d3ee374fe1dcd027abe40b90cef15f3eb Mon Sep 17 00:00:00 2001 From: James M Snell Date: Mon, 1 Jan 2018 17:51:34 -0800 Subject: [PATCH 1/2] http2: properly handle already closed stream error --- src/node_http2.cc | 27 ++++ src/node_http2.h | 5 + test/common/http2.js | 129 ++++++++++++++++++ .../test-http2-misbehaving-multiplex.js | 59 ++++++++ 4 files changed, 220 insertions(+) create mode 100644 test/common/http2.js create mode 100644 test/parallel/test-http2-misbehaving-multiplex.js diff --git a/src/node_http2.cc b/src/node_http2.cc index 442fa64b6ca981..5c8387ff49a416 100644 --- a/src/node_http2.cc +++ b/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(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 = env->context(); + Context::Scope context_scope(context); + + Local 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 diff --git a/src/node_http2.h b/src/node_http2.h index 0e2f489681f8d6..7251f3baae3b93 100644 --- a/src/node_http2.h +++ b/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( diff --git a/test/common/http2.js b/test/common/http2.js new file mode 100644 index 00000000000000..44f5f9191e6e33 --- /dev/null +++ b/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 +}; diff --git a/test/parallel/test-http2-misbehaving-multiplex.js b/test/parallel/test-http2-misbehaving-multiplex.js new file mode 100644 index 00000000000000..7d5a7a2f552d49 --- /dev/null +++ b/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())); +}); From f714aace60c6e6a62849c6a12eb1c17379f2575a Mon Sep 17 00:00:00 2001 From: James M Snell Date: Tue, 2 Jan 2018 10:58:03 -0800 Subject: [PATCH 2/2] doc: add docs for common/http2.js utility --- test/common/README.md | 138 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 138 insertions(+) diff --git a/test/common/README.md b/test/common/README.md index 39b368ae209189..3a71aed858eb7f 100644 --- a/test/common/README.md +++ b/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 + + + + + +```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. + + + + + +```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. + + + + + +```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. + + + + + +```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. + + + + + +```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`. + + + + + +```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`. + + + + + +```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. + + + + + +```js +socket.write(http2.kClientMagic); +``` + + [<Array>]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array [<ArrayBufferView[]>]: https://developer.mozilla.org/en-US/docs/Web/API/ArrayBufferView [<Boolean>]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Data_structures#Boolean_type