http2: Node crashes with an assertion error if an http2 server is closed after receiving and rejecting very large headers

[murgatroid99]

• Version: 14.10.0
• Platform: Linux DESKTOP-OKC3QBQ 4.4.0-18362-Microsoft Crypto tests overhaul #1049-Microsoft Thu Aug 14 12:01:00 PST 2020 x86_64 x86_64 x86_64 GNU/Linux
• Subsystem: http2

What steps will reproduce the bug?

const http2 = require('http2');

const server = http2.createServer({
  maxSendHeaderBlockLength: Number.MAX_SAFE_INTEGER
});

server.on('stream', (stream, headers) => {
  stream.respond();
  stream.end();
});

server.listen(8080, () => {
  const clientSession = http2.connect('http://localhost:8080', {
    maxSendHeaderBlockLength: Number.MAX_SAFE_INTEGER
  });

  clientSession.on('error', (error) => {
    console.log(error);
  })

  const stream = clientSession.request({
    'test-header': 'A'.repeat(90000)
  });

  stream.on('close', () => {
    console.log(`Stream closed with RST_STREAM code ${stream.rstCode}`);
    server.close();
  });

  stream.on('error', (error) => {
    console.log(error);
  })

  stream.end();
});

How often does it reproduce? Is there a required condition?

This reproduction is 100% consistent.

What is the expected behavior?

Error [ERR_HTTP2_SESSION_ERROR]: Session closed with error code 9
    at Http2Session.onGoawayData (internal/http2/core.js:642:21) {
  code: 'ERR_HTTP2_SESSION_ERROR'
}
Stream closed with RST_STREAM code 9
Error [ERR_HTTP2_SESSION_ERROR]: Session closed with error code 9
    at Http2Session.onGoawayData (internal/http2/core.js:642:21) {
  code: 'ERR_HTTP2_SESSION_ERROR'
}

What do you see instead?

Error [ERR_HTTP2_SESSION_ERROR]: Session closed with error code 9
    at Http2Session.onGoawayData (internal/http2/core.js:642:21) {
  code: 'ERR_HTTP2_SESSION_ERROR'
}
Stream closed with RST_STREAM code 9
Error [ERR_HTTP2_SESSION_ERROR]: Session closed with error code 9
    at Http2Session.onGoawayData (internal/http2/core.js:642:21) {
  code: 'ERR_HTTP2_SESSION_ERROR'
}
node[5776]: ../src/node_http2.cc:522:virtual node::http2::Http2Session::~Http2Session(): Assertion `(current_nghttp2_memory_) == (0)' failed.
 1: 0xa02dd0 node::Abort() [node]
 2: 0xa02e4e  [node]
 3: 0xa274aa node::http2::Http2Session::~Http2Session() [node]
 4: 0xa27611 node::http2::Http2Session::~Http2Session() [node]
 5: 0x9a9dbb node::Environment::RunCleanup() [node]
 6: 0x96d3d7 node::FreeEnvironment(node::Environment*) [node]
 7: 0xa42d6f node::NodeMainInstance::Run() [node]
 8: 0x9d10e5 node::Start(int, char**) [node]
 9: 0x7f9b63f60830 __libc_start_main [/lib/x86_64-linux-gnu/libc.so.6]
10: 0x9697bc  [node]
Aborted (core dumped)

Additional information

This is related to #35218 regarding the handling of very large request headers.

[bnoordhuis]

I can confirm that the crash still happens with v14.11.0. It seems to be fixed on the master branch but I can't immediately pinpoint the responsible commit.

cc @nodejs/http2

[zerobytes]

When we get a ERR_HTTP2_SESSION_ERROR, node still crashes.

Happens at 17.0.0 version.

Do we have any ideas on how to prevent that from happening?
try/catching the calls won't work and it causes node to restart everytime.
Should I open a new issue for that?

[mcollina]

@zerobytes it would be awesome to receive a PR to fix this bug.

[RafaelGSS]

Hey! I was working on it, but lately, I'm not having time to work on it. So I'd like to share my findings about this issue to help anyone who wants to fix it.

Well, after several debugging over Node Http2 + nghttp2 I have figured out that the following script is leaking memory (69 bytes in my machine)

const http2 = require('http2');

const server = http2.createServer((req, res) => {
  res.end()
});

server.listen(8080, () => {
  const clientSession = http2.connect('http://localhost:8080', {
    maxSendHeaderBlockLength: Number.MAX_SAFE_INTEGER,
    // maxSendHeaderBlockLength: 10,
  });

  clientSession.on('error', (error) => {
    console.log(error);
  })

  const stream = clientSession.request({
    'test-header': 'A'.repeat(87382)
  });

  stream.on('close', () => {
    console.log(`Stream closed with RST_STREAM code ${stream.rstCode}`);
    clientSession.close();
    server.close();
  });

  stream.on('error', (error) => {
    console.log(error);
  })

  stream.end();
});

And if you decrease the 'A'.repeat(87382) by 1 it works.

What happens under the hood is that nghttp2 contains a header limit as you can see here https://github.com/nghttp2/nghttp2/blob/20079b4c2f688385ba9ecf723f958d0448894879/lib/nghttp2_hd.h#L45.

When you run the above script using nghttp2 debug mode you will get the following results:

using 87381

inflatehd: decoded integer is 8
inflatehd: 8 bytes read
inflatehd: huffman encoded=1
inflatehd: decoded integer is 65536
inflatehd: valuelen=65536
inflatehd: 16355 bytes read
inflatehd: still 49181 bytes to go
recv: proclen=16369
recv: [IB_EXPECT_CONTINUATION]
...

using 87382

inflatehd: decoded integer is 8
inflatehd: 8 bytes read
inflatehd: huffman encoded=1
----> inflatehd: integer exceeded the maximum value 65536
----> inflatehd: error return -523
Http2Session server (6) sending pending data
stream: adjusting kept idle streams num_idle_streams=0, max=100
send: frame preparation failed with The current session is closing
Http2Session server (6) frame type 4 was not sent, code: -530
...

However, it is not the real bug. What happens, in reality, is that the nghttp2_rcbuf_decref wasn't called enough and the memory of the nghttp2_hd_inflater->nv_value_keep is never free because the nghttp2_hd_inflater contains ref > 0 when the deleter of session returns (https://github.com/nodejs/node/blob/master/src/node_http2.cc#L534)

Not sure if I could explain properly, but basically, when 'A'.repeat(X) and X is greater than 87381 (this number might make sense only in my machine - due to architectural reasons) the memory of hd_inflater is never free because the rcbuf->ref (that count the references of an object) is > 0.

An obvious “fix” is to sync the maxHeaderBufferLength with nghttp2 header limit, but I’m not a fan of this approach.

Also, is not clear to me if that is a bug inside nghttp2 or our current implementation. For someone who will take it in the coming days, check which functions are calling the nghttp2_rcbuf_incref for the hd_inflater and never freeing when the maximum header value is thrown.

Note: it might be related to #28632 (comment)

cc: @addaleax (I'm tagging you because you might have a quick solution on mind)

[RafaelGSS]

@mcollina could you close this one?