diff --git a/src/bun.js/api/bun/socket/tls_socket_functions.zig b/src/bun.js/api/bun/socket/tls_socket_functions.zig index 6f4041cf29d7..674e657e5e10 100644 --- a/src/bun.js/api/bun/socket/tls_socket_functions.zig +++ b/src/bun.js/api/bun/socket/tls_socket_functions.zig @@ -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); } } @@ -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; diff --git a/src/bun.js/api/server.zig b/src/bun.js/api/server.zig index 8d2fbaa3237a..d7cba9c192cc 100644 --- a/src/bun.js/api/server.zig +++ b/src/bun.js/api/server.zig @@ -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, }); @@ -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; diff --git a/src/bun.js/bindings/JSX509Certificate.cpp b/src/bun.js/bindings/JSX509Certificate.cpp index 8aa82533daf4..e5fe404bd4b2 100644 --- a/src/bun.js/bindings/JSX509Certificate.cpp +++ b/src/bun.js/bindings/JSX509Certificate.cpp @@ -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 buffer = JSC::ArrayBuffer::createFromBytes(std::span(reinterpret_cast(bptr->data), bptr->length), createSharedTask([](void* data) { - ncrypto::BIOPointer free_me(static_cast(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 buffer = JSC::ArrayBuffer::createFromBytes(std::span(reinterpret_cast(bptr->data), bptr->length), createSharedTask([bio_ptr](void*) { + ncrypto::BIOPointer free_me(bio_ptr); })); RELEASE_AND_RETURN(scope, Bun::createBuffer(globalObject, WTF::move(buffer))); } diff --git a/test/js/bun/http/serve-stream-reject-flush-leak-fixture.ts b/test/js/bun/http/serve-stream-reject-flush-leak-fixture.ts index a1675fd8241c..46f607e7cc91 100644 --- a/test/js/bun/http/serve-stream-reject-flush-leak-fixture.ts +++ b/test/js/bun/http/serve-stream-reject-flush-leak-fixture.ts @@ -24,6 +24,7 @@ import { connect } from "node:net"; const CHUNK = Buffer.alloc(8 * 1024 * 1024, "x"); let flushPending = 0; +let currentSocket: import("node:net").Socket | undefined; const server = Bun.serve({ port: 0, @@ -44,6 +45,12 @@ const server = Bun.serve({ if (p instanceof Promise && Bun.peek.status(p) === "pending") { flushPending++; } + // Tear down the client after handleRejectStream has run. The throw + // below rejects pull()'s promise; onRejectStream → handleRejectStream + // fires on the microtask queue, then this setImmediate fires on the + // next macrotask. Without it the parked tryEnd() write never drains + // (client is paused) and uWS won't close the connection. + setImmediate(() => currentSocket?.destroy()); throw new Error("boom"); }, } as any), @@ -57,19 +64,23 @@ function protectedPromiseCount() { } function oneRequest(): Promise { - // Raw TCP client that sends the request line and then stops reading, so - // the server's first body write (tryEnd of 8 MiB) hits backpressure. + // Raw TCP client that sends the request line and then never reads, so the + // server's first body write (tryEnd of 8 MiB) hits backpressure. Windows' + // loopback fast-path will absorb the full 8 MiB into the kernel if the + // client is draining, so explicitly pause(); the server side destroys the + // socket once handleRejectStream has run. return new Promise(resolve => { const socket = connect({ port: server.port, host: "127.0.0.1" }, () => { socket.write("GET / HTTP/1.1\r\nHost: x\r\nConnection: close\r\n\r\n"); + socket.pause(); }); - socket.on("data", () => socket.destroy()); + currentSocket = socket; socket.on("close", () => resolve()); socket.on("error", () => {}); }); } -const ITERATIONS = 40; +const ITERATIONS = 10; // Warm up so per-process one-time promise protections (module loader, etc.) // don't count against us. diff --git a/test/js/node/tls/node-tls-cert.test.ts b/test/js/node/tls/node-tls-cert.test.ts index 0f7bd1198de2..dc1e426dfa71 100644 --- a/test/js/node/tls/node-tls-cert.test.ts +++ b/test/js/node/tls/node-tls-cert.test.ts @@ -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"; @@ -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(); + 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 { + // 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);