Skip to content

Commit 97995a0

Browse files
committed
crypto: avoid TLSWrap callbacks during GC
TLSWrap destructors can run from V8 weak callbacks. Avoid invoking queued write callbacks from that path because StreamReq::Done mutates JS-visible objects and may allocate on the V8 heap during weak callback processing. Refs: #62393 Made-with: Cursor
1 parent 34adeeb commit 97995a0

3 files changed

Lines changed: 88 additions & 7 deletions

File tree

src/crypto/crypto_tls.cc

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -438,7 +438,9 @@ TLSWrap::TLSWrap(Environment* env,
438438
}
439439

440440
TLSWrap::~TLSWrap() {
441-
Destroy();
441+
// Destructors can run from V8 weak callbacks during garbage collection.
442+
// Do not invoke JS-visible write callbacks from here.
443+
Destroy(false);
442444
}
443445

444446
MaybeLocal<ArrayBufferView> TLSWrap::ocsp_response() const {
@@ -1307,15 +1309,21 @@ void TLSWrap::DestroySSL(const FunctionCallbackInfo<Value>& args) {
13071309
Debug(wrap, "DestroySSL() finished");
13081310
}
13091311

1310-
void TLSWrap::Destroy() {
1312+
void TLSWrap::Destroy(bool invoke_queued) {
13111313
if (!ssl_)
13121314
return;
13131315

1314-
// If there is a write happening, mark it as finished.
1315-
write_callback_scheduled_ = true;
1316+
if (invoke_queued) {
1317+
// If there is a write happening, mark it as finished.
1318+
write_callback_scheduled_ = true;
13161319

1317-
// And destroy
1318-
InvokeQueued(UV_ECANCELED, "Canceled because of SSL destruction");
1320+
// And destroy
1321+
InvokeQueued(UV_ECANCELED, "Canceled because of SSL destruction");
1322+
} else {
1323+
current_write_.reset();
1324+
current_empty_write_.reset();
1325+
write_callback_scheduled_ = false;
1326+
}
13191327

13201328
env()->external_memory_accounter()->Decrease(env()->isolate(), kExternalSize);
13211329
ssl_.reset();

src/crypto/crypto_tls.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -159,7 +159,7 @@ class TLSWrap : public AsyncWrap,
159159
void EncOut(); // Write encrypted data from enc_out_ to underlying stream.
160160
void ClearIn(); // SSL_write() clear data "in" to SSL.
161161
void ClearOut(); // SSL_read() clear text "out" from SSL.
162-
void Destroy();
162+
void Destroy(bool invoke_queued = true);
163163

164164
// Call Done() on outstanding WriteWrap request.
165165
void InvokeQueued(int status, const char* error_str = nullptr);
Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,73 @@
1+
'use strict';
2+
// Flags: --expose-gc
3+
4+
// Regression coverage for TLS sockets that become collectable after a queued
5+
// write. TLSWrap destruction can run from V8 weak callbacks, so destructor-time
6+
// cleanup must not invoke JS-visible write callbacks.
7+
8+
const common = require('../common');
9+
if (!common.hasCrypto)
10+
common.skip('missing crypto');
11+
12+
const assert = require('node:assert');
13+
const fixtures = require('../common/fixtures');
14+
const { gcUntil } = require('../common/gc');
15+
const tls = require('node:tls');
16+
17+
const kClientCount = 32;
18+
const kPayload = Buffer.alloc(512 * 1024);
19+
20+
let finalized = 0;
21+
let queuedWrites = 0;
22+
23+
const registry = new FinalizationRegistry(() => {
24+
finalized++;
25+
});
26+
27+
const server = tls.createServer({
28+
key: fixtures.readKey('agent1-key.pem'),
29+
cert: fixtures.readKey('agent1-cert.pem'),
30+
}, (socket) => {
31+
socket.destroy();
32+
});
33+
34+
function queueWriteAndAbandon(port) {
35+
return new Promise((resolve, reject) => {
36+
let settled = false;
37+
let socket = tls.connect({ port, rejectUnauthorized: false });
38+
39+
socket.on('error', (err) => {
40+
if (!settled) {
41+
settled = true;
42+
reject(err);
43+
}
44+
});
45+
46+
socket.once('secureConnect', () => {
47+
socket.write(kPayload);
48+
queuedWrites++;
49+
registry.register(socket);
50+
settled = true;
51+
resolve();
52+
socket = null;
53+
});
54+
});
55+
}
56+
57+
server.listen(0, common.mustCall(async () => {
58+
try {
59+
const port = server.address().port;
60+
61+
await Promise.all(
62+
Array.from({ length: kClientCount }, () => queueWriteAndAbandon(port)));
63+
64+
assert.strictEqual(queuedWrites, kClientCount);
65+
66+
await gcUntil('abandoned TLS client socket', () => finalized > 0, 50, {
67+
type: 'major',
68+
execution: 'sync',
69+
});
70+
} finally {
71+
server.close();
72+
}
73+
}));

0 commit comments

Comments
 (0)