http2: add aligned padding strategy

[jasnell]

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.

Ping @nodejs/http2

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

Affected core subsystem(s)

http2

[jasnell]

Ping @addaleax ... for some reason I'm not able to open a new issue in the repo... but I can comment on issues and I can add PRs... very strange... In any case, I came across an issue you may want to look at:

---

@addaleax ... heads up... when using the ../common/duplexpair' with http2, and an error is thrown within a data event handler on the serverSide` half, we get an Abort...

That is, modifying test-http2-generic-streams like so (see the serverSide.on('data'... addition).

'use strict';
const common = require('../common');
if (!common.hasCrypto)
  common.skip('missing crypto');
const assert = require('assert');
const http2 = require('http2');
const makeDuplexPair = require('../common/duplexpair');

{
  const testData = '<h1>Hello World</h1>';
  const server = http2.createServer();
  server.on('stream', common.mustCall((stream, headers) => {
    stream.respond({
      'content-type': 'text/html',
      ':status': 200
    });
    stream.end(testData);
  }));

  const { clientSide, serverSide } = makeDuplexPair();
  server.emit('connection', serverSide);

  serverSide.on('data', () => {
    throw 'something';
  });

  const client = http2.connect('http://localhost:80', {
    createConnection: common.mustCall(() => clientSide)
  });

  const req = client.request({ ':path': '/' });

  req.on('response', common.mustCall((headers) => {
    assert.strictEqual(headers[':status'], 200);
  }));

  req.setEncoding('utf8');
  // Note: This is checking that this small amount of data is passed through in
  // a single chunk, which is unusual for our test suite but seems like a
  // reasonable assumption here.
  req.on('data', common.mustCall((data) => {
    assert.strictEqual(data, testData);
  }));
  req.on('end', common.mustCall(() => {
    clientSide.destroy();
    clientSide.end();
  }));
  req.end();
}

results in:

$ ./node test/parallel/test-http2-generic-streams.js
(node:27714) ExperimentalWarning: The http2 module is an experimental API.
FATAL ERROR: v8::ToLocalChecked Empty MaybeLocal.
 1: node::Abort() [./node]
 2: 0x55d8e467592e [./node]
 3: v8::Utils::ReportApiFailure(char const*, char const*) [./node]
 4: node::JSStream::DoWrite(node::WriteWrap*, uv_buf_t*, unsigned long, uv_stream_s*) [./node]
 5: node::http2::Http2Session::SendPendingData() [./node]
 6: node::http2::Http2Session::OnStreamReadImpl(long, uv_buf_t const*, uv_handle_type, void*) [./node]
 7: node::JSStream::ReadBuffer(v8::FunctionCallbackInfo<v8::Value> const&) [./node]
 8: v8::internal::FunctionCallbackArguments::Call(void (*)(v8::FunctionCallbackInfo<v8::Value> const&)) [./node]
 9: 0x55d8e3ed6ba3 [./node]
10: v8::internal::Builtin_HandleApiCall(int, v8::internal::Object**, v8::internal::Isolate*) [./node]
11: 0x76c8100465d
Aborted (core dumped)

[addaleax]

@jasnell I can’t open issues either. :/ Thanks for the pointer though!

[vsemozhetbyt]

A nit: maxmimum -> maximum.

[mcollina]

when it is better to use one strategy or another? In other terms, should this be the default?

[jasnell]

That's still unclear and needs to be benchmarked to determine the best strategy.

[mcollina]

LGTM

[mcollina]

@jasnell after this is landed, can you open up a new issue for picking the best strategy?

[jasnell]

Yep, definitely. The "best" is going to depend on a number of factors and may be difficult to nail down. Padding is supposed to be a security mechanism (see https://tools.ietf.org/html/rfc7540#section-10.7) but it's use really depends on a number of very data-specific and use-case-specific factors.

For instance, one reason why the aligned strategy might not make the best default is this particular passage in the RFC:

   Use of padding can result in less protection than might seem
   immediately obvious.  At best, padding only makes it more difficult
   for an attacker to infer length information by increasing the number
   of frames an attacker has to observe.  Incorrectly implemented
   padding schemes can be easily defeated.  In particular, randomized
   padding with a predictable distribution provides very little
   protection; similarly, padding payloads to a fixed size exposes
   information as payload sizes cross the fixed-sized boundary, which
   could be possible if an attacker can control plaintext.

In other words, aligned padding might may buffer allocation easier, but it also makes frame sizes more predictable.

[jasnell]

CI: https://ci.nodejs.org/job/node-test-pull-request/12385/

[jasnell]

CI is good

[jasnell]

Landed in efdadcc