From 81c2d1515b8a347f5c343f9b30077ff564024dac Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Mon, 12 Oct 2026 12:39:12 +0200 Subject: [PATCH 1/6] tls: coalesce buffered records into one read SSL_read() returns at most one TLS record per call, and ClearOut() emitted every record separately: one buffer allocation and one call into JavaScript, followed by a nextTick drain, per record. Peers that send small records (4 KiB from ingress-nginx's default ssl_buffer_size, or about 1 KiB with dynamic record sizing) made consumers pay those costs once per record instead of once per read. When more input is already buffered after a record has been decrypted, decrypt the following records into the same buffer, up to the 16 KiB of a full record, and emit them as one chunk. Chunks therefore never exceed the size a single record could already produce. Larger chunks were tried and did not pack into the 64 KiB stream read slabs; the extra external memory triggered major GCs that cost more than the saved calls. An SSL_read() failure that ends such a batch is examined before the batch is emitted, because SSL_get_error() and the error queue have to be read before calling into JavaScript. It is reported after the data it follows, as before. 'data' chunks therefore no longer correspond to TLS records, which test-tls-max-send-fragment relied on to check the sender's fragment size. It now checks the record sizes on the wire instead. Assisted-by: Opus 5.5 Signed-off-by: Robert Nagy --- src/crypto/crypto_tls.cc | 170 ++++++++++++-------- src/crypto/crypto_tls.h | 5 + test/parallel/test-tls-coalesce-records.js | 124 ++++++++++++++ test/parallel/test-tls-max-send-fragment.js | 60 +++++-- 4 files changed, 282 insertions(+), 77 deletions(-) create mode 100644 test/parallel/test-tls-coalesce-records.js diff --git a/src/crypto/crypto_tls.cc b/src/crypto/crypto_tls.cc index fbc5fc8c39c9..ab549bda0763 100644 --- a/src/crypto/crypto_tls.cc +++ b/src/crypto/crypto_tls.cc @@ -879,21 +879,44 @@ void TLSWrap::ClearOut() { MarkPopErrorOnReturn mark_pop_error_on_return; + // SSL_read() returns at most one TLS record per call. Records that are + // already buffered are decrypted together, up to the size of a full record, + // and emitted as one chunk: one allocation and one call into JS for a burst + // of small records instead of one per record. char out[kClearOutChunkSize]; int read; for (;;) { - { - SSLLibraryCallScope ssl_library_call_scope(this); - read = SSL_read(ssl_.get(), out, sizeof(out)); + int length = 0; + 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; + if (length == kClearOutChunkSize || + (NodeBIO::FromBIO(enc_in_)->Length() == 0 && + SSL_pending(ssl_.get()) == 0)) { + break; + } } - Debug(this, "Read %d bytes of cleartext output", read); - if (read <= 0) + if (length == 0) break; + // An SSL_read() failure that ended this batch has to be examined before + // calling into JS (see below), and is reported after the data it follows. + HandleScope handle_scope(env()->isolate()); + int err = 0; + Local error; + const bool failed = read <= 0 && GetClearOutError(read, &err, &error); + 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) @@ -909,9 +932,14 @@ void TLSWrap::ClearOut() { return; } - read -= avail; + length -= avail; current += avail; } + + if (failed) + EmitClearOutError(err, error); + if (read <= 0) + return; } // We need to check whether an error occurred or the connection was @@ -922,70 +950,86 @@ void TLSWrap::ClearOut() { // altogether. if (read <= 0) { HandleScope handle_scope(env()->isolate()); + int err; Local error; - int err = SSL_get_error(ssl_.get(), read); - switch (err) { - case SSL_ERROR_ZERO_RETURN: - if (!flags_.eof) { - flags_.eof = true; - EmitRead(UV_EOF); - } - return; - - case SSL_ERROR_SSL: - case SSL_ERROR_SYSCALL: - { - unsigned long ssl_err = ERR_peek_error(); // NOLINT(runtime/int) - - Local context = env()->isolate()->GetCurrentContext(); - if (context.IsEmpty()) [[unlikely]] - return; - const std::string error_str = GetBIOError(); - Local message = OneByteString(env()->isolate(), error_str); - if (message.IsEmpty()) [[unlikely]] - return; - error = Exception::Error(message); - if (error.IsEmpty()) [[unlikely]] - return; - Local obj; - if (!error->ToObject(context).ToLocal(&obj)) [[unlikely]] - return; - - const char* ls = ERR_lib_error_string(ssl_err); + if (GetClearOutError(read, &err, &error)) + EmitClearOutError(err, error); + } +} + +bool TLSWrap::GetClearOutError(int read, int* err, Local* error) { + *err = SSL_get_error(ssl_.get(), read); + switch (*err) { + case SSL_ERROR_ZERO_RETURN: + return true; + + case SSL_ERROR_SSL: + case SSL_ERROR_SYSCALL: + { + unsigned long ssl_err = ERR_peek_error(); // NOLINT(runtime/int) + + Local context = env()->isolate()->GetCurrentContext(); + if (context.IsEmpty()) [[unlikely]] + return false; + const std::string error_str = GetBIOError(); + Local message = OneByteString(env()->isolate(), error_str); + if (message.IsEmpty()) [[unlikely]] + return false; + *error = Exception::Error(message); + if (error->IsEmpty()) [[unlikely]] + return false; + Local obj; + if (!(*error)->ToObject(context).ToLocal(&obj)) [[unlikely]] + return false; + + const char* ls = ERR_lib_error_string(ssl_err); #if NCRYPTO_USE_OPENSSL3_PROVIDER - const char* fs = nullptr; + const char* fs = nullptr; #else - const char* fs = ERR_func_error_string(ssl_err); + const char* fs = ERR_func_error_string(ssl_err); #endif - const char* rs = ERR_reason_error_string(ssl_err); - if (!Set(env(), obj, env()->library_string(), ls) || - !Set(env(), obj, env()->function_string(), fs) || - !Set(env(), obj, env()->reason_string(), rs, false)) return; - // SSL has no API to recover the error name from the number, so we - // transform reason strings like "this error" to "ERR_SSL_THIS_ERROR", - // which ends up being close to the original error macro name. - std::string code(rs); - // TODO(RaisinTen): Pass an appropriate execution policy when it is - // implemented in our supported compilers. - std::transform(code.begin(), code.end(), code.begin(), - [](char c) { return c == ' ' ? '_' : ToUpper(c); }); - if (!Set(env(), obj, - env()->code_string(), ("ERR_SSL_" + code).c_str())) return; + const char* rs = ERR_reason_error_string(ssl_err); + if (!Set(env(), obj, env()->library_string(), ls) || + !Set(env(), obj, env()->function_string(), fs) || + !Set(env(), obj, env()->reason_string(), rs, false)) return false; + // SSL has no API to recover the error name from the number, so we + // transform reason strings like "this error" to "ERR_SSL_THIS_ERROR", + // which ends up being close to the original error macro name. + std::string code(rs); + // TODO(RaisinTen): Pass an appropriate execution policy when it is + // implemented in our supported compilers. + std::transform(code.begin(), code.end(), code.begin(), + [](char c) { return c == ' ' ? '_' : ToUpper(c); }); + if (!Set(env(), + obj, + env()->code_string(), + ("ERR_SSL_" + code).c_str())) { + return false; } - break; + } + return true; - default: - return; + default: + return false; + } +} + +void TLSWrap::EmitClearOutError(int err, Local error) { + if (err == SSL_ERROR_ZERO_RETURN) { + if (!flags_.eof) { + flags_.eof = true; + EmitRead(UV_EOF); } + 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. - if (BIO_pending(enc_out_) != 0) - EncOut(); + Debug(this, "Got SSL error (%d), calling onerror", err); + // When TLS Alert are stored in wbio, + // it should be flushed to socket before destroyed. + if (BIO_pending(enc_out_) != 0) + EncOut(); - MakeCallback(env()->onerror_string(), 1, &error); - } + MakeCallback(env()->onerror_string(), 1, &error); } void TLSWrap::ClearIn() { diff --git a/src/crypto/crypto_tls.h b/src/crypto/crypto_tls.h index ab6024018873..b21f6b4e5b95 100644 --- a/src/crypto/crypto_tls.h +++ b/src/crypto/crypto_tls.h @@ -201,6 +201,11 @@ class TLSWrap : public AsyncWrap, void EncOut(); // Write encrypted data from enc_out_ to underlying stream. void ClearIn(); // SSL_write() clear data "in" to SSL. void ClearOut(); // SSL_read() clear text "out" from SSL. + // Reads the outcome of an SSL_read() that returned `read` <= 0. Must be + // called right after it, before calling into JS. Returns false if there + // is nothing to report. + bool GetClearOutError(int read, int* err, v8::Local* error); + void EmitClearOutError(int err, v8::Local error); void Destroy(); // Call Done() on outstanding WriteWrap request. diff --git a/test/parallel/test-tls-coalesce-records.js b/test/parallel/test-tls-coalesce-records.js new file mode 100644 index 000000000000..108ad28a480f --- /dev/null +++ b/test/parallel/test-tls-coalesce-records.js @@ -0,0 +1,124 @@ +'use strict'; +// TLS records that are already buffered are emitted as one chunk rather than +// one chunk per record. A clean or failed end of the stream that arrives with +// them 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 = 64; +const payload = Buffer.alloc(RECORD_SIZE * RECORDS); +for (let i = 0; i < RECORDS; i++) + payload.fill(i & 0xff, 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', +}; + +// Writes the payload as small records in one go, so that they reach the client +// together, followed by close_notify. +function sendPayload(socket) { + assert(socket.setMaxSendFragment(RECORD_SIZE)); + socket.end(payload); +} + +// Buffered 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 for ${RECORDS} records`); + server.close(); + })); + })); +} + +// A listener buffer smaller than a record still receives every byte. +{ + const server = tls.createServer(serverOptions, common.mustCall(sendPayload)); + server.listen(0, common.mustCall(() => { + const received = []; + const client = tls.connect({ + ...clientOptions, + port: server.address().port, + onread: { + buffer: Buffer.alloc(100), + callback(nread, buf) { + received.push(Buffer.from(buf.subarray(0, nread))); + }, + }, + }); + client.on('end', common.mustCall(() => { + assert.deepStrictEqual(Buffer.concat(received), payload); + server.close(); + })); + })); +} + +// A TLS error that arrives together with data is reported after that data. +{ + const server = tls.createServer(serverOptions, common.mustCall((socket) => { + socket.once('data', common.mustCall(() => sendPayload(socket))); + })); + server.listen(0, common.mustCall(() => { + // Forwards the handshake, then holds the response until the server ends + // and delivers it at once with its last record (the encrypted + // 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 data = Buffer.concat(held); + data[data.length - 1] ^= 1; + socket.end(data); + })); + })); + 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), payload); + proxy.close(); + server.close(); + })); + })); + })); +} 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 b3a4527f88c52a33ed69458aa9ff2d6e73eb4813 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Tue, 13 Oct 2026 09:33:15 +0200 Subject: [PATCH 2/6] tls: format buffered record coalescing Assisted-by: OpenAI Codex Signed-off-by: Robert Nagy --- src/crypto/crypto_tls.cc | 94 ++++++++++++++++++---------------------- 1 file changed, 43 insertions(+), 51 deletions(-) diff --git a/src/crypto/crypto_tls.cc b/src/crypto/crypto_tls.cc index ab549bda0763..dfababc93154 100644 --- a/src/crypto/crypto_tls.cc +++ b/src/crypto/crypto_tls.cc @@ -894,8 +894,7 @@ void TLSWrap::ClearOut() { } Debug(this, "Read %d bytes of cleartext output", read); - if (read <= 0) - break; + if (read <= 0) break; length += read; if (length == kClearOutChunkSize || (NodeBIO::FromBIO(enc_in_)->Length() == 0 && @@ -904,8 +903,7 @@ void TLSWrap::ClearOut() { } } - if (length == 0) - break; + if (length == 0) break; // An SSL_read() failure that ended this batch has to be examined before // calling into JS (see below), and is reported after the data it follows. @@ -936,10 +934,8 @@ void TLSWrap::ClearOut() { current += avail; } - if (failed) - EmitClearOutError(err, error); - if (read <= 0) - return; + if (failed) EmitClearOutError(err, error); + if (read <= 0) return; } // We need to check whether an error occurred or the connection was @@ -952,8 +948,7 @@ void TLSWrap::ClearOut() { HandleScope handle_scope(env()->isolate()); int err; Local error; - if (GetClearOutError(read, &err, &error)) - EmitClearOutError(err, error); + if (GetClearOutError(read, &err, &error)) EmitClearOutError(err, error); } } @@ -964,49 +959,47 @@ bool TLSWrap::GetClearOutError(int read, int* err, Local* error) { return true; case SSL_ERROR_SSL: - case SSL_ERROR_SYSCALL: - { - unsigned long ssl_err = ERR_peek_error(); // NOLINT(runtime/int) - - Local context = env()->isolate()->GetCurrentContext(); - if (context.IsEmpty()) [[unlikely]] - return false; - const std::string error_str = GetBIOError(); - Local message = OneByteString(env()->isolate(), error_str); - if (message.IsEmpty()) [[unlikely]] - return false; - *error = Exception::Error(message); - if (error->IsEmpty()) [[unlikely]] - return false; - Local obj; - if (!(*error)->ToObject(context).ToLocal(&obj)) [[unlikely]] - return false; - - const char* ls = ERR_lib_error_string(ssl_err); + case SSL_ERROR_SYSCALL: { + unsigned long ssl_err = ERR_peek_error(); // NOLINT(runtime/int) + + Local context = env()->isolate()->GetCurrentContext(); + if (context.IsEmpty()) [[unlikely]] + return false; + const std::string error_str = GetBIOError(); + Local message = OneByteString(env()->isolate(), error_str); + if (message.IsEmpty()) [[unlikely]] + return false; + *error = Exception::Error(message); + if (error->IsEmpty()) [[unlikely]] + return false; + Local obj; + if (!(*error)->ToObject(context).ToLocal(&obj)) [[unlikely]] + return false; + + const char* ls = ERR_lib_error_string(ssl_err); #if NCRYPTO_USE_OPENSSL3_PROVIDER - const char* fs = nullptr; + const char* fs = nullptr; #else - const char* fs = ERR_func_error_string(ssl_err); + const char* fs = ERR_func_error_string(ssl_err); #endif - const char* rs = ERR_reason_error_string(ssl_err); - if (!Set(env(), obj, env()->library_string(), ls) || - !Set(env(), obj, env()->function_string(), fs) || - !Set(env(), obj, env()->reason_string(), rs, false)) return false; - // SSL has no API to recover the error name from the number, so we - // transform reason strings like "this error" to "ERR_SSL_THIS_ERROR", - // which ends up being close to the original error macro name. - std::string code(rs); - // TODO(RaisinTen): Pass an appropriate execution policy when it is - // implemented in our supported compilers. - std::transform(code.begin(), code.end(), code.begin(), - [](char c) { return c == ' ' ? '_' : ToUpper(c); }); - if (!Set(env(), - obj, - env()->code_string(), - ("ERR_SSL_" + code).c_str())) { - return false; - } + const char* rs = ERR_reason_error_string(ssl_err); + if (!Set(env(), obj, env()->library_string(), ls) || + !Set(env(), obj, env()->function_string(), fs) || + !Set(env(), obj, env()->reason_string(), rs, false)) + return false; + // SSL has no API to recover the error name from the number, so we + // transform reason strings like "this error" to "ERR_SSL_THIS_ERROR", + // which ends up being close to the original error macro name. + std::string code(rs); + // TODO(RaisinTen): Pass an appropriate execution policy when it is + // implemented in our supported compilers. + std::transform(code.begin(), code.end(), code.begin(), [](char c) { + return c == ' ' ? '_' : ToUpper(c); + }); + if (!Set(env(), obj, env()->code_string(), ("ERR_SSL_" + code).c_str())) { + return false; } + } return true; default: @@ -1026,8 +1019,7 @@ void TLSWrap::EmitClearOutError(int err, Local error) { Debug(this, "Got SSL error (%d), calling onerror", err); // When TLS Alert are stored in wbio, // it should be flushed to socket before destroyed. - if (BIO_pending(enc_out_) != 0) - EncOut(); + if (BIO_pending(enc_out_) != 0) EncOut(); MakeCallback(env()->onerror_string(), 1, &error); } From 2a9cd91d5c478030ad435c10788959f9c9eb39bd Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Tue, 13 Oct 2026 09:38:51 +0200 Subject: [PATCH 3/6] tls: preserve data ordering before renegotiation Flush buffered application data before reading a visible protocol record. This lets a preceding data handler disable TLS 1.2 renegotiation before the following ClientHello is processed. Assisted-by: OpenAI Codex Signed-off-by: Robert Nagy --- src/crypto/crypto_tls.cc | 14 +++- .../test-tls-coalesce-renegotiation.js | 82 +++++++++++++++++++ 2 files changed, 92 insertions(+), 4 deletions(-) create mode 100644 test/parallel/test-tls-coalesce-renegotiation.js diff --git a/src/crypto/crypto_tls.cc b/src/crypto/crypto_tls.cc index dfababc93154..c87f2ae41963 100644 --- a/src/crypto/crypto_tls.cc +++ b/src/crypto/crypto_tls.cc @@ -896,10 +896,16 @@ void TLSWrap::ClearOut() { if (read <= 0) break; length += read; - if (length == kClearOutChunkSize || - (NodeBIO::FromBIO(enc_in_)->Length() == 0 && - SSL_pending(ssl_.get()) == 0)) { - break; + if (length == kClearOutChunkSize) break; + if (SSL_pending(ssl_.get()) == 0) { + size_t pending; + const char* next = NodeBIO::FromBIO(enc_in_)->Peek(&pending); + // Protocol records can invoke JS callbacks, such as TLS 1.2 + // renegotiation. Deliver preceding data before processing them. + if (pending == 0 || + static_cast(next[0]) != SSL3_RT_APPLICATION_DATA) { + break; + } } } 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'))); + })); + })); + })); +})); From 80c17d24bd6d9f9ad0713ecfbb05018a09ec74a2 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Tue, 13 Oct 2026 10:42:25 +0200 Subject: [PATCH 4/6] tls: preserve protocol callback order and use 64 KiB reads Flush preceding cleartext before session, keylog and handshake callbacks. Extend the TLS throughput benchmark with record-size and protocol selectors, and cover coalesced TLS 1.3 KeyUpdate ordering. Assisted-by: OpenAI Codex Signed-off-by: Robert Nagy --- benchmark/tls/throughput-s2c.js | 9 +- src/crypto/crypto_tls.cc | 55 ++++++----- src/crypto/crypto_tls.h | 7 +- test/parallel/test-tls-coalesce-keyupdate.js | 96 ++++++++++++++++++++ test/parallel/test-tls-coalesce-records.js | 3 +- 5 files changed, 144 insertions(+), 26 deletions(-) create mode 100644 test/parallel/test-tls-coalesce-keyupdate.js diff --git a/benchmark/tls/throughput-s2c.js b/benchmark/tls/throughput-s2c.js index 7fb93c304b20..8a0b19359527 100644 --- a/benchmark/tls/throughput-s2c.js +++ b/benchmark/tls/throughput-s2c.js @@ -6,6 +6,8 @@ const bench = common.createBenchmark(main, { sendchunklen: [256, 32 * 1024, 128 * 1024, 16 * 1024 * 1024], recvbuflen: [0, 64 * 1024, 1024 * 1024], recvbufgenfn: ['true', 'false'], + fragment: [1024, 4096, 16384], + tlsVersion: ['TLSv1.2', 'TLSv1.3'], }); const fixtures = require('../../test/common/fixtures'); @@ -14,7 +16,8 @@ let recvbuf; let received = 0; const tls = require('tls'); -function main({ dur, type, sendchunklen, recvbuflen, recvbufgenfn }) { +function main({ dur, type, sendchunklen, recvbuflen, recvbufgenfn, + fragment, tlsVersion }) { if (isFinite(recvbuflen) && recvbuflen > 0) recvbuf = Buffer.alloc(recvbuflen); @@ -41,7 +44,8 @@ function main({ dur, type, sendchunklen, recvbuflen, recvbufgenfn }) { cert: fixtures.readKey('rsa_cert.crt'), ca: fixtures.readKey('rsa_ca.crt'), ciphers: 'AES256-GCM-SHA384', - maxVersion: 'TLSv1.2', + minVersion: tlsVersion, + maxVersion: tlsVersion, }; let socketOpts; @@ -74,6 +78,7 @@ function main({ dur, type, sendchunklen, recvbuflen, recvbufgenfn }) { } const server = tls.createServer(options, (socket) => { + socket.setMaxSendFragment(fragment); socket.on('data', (buf) => { socket.on('drain', write); write(); diff --git a/src/crypto/crypto_tls.cc b/src/crypto/crypto_tls.cc index c87f2ae41963..17ef39d091eb 100644 --- a/src/crypto/crypto_tls.cc +++ b/src/crypto/crypto_tls.cc @@ -150,6 +150,7 @@ ClientHelloResult EarlyClientHelloCallback(const ClientHelloContext& hello) { void KeylogCallback(const SSL* s, const char* line) { TLSWrap* w = static_cast(SSL_get_app_data(s)); + w->FlushPendingClearOut(); Environment* env = w->env(); HandleScope handle_scope(env->isolate()); Context::Scope context_scope(env->context()); @@ -173,6 +174,7 @@ int NewSessionCallback(SSL* s, SSL_SESSION* sess) { if (!w->has_session_callbacks()) [[unlikely]] return 0; + w->FlushPendingClearOut(); // Check if session is small enough to be stored int size = i2d_SSL_SESSION(sess, nullptr); @@ -689,6 +691,7 @@ void TLSWrap::SSLInfoCallback(const SSL* ssl_, int where, int ret) { // SSL_renegotiate_pending() should take `const SSL*`, but it does not. SSL* ssl = const_cast(ssl_); TLSWrap* c = static_cast(SSL_get_app_data(ssl_)); + c->FlushPendingClearOut(); Environment* env = c->env(); HandleScope handle_scope(env->isolate()); Context::Scope context_scope(env->context()); @@ -880,7 +883,7 @@ void TLSWrap::ClearOut() { MarkPopErrorOnReturn mark_pop_error_on_return; // SSL_read() returns at most one TLS record per call. Records that are - // already buffered are decrypted together, up to the size of a full record, + // already buffered are decrypted together, up to 64 KiB, // and emitted as one chunk: one allocation and one call into JS for a burst // of small records instead of one per record. char out[kClearOutChunkSize]; @@ -888,10 +891,18 @@ void TLSWrap::ClearOut() { for (;;) { int length = 0; for (;;) { + pending_cleartext_output_ = {out, static_cast(length)}; { SSLLibraryCallScope ssl_library_call_scope(this); read = SSL_read(ssl_.get(), out + length, kClearOutChunkSize - length); } + // A protocol callback may have delivered the preceding bytes. The + // current read still wrote after that prefix, so move its output back. + if (length > 0 && pending_cleartext_output_.empty()) { + if (read > 0) memmove(out, out + length, read); + length = 0; + } + pending_cleartext_output_ = {}; Debug(this, "Read %d bytes of cleartext output", read); if (read <= 0) break; @@ -918,27 +929,7 @@ void TLSWrap::ClearOut() { Local error; const bool failed = read <= 0 && GetClearOutError(read, &err, &error); - char* current = out; - while (length > 0) { - int avail = length; - - uv_buf_t buf = EmitAlloc(avail); - if (static_cast(buf.len) < avail) - avail = buf.len; - memcpy(buf.base, current, avail); - EmitRead(avail, buf); - - // Caveat emptor: OnRead() calls into JS land which can result in - // the SSL context object being destroyed. We have to carefully - // check that ssl_ != nullptr afterwards. - if (ssl_ == nullptr) { - Debug(this, "Returning from read loop, ssl_ == nullptr"); - return; - } - - length -= avail; - current += avail; - } + if (!EmitClearOut({out, static_cast(length)})) return; if (failed) EmitClearOutError(err, error); if (read <= 0) return; @@ -958,6 +949,26 @@ void TLSWrap::ClearOut() { } } +bool TLSWrap::EmitClearOut(std::span output) { + while (!output.empty()) { + int avail = output.size(); + uv_buf_t buf = EmitAlloc(avail); + if (static_cast(buf.len) < avail) avail = buf.len; + memcpy(buf.base, output.data(), avail); + EmitRead(avail, buf); + // OnRead() calls into JS, which can destroy the SSL context. + if (ssl_ == nullptr) return false; + output = output.subspan(avail); + } + return true; +} + +void TLSWrap::FlushPendingClearOut() { + const auto output = pending_cleartext_output_; + pending_cleartext_output_ = {}; + EmitClearOut(output); +} + bool TLSWrap::GetClearOutError(int read, int* err, Local* error) { *err = SSL_get_error(ssl_.get(), read); switch (*err) { diff --git a/src/crypto/crypto_tls.h b/src/crypto/crypto_tls.h index b21f6b4e5b95..ae0b6d08fba5 100644 --- a/src/crypto/crypto_tls.h +++ b/src/crypto/crypto_tls.h @@ -32,6 +32,7 @@ #include +#include #include #include @@ -82,6 +83,7 @@ class TLSWrap : public AsyncWrap, inline bool is_cert_cb_running() const { return flags_.cert_cb_running; } inline bool is_waiting_cert_cb() const { return cert_cb_ != nullptr; } inline bool has_session_callbacks() const { return flags_.session_callbacks; } + void FlushPendingClearOut(); // We need to suspend the ClientHello only for server session id // callbacks, and only on the first pass. inline bool should_suspend_for_client_hello() const { @@ -162,7 +164,7 @@ class TLSWrap : public AsyncWrap, // NOTE: Actually it is much more than this static constexpr int64_t kExternalSize = 6224 + 1040 + 42 * 1024; - static constexpr int kClearOutChunkSize = 16384; + static constexpr int kClearOutChunkSize = 64 * 1024; // Usual ServerHello + Certificate size static constexpr int kInitialClientBufferLength = 4096; @@ -201,6 +203,7 @@ class TLSWrap : public AsyncWrap, void EncOut(); // Write encrypted data from enc_out_ to underlying stream. void ClearIn(); // SSL_write() clear data "in" to SSL. void ClearOut(); // SSL_read() clear text "out" from SSL. + bool EmitClearOut(std::span output); // Reads the outcome of an SSL_read() that returned `read` <= 0. Must be // called right after it, before calling into JS. Returns false if there // is nothing to report. @@ -325,6 +328,8 @@ class TLSWrap : public AsyncWrap, BIO* enc_out_ = nullptr; // SSL_write()/handshake fills this for EncOut(). // Waiting for ClearIn() to pass to SSL_write(). std::unique_ptr pending_cleartext_input_; + // References ClearOut's stack buffer only while SSL_read() is on the stack. + std::span pending_cleartext_output_; size_t write_size_ = 0; BaseObjectPtr current_write_; BaseObjectPtr current_empty_write_; diff --git a/test/parallel/test-tls-coalesce-keyupdate.js b/test/parallel/test-tls-coalesce-keyupdate.js new file mode 100644 index 000000000000..ff7e7dc9adc8 --- /dev/null +++ b/test/parallel/test-tls-coalesce-keyupdate.js @@ -0,0 +1,96 @@ +'use strict'; +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); +const { opensslCli } = require('../common/crypto'); +if (!opensslCli) + common.skip('missing OpenSSL CLI'); + +const assert = require('assert'); +const { spawn } = require('child_process'); +const net = require('net'); +const tls = require('tls'); +const fixtures = require('../common/fixtures'); + +// TLS 1.3 control records have the same outer type as application records. +// Deliver data, KeyUpdate and more data together, and check callback ordering. +const before = 'before-key-update\n'; +const after = 'after-key-update\n'; +let received = ''; +let holding = false; +let client; + +const onUpdate = common.mustCall(() => { + assert.strictEqual(received, before); +}); +const server = tls.createServer({ + key: fixtures.readKey('agent1-key.pem'), + cert: fixtures.readKey('agent1-cert.pem'), + minVersion: 'TLSv1.3', +}, common.mustCall((socket) => { + holding = true; + socket.on('data', common.mustCallAtLeast((data) => { + received += data; + if (received === before) socket.write('ack'); + if (received === before + after) { + socket.end(); + server.close(); + proxy.close(); + } + })); + socket.on('end', common.mustCall(() => { + assert.strictEqual(received, before + after); + })); + socket.on('error', common.mustNotCall()); + client.stdin.write(before); +})); +server.on('keylog', (line) => { + if (/^CLIENT_TRAFFIC_SECRET_(?:N|1) /.test(line.toString())) onUpdate(); +}); + +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); + let records = 0; + socket.on('data', common.mustCallAtLeast((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; + assert.strictEqual(pending[0], 23); + pending = pending.subarray(5 + length); + records++; + if (records === 1) client.stdin.write('k\n'); + if (records === 2) client.stdin.write(after); + if (records === 3) { + holding = false; + upstream.write(Buffer.concat(held)); + return; + } + } + })); +})); + +server.listen(0, common.mustCall(() => { + proxy.listen(0, common.mustCall(() => { + client = spawn(opensslCli, [ + 's_client', '-tls1_3', '-connect', `127.0.0.1:${proxy.address().port}`, + '-CAfile', fixtures.path('keys', 'ca1-cert.pem'), '-servername', 'agent1', + ]); + client.stdout.resume(); + client.stderr.resume(); + client.on('error', common.mustNotCall()); + client.on('exit', common.mustCall((code) => { + assert.strictEqual(code, 0); + })); + })); +})); diff --git a/test/parallel/test-tls-coalesce-records.js b/test/parallel/test-tls-coalesce-records.js index 108ad28a480f..eb8b49b87c09 100644 --- a/test/parallel/test-tls-coalesce-records.js +++ b/test/parallel/test-tls-coalesce-records.js @@ -13,7 +13,7 @@ const tls = require('tls'); const fixtures = require('../common/fixtures'); const RECORD_SIZE = 512; -const RECORDS = 64; +const RECORDS = 256; const payload = Buffer.alloc(RECORD_SIZE * RECORDS); for (let i = 0; i < RECORDS; i++) payload.fill(i & 0xff, i * RECORD_SIZE, (i + 1) * RECORD_SIZE); @@ -48,6 +48,7 @@ function sendPayload(socket) { assert.deepStrictEqual(Buffer.concat(chunks), payload); assert(chunks.length < RECORDS, `${chunks.length} chunks for ${RECORDS} records`); + assert(chunks.every((chunk) => chunk.length <= 64 * 1024)); server.close(); })); })); From 802b1a89b5fa29ba2f88d9d7889f881f7512dbff Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Tue, 13 Oct 2026 10:48:46 +0200 Subject: [PATCH 5/6] test: preserve the proxy half-close for TLS shutdown Signed-off-by: Robert Nagy --- test/parallel/test-tls-coalesce-keyupdate.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/test/parallel/test-tls-coalesce-keyupdate.js b/test/parallel/test-tls-coalesce-keyupdate.js index ff7e7dc9adc8..240e8bcbf258 100644 --- a/test/parallel/test-tls-coalesce-keyupdate.js +++ b/test/parallel/test-tls-coalesce-keyupdate.js @@ -49,7 +49,10 @@ server.on('keylog', (line) => { }); const proxy = net.createServer(common.mustCall((socket) => { - const upstream = net.connect(server.address().port); + const upstream = net.connect({ + port: server.address().port, + allowHalfOpen: true, + }); upstream.pipe(socket); socket.on('end', () => upstream.end()); socket.on('close', () => upstream.destroy()); From f09b719c804aac271d8ee82f0332316ad647055f Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Tue, 13 Oct 2026 10:59:45 +0200 Subject: [PATCH 6/6] tls: defer destruction until the SSL library returns Signed-off-by: Robert Nagy --- src/crypto/crypto_tls.cc | 32 ++++++++++++++------ src/crypto/crypto_tls.h | 3 +- test/parallel/test-tls-coalesce-keyupdate.js | 30 +++++++++++++++--- 3 files changed, 50 insertions(+), 15 deletions(-) diff --git a/src/crypto/crypto_tls.cc b/src/crypto/crypto_tls.cc index 17ef39d091eb..dc5d58660480 100644 --- a/src/crypto/crypto_tls.cc +++ b/src/crypto/crypto_tls.cc @@ -150,7 +150,7 @@ ClientHelloResult EarlyClientHelloCallback(const ClientHelloContext& hello) { void KeylogCallback(const SSL* s, const char* line) { TLSWrap* w = static_cast(SSL_get_app_data(s)); - w->FlushPendingClearOut(); + if (!w->FlushPendingClearOut()) return; Environment* env = w->env(); HandleScope handle_scope(env->isolate()); Context::Scope context_scope(env->context()); @@ -174,7 +174,7 @@ int NewSessionCallback(SSL* s, SSL_SESSION* sess) { if (!w->has_session_callbacks()) [[unlikely]] return 0; - w->FlushPendingClearOut(); + if (!w->FlushPendingClearOut()) return 0; // Check if session is small enough to be stored int size = i2d_SSL_SESSION(sess, nullptr); @@ -691,7 +691,7 @@ void TLSWrap::SSLInfoCallback(const SSL* ssl_, int where, int ret) { // SSL_renegotiate_pending() should take `const SSL*`, but it does not. SSL* ssl = const_cast(ssl_); TLSWrap* c = static_cast(SSL_get_app_data(ssl_)); - c->FlushPendingClearOut(); + if (!c->FlushPendingClearOut()) return; Environment* env = c->env(); HandleScope handle_scope(env->isolate()); Context::Scope context_scope(env->context()); @@ -728,6 +728,7 @@ void TLSWrap::SSLInfoCallback(const SSL* ssl_, int where, int ret) { } void TLSWrap::EncOut() { + if (destroy_pending_) return; Debug(this, "Trying to write encrypted output"); // Write in progress @@ -875,8 +876,8 @@ void TLSWrap::ClearOut() { return; } - if (ssl_ == nullptr) { - Debug(this, "Returning from ClearOut(), ssl_ == nullptr"); + if (ssl_ == nullptr || destroy_pending_) { + Debug(this, "Returning from ClearOut(), no usable SSL context"); return; } @@ -896,6 +897,10 @@ void TLSWrap::ClearOut() { SSLLibraryCallScope ssl_library_call_scope(this); read = SSL_read(ssl_.get(), out + length, kClearOutChunkSize - length); } + if (destroy_pending_) { + pending_cleartext_output_ = {}; + return; + } // A protocol callback may have delivered the preceding bytes. The // current read still wrote after that prefix, so move its output back. if (length > 0 && pending_cleartext_output_.empty()) { @@ -957,16 +962,16 @@ bool TLSWrap::EmitClearOut(std::span output) { memcpy(buf.base, output.data(), avail); EmitRead(avail, buf); // OnRead() calls into JS, which can destroy the SSL context. - if (ssl_ == nullptr) return false; + if (ssl_ == nullptr || destroy_pending_) return false; output = output.subspan(avail); } return true; } -void TLSWrap::FlushPendingClearOut() { +bool TLSWrap::FlushPendingClearOut() { const auto output = pending_cleartext_output_; pending_cleartext_output_ = {}; - EmitClearOut(output); + return !destroy_pending_ && EmitClearOut(output); } bool TLSWrap::GetClearOutError(int read, int* err, Local* error) { @@ -1044,7 +1049,7 @@ void TLSWrap::EmitClearOutError(int err, Local error) { void TLSWrap::ClearIn() { Debug(this, "Trying to write cleartext input"); - if (ssl_ == nullptr) { + if (ssl_ == nullptr || destroy_pending_) { Debug(this, "Returning from ClearIn(), ssl_ == nullptr"); return; } @@ -1493,6 +1498,15 @@ void TLSWrap::DestroySSL(const FunctionCallbackInfo& args) { } void TLSWrap::Destroy() { + if (in_ssl_library_call()) { + if (!destroy_pending_) { + destroy_pending_ = true; + BaseObjectPtr strong_ref{this}; + env()->SetImmediate([this, strong_ref](Environment* env) { Destroy(); }); + } + return; + } + destroy_pending_ = false; if (!ssl_) return; diff --git a/src/crypto/crypto_tls.h b/src/crypto/crypto_tls.h index ae0b6d08fba5..fc143f5f94df 100644 --- a/src/crypto/crypto_tls.h +++ b/src/crypto/crypto_tls.h @@ -83,7 +83,7 @@ class TLSWrap : public AsyncWrap, inline bool is_cert_cb_running() const { return flags_.cert_cb_running; } inline bool is_waiting_cert_cb() const { return cert_cb_ != nullptr; } inline bool has_session_callbacks() const { return flags_.session_callbacks; } - void FlushPendingClearOut(); + bool FlushPendingClearOut(); // We need to suspend the ClientHello only for server session id // callbacks, and only on the first pass. inline bool should_suspend_for_client_hello() const { @@ -330,6 +330,7 @@ class TLSWrap : public AsyncWrap, std::unique_ptr pending_cleartext_input_; // References ClearOut's stack buffer only while SSL_read() is on the stack. std::span pending_cleartext_output_; + bool destroy_pending_ = false; size_t write_size_ = 0; BaseObjectPtr current_write_; BaseObjectPtr current_empty_write_; diff --git a/test/parallel/test-tls-coalesce-keyupdate.js b/test/parallel/test-tls-coalesce-keyupdate.js index 240e8bcbf258..b997419b2070 100644 --- a/test/parallel/test-tls-coalesce-keyupdate.js +++ b/test/parallel/test-tls-coalesce-keyupdate.js @@ -11,6 +11,13 @@ const { spawn } = require('child_process'); const net = require('net'); const tls = require('tls'); const fixtures = require('../common/fixtures'); +const destroy = process.argv[2] === 'destroy'; +if (!destroy) { + const child = spawn(process.execPath, [__filename, 'destroy'], { + stdio: 'inherit', + }); + child.on('exit', common.mustCall((code) => assert.strictEqual(code, 0))); +} // TLS 1.3 control records have the same outer type as application records. // Deliver data, KeyUpdate and more data together, and check callback ordering. @@ -20,7 +27,7 @@ let received = ''; let holding = false; let client; -const onUpdate = common.mustCall(() => { +const onUpdate = destroy ? common.mustNotCall() : common.mustCall(() => { assert.strictEqual(received, before); }); const server = tls.createServer({ @@ -31,15 +38,24 @@ const server = tls.createServer({ holding = true; socket.on('data', common.mustCallAtLeast((data) => { received += data; - if (received === before) socket.write('ack'); + if (received === before) { + if (destroy) { + socket._destroySSL(); + socket.destroy(); + server.close(); + proxy.close(); + return; + } + socket.write('ack'); + } if (received === before + after) { socket.end(); server.close(); proxy.close(); } })); - socket.on('end', common.mustCall(() => { - assert.strictEqual(received, before + after); + socket.on('close', common.mustCall(() => { + assert.strictEqual(received, destroy ? before : before + after); })); socket.on('error', common.mustNotCall()); client.stdin.write(before); @@ -93,7 +109,11 @@ server.listen(0, common.mustCall(() => { client.stderr.resume(); client.on('error', common.mustNotCall()); client.on('exit', common.mustCall((code) => { - assert.strictEqual(code, 0); + if (destroy) { + assert(code === 0 || code === 1); + } else { + assert.strictEqual(code, 0); + } })); })); }));