-
Notifications
You must be signed in to change notification settings - Fork 29.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(http2): check content-length, fix sending RST #53160
base: main
Are you sure you want to change the base?
Conversation
Review requested:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow. congrats! It would really be better if the patch was landed, yes.
The linter and commit validator are failing, could you check? |
This comment was marked as resolved.
This comment was marked as resolved.
@jasnell could you reach out to the nghttp2 maintainers and ask about the status of that PR? I would sincerely hope we wouldn't have to maintain a floating patch. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Have you tried pinging nghttp2? |
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Umm... gcc segfaults on my side when trying to build with asan for
I'll try upgrading Ubuntu (I hate Ubuntu). |
Well, after upgrading Ubuntu the resolution is now locked to 1024 x 768 (4:3), while my monitor is 16:9. It looks so stretched and pixelated :( I've been hating Ubuntu since at least 2018 (prolly since 2016) and I don't recommend anyone using it. It breaks randomly (black screen of death), can't set default audio device, can't set default programs for file extensions and bunch of other issues I don't remember. I'll try some other distro tomorrow. I just used Ubuntu cuz I needed Linux fast some time ago and didn't have the time to set up some bare-bones distro from scratch. I used to daily drive Arch but it started breaking as well (it was throwing fstab errors, black screen, reinstall worked but repeat this 3 times and I got angry again). I never got the Debian installer past the Installing... progress bar, it always got stuck. Edit: lmao after upgrade it put itself first on the efi boot entry list, what a shitty distro Time to log off for today! |
Fixes #35209
Closes #35378
Time spent: 60h
Caution
These bug fixes are potentially
semver-major
!stream.close(NGHTTP2_CANCEL)
closing with0
akaNGHTTP2_NO_ERROR
.serverStream.destroy()
closed with0
, nowNGHTTP2_INTERNAL_ERROR
.clientStream.destroy()
closed with0
, nowNGHTTP2_CANCEL
.content-length
now throwsNGHTTP2_PROTOCOL_ERROR
as according to the spec.GOAWAY
(server -> client) being greeted withGOAWAY
(client -> server) as it's against the spec.NGHTTP2_CANCEL
on socket close, now respectsgoawayCode
anddestroyCode
.For client, the default remains
NGHTTP2_CANCEL
.For server, the default now is
NGHTTP2_INTERNAL_ERROR
.stream.rstCode
being 0 while active - docs say it should be undefined.sessionCode
overrstCode
when destroying a stream with an error.NO_ERROR
if session receivedGOAWAY
withNO_ERROR
and remote closed connection.NO_ERROR
if session sentGOAWAY
withNO_ERROR
and destroyed.GOAWAY
preventingRST_STREAM
from being sent beforeGOAWAY
.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 afterGOAWAY
. This is not the only thing that nghttp2 does wrong:benchmark/http2/headers.js
callingclient.destroy()
(resulting in dropped requests).Now calls
client.close()
which closes gracefully.connectStreamSocket.destroy()
now closes withNGHTTP2_CONNECT_ERROR
.GOAWAY
onsession.close()
.Http2Session::Close()
and previous writes weren't finished yet.RST_STREAM
on idle streams (to reproduce 16. needs to be fixed).Sorry so many bugs are fixed in a single PR but it's impossible to fix one without fixing them all!
Best if nghttp2/nghttp2#1508 got merged before this, but it has been open for years 😢
Hence the patch for
nghttp2_session.c
/cc @jasnell