From 7ae2c5a092c738923513f1f8f76c09df9bba18a3 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Sat, 17 Oct 2026 09:16:36 +0200 Subject: [PATCH 1/3] tls: coalesce buffered records into one read SSL_read() returns at most one TLS record, and ClearOut() emitted a separate read for each one. Peers that use small records, such as nginx with its default 4 KiB ssl_buffer_size or servers doing dynamic record sizing, made every record pay for its own buffer allocation and call into JavaScript. While the next buffered record is application data, keep reading into the same 16 KiB buffer before emitting it, so reads never get larger than one full record already makes them. Batching stops before any other record type, so that e.g. a TLS 1.2 renegotiation is still processed after the data in front of it has been emitted. It also stops before records short enough to be empty, because SSL_read() skips empty records and carries on with the next one. If SSL_read() fails in the middle of a batch, the error is captured first and reported after the data. test-tls-max-send-fragment relied on 'data' chunks matching records, so it now checks the record sizes on the wire. Signed-off-by: Robert Nagy Assisted-by: claude:opus-5.5 --- benchmark/tls/throughput-s2c.js | 5 +- src/crypto/crypto_tls.cc | 56 ++++++-- test/parallel/test-tls-coalesce-records.js | 128 ++++++++++++++++++ .../test-tls-coalesce-renegotiation.js | 82 +++++++++++ test/parallel/test-tls-max-send-fragment.js | 60 ++++++-- 5 files changed, 302 insertions(+), 29 deletions(-) create mode 100644 test/parallel/test-tls-coalesce-records.js create mode 100644 test/parallel/test-tls-coalesce-renegotiation.js diff --git a/benchmark/tls/throughput-s2c.js b/benchmark/tls/throughput-s2c.js index 7fb93c304b20..5d31949e4a84 100644 --- a/benchmark/tls/throughput-s2c.js +++ b/benchmark/tls/throughput-s2c.js @@ -6,6 +6,7 @@ const bench = common.createBenchmark(main, { sendchunklen: [256, 32 * 1024, 128 * 1024, 16 * 1024 * 1024], recvbuflen: [0, 64 * 1024, 1024 * 1024], recvbufgenfn: ['true', 'false'], + recordlen: [1024, 4096, 16384], }); const fixtures = require('../../test/common/fixtures'); @@ -14,7 +15,8 @@ let recvbuf; let received = 0; const tls = require('tls'); -function main({ dur, type, sendchunklen, recvbuflen, recvbufgenfn }) { +function main({ dur, type, sendchunklen, recvbuflen, recvbufgenfn, + recordlen }) { if (isFinite(recvbuflen) && recvbuflen > 0) recvbuf = Buffer.alloc(recvbuflen); @@ -74,6 +76,7 @@ function main({ dur, type, sendchunklen, recvbuflen, recvbufgenfn }) { } const server = tls.createServer(options, (socket) => { + socket.setMaxSendFragment(recordlen); socket.on('data', (buf) => { socket.on('drain', write); write(); diff --git a/src/crypto/crypto_tls.cc b/src/crypto/crypto_tls.cc index fbc5fc8c39c9..d6d6c6416fcc 100644 --- a/src/crypto/crypto_tls.cc +++ b/src/crypto/crypto_tls.cc @@ -880,20 +880,14 @@ void TLSWrap::ClearOut() { MarkPopErrorOnReturn mark_pop_error_on_return; char out[kClearOutChunkSize]; + int length = 0; int read; - for (;;) { - { - SSLLibraryCallScope ssl_library_call_scope(this); - read = SSL_read(ssl_.get(), out, sizeof(out)); - } - Debug(this, "Read %d bytes of cleartext output", read); - - if (read <= 0) - break; + // Emits out[0, length). Returns false if JS destroyed the SSL context. + auto emit = [&]() { char* current = out; - while (read > 0) { - int avail = read; + while (length > 0) { + int avail = length; uv_buf_t buf = EmitAlloc(avail); if (static_cast(buf.len) < avail) @@ -906,12 +900,41 @@ void TLSWrap::ClearOut() { // check that ssl_ != nullptr afterwards. if (ssl_ == nullptr) { Debug(this, "Returning from read loop, ssl_ == nullptr"); - return; + return false; } - read -= avail; + length -= avail; current += avail; } + return true; + }; + + for (;;) { + { + SSLLibraryCallScope ssl_library_call_scope(this); + read = SSL_read(ssl_.get(), out + length, kClearOutChunkSize - length); + } + Debug(this, "Read %d bytes of cleartext output", read); + + if (read <= 0) break; + length += read; + + // SSL_read() returns at most one record. If the next record is already + // buffered, decrypt it into the same chunk instead of emitting one read + // per record. This is limited to application data records that are too + // long to be empty: other record types can trigger callbacks, e.g. for + // renegotiation, that have to run after the preceding data has been + // emitted, and SSL_read() skips over empty records to whatever follows. + size_t pending; + const unsigned char* next = reinterpret_cast( + NodeBIO::FromBIO(enc_in_)->Peek(&pending)); + if (length < kClearOutChunkSize && pending >= SSL3_RT_HEADER_LENGTH && + next[0] == SSL3_RT_APPLICATION_DATA && + (next[3] << 8 | next[4]) > SSL3_RT_MAX_ENCRYPTED_OVERHEAD) { + continue; + } + + if (!emit()) return; } // We need to check whether an error occurred or the connection was @@ -919,13 +942,15 @@ void TLSWrap::ClearOut() { // See node#1642 and SSL_read(3SSL) for details. SSL_get_error must be // called immediately after SSL_read, without calling into JS, which may // change OpenSSL's error queue, modify ssl_, or even destroy ssl_ - // altogether. + // altogether. Data that was read before the failure is emitted once the + // error has been captured, before it is reported. if (read <= 0) { HandleScope handle_scope(env()->isolate()); Local error; int err = SSL_get_error(ssl_.get(), read); switch (err) { case SSL_ERROR_ZERO_RETURN: + if (!emit()) return; if (!flags_.eof) { flags_.eof = true; EmitRead(UV_EOF); @@ -975,9 +1000,12 @@ void TLSWrap::ClearOut() { break; default: + emit(); return; } + if (!emit()) return; + Debug(this, "Got SSL error (%d), calling onerror", err); // When TLS Alert are stored in wbio, // it should be flushed to socket before destroyed. diff --git a/test/parallel/test-tls-coalesce-records.js b/test/parallel/test-tls-coalesce-records.js new file mode 100644 index 000000000000..f276cf14b4db --- /dev/null +++ b/test/parallel/test-tls-coalesce-records.js @@ -0,0 +1,128 @@ +'use strict'; +// TLS records that are already buffered are emitted as one chunk rather than +// one chunk per record. The end of the stream, clean or not, must still be +// reported after the data that precedes it. + +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); + +const assert = require('assert'); +const net = require('net'); +const tls = require('tls'); +const fixtures = require('../common/fixtures'); + +const RECORD_SIZE = 512; +const RECORDS = 256; +const payload = Buffer.alloc(RECORD_SIZE * RECORDS); +for (let i = 0; i < RECORDS; i++) + payload.fill(i, i * RECORD_SIZE, (i + 1) * RECORD_SIZE); + +const serverOptions = { + key: fixtures.readKey('agent1-key.pem'), + cert: fixtures.readKey('agent1-cert.pem'), +}; +const clientOptions = { + ca: fixtures.readKey('ca1-cert.pem'), + servername: 'agent1', +}; + +function sendPayload(socket) { + assert(socket.setMaxSendFragment(RECORD_SIZE)); + socket.end(payload); +} + +// Small records are coalesced, and 'end' follows all of the data. +{ + const server = tls.createServer(serverOptions, common.mustCall(sendPayload)); + server.listen(0, common.mustCall(() => { + const chunks = []; + const client = tls.connect({ ...clientOptions, port: server.address().port }); + client.on('data', (chunk) => chunks.push(chunk)); + client.on('end', common.mustCall(() => { + assert.deepStrictEqual(Buffer.concat(chunks), payload); + assert(chunks.length < RECORDS, `${chunks.length} chunks`); + assert(chunks.every((chunk) => chunk.length <= 16 * 1024)); + server.close(); + })); + })); +} + +// An onread buffer smaller than a record still receives every byte. +{ + const server = tls.createServer(serverOptions, common.mustCall(sendPayload)); + server.listen(0, common.mustCall(() => { + const chunks = []; + const client = tls.connect({ + ...clientOptions, + port: server.address().port, + onread: { + buffer: Buffer.alloc(100), + callback(nread, buf) { + chunks.push(Buffer.from(buf.subarray(0, nread))); + }, + }, + }); + client.on('end', common.mustCall(() => { + assert.deepStrictEqual(Buffer.concat(chunks), payload); + server.close(); + })); + })); +} + +// A bad record that arrives together with data is reported after the data. +{ + // Small enough to arrive in one read and to fit into one batch. + const data = payload.subarray(0, 8 * RECORD_SIZE); + const server = tls.createServer(serverOptions, common.mustCall((socket) => { + socket.once('data', common.mustCall(() => { + assert(socket.setMaxSendFragment(RECORD_SIZE)); + socket.end(data); + })); + })); + server.listen(0, common.mustCall(() => { + // Passes the handshake through, then holds the response until the server + // ends and forwards it in one write, with the last data record (the one + // before close_notify) corrupted. + let holding = false; + const held = []; + const proxy = net.createServer(common.mustCall((socket) => { + const upstream = net.connect(server.address().port); + socket.pipe(upstream); + socket.on('error', () => {}); + upstream.on('data', (chunk) => { + if (holding) + held.push(chunk); + else + socket.write(chunk); + }); + upstream.on('end', common.mustCall(() => { + const response = Buffer.concat(held); + let closeNotify = 0; + for (let i = 0; i < response.length; i += 5 + response.readUInt16BE(i + 3)) + closeNotify = i; + response[closeNotify - 1] ^= 1; + socket.end(response); + })); + })); + proxy.listen(0, common.mustCall(() => { + const chunks = []; + const client = tls.connect({ + ...clientOptions, + port: proxy.address().port, + }, common.mustCall(() => { + holding = true; + client.write('go'); + })); + client.on('data', (chunk) => chunks.push(chunk)); + client.on('error', common.mustCall((err) => { + assert.strictEqual(err.code, + 'ERR_SSL_DECRYPTION_FAILED_OR_BAD_RECORD_MAC'); + assert.deepStrictEqual(Buffer.concat(chunks), + data.subarray(0, -RECORD_SIZE)); + proxy.close(); + server.close(); + })); + })); + })); +} diff --git a/test/parallel/test-tls-coalesce-renegotiation.js b/test/parallel/test-tls-coalesce-renegotiation.js new file mode 100644 index 000000000000..1fd9326a88be --- /dev/null +++ b/test/parallel/test-tls-coalesce-renegotiation.js @@ -0,0 +1,82 @@ +'use strict'; +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); +const { isBoringSSL } = require('../common/crypto'); +if (isBoringSSL) + common.skip('BoringSSL does not support renegotiation'); + +const assert = require('assert'); +const net = require('net'); +const tls = require('tls'); +const fixtures = require('../common/fixtures'); + +// Deliver application data and a TLS 1.2 ClientHello in one transport write. +// The data handler must run before renegotiation starts, so it can disable it. +const payload = Buffer.from('disable renegotiation'); +let holding = false; +let client; + +const server = tls.createServer({ + key: fixtures.readKey('agent1-key.pem'), + cert: fixtures.readKey('agent1-cert.pem'), + maxVersion: 'TLSv1.2', +}, common.mustCall((socket) => { + socket.on('data', common.mustCall((data) => { + assert.deepStrictEqual(data, payload); + socket.disableRenegotiation(); + })); + socket.on('secure', common.mustNotCall('renegotiation completed')); + socket.on('error', common.mustCall((err) => { + assert.strictEqual(err.code, 'ERR_TLS_RENEGOTIATION_DISABLED'); + client.destroy(); + socket.destroy(); + server.close(); + proxy.close(); + })); +})); + +const proxy = net.createServer(common.mustCall((socket) => { + const upstream = net.connect(server.address().port); + upstream.pipe(socket); + socket.on('end', () => upstream.end()); + socket.on('close', () => upstream.destroy()); + const held = []; + let pending = Buffer.alloc(0); + socket.on('data', (data) => { + if (!holding) { + upstream.write(data); + return; + } + held.push(data); + pending = Buffer.concat([pending, data]); + while (pending.length >= 5) { + const length = pending.readUInt16BE(3); + if (pending.length < 5 + length) + break; + if (pending[0] === 22) { // handshake + holding = false; + upstream.write(Buffer.concat(held)); + return; + } + pending = pending.subarray(5 + length); + } + }); +})); + +server.listen(0, common.mustCall(() => { + proxy.listen(0, common.mustCall(() => { + client = tls.connect({ + port: proxy.address().port, + ca: fixtures.readKey('ca1-cert.pem'), + servername: 'agent1', + maxVersion: 'TLSv1.2', + }, common.mustCall(() => { + holding = true; + client.write(payload, common.mustCall(() => { + assert(client.renegotiate({}, + common.mustNotCall('renegotiation completed'))); + })); + })); + })); +})); diff --git a/test/parallel/test-tls-max-send-fragment.js b/test/parallel/test-tls-max-send-fragment.js index d389cf50d3d7..1dc34c53481e 100644 --- a/test/parallel/test-tls-max-send-fragment.js +++ b/test/parallel/test-tls-max-send-fragment.js @@ -29,20 +29,26 @@ if (!common.hasCrypto) common.skip('missing crypto'); const assert = require('assert'); +const net = require('net'); const tls = require('tls'); const buf = Buffer.allocUnsafe(10000); let received = 0; const maxChunk = 768; +// Largest application_data record the server sent, as seen on the wire. +let maxRecord = 0; const invalidArgumentError = { name: 'TypeError', code: 'ERR_INVALID_ARG_TYPE' }; +// TLS 1.2, so that application_data records are not confused with the +// encrypted handshake records of TLS 1.3. const server = tls.createServer({ key: fixtures.readKey('agent1-key.pem'), - cert: fixtures.readKey('agent1-cert.pem') + cert: fixtures.readKey('agent1-cert.pem'), + maxVersion: 'TLSv1.2', }, common.mustCall((c) => { // No size is passed. @@ -76,20 +82,46 @@ const server = tls.createServer({ assert(c.setMaxSendFragment(maxChunk)); c.end(buf); -})).listen(0, common.mustCall(function() { - const c = tls.connect(this.address().port, { - rejectUnauthorized: false - }, common.mustCall(function() { - c.on('data', common.mustCallAtLeast((chunk) => { - assert(chunk.length <= maxChunk); - received += chunk.length; - })); +})).listen(0, common.mustCall(() => { + // Records that arrive together are emitted as one 'data' chunk, so the + // fragment size is checked on the wire: a proxy parses the TLS record + // headers of everything the server sends. + const proxy = net.createServer(common.mustCall((socket) => { + const upstream = net.connect(server.address().port); + socket.pipe(upstream); + let pending = Buffer.alloc(0); + upstream.on('data', (data) => { + socket.write(data); + pending = Buffer.concat([pending, data]); + while (pending.length >= 5) { + const length = pending.readUInt16BE(3); + if (pending.length < 5 + length) + break; + if (pending[0] === 23) // application_data + maxRecord = Math.max(maxRecord, length); + pending = pending.subarray(5 + length); + } + }); + upstream.on('end', () => socket.end()); + })).listen(0, common.mustCall(() => { + const c = tls.connect(proxy.address().port, { + rejectUnauthorized: false + }, common.mustCall(() => { + c.on('data', common.mustCallAtLeast((chunk) => { + received += chunk.length; + })); - // Ensure that we receive 'end' event anyway - c.on('end', common.mustCall(function() { - c.destroy(); - server.close(); - assert.strictEqual(received, buf.length); + // Ensure that we receive 'end' event anyway + c.on('end', common.mustCall(() => { + c.destroy(); + proxy.close(); + server.close(); + assert.strictEqual(received, buf.length); + // A record carries at most maxChunk bytes of data plus the cipher's + // overhead (IV, MAC or tag, padding). + assert(maxRecord > 0 && maxRecord <= maxChunk + 256, + `largest record ${maxRecord}`); + })); })); })); })); From d25f6a8b113f53759b9606110264edf49de24abf Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Sun, 18 Oct 2026 09:23:59 +0200 Subject: [PATCH 2/3] test: fix TLS tests for coalesced records test-double-tls-client expected the head and the inner ClientHello in separate 'data' chunks. Now that buffered records are read together, both can arrive in one chunk, so put back whatever follows the head before handing the socket to the inner server. BoringSSL reports the corrupted record in test-tls-coalesce-records as ERR_SSL_BAD_DECRYPT. Signed-off-by: Robert Nagy Assisted-by: claude:opus-5.5 --- test/parallel/test-double-tls-client.js | 11 +++++++++-- test/parallel/test-tls-coalesce-records.js | 6 ++++-- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/test/parallel/test-double-tls-client.js b/test/parallel/test-double-tls-client.js index 32e62c6c5d21..37c7fb359f22 100644 --- a/test/parallel/test-double-tls-client.js +++ b/test/parallel/test-double-tls-client.js @@ -23,8 +23,15 @@ const server = tls.createServer({ }) .listen(client) .on('secureConnection', common.mustCall((serverTlsSock) => { - serverTlsSock.on('data', common.mustCallAtLeast((chunk) => { - assert.strictEqual(chunk.toString(), HEAD); + serverTlsSock.once('data', common.mustCall((chunk) => { + assert.strictEqual(chunk.toString('latin1', 0, HEAD.length), HEAD); + // The start of the inner handshake can arrive in the same chunk as + // HEAD. Put it back so that the inner TLSSocket, which reads whatever + // is buffered on the socket it wraps, receives it. + if (chunk.length > HEAD.length) { + serverTlsSock.pause(); + serverTlsSock.unshift(chunk.subarray(HEAD.length)); + } subserver.emit('connection', serverTlsSock); })); })); diff --git a/test/parallel/test-tls-coalesce-records.js b/test/parallel/test-tls-coalesce-records.js index f276cf14b4db..9eb6289c73ab 100644 --- a/test/parallel/test-tls-coalesce-records.js +++ b/test/parallel/test-tls-coalesce-records.js @@ -7,6 +7,7 @@ const common = require('../common'); if (!common.hasCrypto) common.skip('missing crypto'); +const { isBoringSSL } = require('../common/crypto'); const assert = require('assert'); const net = require('net'); const tls = require('tls'); @@ -116,8 +117,9 @@ function sendPayload(socket) { })); client.on('data', (chunk) => chunks.push(chunk)); client.on('error', common.mustCall((err) => { - assert.strictEqual(err.code, - 'ERR_SSL_DECRYPTION_FAILED_OR_BAD_RECORD_MAC'); + assert.strictEqual(err.code, isBoringSSL ? + 'ERR_SSL_BAD_DECRYPT' : + 'ERR_SSL_DECRYPTION_FAILED_OR_BAD_RECORD_MAC'); assert.deepStrictEqual(Buffer.concat(chunks), data.subarray(0, -RECORD_SIZE)); proxy.close(); From 6f8a340acc329e05452eb1a9bbbbee45c94d2e7d Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Mon, 19 Oct 2026 15:30:21 +0200 Subject: [PATCH 3/3] tls: retain debug output when buffered reads destroy SSL Log the captured SSL error when emitting buffered plaintext destroys the SSL context before onerror can run. Clarify that an incomplete next record can return WANT_READ and still flush the accumulated plaintext. Signed-off-by: Robert Nagy Assisted-by: Codex --- src/crypto/crypto_tls.cc | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/crypto/crypto_tls.cc b/src/crypto/crypto_tls.cc index d6d6c6416fcc..55b57a43c76e 100644 --- a/src/crypto/crypto_tls.cc +++ b/src/crypto/crypto_tls.cc @@ -925,6 +925,9 @@ void TLSWrap::ClearOut() { // long to be empty: other record types can trigger callbacks, e.g. for // renegotiation, that have to run after the preceding data has been // emitted, and SSL_read() skips over empty records to whatever follows. + // With read-ahead disabled, enc_in_ is at a record boundary after a + // successful read. The header need not be followed by a complete record: + // SSL_read() can return WANT_READ, and the path below emits the batch. size_t pending; const unsigned char* next = reinterpret_cast( NodeBIO::FromBIO(enc_in_)->Peek(&pending)); @@ -1004,7 +1007,10 @@ void TLSWrap::ClearOut() { return; } - if (!emit()) return; + if (!emit()) { + Debug(this, "Got SSL error (%d) but not calling onerror", err); + return; + } Debug(this, "Got SSL error (%d), calling onerror", err); // When TLS Alert are stored in wbio,