http2: fix memory leak when nghttp2 hd threshold is reached

[RafaelGSS]

Address: #35233

nghttp2 contains a header limit as you can see here https://github.com/nghttp2/nghttp2/blob/20079b4c2f688385ba9ecf723f958d0448894879/lib/nghttp2_hd.h#L45 and when this limit is reached, the nghttp2 stops the pipelining with https://github.com/nghttp2/nghttp2/blob/master/lib/nghttp2_hd.c#L1658.

However, due to the fact of headers were processed before the error is thrown, the session still holds the memory reference for those headers and in this situation, it's never free causing the memory leak.

[nodejs-github-bot]

Review requested:

• @nodejs/http2
• @nodejs/net

[mcollina]

lgtm

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[mcollina]

@RafaelGSS apparently a relevant test is failing on some platforms: https://ci.nodejs.org/job/node-test-commit-arm/40343/nodes=ubuntu2004-armv7l/testReport/junit/(root)/test/parallel_test_http2_options_max_headers_exceeds_nghttp2/.

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[RafaelGSS]

The ARM build produces a slighty different output. Instead of closing the connection on nghttp2 error, it propagates a frameError with NGHTTP2_HEADER_COMP error. I'll look at this at environment with more calm.

[mcollina]

The ARM build produces a slighty different output. Instead of closing the connection on nghttp2 error, it propagates a frameError with NGHTTP2_HEADER_COMP error. I'll look at this at environment with more calm.

It might well be a difference that we would have to deal with. Maybe add an if depending on the environment?

[RafaelGSS]

@nodejs/build Looks like the nghttp2 behaves slightly different in x86 architectures (Windows and ARM). However, I do not have a machine with these architectures to debug it further. Could somebody help me?

By the way, I have tried with Windows 10 x64 and Ubuntu 20.04 x64, and works fine.

[Trott]

@nodejs/build Looks like the nghttp2 behaves slightly different in x86 architectures (Windows and ARM). However, I do not have a machine with these architectures to debug it further. Could somebody help me?

By the way, I have tried with Windows 10 x64 and Ubuntu 20.04 x64, and works fine.

If all else fails, there is a process to get temporary access to a machine: https://github.com/nodejs/build/blob/09308290d8401e15fcd3f7f5c6610a6ea13df75a/GOVERNANCE.md#temporary-access

You can search for open/closed issues with "temporary access" in the subject/title in the https://github.com/nodejs/build to see examples of requests.

[sxa]

nghttp2 behaves slightly different in x86 architectures (Windows and ARM).

I'm slightly confused as to what you mean there. Is the failing difference happening on more than just ARM? It sounds like you have a pass on Windows/x86 at least.

Looking at https://ci.nodejs.org/job/node-test-commit-arm/40343/ it looks like it's not failing on 64-bit ARM builds (aarch64/arm64) and is only occurring on 32-bit arm (armv7l). It's also worth noting that the machine it's running on in Matteo's log referenced above is a 32-bit docker container running on a 64-bit OS+kernel, although it's also failing on the "real" 32-bit OSs at https://ci.nodejs.org/job/node-test-binary-arm-12+/14026/

I'll try running a build+tests on one of my personal 32-bit ARM32 hosts and see how it goes ...

[RafaelGSS]

I'm slightly confused as to what you mean there. Is the failing difference happening on more than just ARM? It sounds like you have a pass on Windows/x86 at least.

I may be wrong, but looks like it is failing in Windows 2012 R2 x86 as well https://ci.nodejs.org/job/node-test-binary-windows-js-suites/13225/RUN_SUBSET=2,nodes=win2012r2-COMPILED_BY-vs2019-x86/testReport/junit/(root)/test/parallel_test_http2_options_max_headers_exceeds_nghttp2/

[nodejs-github-bot]

Commit Queue failed

- Loading data for nodejs/node/pull/41502
✔  Done loading data for nodejs/node/pull/41502
----------------------------------- PR info ------------------------------------
Title      http2: fix memory leak when nghttp2 hd threshold is reached (#41502)
Author     Rafael Silva  (@RafaelGSS)
Branch     RafaelGSS:fix/http2-memory-leak-nghttp2 -> nodejs:master
Labels     c++, http2, needs-ci
Commits    1
 - http2: fix memory leak on nghttp2 hd threshold
Committers 1
 - RafaelGSS 
PR-URL: https://github.com/nodejs/node/pull/41502
Reviewed-By: Matteo Collina 
Reviewed-By: Stephen Belanger 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/41502
Reviewed-By: Matteo Collina 
Reviewed-By: Stephen Belanger 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - http2: fix memory leak on nghttp2 hd threshold
   ℹ  This PR was created on Thu, 13 Jan 2022 19:41:46 GMT
   ✔  Approvals: 2
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/41502#pullrequestreview-852212678
   ✔  - Stephen Belanger (@Qard): https://github.com/nodejs/node/pull/41502#pullrequestreview-855912162
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2022-01-27T17:14:01Z: https://ci.nodejs.org/job/node-test-pull-request/42196/
- Querying data for job/node-test-pull-request/42196/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/1762462520

[mcollina]

lgtm

[nodejs-github-bot]

Landed in f98a785

[ywave620]

@RafaelGSS Hey, I wonder is there any plan to backport this commit to v14.x?