http2: add aligned padding strategy

jasnell · MylesBorins · commit 0078a9779363 · 2018-05-02T14:30:39.000-04:00
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. Backport-PR-URL: #18050 Backport-PR-URL: #20456 PR-URL: #17938 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
diff --git a/doc/api/http2.md b/doc/api/http2.md
@@ -1671,6 +1671,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.
@@ -1742,6 +1749,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
@@ -1822,6 +1836,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
@@ -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;
@@ -682,6 +681,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,
@@ -988,9 +1006,21 @@ inline ssize_t Http2Session::OnSelectPadding(nghttp2_session* handle,
   Http2Session* session = static_cast<Http2Session*>(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 "     \
@@ -2873,6 +2903,7 @@ void Initialize(Local<Object> 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
@@ -358,6 +358,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
@@ -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 = '<h1>Hello World.</h1>';
+  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();
+}
