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

[kumarak]

The PR changes add the stream to the pending list on receiving
RST_STREAM frame with error code nghttp2_cancel. This is to
avoid the force purging of data on receiving the rst_stream frame.

On force purging, the nghttp2 raises close callback for already
destroyed stream which is causing the double-free memory error.

Fixes: #38964

[kumarak]

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.

[mcollina]

lgtm

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/39151/

[github-actions]

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

https://github.com/nodejs/node/actions/runs/1048570186

[mcollina]

Landed in c0f1000

[richardlau]

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]

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

[richardlau]

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]

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]

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