From 9b72053f633ce90cf32916b8c577315cd57e2dc5 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 22 Feb 2018 02:52:59 +0100 Subject: [PATCH 1/3] http2: fix endless loop when writing empty string Refs: https://github.com/nodejs/node/pull/18673 Fixes: https://github.com/nodejs/node/issues/18169 Refs: https://github.com/nodejs/node/blob/v9.5.0/src/node_http2.cc#L1481-L1484 Refs: https://github.com/nodejs/node/blob/v9.5.0/lib/_http_outgoing.js#L659-L661 --- src/node_http2.cc | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index 0ef77da13aca4f..f1454aa11e58e9 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -2184,6 +2184,17 @@ ssize_t Http2Stream::Provider::Stream::OnRead(nghttp2_session* handle, size_t amount = 0; // amount of data being sent in this data frame. + // Remove all empty chunks from the head of the queue. + // This is done here so that .write('', cb) is still a meaningful way to + // find out when the HTTP2 stream wants to consume data, and because the + // StreamBase API allows empty input chunks. + while (!stream->queue_.empty() && stream->queue_.front().buf.len == 0) { + WriteWrap* finished = stream->queue_.front().req_wrap; + stream->queue_.pop(); + if (finished != nullptr) + finished->Done(0); + } + if (!stream->queue_.empty()) { DEBUG_HTTP2SESSION2(session, "stream %d has pending outbound data", id); amount = std::min(stream->available_outbound_length_, length); @@ -2197,7 +2208,8 @@ ssize_t Http2Stream::Provider::Stream::OnRead(nghttp2_session* handle, } } - if (amount == 0 && stream->IsWritable() && stream->queue_.empty()) { + if (amount == 0 && stream->IsWritable()) { + CHECK(stream->queue_.empty()); DEBUG_HTTP2SESSION2(session, "deferring stream %d", id); return NGHTTP2_ERR_DEFERRED; } From 74f489c4b2d8afa02ebb72e0181714f19db97b76 Mon Sep 17 00:00:00 2001 From: XadillaX Date: Sun, 11 Feb 2018 15:57:35 +0800 Subject: [PATCH 2/3] test: check endless loop while writing empty string Refs: https://github.com/nodejs/node/pull/18673 --- .../test-http2-client-write-empty-string.js | 48 +++++++++++++++++++ 1 file changed, 48 insertions(+) create mode 100644 test/parallel/test-http2-client-write-empty-string.js diff --git a/test/parallel/test-http2-client-write-empty-string.js b/test/parallel/test-http2-client-write-empty-string.js new file mode 100644 index 00000000000000..757dde906cbfc8 --- /dev/null +++ b/test/parallel/test-http2-client-write-empty-string.js @@ -0,0 +1,48 @@ +'use strict'; + +const assert = require('assert'); +const http2 = require('http2'); + +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); + +const server = http2.createServer(); +server.on('stream', common.mustCall((stream, headers, flags) => { + stream.respond({ 'content-type': 'text/html' }); + + let data = ''; + stream.on('data', common.mustNotCall((chunk) => { + data += chunk.toString(); + })); + stream.on('end', common.mustCall(() => { + stream.end(`"${data}"`); + })); +})); + +server.listen(0, common.mustCall(() => { + const port = server.address().port; + const client = http2.connect(`http://localhost:${port}`); + + const req = client.request({ + ':method': 'POST', + ':path': '/' + }); + + req.on('response', common.mustCall((headers) => { + assert.strictEqual(headers[':status'], 200); + assert.strictEqual(headers['content-type'], 'text/html'); + })); + + let data = ''; + req.setEncoding('utf8'); + req.on('data', common.mustCallAtLeast((d) => data += d)); + req.on('end', common.mustCall(() => { + assert.strictEqual(data, '""'); + server.close(); + client.close(); + })); + + req.write(''); + req.end(); +})); From b702ba350ff4ea43b1894ae436bc67d216040908 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 22 Feb 2018 02:52:05 +0100 Subject: [PATCH 3/3] [squash] extend test case --- .../test-http2-client-write-empty-string.js | 78 ++++++++++--------- 1 file changed, 42 insertions(+), 36 deletions(-) diff --git a/test/parallel/test-http2-client-write-empty-string.js b/test/parallel/test-http2-client-write-empty-string.js index 757dde906cbfc8..c10698d417038d 100644 --- a/test/parallel/test-http2-client-write-empty-string.js +++ b/test/parallel/test-http2-client-write-empty-string.js @@ -7,42 +7,48 @@ const common = require('../common'); if (!common.hasCrypto) common.skip('missing crypto'); -const server = http2.createServer(); -server.on('stream', common.mustCall((stream, headers, flags) => { - stream.respond({ 'content-type': 'text/html' }); - - let data = ''; - stream.on('data', common.mustNotCall((chunk) => { - data += chunk.toString(); - })); - stream.on('end', common.mustCall(() => { - stream.end(`"${data}"`); +for (const chunkSequence of [ + [ '' ], + [ '', '' ] +]) { + const server = http2.createServer(); + server.on('stream', common.mustCall((stream, headers, flags) => { + stream.respond({ 'content-type': 'text/html' }); + + let data = ''; + stream.on('data', common.mustNotCall((chunk) => { + data += chunk.toString(); + })); + stream.on('end', common.mustCall(() => { + stream.end(`"${data}"`); + })); })); -})); - -server.listen(0, common.mustCall(() => { - const port = server.address().port; - const client = http2.connect(`http://localhost:${port}`); - - const req = client.request({ - ':method': 'POST', - ':path': '/' - }); - req.on('response', common.mustCall((headers) => { - assert.strictEqual(headers[':status'], 200); - assert.strictEqual(headers['content-type'], 'text/html'); + server.listen(0, common.mustCall(() => { + const port = server.address().port; + const client = http2.connect(`http://localhost:${port}`); + + const req = client.request({ + ':method': 'POST', + ':path': '/' + }); + + req.on('response', common.mustCall((headers) => { + assert.strictEqual(headers[':status'], 200); + assert.strictEqual(headers['content-type'], 'text/html'); + })); + + let data = ''; + req.setEncoding('utf8'); + req.on('data', common.mustCallAtLeast((d) => data += d)); + req.on('end', common.mustCall(() => { + assert.strictEqual(data, '""'); + server.close(); + client.close(); + })); + + for (const chunk of chunkSequence) + req.write(chunk); + req.end(); })); - - let data = ''; - req.setEncoding('utf8'); - req.on('data', common.mustCallAtLeast((d) => data += d)); - req.on('end', common.mustCall(() => { - assert.strictEqual(data, '""'); - server.close(); - client.close(); - })); - - req.write(''); - req.end(); -})); +}