From 6a7d24b69c2f5cb930258d8666ea1a67bf547c27 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 5 Sep 2019 20:01:18 +0200 Subject: [PATCH] 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 45041250b8f72c..01a8a823adac08 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -434,23 +434,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(); +}));