http2: fix double free due to handling of rst_stream with cancel code by kumarak...

Copy link

Member

kumarak commented on Jul 19, 2021

I updated the conditions to add streams to the pending list on receiving RST_STREAM frame. Looking into the failing test cases, I found the RST_STREAM frames may get received when the session is not in scope. Adding streams to the pending list in such cases causes the endpoint to hang. The check prevents it. There could be other possible cases where the pending streams may not get processed and I may have missed handling it. It will be great to get feedback from the community on this.

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/.ncu

mcollina commented on Jul 20, 2021

Landed in c0f1000

richardlau commented on Jul 23, 2021

This cherry-picks to v12.x-staging without conflicts but doesn't compile:

../src/node_http2.cc: In member function ‘void node::http2::Http2Stream::SubmitRstStream(uint32_t)’:
../src/node_http2.cc:2165:17: error: ‘class node::http2::Http2Session’ has no member named ‘is_in_scope’
   if (session_->is_in_scope() &&
                 ^~~~~~~~~~~
../src/node_http2.cc:2166:20: error: ‘is_writable’ was not declared in this scope
      !is_writable() && is_reading()) {
                    ^
../src/node_http2.cc:2166:36: error: ‘is_reading’ was not declared in this scope
       !is_writable() && is_reading()) {
                                    ^

kumarak commented on Jul 23, 2021

Thanks, @richardlau for the notification. I will check the compile issue with v12.x-staging.

richardlau commented on Jul 26, 2021

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 nghttp2

=== release test-http2-server-shutdown-options-errors ===
Path: parallel/test-http2-server-shutdown-options-errors
Command: out/Release/node /home/iojs/node-tree/v12.x/test/parallel/test-http2-server-shutdown-options-errors.js
--- TIMEOUT ---
=== release test-http2-server-stream-session-destroy ===
Path: parallel/test-http2-server-stream-session-destroy
Command: out/Release/node /home/iojs/node-tree/v12.x/test/parallel/test-http2-server-stream-session-destroy.js
--- TIMEOUT ---
[06:27|% 100|+ 3038|-   2]: Done

cc @nodejs/http2

kumarak commented on Jul 26, 2021

Hi @richardlau, there is no concept of scope with Http2Stream. The flags_ from Http2Session should be exposed and tested if it is set to SESSION_STATE_HAS_SCOPE.

Can I raise a PR specifically with v12.x-staging branch?

richardlau commented on Jul 26, 2021

Yes, please raise a PR targeting the v12.x-staging branch.