From a6ca82298c24e2a1a7231529e274bfeb72279155 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 28 Aug 2020 17:11:45 +0200 Subject: [PATCH 1/3] tls: account for buffered data before creating TLSSocket wrap If there is data stored in the socket, create a `JSStreamSocket` wrapper so that no data is lost when passing the socket to the C++ layer (which is unaware of data buffered in JS land). Refs: https://github.com/nodejs/node/issues/34532 --- lib/_tls_wrap.js | 5 ++- .../test-tls-generic-stream-unshift.js | 43 +++++++++++++++++++ 2 files changed, 47 insertions(+), 1 deletion(-) create mode 100644 test/parallel/test-tls-generic-stream-unshift.js diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js index c5f30c01fa18e7..8e83a04abfbb4f 100644 --- a/lib/_tls_wrap.js +++ b/lib/_tls_wrap.js @@ -479,7 +479,10 @@ function TLSSocket(socket, opts) { this[kPendingSession] = null; let wrap; - if ((socket instanceof net.Socket && socket._handle) || !socket) { + // Ideally, the readableLength check would not be necessary, and we would + // have a way to handle the buffered data at the C++ StreamBase level. + if (((socket instanceof net.Socket && socket._handle) || !socket) && + socket.readableLength === 0) { // 1. connected socket // 2. no socket, one will be created with net.Socket().connect wrap = socket; diff --git a/test/parallel/test-tls-generic-stream-unshift.js b/test/parallel/test-tls-generic-stream-unshift.js new file mode 100644 index 00000000000000..1045c91220571c --- /dev/null +++ b/test/parallel/test-tls-generic-stream-unshift.js @@ -0,0 +1,43 @@ +'use strict'; +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); + +const fixtures = require('../common/fixtures'); +const makeDuplexPair = require('../common/duplexpair'); +const assert = require('assert'); +const { TLSSocket, connect } = require('tls'); + +const key = fixtures.readKey('agent1-key.pem'); +const cert = fixtures.readKey('agent1-cert.pem'); +const ca = fixtures.readKey('ca1-cert.pem'); + +const { clientSide, serverSide } = makeDuplexPair(); + +const clientTLS = connect({ + socket: clientSide, + ca, + host: 'agent1' // Hostname from certificate +}); + +let serverTLS; +serverSide.once('data', common.mustCall((chunk) => { + serverSide.unshift(chunk); + serverTLS = new TLSSocket(serverSide, { + isServer: true, + key, + cert, + ca + }); +})); + +assert.strictEqual(clientTLS.connecting, false); + +clientTLS.on('secureConnect', common.mustCall(() => { + clientTLS.write('foobar', common.mustCall(() => { + assert(serverTLS); + assert.strictEqual(serverTLS.read().toString(), 'foobar'); + assert.strictEqual(clientTLS._handle.writeQueueSize, 0); + })); + assert.ok(clientTLS._handle.writeQueueSize > 0); +})); From 54fc9a181cd842041cdc0ebe2c1d0dc15d3a613d Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 28 Aug 2020 17:13:24 +0200 Subject: [PATCH 2/3] http2: account for buffered data before creating HTTP2 socket wrap If there is data stored in the socket, create a `JSStreamSocket` wrapper so that no data is lost when passing the socket to the C++ layer (which is unaware of data buffered in JS land). Fixes: https://github.com/nodejs/node/issues/34532 --- lib/internal/http2/core.js | 6 +- .../test-http2-autoselect-protocol.js | 72 +++++++++++++++++++ 2 files changed, 77 insertions(+), 1 deletion(-) create mode 100644 test/parallel/test-http2-autoselect-protocol.js diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index b913b3b5bdefbe..0679c6ae2d7e74 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -1122,7 +1122,11 @@ class Http2Session extends EventEmitter { constructor(type, options, socket) { super(); - if (!socket._handle || !socket._handle.isStreamBase) { + // Ideally, the readableLength check would not be necessary, and we would + // have a way to handle the buffered data at the C++ StreamBase level. + if (!socket._handle || + !socket._handle.isStreamBase || + socket.readableLength > 0) { socket = new JSStreamSocket(socket); } diff --git a/test/parallel/test-http2-autoselect-protocol.js b/test/parallel/test-http2-autoselect-protocol.js new file mode 100644 index 00000000000000..abd35d4ba75a38 --- /dev/null +++ b/test/parallel/test-http2-autoselect-protocol.js @@ -0,0 +1,72 @@ +'use strict'; +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); + +const assert = require('assert'); +const net = require('net'); +const http = require('http'); +const http2 = require('http2'); + +// Example test for HTTP/1 vs HTTP/2 protocol autoselection. +// Refs: https://github.com/nodejs/node/issues/34532 + +const h1Server = http.createServer(common.mustCall((req, res) => { + res.end('HTTP/1 Response'); +})); + +const h2Server = http2.createServer(common.mustCall((req, res) => { + res.end('HTTP/2 Response'); +})); + +const rawServer = net.createServer(common.mustCall(function listener(socket) { + const data = socket.read(3); + + if (!data) { // Repeat until data is available + socket.once('readable', () => listener(socket)); + return; + } + + // Put the data back, so the real server can handle it: + socket.unshift(data); + + if (data.toString('ascii') === 'PRI') { // Very dumb preface check + h2Server.emit('connection', socket); + } else { + h1Server.emit('connection', socket); + } +}, 2)); + +rawServer.listen(common.mustCall(() => { + const { port } = rawServer.address(); + + let done = 0; + { + // HTTP/2 Request + const client = http2.connect(`http://localhost:${port}`); + const req = client.request({ ':path': '/' }); + req.end(); + + let content = ''; + req.setEncoding('utf8'); + req.on('data', (chunk) => content += chunk); + req.on('end', common.mustCall(() => { + assert.strictEqual(content, 'HTTP/2 Response'); + if (++done === 2) rawServer.close(); + client.close(); + })); + } + + { + // HTTP/1 Request + http.get(`http://localhost:${port}`, common.mustCall((res) => { + let content = ''; + res.setEncoding('utf8'); + res.on('data', (chunk) => content += chunk); + res.on('end', common.mustCall(() => { + assert.strictEqual(content, 'HTTP/1 Response'); + if (++done === 2) rawServer.close(); + })); + })); + } +})); From 52863d56a12a01dd6155c3778ce15dc602c4ce68 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 28 Aug 2020 18:16:39 +0200 Subject: [PATCH 3/3] fixup! tls: account for buffered data before creating TLSSocket wrap --- lib/_tls_wrap.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js index 8e83a04abfbb4f..441ac81fe4c139 100644 --- a/lib/_tls_wrap.js +++ b/lib/_tls_wrap.js @@ -481,8 +481,9 @@ function TLSSocket(socket, opts) { let wrap; // Ideally, the readableLength check would not be necessary, and we would // have a way to handle the buffered data at the C++ StreamBase level. - if (((socket instanceof net.Socket && socket._handle) || !socket) && - socket.readableLength === 0) { + if ((socket instanceof net.Socket && + socket._handle && + socket.readableLength === 0) || !socket) { // 1. connected socket // 2. no socket, one will be created with net.Socket().connect wrap = socket;