From f9edaae48bc65dfb548a55c375973a814957afad Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 21 Jul 2017 13:26:50 +0200 Subject: [PATCH 1/2] http: reset stream to unconsumed in `unconsume()` Reset the underlying socket of an HTTP stream to be marked as unconsume after the HTTP parser no longer owns it. Fixes: https://github.com/nodejs/node/issues/14407 --- src/node_http_parser.cc | 1 + src/stream_base.h | 5 ++++ .../test-http-upgrade-reconsume-stream.js | 29 +++++++++++++++++++ 3 files changed, 35 insertions(+) create mode 100644 test/parallel/test-http-upgrade-reconsume-stream.js diff --git a/src/node_http_parser.cc b/src/node_http_parser.cc index a339043bdca5bd..90fc3b41b525f7 100644 --- a/src/node_http_parser.cc +++ b/src/node_http_parser.cc @@ -527,6 +527,7 @@ class Parser : public AsyncWrap { stream->set_alloc_cb(parser->prev_alloc_cb_); stream->set_read_cb(parser->prev_read_cb_); + stream->Unconsume(); } parser->prev_alloc_cb_.clear(); diff --git a/src/stream_base.h b/src/stream_base.h index 75bf0e937f338b..68c82d243f2913 100644 --- a/src/stream_base.h +++ b/src/stream_base.h @@ -234,6 +234,11 @@ class StreamBase : public StreamResource { consumed_ = true; } + inline void Unconsume() { + CHECK_EQ(consumed_, true); + consumed_ = false; + } + template inline Outer* Cast() { return static_cast(Cast()); } diff --git a/test/parallel/test-http-upgrade-reconsume-stream.js b/test/parallel/test-http-upgrade-reconsume-stream.js new file mode 100644 index 00000000000000..1c4bd16180b14e --- /dev/null +++ b/test/parallel/test-http-upgrade-reconsume-stream.js @@ -0,0 +1,29 @@ +'use strict'; +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); + +const tls = require('tls'); +const http = require('http'); + +// Tests that, after the HTTP parser stopped owning a socket that emits an +// 'upgrade' event, another C++ stream can start owning it (e.g. a TLSSocket). + +const server = http.createServer(common.mustNotCall()); + +server.on('upgrade', common.mustCall((request, socket, head) => { + // This should not crash. + new tls.TLSSocket(socket); + server.close(); + socket.destroy(); +})); + +server.listen(0, common.mustCall(() => { + http.request({ + port: server.address().port, + headers: { + 'Connection': 'Upgrade', + 'Upgrade': 'websocket' + } + }).on('error', () => {}).end(); +})); From 2e84c7bf1ad460106f6ebc03f9748f5d138b6283 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 21 Jul 2017 21:18:33 +0200 Subject: [PATCH 2/2] [squash] address nit --- test/parallel/test-http-upgrade-reconsume-stream.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/parallel/test-http-upgrade-reconsume-stream.js b/test/parallel/test-http-upgrade-reconsume-stream.js index 1c4bd16180b14e..e712ea647b3ad9 100644 --- a/test/parallel/test-http-upgrade-reconsume-stream.js +++ b/test/parallel/test-http-upgrade-reconsume-stream.js @@ -19,11 +19,11 @@ server.on('upgrade', common.mustCall((request, socket, head) => { })); server.listen(0, common.mustCall(() => { - http.request({ + http.get({ port: server.address().port, headers: { 'Connection': 'Upgrade', 'Upgrade': 'websocket' } - }).on('error', () => {}).end(); + }).on('error', () => {}); }));