From efdadcc193dc1a2b2121fecf1a3ef24d5c35b564 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Mon, 1 Jan 2018 13:06:46 -0800 Subject: [PATCH] http2: add aligned padding strategy Add a padding strategy option that makes a best attempt to ensure that total frame length for DATA and HEADERS frames are aligned on multiples of 8-bytes. PR-URL: https://github.com/nodejs/node/pull/17938 Reviewed-By: Anna Henningsen Reviewed-By: Matteo Collina --- doc/api/http2.md | 21 +++++++ src/node_http2.cc | 41 +++++++++++-- src/node_http2.h | 4 ++ test/parallel/test-http2-padding-aligned.js | 68 +++++++++++++++++++++ 4 files changed, 129 insertions(+), 5 deletions(-) create mode 100644 test/parallel/test-http2-padding-aligned.js diff --git a/doc/api/http2.md b/doc/api/http2.md index f6130723241cfe..4a071272fa85b7 100644 --- a/doc/api/http2.md +++ b/doc/api/http2.md @@ -1652,6 +1652,13 @@ changes: * `http2.constants.PADDING_STRATEGY_CALLBACK` - Specifies that the user provided `options.selectPadding` callback is to be used to determine the amount of padding. + * `http2.constants.PADDING_STRATEGY_ALIGNED` - Will *attempt* to apply + enough padding to ensure that the total frame length, including the + 9-byte header, is a multiple of 8. For each frame, however, there is a + maxmimum allowed number of padding bytes that is determined by current + flow control state and settings. If this maximum is less than the + calculated amount needed to ensure alignment, the maximum will be used + and the total frame length will *not* necessarily be aligned at 8 bytes. * `peerMaxConcurrentStreams` {number} Sets the maximum number of concurrent streams for the remote peer as if a SETTINGS frame had been received. Will be overridden if the remote peer sets its own value for. @@ -1723,6 +1730,13 @@ changes: * `http2.constants.PADDING_STRATEGY_CALLBACK` - Specifies that the user provided `options.selectPadding` callback is to be used to determine the amount of padding. + * `http2.constants.PADDING_STRATEGY_ALIGNED` - Will *attempt* to apply + enough padding to ensure that the total frame length, including the + 9-byte header, is a multiple of 8. For each frame, however, there is a + maxmimum allowed number of padding bytes that is determined by current + flow control state and settings. If this maximum is less than the + calculated amount needed to ensure alignment, the maximum will be used + and the total frame length will *not* necessarily be aligned at 8 bytes. * `peerMaxConcurrentStreams` {number} Sets the maximum number of concurrent streams for the remote peer as if a SETTINGS frame had been received. Will be overridden if the remote peer sets its own value for @@ -1803,6 +1817,13 @@ changes: * `http2.constants.PADDING_STRATEGY_CALLBACK` - Specifies that the user provided `options.selectPadding` callback is to be used to determine the amount of padding. + * `http2.constants.PADDING_STRATEGY_ALIGNED` - Will *attempt* to apply + enough padding to ensure that the total frame length, including the + 9-byte header, is a multiple of 8. For each frame, however, there is a + maxmimum allowed number of padding bytes that is determined by current + flow control state and settings. If this maximum is less than the + calculated amount needed to ensure alignment, the maximum will be used + and the total frame length will *not* necessarily be aligned at 8 bytes. * `peerMaxConcurrentStreams` {number} Sets the maximum number of concurrent streams for the remote peer as if a SETTINGS frame had been received. Will be overridden if the remote peer sets its own value for diff --git a/src/node_http2.cc b/src/node_http2.cc index 17265a28cda442..d37e97b696f05c 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -492,8 +492,7 @@ Http2Session::Http2Session(Environment* env, padding_strategy_ = opts.GetPaddingStrategy(); bool hasGetPaddingCallback = - padding_strategy_ == PADDING_STRATEGY_MAX || - padding_strategy_ == PADDING_STRATEGY_CALLBACK; + padding_strategy_ != PADDING_STRATEGY_NONE; nghttp2_session_callbacks* callbacks = callback_struct_saved[hasGetPaddingCallback ? 1 : 0].callbacks; @@ -681,6 +680,25 @@ inline void Http2Session::RemoveStream(int32_t id) { streams_.erase(id); } +// Used as one of the Padding Strategy functions. Will attempt to ensure +// that the total frame size, including header bytes, are 8-byte aligned. +// If maxPayloadLen is smaller than the number of bytes necessary to align, +// will return maxPayloadLen instead. +inline ssize_t Http2Session::OnDWordAlignedPadding(size_t frameLen, + size_t maxPayloadLen) { + size_t r = (frameLen + 9) % 8; + if (r == 0) return frameLen; // If already a multiple of 8, return. + + size_t pad = frameLen + (8 - r); + + // If maxPayloadLen happens to be less than the calculated pad length, + // use the max instead, even tho this means the frame will not be + // aligned. + pad = std::min(maxPayloadLen, pad); + DEBUG_HTTP2SESSION2(this, "using frame size padding: %d", pad); + return pad; +} + // Used as one of the Padding Strategy functions. Uses the maximum amount // of padding allowed for the current frame. inline ssize_t Http2Session::OnMaxFrameSizePadding(size_t frameLen, @@ -987,9 +1005,21 @@ inline ssize_t Http2Session::OnSelectPadding(nghttp2_session* handle, Http2Session* session = static_cast(user_data); ssize_t padding = frame->hd.length; - return session->padding_strategy_ == PADDING_STRATEGY_MAX - ? session->OnMaxFrameSizePadding(padding, maxPayloadLen) - : session->OnCallbackPadding(padding, maxPayloadLen); + switch (session->padding_strategy_) { + case PADDING_STRATEGY_NONE: + // Fall-through + break; + case PADDING_STRATEGY_MAX: + padding = session->OnMaxFrameSizePadding(padding, maxPayloadLen); + break; + case PADDING_STRATEGY_ALIGNED: + padding = session->OnDWordAlignedPadding(padding, maxPayloadLen); + break; + case PADDING_STRATEGY_CALLBACK: + padding = session->OnCallbackPadding(padding, maxPayloadLen); + break; + } + return padding; } #define BAD_PEER_MESSAGE "Remote peer returned unexpected data while we " \ @@ -2872,6 +2902,7 @@ void Initialize(Local target, NODE_DEFINE_CONSTANT(constants, NGHTTP2_SETTINGS_MAX_HEADER_LIST_SIZE); NODE_DEFINE_CONSTANT(constants, PADDING_STRATEGY_NONE); + NODE_DEFINE_CONSTANT(constants, PADDING_STRATEGY_ALIGNED); NODE_DEFINE_CONSTANT(constants, PADDING_STRATEGY_MAX); NODE_DEFINE_CONSTANT(constants, PADDING_STRATEGY_CALLBACK); diff --git a/src/node_http2.h b/src/node_http2.h index 2b8542d951095c..f0824e556a066a 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -359,6 +359,8 @@ HTTP_STATUS_CODES(V) enum padding_strategy_type { // No padding strategy. This is the default. PADDING_STRATEGY_NONE, + // Attempts to ensure that the frame is 8-byte aligned + PADDING_STRATEGY_ALIGNED, // Padding will ensure all data frames are maxFrameSize PADDING_STRATEGY_MAX, // Padding will be determined via a JS callback. Note that this can be @@ -917,6 +919,8 @@ class Http2Session : public AsyncWrap { private: // Frame Padding Strategies + inline ssize_t OnDWordAlignedPadding(size_t frameLength, + size_t maxPayloadLen); inline ssize_t OnMaxFrameSizePadding(size_t frameLength, size_t maxPayloadLen); inline ssize_t OnCallbackPadding(size_t frame, diff --git a/test/parallel/test-http2-padding-aligned.js b/test/parallel/test-http2-padding-aligned.js new file mode 100644 index 00000000000000..183eaef7389360 --- /dev/null +++ b/test/parallel/test-http2-padding-aligned.js @@ -0,0 +1,68 @@ +'use strict'; + +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); +const assert = require('assert'); +const http2 = require('http2'); +const { PADDING_STRATEGY_ALIGNED } = http2.constants; +const makeDuplexPair = require('../common/duplexpair'); + +{ + const testData = '

Hello World.

'; + const server = http2.createServer({ + paddingStrategy: PADDING_STRATEGY_ALIGNED + }); + server.on('stream', common.mustCall((stream, headers) => { + stream.respond({ + 'content-type': 'text/html', + ':status': 200 + }); + stream.end(testData); + })); + + const { clientSide, serverSide } = makeDuplexPair(); + + // The lengths of the expected writes... note that this is highly + // sensitive to how the internals are implemented. + const serverLengths = [24, 9, 9, 32]; + const clientLengths = [9, 9, 48, 9, 1, 21, 1, 16]; + + // Adjust for the 24-byte preamble and two 9-byte settings frames, and + // the result must be equally divisible by 8 + assert.strictEqual( + (serverLengths.reduce((i, n) => i + n) - 24 - 9 - 9) % 8, 0); + + // Adjust for two 9-byte settings frames, and the result must be equally + // divisible by 8 + assert.strictEqual( + (clientLengths.reduce((i, n) => i + n) - 9 - 9) % 8, 0); + + serverSide.on('data', common.mustCall((chunk) => { + assert.strictEqual(chunk.length, serverLengths.shift()); + }, serverLengths.length)); + clientSide.on('data', common.mustCall((chunk) => { + assert.strictEqual(chunk.length, clientLengths.shift()); + }, clientLengths.length)); + + server.emit('connection', serverSide); + + const client = http2.connect('http://localhost:80', { + paddingStrategy: PADDING_STRATEGY_ALIGNED, + createConnection: common.mustCall(() => clientSide) + }); + + const req = client.request({ ':path': '/a' }); + + req.on('response', common.mustCall()); + + req.setEncoding('utf8'); + req.on('data', common.mustCall((data) => { + assert.strictEqual(data, testData); + })); + req.on('close', common.mustCall(() => { + clientSide.destroy(); + clientSide.end(); + })); + req.end(); +}