From d9220aee8b2d52b4fc0deb63019df4bfe90f1ccd Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 7 Jul 2020 23:30:04 +0200 Subject: [PATCH 1/3] http: fix crash for sync write errors during header parsing Fix a crash that occurs when `parser.finish()` is called during `parser.execute()`. In this particular case, this happened because a 100 continue response is a place in which `.end()` can be called which can in turn lead to a write error, which is emitted synchronously, thus inside the outer `parser.execute()` call. Resolve that by delaying the `parser.finish()` call until after the `parser.execute()` call is done. This only affects v12.x, because on later versions, errors are not emitted synchronously. Fixes: https://github.com/nodejs/node/issues/15102 --- lib/_http_client.js | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/lib/_http_client.js b/lib/_http_client.js index dda29e3e8e6be5..3dfc5f5d34af74 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -431,8 +431,12 @@ function socketErrorListener(err) { const parser = socket.parser; if (parser) { - parser.finish(); - freeParser(parser, req, socket); + // Use process.nextTick() on v12.x since 'error' events might be + // emitted synchronously from e.g. a failed write() call on the socket. + process.nextTick(() => { + parser.finish(); + freeParser(parser, req, socket); + }); } // Ensure that no further data will come out of the socket From 11406b958fe266da3f336445890823d96b92282e Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 7 Jul 2020 23:28:07 +0200 Subject: [PATCH 2/3] test: add regression tests for HTTP parser crash Since the tests only crash on v12.x, this commit adds separate regression tests. Refs: https://github.com/nodejs/node/issues/15102 --- ...t-http-fake-socket-error-during-headers.js | 34 ++++++++++++ ...t-http-sync-write-error-during-continue.js | 53 +++++++++++++++++++ 2 files changed, 87 insertions(+) create mode 100644 test/parallel/test-http-fake-socket-error-during-headers.js create mode 100644 test/parallel/test-http-sync-write-error-during-continue.js diff --git a/test/parallel/test-http-fake-socket-error-during-headers.js b/test/parallel/test-http-fake-socket-error-during-headers.js new file mode 100644 index 00000000000000..3654e210296bc6 --- /dev/null +++ b/test/parallel/test-http-fake-socket-error-during-headers.js @@ -0,0 +1,34 @@ +'use strict'; +const common = require('../common'); +const assert = require('assert'); +const http = require('http'); +const MakeDuplexPair = require('../common/duplexpair'); + +// Regression test for the crash reported in +// https://github.com/nodejs/node/issues/15102 (httpParser.finish() is called +// during httpParser.execute()): + +{ + const { clientSide, serverSide } = MakeDuplexPair(); + + serverSide.on('data', common.mustCall((data) => { + assert.strictEqual(data.toString('utf8'), `\ +GET / HTTP/1.1 +Host: localhost:80 +Connection: close + +`.replace(/\n/g, '\r\n')); + + setImmediate(() => { + serverSide.write('HTTP/1.1 200 OK\r\n\r\n'); + }); + })); + + const req = http.get({ + createConnection: common.mustCall(() => clientSide) + }, common.mustCall((res) => { + req.on('error', common.mustCall()); + req.socket.emit('error', new Error('very fake error')); + })); + req.end(); +} diff --git a/test/parallel/test-http-sync-write-error-during-continue.js b/test/parallel/test-http-sync-write-error-during-continue.js new file mode 100644 index 00000000000000..5476160942dadc --- /dev/null +++ b/test/parallel/test-http-sync-write-error-during-continue.js @@ -0,0 +1,53 @@ +'use strict'; +const common = require('../common'); +const assert = require('assert'); +const http = require('http'); +const MakeDuplexPair = require('../common/duplexpair'); + +// Regression test for the crash reported in +// https://github.com/nodejs/node/issues/15102 (httpParser.finish() is called +// during httpParser.execute()): + +{ + const { clientSide, serverSide } = MakeDuplexPair(); + + serverSide.on('data', common.mustCall((data) => { + assert.strictEqual(data.toString('utf8'), `\ +GET / HTTP/1.1 +Expect: 100-continue +Host: localhost:80 +Connection: close + +`.replace(/\n/g, '\r\n')); + + setImmediate(() => { + serverSide.write('HTTP/1.1 100 Continue\r\n\r\n'); + }); + })); + + const req = http.request({ + createConnection: common.mustCall(() => clientSide), + headers: { + 'Expect': '100-continue' + } + }); + req.on('continue', common.mustCall((res) => { + let sync = true; + + clientSide._writev = null; + clientSide._write = common.mustCall((chunk, enc, cb) => { + assert(sync); + // On affected versions of Node.js, the error would be emitted on `req` + // synchronously (i.e. before commit f663b31cc2aec), which would cause + // parser.finish() to be called while we are here in the 'continue' + // callback, which is inside a parser.execute() call. + + assert.strictEqual(chunk.length, 0); + clientSide.destroy(new Error('sometimes the code just doesn’t work'), cb); + }); + req.on('error', common.mustCall()); + req.end(); + + sync = false; + })); +} From f30da15addbf6ab604ca215a22e2b7ddaf3cfd2a Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 8 Jul 2020 00:24:58 +0200 Subject: [PATCH 3/3] fixup! test: add regression tests for HTTP parser crash --- ...t-http-fake-socket-error-during-headers.js | 34 ------------------- 1 file changed, 34 deletions(-) delete mode 100644 test/parallel/test-http-fake-socket-error-during-headers.js diff --git a/test/parallel/test-http-fake-socket-error-during-headers.js b/test/parallel/test-http-fake-socket-error-during-headers.js deleted file mode 100644 index 3654e210296bc6..00000000000000 --- a/test/parallel/test-http-fake-socket-error-during-headers.js +++ /dev/null @@ -1,34 +0,0 @@ -'use strict'; -const common = require('../common'); -const assert = require('assert'); -const http = require('http'); -const MakeDuplexPair = require('../common/duplexpair'); - -// Regression test for the crash reported in -// https://github.com/nodejs/node/issues/15102 (httpParser.finish() is called -// during httpParser.execute()): - -{ - const { clientSide, serverSide } = MakeDuplexPair(); - - serverSide.on('data', common.mustCall((data) => { - assert.strictEqual(data.toString('utf8'), `\ -GET / HTTP/1.1 -Host: localhost:80 -Connection: close - -`.replace(/\n/g, '\r\n')); - - setImmediate(() => { - serverSide.write('HTTP/1.1 200 OK\r\n\r\n'); - }); - })); - - const req = http.get({ - createConnection: common.mustCall(() => clientSide) - }, common.mustCall((res) => { - req.on('error', common.mustCall()); - req.socket.emit('error', new Error('very fake error')); - })); - req.end(); -}