From 2f878d2d6f48542ec82082fcc76586ee817e2edc Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 5 Sep 2019 20:01:18 +0200 Subject: [PATCH 1/2] http2: do not crash on stream listener removal w/ destroyed session Do not crash when the session is no longer available. Fixes: https://github.com/nodejs/node/issues/29457 PR-URL: https://github.com/nodejs/node/pull/29459 Reviewed-By: Luigi Pinca Reviewed-By: Trivikram Kamat Reviewed-By: Minwoo Jung Reviewed-By: Colin Ihrig --- lib/internal/http2/core.js | 12 ++++--- ...ttp2-stream-removelisteners-after-close.js | 31 +++++++++++++++++++ 2 files changed, 39 insertions(+), 4 deletions(-) create mode 100644 test/parallel/test-http2-stream-removelisteners-after-close.js diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index d2f1c22ac705df..5046539c570841 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -422,23 +422,27 @@ function sessionListenerRemoved(name) { // Also keep track of listeners for the Http2Stream instances, as some events // are emitted on those objects. function streamListenerAdded(name) { + const session = this[kSession]; + if (!session) return; switch (name) { case 'priority': - this[kSession][kNativeFields][kSessionPriorityListenerCount]++; + session[kNativeFields][kSessionPriorityListenerCount]++; break; case 'frameError': - this[kSession][kNativeFields][kSessionFrameErrorListenerCount]++; + session[kNativeFields][kSessionFrameErrorListenerCount]++; break; } } function streamListenerRemoved(name) { + const session = this[kSession]; + if (!session) return; switch (name) { case 'priority': - this[kSession][kNativeFields][kSessionPriorityListenerCount]--; + session[kNativeFields][kSessionPriorityListenerCount]--; break; case 'frameError': - this[kSession][kNativeFields][kSessionFrameErrorListenerCount]--; + session[kNativeFields][kSessionFrameErrorListenerCount]--; break; } } diff --git a/test/parallel/test-http2-stream-removelisteners-after-close.js b/test/parallel/test-http2-stream-removelisteners-after-close.js new file mode 100644 index 00000000000000..753467f0f88438 --- /dev/null +++ b/test/parallel/test-http2-stream-removelisteners-after-close.js @@ -0,0 +1,31 @@ +'use strict'; + +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); +const http2 = require('http2'); + +// Regression test for https://github.com/nodejs/node/issues/29457: +// HTTP/2 stream event listeners can be added and removed after the +// session has been destroyed. + +const server = http2.createServer((req, res) => { + res.end('Hi!\n'); +}); + +server.listen(0, common.mustCall(() => { + const client = http2.connect(`http://localhost:${server.address().port}`); + const headers = { ':path': '/' }; + const req = client.request(headers); + + req.on('close', common.mustCall(() => { + req.removeAllListeners(); + req.on('priority', common.mustNotCall()); + server.close(); + })); + + req.on('priority', common.mustNotCall()); + req.on('error', common.mustCall()); + + client.destroy(); +})); From f1359fd27be79b824ced5bba7452f4450f2333e5 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 2 Sep 2019 01:32:15 +0200 Subject: [PATCH 2/2] http2: do not start reading after write if new write is on wire MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Don’t start reading more input data if we’re still busy writing output. This was overlooked in 8a4a1931b8b98. Fixes: https://github.com/nodejs/node/issues/29353 Fixes: https://github.com/nodejs/node/issues/29393 PR-URL: https://github.com/nodejs/node/pull/29399 Reviewed-By: David Carlier Reviewed-By: Colin Ihrig Reviewed-By: James M Snell Reviewed-By: Matteo Collina --- src/node_http2.cc | 6 ++- ...t-http2-multistream-destroy-on-read-tls.js | 47 +++++++++++++++++++ 2 files changed, 52 insertions(+), 1 deletion(-) create mode 100644 test/parallel/test-http2-multistream-destroy-on-read-tls.js diff --git a/src/node_http2.cc b/src/node_http2.cc index 43d1c2ea9f277a..cdf83350bb2a53 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -746,8 +746,10 @@ void Http2Session::Close(uint32_t code, bool socket_closed) { flags_ |= SESSION_STATE_CLOSING; // Stop reading on the i/o stream - if (stream_ != nullptr) + if (stream_ != nullptr) { + flags_ |= SESSION_STATE_READING_STOPPED; stream_->ReadStop(); + } // If the socket is not closed, then attempt to send a closing GOAWAY // frame. There is no guarantee that this GOAWAY will be received by @@ -1228,6 +1230,7 @@ int Http2Session::OnDataChunkReceived(nghttp2_session* handle, // If we are currently waiting for a write operation to finish, we should // tell nghttp2 that we want to wait before we process more input data. if (session->flags_ & SESSION_STATE_WRITE_IN_PROGRESS) { + CHECK_NE(session->flags_ & SESSION_STATE_READING_STOPPED, 0); session->flags_ |= SESSION_STATE_NGHTTP2_RECV_PAUSED; return NGHTTP2_ERR_PAUSE; } @@ -1616,6 +1619,7 @@ void Http2Session::OnStreamAfterWrite(WriteWrap* w, int status) { ClearOutgoing(status); if ((flags_ & SESSION_STATE_READING_STOPPED) && + !(flags_ & SESSION_STATE_WRITE_IN_PROGRESS) && nghttp2_session_want_read(session_)) { flags_ &= ~SESSION_STATE_READING_STOPPED; stream_->ReadStart(); diff --git a/test/parallel/test-http2-multistream-destroy-on-read-tls.js b/test/parallel/test-http2-multistream-destroy-on-read-tls.js new file mode 100644 index 00000000000000..91cbec6b2dc975 --- /dev/null +++ b/test/parallel/test-http2-multistream-destroy-on-read-tls.js @@ -0,0 +1,47 @@ +'use strict'; +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); +const fixtures = require('../common/fixtures'); +const http2 = require('http2'); + +// Regression test for https://github.com/nodejs/node/issues/29353. +// Test that it’s okay for an HTTP2 + TLS server to destroy a stream instance +// while reading it. + +const server = http2.createSecureServer({ + key: fixtures.readKey('agent2-key.pem'), + cert: fixtures.readKey('agent2-cert.pem') +}); + +const filenames = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j']; + +server.on('stream', common.mustCall((stream) => { + function write() { + stream.write('a'.repeat(10240)); + stream.once('drain', write); + } + write(); +}, filenames.length)); + +server.listen(0, common.mustCall(() => { + const client = http2.connect(`https://localhost:${server.address().port}`, { + ca: fixtures.readKey('agent2-cert.pem'), + servername: 'agent2' + }); + + let destroyed = 0; + for (const entry of filenames) { + const stream = client.request({ + ':path': `/${entry}` + }); + stream.once('data', common.mustCall(() => { + stream.destroy(); + + if (++destroyed === filenames.length) { + client.destroy(); + server.close(); + } + })); + } +}));