http2: fix write deadlock exposed by larger window sizes

[pimterry]

This completes the fix from #65079. That PR resolved one test flake (test-worker-terminate-http2-respond-with-file), and reduced the second (test-stream-pipeline-http2) but left an underlying issue there which is still causing flakes.

This took more work, as it's a bit complicated. The issue is a real bug which can result in deadlocks between two Node HTTP/2 peers, not a test issue. This was preexisting though very hard to hit, but is exposed in some cases recently by the new window size update (#64623).

This PR fixes this by dropping a security guard (no reading while writing) completely. I've put it in two commits: the first does the tiny fix to drop the guard & tests it, the second removes various code which is now unreachable without the guard.

Dropping this guard needs careful review, but I think that dropping this guard is safe due to the various other mechanisms in place. As long as we're happy that this is safe, it has a lot of upsides: it fixes the flake, resolves a real deadlock, simply deletes some code, and provides some performance boosts.

An example deadlock flow looks roughly like this:

• Both peers are reading & writing.
• Peers write enough to fill the remote peer's TCP receive buffers (only now easily reachable, due to the larger window sizes).
• While TCP receive buffers are full, both peers try to write at the same time.
• The security guard here blocks reading while writing, so both peers stop reading.
• The buffers are all full, so TCP backpressure stops the writes completing - they wait for space.
• Both peers wait for their writes to complete, which requires a read to empty the buffers, but neither is reading => 🔒

This PR fixes that deadlock, by removing the "no reading while writing" guard completely, in its two forms.

These were added as part of a larger set of many HTTP/2 DoS mitigations in 2019 by @addaleax in #29122, so this needs careful review please!

I do think it's safe though: the key thing it's protecting against (one peer forcing the other to buffer large amounts of outgoing data through manipulation of flow control & window updates) is covered by other existing mechanisms here (most notably maxSessionMemory). I can't find any attacks that depend on this guard alone. I've added an additional test which covers CVE-2019-9517 directly, and confirms that maxSessionMemory blocks the key attack scenario regardless.

Removing this guard has notable performance benefits. @mcollina previously looked at this same issue briefly with #63009 testing small focused tweaks on the same guard. AFAICT tweaks alone had minimal benefit, but the guard is still a real perf problem, because it blocks all reads on the whole H2 session while writing any frames anywhere. That means during e.g. a large read (server large upload/client large download), every sent window update to read more data from any stream pauses reading on all streams until the write completes.

Many different dimensions where removing this entirely should improve things, but I've included one example in the benchmark here: this saturates TLS bidirectional streaming, and with this change you get roughly 25% throughput boost. This is using default settings except it shrinks the stream window size back to the standard H2 default - without that you can't benchmark against previous versions, because they deadlock.

[nodejs-github-bot]

Review requested:

• @nodejs/http2
• @nodejs/net
• @nodejs/performance

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.13%. Comparing base (fd5b135) to head (364edc0).
⚠️ Report is 41 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #65440      +/-   ##
==========================================
+ Coverage   90.12%   90.13%   +0.01%     
==========================================
  Files         752      752              
  Lines      252325   252274      -51     
  Branches    47456    47425      -31     
==========================================
- Hits       227407   227387      -20     
+ Misses      16217    16208       -9     
+ Partials     8701     8679      -22     

Files with missing lines	Coverage Δ
src/node_http2.cc	81.84% <100.00%> (+0.12%)	⬆️
src/node_http2.h	92.22% <ø> (-0.05%)	⬇️

... and 54 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[mcollina]

lgtm

[nodejs-github-bot]

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

[pimterry]

The previous extra security test I added wasn't stable on MacOS, seemingly due to differences in buffer & batching delivery. It didn't fail the security assertions, it just never completed.

This new test is aiming to cover the relevant security behaviour and show it passes with and without this change. I've now rewritten the test completely, to focus directly on the important CVE (my understanding: HTTP/2 writes must correctly apply transport backpressure: they should not silently accept & buffer content when writes stall but the HTTP/2 window is wide open). This now uses a raw socket client and readable with specific config to tightly target this in a way that should behave identically across platforms (I hope 🤞).

Open to adding other security tests here if anybody thinks there's more specific constraints we should validate. I have done even more digging, and I still can't find any cases this guard protects against that other mitigations don't already cover. A review from @nodejs/security would be nice though, especially if anybody was involved in the original 2019 HTTP/2 tightening.

[nodejs-github-bot]

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

[nodejs-github-bot]

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