node
node copied to clipboard
http2, tls: check content-length, fix RST and GOAWAY logic
Fixes #35209 Closes #35378
Time spent: 90h
[!CAUTION] These bug fixes are potentially
semver-major
!
- Fixed
stream.close(NGHTTP2_CANCEL)
closing with0
akaNGHTTP2_NO_ERROR
. -
serverStream.destroy()
closed with0
, nowNGHTTP2_INTERNAL_ERROR
. -
clientStream.destroy()
closed with0
, nowNGHTTP2_CANCEL
. - Mismatched
content-length
now throwsNGHTTP2_PROTOCOL_ERROR
as according to the spec. - Fixed
GOAWAY
(server -> client) being greeted withGOAWAY
(client -> server) as it's against the spec. - Fixed streams being always closed with
NGHTTP2_CANCEL
on socket close, now respectsgoawayCode
anddestroyCode
. The default isNGHTTP2_INTERNAL_ERROR
for premature TCP FIN and TCP RST. - Fixed
stream.rstCode
being 0 while active - docs say it should be undefined. - Fixed preferring
sessionCode
overrstCode
when destroying a stream with an error. - Fixed streams being closed with
NO_ERROR
if session receivedGOAWAY
withNO_ERROR
and remote closed connection. - Fixed streams being closed with
NO_ERROR
if session sentGOAWAY
withNO_ERROR
and destroyed. - Fixed
GOAWAY
preventingRST_STREAM
from being sent beforeGOAWAY
. nghttp2 correctly assumes that it should preventRST_STREAM
from being sent but incorretly applies it to all frames in a packet instead of frames defined afterGOAWAY
. This is not the only thing that nghttp2 does wrong:- https://github.com/nghttp2/nghttp2/issues/1166
- https://github.com/nghttp2/nghttp2/issues/692
- Fixed
benchmark/http2/headers.js
callingclient.destroy()
(resulting in dropped requests). Now callsclient.close()
which closes gracefully. -
connectStreamSocket.destroy()
now closes withNGHTTP2_CONNECT_ERROR
. - Fixed double
GOAWAY
onsession.close()
. - Fixed queued RST_STREAM and GOAWAY being dropped if
Http2Session::Close()
and previous writes weren't finished yet. - Fixed stream frame errors closing the entire session instead of just the stream.
- Fixed stream frame errors sending
RST_STREAM
on idle streams (to reproduce 16. needs to be fixed). - Fixed https://github.com/nodejs/node/issues/49484
- Fixed server close not being emitted if TLS socket RSTs
- 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