server: use bun.default_allocator for server.allocator - #29916
Merged
Conversation
SSL_get_peer_certificate returns a +1 X509 reference which the caller must X509_free. The server paths in TLSSocket.getPeerCertificate() passed this through a non-owning X509View (abbreviated) or dropped it on the floor (non-abbreviated), pinning the peer's X509 past session close. While investigating, also found that JSX509Certificate::computeRaw (used by every getPeerCertificate()/getCertificate() call) released the BIO from its RAII wrapper but never freed it, and the ArrayBuffer destructor BIO_free'd the data pointer instead of the BIO pointer. That was the dominant per-call leak (~800 bytes) and UB in the destructor. Capture the BIO pointer in the destructor lambda instead. Add an mTLS RSS test that exercises the server getPeerCertificate() path and asserts bounded growth after warmup.
…ult() `Arena.getThreadLocalDefault()` returns a different vtable (`global_mimalloc_vtable`) than `bun.default_allocator` (`c_allocator_vtable`) on non-ASAN builds, even though both route to the same mimalloc calls. This trips `BabyList`'s `CheckedAllocator` when `HTTPServerWritable.finalize()` frees a >64 KiB pooled buffer with `bun.default_allocator` after `start()` grew it with `this.allocator`. Only visible on `ci_assert && !asan` builds (Windows canary), since under ASAN `getThreadLocalDefault()` already returns `bun.default_allocator`. Fixes serve-stream-reject-flush-leak.test.ts on Windows x64-baseline.
Collaborator
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughFixes native memory ownership in TLS certificate handling (frees peer X509, owns BIO for ArrayBuffer); switches server VM allocator to Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Review rate limit: 1/5 review remaining, refill in 43 minutes and 32 seconds. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/js/node/tls/node-tls-cert.test.ts`:
- Around line 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.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 37afd8f3-029c-45ae-a116-fe8393b399eb
📒 Files selected for processing (4)
src/bun.js/api/bun/socket/tls_socket_functions.zigsrc/bun.js/api/server.zigsrc/bun.js/bindings/JSX509Certificate.cpptest/js/node/tls/node-tls-cert.test.ts
Comment on lines
+617
to
+640
| 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 { |
Contributor
There was a problem hiding this comment.
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.
Contributor
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
The changes look correct to me, but note this PR bundles two unrelated fixes beyond the titular allocator swap — an X509 ref leak in getPeerCertificate() and a BIO destructor fix in computeRaw (the old lambda was calling BIO_free on bptr->data rather than the BIO itself). Worth a human glance given it's TLS cert lifetime management.
Extended reasoning...
Overview
This PR touches four files across three distinct fixes:
src/bun.js/api/server.zig— replacesArena.getThreadLocalDefault()withbun.default_allocatorforserver.allocator. Both route to mimalloc but use different vtable pointers, which tripsBabyList'sCheckedAllocatorwhenHTTPServerWritable.finalize()later frees withbun.default_allocator. Verified againstMimallocArena.zig:121-124—getThreadLocalDefault()indeed returns&global_mimalloc_vtableon non-ASAN builds and no longer provides a per-thread heap, so the swap is semantically a no-op outside the assertion.src/bun.js/api/bun/socket/tls_socket_functions.zig— addsdefer x509.free()afterSSL_get_peer_certificate()in the server-side branches ofgetPeerCertificate().SSL_get_peer_certificatereturns a +1 ref;X509.toJS→Bun__X509__toJSLegacyEncodingwraps it in a non-owningX509Viewand synchronously copies all fields into JS values, so freeing after return is safe.src/bun.js/bindings/JSX509Certificate.cpp— fixes thecomputeRawArrayBuffer destructor. Previously the lambda received the data pointer (bptr->data) and cast it toBIO*before handing it toBIOPointer, which is an invalid free (and a leak of the actual BIO). The fix capturesbio_ptrby value so the correct object is freed when the ArrayBuffer is destroyed.test/js/node/tls/node-tls-cert.test.ts— adds an RSS-growth regression test that hammers server-sidegetPeerCertificate().
Security risks
No new attack surface is introduced — these are leak/lifetime fixes. The main risk class would be a use-after-free if the X509 were freed while JS still referenced it, but toLegacyObject copies everything (strings, fingerprints, raw DER) before returning, and the non-abbreviated path currently returns undefined without using first_cert. The computeRaw change strictly improves correctness (the old destructor was calling BIO_free on the wrong pointer).
Level of scrutiny
Medium-high. The server.allocator change is trivial and well-justified by the PR body. The X509/BIO changes, while small, sit in TLS certificate handling and C++↔Zig memory ownership — exactly the area where subtle lifetime bugs become security issues. They look correct, but the PR title/description only covers the allocator fix, so a reviewer skimming by title would miss the crypto changes entirely.
Other factors
- No CODEOWNERS cover these paths.
- A regression test is included with reasonable thresholds and warmup.
- No prior reviews on the PR.
- Branch name (
fix-getpeercert-x509-leak) suggests the X509 work is the original intent and the allocator fix was added on top, which explains the title/content mismatch.
…roy for Windows backpressure Windows' loopback fast-path absorbs the full 8 MiB tryEnd() into the kernel when the client is reading, so controller.end() never returns a pending promise and the test reports "insufficient backpressure: 0/40". Pause the client so it never drains; then destroy it from the server side via setImmediate after the throw — onRejectStream → handleRejectStream runs on the microtask queue first, so pending_flush is observed/unprotected before the socket is torn down. Without the explicit destroy, uWS waits on the parked write that the paused client will never drain. Linux post-fix: flushPending=42/40, delta=0, exit 0 Linux pre-fix: flushPending=42/40, delta=40, exit 1 (still detects leak)
Jarred-Sumner
added a commit
that referenced
this pull request
Apr 29, 2026
…nstead of skipping Replace skipIf(isWindows) with the same fixture change pushed to #29916: pause the client so Windows' loopback fast-path can't absorb the 8 MiB write, then destroy the socket from the server side via setImmediate after the throw (handleRejectStream runs on the microtask queue first).
Backpressure is now deterministic per-iteration (paused client), and the
metric is heapStats().protectedObjectTypeCounts.Promise (exact). 40 was
hedging for the old on('data', destroy) timing.
Contributor
There was a problem hiding this comment.
The fixes look correct to me (verified X509.toJS is non-owning and the BIO destructor was indeed wrapping the wrong pointer), but since this bundles three separate native memory-ownership changes across the core server allocator and TLS cert handling — and overlaps with #29881 — I'd like a human to confirm the ownership semantics before merge.
Extended reasoning...
Overview
This PR contains three distinct native fixes plus test adjustments:
server.zig: swapArena.getThreadLocalDefault()→bun.default_allocatorforserver.allocatorto fix aCheckedAllocatorvtable-mismatch panic inHTTPServerWritable.finalize().tls_socket_functions.zig: adddefer x509.free()afterSSL_get_peer_certificate(which returns a +1 ref) on both the abbreviated and non-abbreviated server paths.JSX509Certificate.cpp::computeRaw: capturebio_ptrin the ArrayBuffer destructor lambda instead of casting the data pointer toBIO*, so the BIO actually gets freed.- New RSS-growth regression test for
getPeerCertificate()and Windows-backpressure tweaks toserve-stream-reject-flush-leak-fixture.ts.
Security risks
The X509/BIO changes alter ownership/lifetime of BoringSSL objects in TLS certificate handling. I verified X509.toJS calls Bun__X509__toJSLegacyEncoding, which uses a non-owning ncrypto::X509View, so freeing the +1 ref afterward is correct and not a UAF. The BIO fix is clearly right (the old destructor wrapped bptr->data as a BIO*, which is bogus). Still, mistakes here would manifest as UAF/double-free in TLS paths, so this is security-adjacent and deserves human eyes.
Level of scrutiny
High. The allocator swap touches every Bun.serve() instance, and the TLS changes touch native crypto memory ownership. Each individual change is small and looks correct, but the blast radius is large and the failure mode (UAF, double-free, allocator mismatch) is severe.
Other factors
- robobun reports widespread build-zig/build-cpp failures on commit 654f30a; the newer a938c90 only changed test iteration count, so CI status should be confirmed green before merge.
- The duplicate-PR bot flagged #29881 as containing the same X509/BIO fixes — a human should decide which PR lands them.
- The PR title only mentions the allocator change, but two of the three native fixes are unrelated TLS leak fixes; scope is broader than advertised.
Jarred-Sumner
added a commit
that referenced
this pull request
Apr 29, 2026
…29926) Clears four pre-existing failures showing up across open PRs. ## `server.allocator` → `bun.default_allocator` `MimallocArena.getThreadLocalDefault()` and `bun.default_allocator` both call mimalloc, but their vtables differ. Collections that flow between server-owned and default-owned code (e.g. `BabyList` in the response sink, the `onUpgrade` path) trip `CheckedAllocator.assertEq` in `ci_assert` builds: ``` allocator mismatch: cannot use multiple allocators with the same collection panic(main thread): Internal assertion failure: allocators do not match ``` This was crashing both `serve-stream-reject-flush-leak.test.ts` and the websocket upgrade-UAF test on Windows release lanes. Same change as #29916 minus the X509/BIO hunks that already landed via #29881. ## `html-rewriter-leak.test.ts` — RSS path never worked on release The test was added in #29879 and **failed on every release platform in that PR's own CI** (113–233 MB across darwin/linux/windows/asan, all on the *fixed* build). Only the debug path (precise mimalloc counters) passed. Root cause: a single trailing `Bun.gc(true)` does collect all 16k rewriters, but neither mimalloc nor ASAN's allocator promptly return freed pages to the OS, so RSS pins at the *peak* live set rather than what's retained. Now GCs every 1k iterations to bound peak ≈ retained. | build | before | after | |---|---|---| | release post-fix | 148 MB ❌ | 0.0 MB ✅ | | asan post-fix | 233 MB ❌ | 0.6 MB ✅ | | release **pre**-fix | — | 68.5 MB ❌ (still detects the leak) | | debug post-fix | ✅ | ✅ | ## `serve-stream-reject-flush-leak.test.ts` — skip on Windows Once the allocator panic is fixed, the test next fails on Windows with `insufficient backpressure: 0/40` (#29916 CI) — the 8 MiB `tryEnd()` fits in Windows' auto-tuned loopback send buffer so `pending_flush` is never created. The leak is platform-agnostic Zig; POSIX coverage is sufficient. ## `websocket-server.test.ts` — un-truncate stderr The upgrade-UAF test's `stderr.split("\\n", 3)` hid the panic line, leaving `{stdout:"", stderr:""}` and a misleading diff. ## `test-integration-rspack.ts` — pin to `rsbuild@1` `create-rsbuild@2.0.0` (Apr 22) pulls `@rspack/binding-win32-arm64-msvc@2.0.x` which bundles mimalloc v3. Two static mimalloc instances → deterministic segfault in ntdll during `ExitProcess` on Windows arm64 (crash addr ends in `b9c8` across 5 builds). The test exists to guard the napi TSFN finalizer, not rsbuild HEAD. Supersedes #29916.
xhjkl
pushed a commit
to xhjkl/bun
that referenced
this pull request
May 14, 2026
## What `server.allocator` was set to `Arena.getThreadLocalDefault()`, which on non-ASAN builds returns `&global_mimalloc_vtable` — a different `Allocator.VTable` from `bun.default_allocator` (`c_allocator_vtable`), even though both route to the same mimalloc calls. This trips `BabyList`'s `CheckedAllocator` in `HTTPServerWritable.finalize()`: 1. `start()` grows `this.buffer` via `ensureTotalCapacityPrecise(this.allocator, …)` → `CheckedAllocator` records `global_mimalloc_vtable` 2. on the next request, `finalize()` sees `pooled_buffer != null` and `cap > 64 KiB`, calls `this.buffer.clearAndFree(bun.default_allocator)` → vtable mismatch → panic ``` allocator mismatch: cannot use multiple allocators with the same collection allocator mismatch: vtables differ: mem.Allocator.VTable@7ff6c69eed78 and mem.Allocator.VTable@7ff6c69eeac0 panic(main thread): Internal assertion failure: allocators do not match ``` Only fires on `ci_assert && !enable_asan` builds (Windows x64-baseline canary). Under ASAN, `getThreadLocalDefault()` already returns `bun.default_allocator`, so the mismatch is invisible on local mac/linux debug. `getThreadLocalDefault()` no longer provides a per-thread heap (the v3 comment says as much), so there is no reason to use it here over `bun.default_allocator`. ## Verification - `bun bd test test/js/bun/http/serve-stream-reject-flush-leak.test.ts` — pass - `bun run build --asan=off test test/js/bun/http/serve-stream-reject-flush-leak.test.ts` — pass (this config reproduced the crash before the fix) - `bun bd test test/js/bun/http/serve.test.ts` — 189 pass, 1 skip - `bun run zig:check-all` — clean Fixes `serve-stream-reject-flush-leak.test.ts` failure on Windows x64-baseline introduced by oven-sh#29865. --------- Co-authored-by: robobun <robobun@users.noreply.github.com> Co-authored-by: Dylan Conway <dylan.conway567@gmail.com>
xhjkl
pushed a commit
to xhjkl/bun
that referenced
this pull request
May 14, 2026
…ven-sh#29926) Clears four pre-existing failures showing up across open PRs. ## `server.allocator` → `bun.default_allocator` `MimallocArena.getThreadLocalDefault()` and `bun.default_allocator` both call mimalloc, but their vtables differ. Collections that flow between server-owned and default-owned code (e.g. `BabyList` in the response sink, the `onUpgrade` path) trip `CheckedAllocator.assertEq` in `ci_assert` builds: ``` allocator mismatch: cannot use multiple allocators with the same collection panic(main thread): Internal assertion failure: allocators do not match ``` This was crashing both `serve-stream-reject-flush-leak.test.ts` and the websocket upgrade-UAF test on Windows release lanes. Same change as oven-sh#29916 minus the X509/BIO hunks that already landed via oven-sh#29881. ## `html-rewriter-leak.test.ts` — RSS path never worked on release The test was added in oven-sh#29879 and **failed on every release platform in that PR's own CI** (113–233 MB across darwin/linux/windows/asan, all on the *fixed* build). Only the debug path (precise mimalloc counters) passed. Root cause: a single trailing `Bun.gc(true)` does collect all 16k rewriters, but neither mimalloc nor ASAN's allocator promptly return freed pages to the OS, so RSS pins at the *peak* live set rather than what's retained. Now GCs every 1k iterations to bound peak ≈ retained. | build | before | after | |---|---|---| | release post-fix | 148 MB ❌ | 0.0 MB ✅ | | asan post-fix | 233 MB ❌ | 0.6 MB ✅ | | release **pre**-fix | — | 68.5 MB ❌ (still detects the leak) | | debug post-fix | ✅ | ✅ | ## `serve-stream-reject-flush-leak.test.ts` — skip on Windows Once the allocator panic is fixed, the test next fails on Windows with `insufficient backpressure: 0/40` (oven-sh#29916 CI) — the 8 MiB `tryEnd()` fits in Windows' auto-tuned loopback send buffer so `pending_flush` is never created. The leak is platform-agnostic Zig; POSIX coverage is sufficient. ## `websocket-server.test.ts` — un-truncate stderr The upgrade-UAF test's `stderr.split("\\n", 3)` hid the panic line, leaving `{stdout:"", stderr:""}` and a misleading diff. ## `test-integration-rspack.ts` — pin to `rsbuild@1` `create-rsbuild@2.0.0` (Apr 22) pulls `@rspack/binding-win32-arm64-msvc@2.0.x` which bundles mimalloc v3. Two static mimalloc instances → deterministic segfault in ntdll during `ExitProcess` on Windows arm64 (crash addr ends in `b9c8` across 5 builds). The test exists to guard the napi TSFN finalizer, not rsbuild HEAD. Supersedes oven-sh#29916.
Collaborator
|
Heads up: this PR added a second copy of the |
Jarred-Sumner
pushed a commit
that referenced
this pull request
Jul 15, 2026
…test (#34163) ## Problem `test/js/node/tls/node-tls-cert.test.ts` went red on `:darwin: 26 aarch64` in [build 72915](https://buildkite.com/bun/bun/builds/72915): ``` error: expect(received).toBeLessThan(expected) Expected: < 12582912 Received: 21839872 ✗ server-side getPeerCertificate() should not leak ``` The same assertion also flaked in `node-tls-getpeercert-leak.test.ts` on `:darwin: 14 x64` in [build 72900](https://buildkite.com/bun/bun/builds/72900) (21008384 bytes, passed on retry). ## Cause The `server-side getPeerCertificate() should not leak` test exists twice: #29881 landed it as the standalone `node-tls-getpeercert-leak.test.ts`, and #29916 (merged a few hours later) appended an identical copy to `node-tls-cert.test.ts`. Both copies baseline RSS after four 5k-call warmup rounds and assert less than 12MB of growth afterwards. On the macOS tart runners the allocator's high-water mark can climb by ~21MB over that window with no leak present, while a probe on `darwin-test-arm64-3` with the current canary shows RSS flat within ~1MB once it has had a couple more rounds to settle, and the full test file passes 8/8 on that box. The copy inside `node-tls-cert.test.ts` is the worse of the two because it runs after 30 other TLS tests in the same process; in build 72915 the standalone file passed on the same commit in the same lane. There is no regression in the underlying `SSL_get_peer_certificate`/`computeRaw` paths: the growth is the same for 5 and 10 measurement rounds, which rules out the per-call leak the test guards against. ## Fix * Remove the duplicate from `node-tls-cert.test.ts` and leave a pointer to the standalone file. * In `node-tls-getpeercert-leak.test.ts`: * apply `skipIf(isDebug)` (the refinement #31155 had applied only to the duplicate) so the ASAN quarantine cannot mask the signal on debug builds, * lengthen the warmup from 4 to 8 rounds so RSS has settled before the baseline sample, * lengthen the measured window from 10 to 15 rounds, * raise the release threshold from 12MB to 32MB (40MB under the release ASAN lane). The unpatched BIO/X509 leak is ~800 bytes per call; over the 75k calls now measured that is ~60MB of growth, so the regression the test exists to catch remains well above the new threshold, while the ~21MB noise ceiling observed on the macOS runners sits comfortably below it. ## Verification ``` $ USE_SYSTEM_BUN=1 bun test test/js/node/tls/node-tls-getpeercert-leak.test.ts (pass) server-side getPeerCertificate() should not leak [5150.55ms] 1 pass 0 fail $ USE_SYSTEM_BUN=1 bun test test/js/node/tls/node-tls-cert.test.ts 27 pass 3 todo 0 fail $ bun bd test test/js/node/tls/node-tls-getpeercert-leak.test.ts (skip) server-side getPeerCertificate() should not leak ``` Ran the full `node-tls-cert.test.ts` eight times on `darwin-test-arm64-3` (macOS 26) against canary `cc0c1e835`; 8/8 pass. <!-- robobun:evidence:begin --> --- **[stamp-90s]** gate passed · iteration 8 · 2 files touched <details><summary>passes on PR (with fix)</summary> ```console Test-only change. Debug/ASAN (expected pass): $ bun bd test 'test/js/node/tls/node-tls-cert.test.ts' 'test/js/node/tls/node-tls-getpeercert-leak.test.ts' $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test test/js/node/tls/node-tls-cert.test.ts test/js/node/tls/node-tls-getpeercert-leak.test.ts info: syncing channel updates for nightly-2026-05-06-x86_64-unknown-linux-gnu info: latest update on 2026-05-06 for version 1.97.0-nightly (e95e73209 2026-05-05) info: component rust-src is up to date info: checking for self-update (current version: 1.29.0) bun test v1.4.0 (81bb980) test/js/node/tls/node-tls-getpeercert-leak.test.ts: (skip) server-side getPeerCertificate() should not leak test/js/node/tls/node-tls-cert.test.ts: (pass) complete cert chains sent to peer. [576.13ms] (pass) complete cert chains sent to peer, but without requesting client's cert. [162.21ms] (todo) Request cert from TLS1.2 client that doesn't have one. (pass) Typical configuration error, incomplete cert chains sent, we have to know the peer's subordinate CAs in order to verify the peer. [107.76ms] (pass) Typical configuration error, incomplete cert chains sent, we have to know the peer's subordinate CAs in order to verify the peer. But using multi-PEM [102.96ms] (pass) Typical configuration error, incomplete cert chains sent, we have to know the peer's subordinate CAs in order to verify the peer. But using multi-PEM in an array [97.10ms] (pass) Fail to complete server's chain [83.14ms] (pass) Fail to complete client's chain. [103.92ms] (pass) rejects an unverifiable client certificate by default when requestCert is true [247.07ms] (pass) explicit rejectUnauthorized: false still admits an unverified client certificate [79.04ms] (pass) Fail to find CA for server. [151.43ms] (pass) Server sent their CA, but CA cannot be trusted if it is not locally known. [69.40ms] (pass) Server sent their CA, wrongly, but its OK since we know the CA locally. [83.05ms] (todo) Confirm client support for "BEGIN TRUSTED CERTIFICATE". (todo) Confirm server support for "BEGIN TRUSTED CERTIFICATE". (pass) Confirm client support for ... (truncated) Exit: 0 ``` </details> <details><summary>diff hotspot</summary> ``` test/js/node/tls/node-tls-cert.test.ts | 150 ++++++--------------- test/js/node/tls/node-tls-getpeercert-leak.test.ts | 127 +++++++++-------- 2 files changed, 107 insertions(+), 170 deletions(-) ``` </details> **gate history** · 3 passed · 1 rejected · iteration 8 <details><summary>evidence per changed file</summary> ``` file reads edits tests test/js/node/tls/node-tls-cert.test.ts 5 6 0 test/js/node/tls/node-tls-getpeercert-leak.test.ts 2 2 0 ``` </details> <!-- robobun:evidence:end -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
server.allocatorwas set toArena.getThreadLocalDefault(), which on non-ASAN builds returns&global_mimalloc_vtable— a differentAllocator.VTablefrombun.default_allocator(c_allocator_vtable), even though both route to the same mimalloc calls.This trips
BabyList'sCheckedAllocatorinHTTPServerWritable.finalize():start()growsthis.bufferviaensureTotalCapacityPrecise(this.allocator, …)→CheckedAllocatorrecordsglobal_mimalloc_vtablefinalize()seespooled_buffer != nullandcap > 64 KiB, callsthis.buffer.clearAndFree(bun.default_allocator)→ vtable mismatch → panicOnly fires on
ci_assert && !enable_asanbuilds (Windows x64-baseline canary). Under ASAN,getThreadLocalDefault()already returnsbun.default_allocator, so the mismatch is invisible on local mac/linux debug.getThreadLocalDefault()no longer provides a per-thread heap (the v3 comment says as much), so there is no reason to use it here overbun.default_allocator.Verification
bun bd test test/js/bun/http/serve-stream-reject-flush-leak.test.ts— passbun run build --asan=off test test/js/bun/http/serve-stream-reject-flush-leak.test.ts— pass (this config reproduced the crash before the fix)bun bd test test/js/bun/http/serve.test.ts— 189 pass, 1 skipbun run zig:check-all— cleanFixes
serve-stream-reject-flush-leak.test.tsfailure on Windows x64-baseline introduced by #29865.