Skip to content

Commit c7974d9

Browse files
authored
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 <ronagy@icloud.com> Assisted-by: claude:opus-5.5 PR-URL: #66698 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
1 parent 697ecce commit c7974d9

6 files changed

Lines changed: 319 additions & 31 deletions

File tree

‎benchmark/tls/throughput-s2c.js‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ const bench = common.createBenchmark(main, {
66
sendchunklen: [256, 32 * 1024, 128 * 1024, 16 * 1024 * 1024],
77
recvbuflen: [0, 64 * 1024, 1024 * 1024],
88
recvbufgenfn: ['true', 'false'],
9+
recordlen: [1024, 4096, 16384],
910
});
1011

1112
const fixtures = require('../../test/common/fixtures');
@@ -14,7 +15,8 @@ let recvbuf;
1415
let received = 0;
1516
const tls = require('tls');
1617

17-
function main({ dur, type, sendchunklen, recvbuflen, recvbufgenfn }) {
18+
function main({ dur, type, sendchunklen, recvbuflen, recvbufgenfn,
19+
recordlen }) {
1820
if (isFinite(recvbuflen) && recvbuflen > 0)
1921
recvbuf = Buffer.alloc(recvbuflen);
2022

@@ -74,6 +76,7 @@ function main({ dur, type, sendchunklen, recvbuflen, recvbufgenfn }) {
7476
}
7577

7678
const server = tls.createServer(options, (socket) => {
79+
socket.setMaxSendFragment(recordlen);
7780
socket.on('data', (buf) => {
7881
socket.on('drain', write);
7982
write();

‎src/crypto/crypto_tls.cc‎

Lines changed: 48 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -851,20 +851,14 @@ void TLSWrap::ClearOut() {
851851
MarkPopErrorOnReturn mark_pop_error_on_return;
852852

853853
char out[kClearOutChunkSize];
854+
int length = 0;
854855
int read;
855-
for (;;) {
856-
{
857-
SSLLibraryCallScope ssl_library_call_scope(this);
858-
read = SSL_read(ssl_.get(), out, sizeof(out));
859-
}
860-
Debug(this, "Read %d bytes of cleartext output", read);
861-
862-
if (read <= 0)
863-
break;
864856

857+
// Emits out[0, length). Returns false if JS destroyed the SSL context.
858+
auto emit = [&]() {
865859
char* current = out;
866-
while (read > 0) {
867-
int avail = read;
860+
while (length > 0) {
861+
int avail = length;
868862

869863
uv_buf_t buf = EmitAlloc(avail);
870864
if (static_cast<int>(buf.len) < avail)
@@ -877,26 +871,60 @@ void TLSWrap::ClearOut() {
877871
// check that ssl_ != nullptr afterwards.
878872
if (ssl_ == nullptr) {
879873
Debug(this, "Returning from read loop, ssl_ == nullptr");
880-
return;
874+
return false;
881875
}
882876

883-
read -= avail;
877+
length -= avail;
884878
current += avail;
885879
}
880+
return true;
881+
};
882+
883+
for (;;) {
884+
{
885+
SSLLibraryCallScope ssl_library_call_scope(this);
886+
read = SSL_read(ssl_.get(), out + length, kClearOutChunkSize - length);
887+
}
888+
Debug(this, "Read %d bytes of cleartext output", read);
889+
890+
if (read <= 0) break;
891+
length += read;
892+
893+
// SSL_read() returns at most one record. If the next record is already
894+
// buffered, decrypt it into the same chunk instead of emitting one read
895+
// per record. This is limited to application data records that are too
896+
// long to be empty: other record types can trigger callbacks, e.g. for
897+
// renegotiation, that have to run after the preceding data has been
898+
// emitted, and SSL_read() skips over empty records to whatever follows.
899+
// With read-ahead disabled, enc_in_ is at a record boundary after a
900+
// successful read. The header need not be followed by a complete record:
901+
// SSL_read() can return WANT_READ, and the path below emits the batch.
902+
size_t pending;
903+
const unsigned char* next = reinterpret_cast<const unsigned char*>(
904+
NodeBIO::FromBIO(enc_in_)->Peek(&pending));
905+
if (length < kClearOutChunkSize && pending >= SSL3_RT_HEADER_LENGTH &&
906+
next[0] == SSL3_RT_APPLICATION_DATA &&
907+
(next[3] << 8 | next[4]) > SSL3_RT_MAX_ENCRYPTED_OVERHEAD) {
908+
continue;
909+
}
910+
911+
if (!emit()) return;
886912
}
887913

888914
// We need to check whether an error occurred or the connection was
889915
// shutdown cleanly (SSL_ERROR_ZERO_RETURN) even when read == 0.
890916
// See node#1642 and SSL_read(3SSL) for details. SSL_get_error must be
891917
// called immediately after SSL_read, without calling into JS, which may
892918
// change OpenSSL's error queue, modify ssl_, or even destroy ssl_
893-
// altogether.
919+
// altogether. Data that was read before the failure is emitted once the
920+
// error has been captured, before it is reported.
894921
if (read <= 0) {
895922
HandleScope handle_scope(env()->isolate());
896923
Local<Value> error;
897924
int err = SSL_get_error(ssl_.get(), read);
898925
switch (err) {
899926
case SSL_ERROR_ZERO_RETURN:
927+
if (!emit()) return;
900928
if (!flags_.eof) {
901929
flags_.eof = true;
902930
EmitRead(UV_EOF);
@@ -946,9 +974,15 @@ void TLSWrap::ClearOut() {
946974
break;
947975

948976
default:
977+
emit();
949978
return;
950979
}
951980

981+
if (!emit()) {
982+
Debug(this, "Got SSL error (%d) but not calling onerror", err);
983+
return;
984+
}
985+
952986
Debug(this, "Got SSL error (%d), calling onerror", err);
953987
// When TLS Alert are stored in wbio,
954988
// it should be flushed to socket before destroyed.

‎test/parallel/test-double-tls-client.js‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,8 +23,15 @@ const server = tls.createServer({
2323
})
2424
.listen(client)
2525
.on('secureConnection', common.mustCall((serverTlsSock) => {
26-
serverTlsSock.on('data', common.mustCallAtLeast((chunk) => {
27-
assert.strictEqual(chunk.toString(), HEAD);
26+
serverTlsSock.once('data', common.mustCall((chunk) => {
27+
assert.strictEqual(chunk.toString('latin1', 0, HEAD.length), HEAD);
28+
// The start of the inner handshake can arrive in the same chunk as
29+
// HEAD. Put it back so that the inner TLSSocket, which reads whatever
30+
// is buffered on the socket it wraps, receives it.
31+
if (chunk.length > HEAD.length) {
32+
serverTlsSock.pause();
33+
serverTlsSock.unshift(chunk.subarray(HEAD.length));
34+
}
2835
subserver.emit('connection', serverTlsSock);
2936
}));
3037
}));
Lines changed: 130 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,130 @@
1+
'use strict';
2+
// TLS records that are already buffered are emitted as one chunk rather than
3+
// one chunk per record. The end of the stream, clean or not, must still be
4+
// reported after the data that precedes it.
5+
6+
const common = require('../common');
7+
if (!common.hasCrypto)
8+
common.skip('missing crypto');
9+
10+
const { isBoringSSL } = require('../common/crypto');
11+
const assert = require('assert');
12+
const net = require('net');
13+
const tls = require('tls');
14+
const fixtures = require('../common/fixtures');
15+
16+
const RECORD_SIZE = 512;
17+
const RECORDS = 256;
18+
const payload = Buffer.alloc(RECORD_SIZE * RECORDS);
19+
for (let i = 0; i < RECORDS; i++)
20+
payload.fill(i, i * RECORD_SIZE, (i + 1) * RECORD_SIZE);
21+
22+
const serverOptions = {
23+
key: fixtures.readKey('agent1-key.pem'),
24+
cert: fixtures.readKey('agent1-cert.pem'),
25+
};
26+
const clientOptions = {
27+
ca: fixtures.readKey('ca1-cert.pem'),
28+
servername: 'agent1',
29+
};
30+
31+
function sendPayload(socket) {
32+
assert(socket.setMaxSendFragment(RECORD_SIZE));
33+
socket.end(payload);
34+
}
35+
36+
// Small records are coalesced, and 'end' follows all of the data.
37+
{
38+
const server = tls.createServer(serverOptions, common.mustCall(sendPayload));
39+
server.listen(0, common.mustCall(() => {
40+
const chunks = [];
41+
const client = tls.connect({ ...clientOptions, port: server.address().port });
42+
client.on('data', (chunk) => chunks.push(chunk));
43+
client.on('end', common.mustCall(() => {
44+
assert.deepStrictEqual(Buffer.concat(chunks), payload);
45+
assert(chunks.length < RECORDS, `${chunks.length} chunks`);
46+
assert(chunks.every((chunk) => chunk.length <= 16 * 1024));
47+
server.close();
48+
}));
49+
}));
50+
}
51+
52+
// An onread buffer smaller than a record still receives every byte.
53+
{
54+
const server = tls.createServer(serverOptions, common.mustCall(sendPayload));
55+
server.listen(0, common.mustCall(() => {
56+
const chunks = [];
57+
const client = tls.connect({
58+
...clientOptions,
59+
port: server.address().port,
60+
onread: {
61+
buffer: Buffer.alloc(100),
62+
callback(nread, buf) {
63+
chunks.push(Buffer.from(buf.subarray(0, nread)));
64+
},
65+
},
66+
});
67+
client.on('end', common.mustCall(() => {
68+
assert.deepStrictEqual(Buffer.concat(chunks), payload);
69+
server.close();
70+
}));
71+
}));
72+
}
73+
74+
// A bad record that arrives together with data is reported after the data.
75+
{
76+
// Small enough to arrive in one read and to fit into one batch.
77+
const data = payload.subarray(0, 8 * RECORD_SIZE);
78+
const server = tls.createServer(serverOptions, common.mustCall((socket) => {
79+
socket.once('data', common.mustCall(() => {
80+
assert(socket.setMaxSendFragment(RECORD_SIZE));
81+
socket.end(data);
82+
}));
83+
}));
84+
server.listen(0, common.mustCall(() => {
85+
// Passes the handshake through, then holds the response until the server
86+
// ends and forwards it in one write, with the last data record (the one
87+
// before close_notify) corrupted.
88+
let holding = false;
89+
const held = [];
90+
const proxy = net.createServer(common.mustCall((socket) => {
91+
const upstream = net.connect(server.address().port);
92+
socket.pipe(upstream);
93+
socket.on('error', () => {});
94+
upstream.on('data', (chunk) => {
95+
if (holding)
96+
held.push(chunk);
97+
else
98+
socket.write(chunk);
99+
});
100+
upstream.on('end', common.mustCall(() => {
101+
const response = Buffer.concat(held);
102+
let closeNotify = 0;
103+
for (let i = 0; i < response.length; i += 5 + response.readUInt16BE(i + 3))
104+
closeNotify = i;
105+
response[closeNotify - 1] ^= 1;
106+
socket.end(response);
107+
}));
108+
}));
109+
proxy.listen(0, common.mustCall(() => {
110+
const chunks = [];
111+
const client = tls.connect({
112+
...clientOptions,
113+
port: proxy.address().port,
114+
}, common.mustCall(() => {
115+
holding = true;
116+
client.write('go');
117+
}));
118+
client.on('data', (chunk) => chunks.push(chunk));
119+
client.on('error', common.mustCall((err) => {
120+
assert.strictEqual(err.code, isBoringSSL ?
121+
'ERR_SSL_BAD_DECRYPT' :
122+
'ERR_SSL_DECRYPTION_FAILED_OR_BAD_RECORD_MAC');
123+
assert.deepStrictEqual(Buffer.concat(chunks),
124+
data.subarray(0, -RECORD_SIZE));
125+
proxy.close();
126+
server.close();
127+
}));
128+
}));
129+
}));
130+
}
Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
1+
'use strict';
2+
const common = require('../common');
3+
if (!common.hasCrypto)
4+
common.skip('missing crypto');
5+
const { isBoringSSL } = require('../common/crypto');
6+
if (isBoringSSL)
7+
common.skip('BoringSSL does not support renegotiation');
8+
9+
const assert = require('assert');
10+
const net = require('net');
11+
const tls = require('tls');
12+
const fixtures = require('../common/fixtures');
13+
14+
// Deliver application data and a TLS 1.2 ClientHello in one transport write.
15+
// The data handler must run before renegotiation starts, so it can disable it.
16+
const payload = Buffer.from('disable renegotiation');
17+
let holding = false;
18+
let client;
19+
20+
const server = tls.createServer({
21+
key: fixtures.readKey('agent1-key.pem'),
22+
cert: fixtures.readKey('agent1-cert.pem'),
23+
maxVersion: 'TLSv1.2',
24+
}, common.mustCall((socket) => {
25+
socket.on('data', common.mustCall((data) => {
26+
assert.deepStrictEqual(data, payload);
27+
socket.disableRenegotiation();
28+
}));
29+
socket.on('secure', common.mustNotCall('renegotiation completed'));
30+
socket.on('error', common.mustCall((err) => {
31+
assert.strictEqual(err.code, 'ERR_TLS_RENEGOTIATION_DISABLED');
32+
client.destroy();
33+
socket.destroy();
34+
server.close();
35+
proxy.close();
36+
}));
37+
}));
38+
39+
const proxy = net.createServer(common.mustCall((socket) => {
40+
const upstream = net.connect(server.address().port);
41+
upstream.pipe(socket);
42+
socket.on('end', () => upstream.end());
43+
socket.on('close', () => upstream.destroy());
44+
const held = [];
45+
let pending = Buffer.alloc(0);
46+
socket.on('data', (data) => {
47+
if (!holding) {
48+
upstream.write(data);
49+
return;
50+
}
51+
held.push(data);
52+
pending = Buffer.concat([pending, data]);
53+
while (pending.length >= 5) {
54+
const length = pending.readUInt16BE(3);
55+
if (pending.length < 5 + length)
56+
break;
57+
if (pending[0] === 22) { // handshake
58+
holding = false;
59+
upstream.write(Buffer.concat(held));
60+
return;
61+
}
62+
pending = pending.subarray(5 + length);
63+
}
64+
});
65+
}));
66+
67+
server.listen(0, common.mustCall(() => {
68+
proxy.listen(0, common.mustCall(() => {
69+
client = tls.connect({
70+
port: proxy.address().port,
71+
ca: fixtures.readKey('ca1-cert.pem'),
72+
servername: 'agent1',
73+
maxVersion: 'TLSv1.2',
74+
}, common.mustCall(() => {
75+
holding = true;
76+
client.write(payload, common.mustCall(() => {
77+
assert(client.renegotiate({},
78+
common.mustNotCall('renegotiation completed')));
79+
}));
80+
}));
81+
}));
82+
}));

0 commit comments

Comments
 (0)