Skip to content
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

http2: update handling of streams on rst_stream frames #39622

Closed
wants to merge 2 commits into from

Conversation

kumarak
Copy link
Contributor

@kumarak kumarak commented Aug 2, 2021

The PR updates the handling of rst_stream frames and add
all streams to the pending list on receiving rst frames with
the error code NGHTTP2_CANCEL.

The changes will remove dependency on the stream state
that may allow bypassing the checks in certain cases. I think
a better solution is to delay streams in all cases if rst_stream
is received for the cancel events.

The rst_stream frames can be received for protocol/connection
error as well it should be handled immediately. Adding
streams to the pending list in such cases may cause errors.

Previous PR ref: #39423

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. http2 Issues or PRs related to the http2 subsystem. needs-ci PRs that need a full CI run. labels Aug 2, 2021
@kumarak kumarak force-pushed the kumarak/handling_rst_stream branch from 74bdd33 to 9e6745c Compare August 2, 2021 05:58
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Could you add a test for the other cases?

@kumarak
Copy link
Contributor Author

kumarak commented Aug 3, 2021

Thanks, @mcollina! I will add tests for specific cases.

@BethGriggs BethGriggs added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 4, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 4, 2021
@nodejs-github-bot
Copy link
Collaborator

@BethGriggs
Copy link
Member

BethGriggs commented Aug 4, 2021

@kumarak, I'm guessing this may need a manual backport for Node.js 12 (as #39423 did)?

@kumarak
Copy link
Contributor Author

kumarak commented Aug 4, 2021

@BethGriggs, The changes should not need to backport for v12.x. I will verify and raise a PR with v12.x if needed.

@kumarak kumarak force-pushed the kumarak/handling_rst_stream branch from 701ef63 to 9b39a6b Compare August 5, 2021 07:22
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

@BethGriggs BethGriggs removed the needs-ci PRs that need a full CI run. label Aug 6, 2021
BethGriggs pushed a commit that referenced this pull request Aug 6, 2021
The PR updates the handling of rst_stream frames and adds all streams
to the pending list on receiving rst frames with the error code
NGHTTP2_CANCEL.

The changes will remove dependency on the stream state that may allow
bypassing the checks in certain cases. I think a better solution is to
delay streams in all cases if rst_stream is received for the cancel
events.

The rst_stream frames can be received for protocol/connection error as
well it should be handled immediately. Adding streams to the pending
list in such cases may cause errors.

PR-URL: #39622
Refs: #39423
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Beth Griggs <bgriggs@redhat.com>
BethGriggs pushed a commit that referenced this pull request Aug 6, 2021
PR-URL: #39622
Refs: #39423
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Beth Griggs <bgriggs@redhat.com>
@BethGriggs
Copy link
Member

Landed in c7a0f1d...b00d1ae

foxxyz pushed a commit to foxxyz/node that referenced this pull request Oct 18, 2021
The PR updates the handling of rst_stream frames and adds all streams
to the pending list on receiving rst frames with the error code
NGHTTP2_CANCEL.

The changes will remove dependency on the stream state that may allow
bypassing the checks in certain cases. I think a better solution is to
delay streams in all cases if rst_stream is received for the cancel
events.

The rst_stream frames can be received for protocol/connection error as
well it should be handled immediately. Adding streams to the pending
list in such cases may cause errors.

CVE-ID: CVE-2021-22930
Refs: https://nvd.nist.gov/vuln/detail/CVE-2021-22930
PR-URL: nodejs#39622
Refs: nodejs#39423
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Beth Griggs <bgriggs@redhat.com>
foxxyz pushed a commit to foxxyz/node that referenced this pull request Oct 18, 2021
PR-URL: nodejs#39622
Refs: nodejs#39423
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Beth Griggs <bgriggs@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. http2 Issues or PRs related to the http2 subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants