Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/bun.js/api/bun/socket/tls_socket_functions.zig
Original file line number Diff line number Diff line change
Expand Up @@ -119,8 +119,11 @@ pub fn getPeerCertificate(this: *This, globalObject: *jsc.JSGlobalObject, callfr

if (abbreviated) {
if (this.isServer()) {
// SSL_get_peer_certificate returns a +1 reference; we must free it.
// X509.toJS only borrows the pointer (X509View is non-owning).
const cert = BoringSSL.SSL_get_peer_certificate(ssl_ptr);
if (cert) |x509| {
defer x509.free();
return X509.toJS(x509, globalObject);
}
}
Expand All @@ -131,8 +134,10 @@ pub fn getPeerCertificate(this: *This, globalObject: *jsc.JSGlobalObject, callfr
}
var cert: ?*BoringSSL.X509 = null;
if (this.isServer()) {
// SSL_get_peer_certificate returns a +1 reference; we must free it.
cert = BoringSSL.SSL_get_peer_certificate(ssl_ptr);
}
defer if (cert) |c| c.free();

const cert_chain = BoringSSL.SSL_get_peer_cert_chain(ssl_ptr);
const first_cert = if (cert) |c| c else if (cert_chain) |cc| BoringSSL.sk_X509_value(cc, 0) else null;
Expand Down
3 changes: 1 addition & 2 deletions src/bun.js/api/server.zig
Original file line number Diff line number Diff line change
Expand Up @@ -1766,7 +1766,7 @@ pub fn NewServer(protocol_enum: enum { http, https }, development_kind: enum { d
.config = config.*,
.base_url_string_for_joining = base_url,
.vm = jsc.VirtualMachine.get(),
.allocator = Arena.getThreadLocalDefault(),
.allocator = bun.default_allocator,
.dev_server = dev_server,
});

Expand Down Expand Up @@ -3787,7 +3787,6 @@ const js_printer = bun.js_printer;
const logger = bun.logger;
const strings = bun.strings;
const uws = bun.uws;
const Arena = bun.allocators.MimallocArena;
const BoringSSL = bun.BoringSSL.c;
const SocketAddress = bun.api.socket.SocketAddress;

Expand Down
10 changes: 7 additions & 3 deletions src/bun.js/bindings/JSX509Certificate.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -566,12 +566,16 @@ JSUint8Array* JSX509Certificate::computeRaw(ncrypto::X509View view, JSGlobalObje
return nullptr;
}

auto bio_ptr = bio.release();
BIO* bio_ptr = bio.release();
BUF_MEM* bptr = nullptr;
BIO_get_mem_ptr(bio_ptr, &bptr);

Ref<JSC::ArrayBuffer> buffer = JSC::ArrayBuffer::createFromBytes(std::span(reinterpret_cast<uint8_t*>(bptr->data), bptr->length), createSharedTask<void(void*)>([](void* data) {
ncrypto::BIOPointer free_me(static_cast<BIO*>(data));
// The ArrayBuffer aliases the BIO's internal buffer; free the BIO (which
// owns the buffer) when the ArrayBuffer is destroyed. The destructor is
// invoked with the data pointer (bptr->data), not the BIO, so capture
// bio_ptr explicitly.
Ref<JSC::ArrayBuffer> buffer = JSC::ArrayBuffer::createFromBytes(std::span(reinterpret_cast<uint8_t*>(bptr->data), bptr->length), createSharedTask<void(void*)>([bio_ptr](void*) {
ncrypto::BIOPointer free_me(bio_ptr);
}));
RELEASE_AND_RETURN(scope, Bun::createBuffer(globalObject, WTF::move(buffer)));
}
Expand Down
69 changes: 68 additions & 1 deletion test/js/node/tls/node-tls-cert.test.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { describe, expect, it } from "bun:test";
import { once } from "events";
import { readFileSync } from "fs";
import { bunEnv, bunExe, invalidTls, tmpdirSync } from "harness";
import { bunEnv, bunExe, invalidTls, isASAN, isDebug, tmpdirSync } from "harness";
import type { AddressInfo } from "node:net";
import type { Server, TLSSocket } from "node:tls";
import { join } from "path";
Expand Down Expand Up @@ -609,3 +609,70 @@ describe("tls ciphers should work", () => {
);
});
});

it("server-side getPeerCertificate() should not leak", async () => {
// Guards against the SSL_get_peer_certificate X509 ref leak and the
// computeRaw BIO leak on the server getPeerCertificate() path.
const { promise: serverSocketPromise, resolve: onServerSocket } = Promise.withResolvers<TLSSocket>();
const server = tls.createServer(
{
key: serverTls.key,
cert: serverTls.cert,
ca: [clientTls.ca],
requestCert: true,
rejectUnauthorized: false,
},
socket => onServerSocket(socket),
);
await once(server.listen(0, "127.0.0.1"), "listening");

const client = tls.connect({
host: "127.0.0.1",
port: (server.address() as AddressInfo).port,
key: clientTls.key,
cert: clientTls.cert,
ca: [serverTls.ca],
checkServerIdentity,
});
await once(client, "secureConnect");

const serverSocket = await serverSocketPromise;
try {
Comment on lines +617 to +640

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Move lifecycle cleanup to cover setup failures too.

finally starts after await serverSocketPromise (Line 639), so failures during listen/secureConnect can bypass cleanup and leave open handles.

Proposed fix
 it("server-side getPeerCertificate() should not leak", async () => {
+  let server: tls.Server | null = null;
+  let client: TLSSocket | null = null;
+  let serverSocket: TLSSocket | null = null;
+  const { promise: serverSocketPromise, resolve: onServerSocket } = Promise.withResolvers<TLSSocket>();
+
+  try {
-  const { promise: serverSocketPromise, resolve: onServerSocket } = Promise.withResolvers<TLSSocket>();
-  const server = tls.createServer(
+  server = tls.createServer(
     {
       key: serverTls.key,
       cert: serverTls.cert,
       ca: [clientTls.ca],
       requestCert: true,
       rejectUnauthorized: false,
     },
     socket => onServerSocket(socket),
   );
   await once(server.listen(0, "127.0.0.1"), "listening");
 
-  const client = tls.connect({
+  client = tls.connect({
     host: "127.0.0.1",
     port: (server.address() as AddressInfo).port,
     key: clientTls.key,
     cert: clientTls.cert,
     ca: [serverTls.ca],
     checkServerIdentity,
   });
   await once(client, "secureConnect");
 
-  const serverSocket = await serverSocketPromise;
-  try {
+  serverSocket = await serverSocketPromise;
     // ... existing assertions/loops ...
-  } finally {
-    client.end();
-    serverSocket.end();
-    server.close();
-  }
+  } finally {
+    client?.end();
+    serverSocket?.end();
+    server?.close();
+  }
 }, 180_000);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/js/node/tls/node-tls-cert.test.ts` around lines 617 - 640, The test
currently awaits server.listen and client secureConnect before entering the
try/finally that closes resources, so failures during setup can leak handles;
wrap the entire setup including creation of server, awaiting
once(server.listen(...),"listening"), creating client (tls.connect) and awaiting
once(client,"secureConnect") in a try/finally (or move the existing finally to
start before awaiting serverSocketPromise) and in the finally ensure you cleanly
close the server (server.close()) and destroy/end the client socket if they were
created, and also handle the serverSocketPromise/serverSocket (onServerSocket)
cleanup so any partially initialized resources are closed even on listen/connect
failures.

// Make sure the client actually sent a cert so we exercise the
// SSL_get_peer_certificate path rather than falling through to the
// cert-chain branch.
const first = serverSocket.getPeerCertificate();
expect(first).toBeDefined();
expect(first?.subject).toBeDefined();

function spin(n: number) {
for (let i = 0; i < n; i++) {
serverSocket.getPeerCertificate();
serverSocket.getPeerCertificate(false);
}
Bun.gc(true);
Bun.gc(true);
}

// Run in fixed-size rounds with a GC after each so the steady-state
// heap footprint stays bounded. The first few rounds grow the heap
// regardless of leaks, so take the baseline after warmup.
const perRound = isDebug ? 2_500 : 5_000;
for (let round = 0; round < 4; round++) spin(perRound);
const baseline = process.memoryUsage.rss();

for (let round = 0; round < 10; round++) spin(perRound);
const after = process.memoryUsage.rss();
const growth = after - baseline;

// Unpatched, the BIO leak alone is ~800 bytes/call → ~40MB over the
// 50k abbreviated calls here (~20MB for 25k in debug). Leave slack for
// allocator/ASAN noise but stay well below that.
const threshold = 1024 * 1024 * (isDebug ? 10 : isASAN ? 16 : 12);
expect(growth).toBeLessThan(threshold);
} finally {
client.end();
serverSocket.end();
server.close();
}
}, 180_000);
Loading