node icon indicating copy to clipboard operation
node copied to clipboard

http2, tls: check content-length, fix RST and GOAWAY logic

Open szmarczak opened this issue 9 months ago • 35 comments

Fixes #35209 Closes #35378

Time spent: 90h

[!CAUTION] These bug fixes are potentially semver-major!

  1. Fixed stream.close(NGHTTP2_CANCEL) closing with 0 aka NGHTTP2_NO_ERROR.
  2. serverStream.destroy() closed with 0, now NGHTTP2_INTERNAL_ERROR.
  3. clientStream.destroy() closed with 0, now NGHTTP2_CANCEL.
  4. Mismatched content-length now throws NGHTTP2_PROTOCOL_ERROR as according to the spec.
  5. Fixed GOAWAY (server -> client) being greeted with GOAWAY (client -> server) as it's against the spec.
  6. Fixed streams being always closed with NGHTTP2_CANCEL on socket close, now respects goawayCode and destroyCode. The default is NGHTTP2_INTERNAL_ERROR for premature TCP FIN and TCP RST.
  7. Fixed stream.rstCode being 0 while active - docs say it should be undefined.
  8. Fixed preferring sessionCode over rstCode when destroying a stream with an error.
  9. Fixed streams being closed with NO_ERROR if session received GOAWAY with NO_ERROR and remote closed connection.
  10. Fixed streams being closed with NO_ERROR if session sent GOAWAY with NO_ERROR and destroyed.
  11. Fixed GOAWAY preventing RST_STREAM from being sent before GOAWAY. nghttp2 correctly assumes that it should prevent RST_STREAM from being sent but incorretly applies it to all frames in a packet instead of frames defined after GOAWAY. This is not the only thing that nghttp2 does wrong:
    • https://github.com/nghttp2/nghttp2/issues/1166
    • https://github.com/nghttp2/nghttp2/issues/692
  12. Fixed benchmark/http2/headers.js calling client.destroy() (resulting in dropped requests). Now calls client.close() which closes gracefully.
  13. connectStreamSocket.destroy() now closes with NGHTTP2_CONNECT_ERROR.
  14. Fixed double GOAWAY on session.close().
  15. Fixed queued RST_STREAM and GOAWAY being dropped if Http2Session::Close() and previous writes weren't finished yet.
  16. Fixed stream frame errors closing the entire session instead of just the stream.
  17. Fixed stream frame errors sending RST_STREAM on idle streams (to reproduce 16. needs to be fixed).
  18. Fixed https://github.com/nodejs/node/issues/49484
  19. Fixed server close not being emitted if TLS socket RSTs
  20. Fixed deadlock when sending GOAWAY before receiving server's GOAWAY. It won't gracefully close because the server may never ACK our GOAWAY. Now it sends TCP RST after terminating GOAWAY.

Sorry so many bugs are fixed in a single PR but it's impossible to fix one without fixing them all!

Best if https://github.com/nghttp2/nghttp2/pull/1508 got merged before this, but it has been open for years 😢 Hence the patch for nghttp2_session.c


/cc @jasnell

szmarczak avatar May 26 '24 05:05 szmarczak