http2: fix double free due to handling of rst_stream with cancel code #39423
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
1ca82d3 to
8c6315e
Compare
8c6315e to
f3ea45e
Compare
http2: add checks to update the pending list if stream received in scope
ff75ebb to
59497fb
Compare
Contributor
Author
|
I updated the conditions to add streams to the pending list on receiving |
jasnell
approved these changes
Jul 19, 2021
Collaborator
Contributor
Commit Queue failed- Loading data for nodejs/node/pull/39423 ✔ Done loading data for nodejs/node/pull/39423 ----------------------------------- PR info ------------------------------------ Title http2: fix double free due to handling of rst_stream with cancel code (#39423) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch kumarak:kumarak/fix_38964 -> nodejs:master Labels c++, commit-queue, http2, lts-watch-v12.x, lts-watch-v14.x, needs-ci Commits 2 - http2: on receiving rst_stream with cancel code add it to pending list - http2: update checks for adding rst_stream to pending list Committers 1 - Akshay K PR-URL: https://github.com/nodejs/node/pull/39423 Fixes: https://github.com/nodejs/node/issues/38964 Reviewed-By: James M Snell Reviewed-By: Matteo Collina ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/39423 Fixes: https://github.com/nodejs/node/issues/38964 Reviewed-By: James M Snell Reviewed-By: Matteo Collina -------------------------------------------------------------------------------- ℹ This PR was created on Sun, 18 Jul 2021 02:26:43 GMT ✔ Approvals: 2 ✔ - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/39423#pullrequestreview-709620997 ✔ - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/39423#pullrequestreview-710283289 ✖ Last GitHub CI failed ℹ Last Full PR CI on 2021-07-20T07:57:13Z: https://ci.nodejs.org/job/node-test-pull-request/39151/ - Querying data for job/node-test-pull-request/39151/ ✔ Build data downloaded ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/1048570186 |
Member
|
Landed in c0f1000 |
Member
|
This cherry-picks to v12.x-staging without conflicts but doesn't compile: |
Contributor
Author
|
Thanks, @richardlau for the notification. I will check the compile issue with |
Merged
Member
|
FWIW I tried this on v12.x-staging, which compiled but ended up with two http2 tests timing out diff --git a/src/node_http2.cc b/src/node_http2.cc
index dec6d7dab9..46b7dbf80e 100644
--- a/src/node_http2.cc
+++ b/src/node_http2.cc
@@ -2150,6 +2150,25 @@ int Http2Stream::SubmitPriority(nghttp2_priority_spec* prispec,
void Http2Stream::SubmitRstStream(const uint32_t code) {
CHECK(!this->IsDestroyed());
code_ = code;
+
+ // If RST_STREAM frame is received and stream is not writable
+ // because it is busy reading data, don't try force purging it.
+ // Instead add the stream to pending stream list and process
+ // the pending data when it is safe to do so. This is to avoid
+ // double free error due to unwanted behavior of nghttp2.
+ // Ref:https://github.com/nodejs/node/issues/38964
+
+ // Add stream to the pending list if it is received with scope
+ // below in the stack. The pending list may not get processed
+ // if RST_STREAM received is not in scope and added to the list
+ // causing endpoint to hang.
+ if ((flags_ & SESSION_STATE_HAS_SCOPE) &&
+ !IsWritable() && IsReading()) {
+ session_->AddPendingRstStream(id_);
+ return;
+ }
+
+
// If possible, force a purge of any currently pending data here to make sure
// it is sent before closing the stream. If it returns non-zero then we need
// to wait until the current write finishes and try again to avoid nghttp2cc @nodejs/http2 |
Contributor
Author
|
Hi @richardlau, there is no concept of scope with Can I raise a PR specifically with |
Member
|
Yes, please raise a PR targeting the |
kumarak
added a commit
to kumarak/node
that referenced
this pull request
Jul 26, 2021
kumarak
added a commit
to kumarak/node
that referenced
this pull request
Jul 26, 2021
Merged
Merged
BethGriggs
added a commit
that referenced
this pull request
Jul 29, 2021
This is a security release. Notable Changes: - CVE-2021-22930: Use after free on close http2 on stream canceling (High) [#39423](#39423) - (SEMVER-MINOR) deps: update V8 to 9.2.230.21 (Michaël Zasso) [#39470](#39470) - inspector: mark as stable (Gireesh Punathil) [#37748](#37748) - (SEMVER-MINOR) perf_hooks: web performance timeline compliance (legendecas) [#39297](#39297) - punycode: add pending deprecation (Antoine du Hamel) [#38444](#38444) - (SEMVER-MINOR) repl: enable --experimental-repl-await /w opt-out (hemanth.hm) [#34733](#34733) PR-URL: #39534
BethGriggs
added a commit
that referenced
this pull request
Jul 29, 2021
This is a security release. Notable Changes: - CVE-2021-22930: Use after free on close http2 on stream canceling (High) [#39423](#39423) - (SEMVER-MINOR) deps: update V8 to 9.2.230.21 (Michaël Zasso) [#39470](#39470) - inspector: mark as stable (Gireesh Punathil) [#37748](#37748) - (SEMVER-MINOR) perf_hooks: web performance timeline compliance (legendecas) [#39297](#39297) - punycode: add pending deprecation (Antoine du Hamel) [#38444](#38444) - (SEMVER-MINOR) repl: enable --experimental-repl-await /w opt-out (hemanth.hm) [#34733](#34733) PR-URL: #39534
BethGriggs
added a commit
that referenced
this pull request
Jul 29, 2021
This is a security release. Notable Changes: - CVE-2021-22930: Use after free on close http2 on stream canceling (High) [#39423](#39423) - (SEMVER-MINOR) deps: update V8 to 9.2.230.21 (Michaël Zasso) [#39470](#39470) - inspector: mark as stable (Gireesh Punathil) [#37748](#37748) - punycode: add pending deprecation (Antoine du Hamel) [#38444](#38444) - (SEMVER-MINOR) repl: enable --experimental-repl-await /w opt-out (hemanth.hm) [#34733](#34733) PR-URL: #39534
BethGriggs
added a commit
that referenced
this pull request
Jul 29, 2021
This is a security release. Notable Changes: - CVE-2021-22930: Use after free on close http2 on stream canceling (High) [#39423](#39423) - (SEMVER-MINOR) deps: update V8 to 9.2.230.21 (Michaël Zasso) [#39470](#39470) - inspector: mark as stable (Gireesh Punathil) [#37748](#37748) - punycode: add pending deprecation (Antoine du Hamel) [#38444](#38444) - (SEMVER-MINOR) repl: enable --experimental-repl-await /w opt-out (hemanth.hm) [#34733](#34733) PR-URL: #39534
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>
foxxyz
pushed a commit
to foxxyz/node
that referenced
this pull request
Oct 18, 2021
PR-URL: nodejs#39423 Fixes: nodejs#38964 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
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
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The PR changes add the stream to the pending list on receiving
RST_STREAMframe with error codenghttp2_cancel. This is toavoid the force purging of data on receiving the rst_stream frame.
On force purging, the
nghttp2raises close callback for alreadydestroyed stream which is causing the double-free memory error.
Fixes: #38964