Skip to content

boringssl: route OPENSSL_malloc to mimalloc on macOS and Windows - #34847

Merged
Jarred-Sumner merged 4 commits into
mainfrom
farm/b107465a/boringssl-mimalloc-hooks
Jul 21, 2026
Merged

Jarred-Sumner merged 4 commits into
mainfrom
farm/b107465a/boringssl-mimalloc-hooks

Conversation

@robobun

@robobun robobun commented Jul 20, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

vendor/boringssl/crypto/mem.cc declares the OPENSSL_memory_alloc / OPENSSL_memory_free / OPENSSL_memory_get_size override hooks as weak externs only under #if defined(__ELF__). On Mach-O and COFF the macro expands to static ... = nullptr, so the if (OPENSSL_memory_alloc != nullptr) branch is folded away at compile time and OPENSSL_malloc compiles to a direct call to libc malloc. The mimalloc-backed definitions in src/boringssl/lib.rs were dead code on those two platforms, and every TLS record read / handshake allocation hit the system allocator.

The Windows /INCLUDE:OPENSSL_memory_* linker flags in scripts/build/flags.ts didn't help either: they forced the Rust symbols into the image, but the call sites in mem.cc had already been compiled out. The comment claiming the hooks "already bind on ELF/Mach-O" was wrong for Mach-O.

Fix

Patch crypto/mem.cc to honor a new BORINGSSL_REQUIRE_MEMORY_HOOKS define that declares the three hooks as ordinary extern "C" functions and calls them unconditionally. Bun always defines the hooks, so the runtime null check is unnecessary. Pass the define from scripts/build/deps/boringssl.ts on all platforms.

mem.cc.o now carries strong undefined references on every target, which pull the defining archive member at link time on ELF, Mach-O and COFF alike. The Windows /INCLUDE: flags are removed since the strong refs make the link fail if the hooks ever go missing.

The patch lives in patches/boringssl/ (same mechanism as libarchive/zlib/etc.); it can be folded into the oven-sh/boringssl fork on the next bump.

Verification

Simulating the non-ELF path without the fix (unpatched mem.cc, -U__ELF__):

$ nm mem.cc.o | grep -E 'OPENSSL_memory|OPENSSL_malloc| malloc$'
0000000000000000 T OPENSSL_malloc
                 U malloc

OPENSSL_memory_* are completely absent; OPENSSL_malloc calls malloc directly.

With the fix (-DBORINGSSL_REQUIRE_MEMORY_HOOKS):

$ nm mem.cc.o | grep -E 'OPENSSL_memory|OPENSSL_malloc'
0000000000000000 T OPENSSL_malloc
                 U OPENSSL_memory_alloc
                 U OPENSSL_memory_free
                 U OPENSSL_memory_get_size

$ objdump -d -r mem.cc.o   # OPENSSL_malloc
   0: push   %rbx
   1: mov    %rdi,%rbx
   4: call   9 <OPENSSL_malloc+0x9>
        R_X86_64_PLT32  OPENSSL_memory_alloc-0x4
   ...

$ objdump -d -r mem.cc.o   # OPENSSL_free
 150: test   %rdi,%rdi
 153: jne    159 <OPENSSL_free+0x9>
        R_X86_64_PLT32  OPENSSL_memory_free-0x4
 159: ret

The libc fallback is dead-stripped; OPENSSL_free is a tail call to the hook.

Linux bun bd build succeeds and test/js/node/tls/node-tls-connect.test.ts + test/js/node/crypto/node-crypto.test.js (202 tests) pass. ELF codegen is also slightly tighter now since the weak-symbol null checks are gone.

Darwin arm64 (Apple clang 16, -O2 -mcpu=apple-m1)

Without -DBORINGSSL_REQUIRE_MEMORY_HOOKS:

$ nm mem.cc.o | grep -E 'OPENSSL_memory|OPENSSL_malloc| _malloc$'
0000000000000000 T _OPENSSL_malloc
                 U _malloc

With -DBORINGSSL_REQUIRE_MEMORY_HOOKS:

$ nm mem.cc.o | grep -E 'OPENSSL_memory|OPENSSL_malloc'
0000000000000000 T _OPENSSL_malloc
                 U _OPENSSL_memory_alloc
                 U _OPENSSL_memory_free
                 U _OPENSSL_memory_get_size

$ otool -tV mem.cc.o    # _OPENSSL_free
_OPENSSL_free:
014c  cbz  x0, 0x154
0150  b    _OPENSSL_memory_free
0154  ret

_OPENSSL_malloc no longer has a bl _malloc; the only branch targets in its body are _OPENSSL_memory_alloc and _ERR_put_error.


no test proof · iteration 2 · docs-only change; test-proof not applicable

crypto/mem.cc declares the OPENSSL_memory_alloc/free/get_size override hooks as
weak externs only under #if defined(__ELF__); on Mach-O and COFF the macro
expands to 'static ... = nullptr', so the 'if (OPENSSL_memory_alloc != nullptr)'
branch is folded away at compile time and OPENSSL_malloc calls libc malloc
directly. The Rust definitions in src/boringssl/lib.rs (routing to mimalloc)
were dead code on those platforms.

Patch mem.cc to honor a new BORINGSSL_REQUIRE_MEMORY_HOOKS define that declares
the three hooks as ordinary extern "C" functions and calls them
unconditionally; pass the define from the build. mem.cc.o now carries strong
undefined references that pull the defining member on every linker, so the
Windows /INCLUDE: flags (which never actually made the hooks get called, since
the call sites were compiled out) are no longer needed.
@robobun

robobun commented Jul 20, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 8:40 PM PT - Jul 20th, 2026

❌ @robobun, your commit 690058f has 3 failures in Build #76541 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 34847

That installs a local version of the PR into your bun-34847 executable, so you can run:

bun-34847 --bun

@coderabbitai

coderabbitai Bot commented Jul 20, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

Changes

BoringSSL memory hook enforcement

Layer / File(s) Summary
Direct memory-hook runtime paths
patches/boringssl/require-memory-hooks.patch
Adds direct declarations and required-mode branches for allocation, freeing, and memory-size retrieval while retaining the existing weak-symbol paths when disabled.
Build configuration and Windows linking
scripts/build/deps/boringssl.ts, scripts/build/flags.ts
Applies the BoringSSL patch, enables BORINGSSL_REQUIRE_MEMORY_HOOKS, and removes Windows /INCLUDE: allocator-hook flags.

Suggested reviewers: jarred-sumner

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title is clear and matches the main change: routing BoringSSL malloc hooks to mimalloc on macOS and Windows.
Description check ✅ Passed It covers the problem, fix, and verification, though the headings differ from the repository template.

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

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.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@patches/boringssl/require-memory-hooks.patch`:
- Around line 8-11: Shorten the added comment above the extern declarations to
no more than three lines while preserving its explanation of the embedder
guarantee and why WEAK_SYMBOL_FUNC cannot be used on Mach-O and COFF.
🪄 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: de3d7399-5195-4eef-abbe-412fabb925a3

📥 Commits

Reviewing files that changed from the base of the PR and between b18d5df and 9e67933.

📒 Files selected for processing (3)
  • patches/boringssl/require-memory-hooks.patch
  • scripts/build/deps/boringssl.ts
  • scripts/build/flags.ts
💤 Files with no reviewable changes (1)
  • scripts/build/flags.ts

Comment thread patches/boringssl/require-memory-hooks.patch Outdated

@claude claude Bot left a comment

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.

No issues found, but deferring to a human — this reroutes BoringSSL's allocator on macOS/Windows and drops the Windows /INCLUDE: flags, which is a cross-platform behavioral change to a crypto dependency worth a maintainer sign-off.

What was reviewed

  • Patch structure vs. crypto/mem.cc: the {} scope around ptr2 keeps the earlier goto err legal, and OPENSSL_free/OPENSSL_realloc hunks preserve the null-ptr early-return and old_size semantics.
  • Confirmed src/boringssl/lib.rs defines all three hooks unconditionally with #[no_mangle], so the strong undefined refs will resolve on every target and the removed /INCLUDE: flags are redundant.
  • patches: field on the Dependency type is the established mechanism (libarchive/lsquic/zlib use it) and is threaded through emitFetch correctly.
Extended reasoning...

Overview

This PR fixes BoringSSL's OPENSSL_malloc to route through Bun's mimalloc-backed hooks on macOS and Windows. Upstream mem.cc only declares the OPENSSL_memory_* overrides as weak symbols under #if defined(__ELF__); on Mach-O/COFF the macro expands to static ... = nullptr and the hook branch is folded away at compile time, so those platforms were silently using libc malloc for all TLS/crypto allocations. The fix adds a patch that declares the hooks as ordinary extern "C" under a new BORINGSSL_REQUIRE_MEMORY_HOOKS define, sets that define in scripts/build/deps/boringssl.ts, and removes the now-redundant Windows /INCLUDE:OPENSSL_memory_* linker flags.

Security risks

BoringSSL is the TLS/crypto library. The change swaps the allocator backing every crypto/TLS allocation on two platforms from libc to mimalloc. That's not a security-logic change (no cipher/protocol/verification code is touched), but an allocator mismatch here would UAF or corrupt memory in the middle of a TLS handshake. The mitigating factor is that ELF builds have already been running this exact hook path (weak symbols resolve to the Rust definitions there), so any OPENSSL_free(malloc(...))-style mismatch would already be caught on Linux CI. I did not find any such mismatch, and the Rust hooks in src/boringssl/lib.rs:209-233 are unconditionally exported.

Level of scrutiny

High. This is a vendored-dependency patch to a crypto library that changes runtime allocator routing on macOS and Windows — platforms the author verified only via nm/otool on the object file, not via a full build+test on those hosts (Linux bun bd + TLS/crypto tests were run). The reasoning in the PR is sound and the patch is minimal and well-scoped, but per the repo's guidance on touching vendor/ and cross-platform allocator paths, a maintainer should confirm the macOS/Windows CI comes back green before this lands.

Other factors

  • No automated test is added; the PR marks itself "docs-only change; test-proof not applicable", which is inaccurate — though testing allocator routing directly is genuinely awkward. The existing TLS/crypto suites exercise the code path indirectly.
  • The removed /INCLUDE: block's comment claimed the hooks "already bind on ELF/Mach-O", which the PR correctly identifies as wrong for Mach-O — deleting that stale comment is appropriate.
  • The CodeRabbit 3-line-comment nit was addressed in 09fa1dc.

@robobun

robobun commented Jul 20, 2026 •

Copy link
Copy Markdown
Collaborator Author

CI status for 690058f (build 76541, finished):

  • 285/286 jobs passed. The one red is a Buildkite job-level timed_out on darwin-flatbread-x64-1.
  • Windows: x64 (8/8), x64-baseline (8/8), aarch64 (8/8) test shards passed. The /INCLUDE: removal links correctly and the strong extern refs pull the Rust hooks.
  • darwin-aarch64: macOS 26 (2/2) and macOS 14 (2/2) test shards passed.
  • darwin-x64: darwin-naan-x64-1 passed; darwin-flatbread-x64-1 hit a job-level timed_out with bake/dev-server tests hanging. Same machine also timed out in build 76410 while its sibling shard passed both times. The bake tests are plain localhost HTTP/WebSocket (no TLS), so this is the runner, not the diff.
  • All other annotated tests are [flaky] Linux-side passes-on-retry (fs-promises GC, proxy-stress, http3 content-length, webview animation timing, watch-many-dirs, require-cache RSS).

@claude claude Bot left a comment

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.

I didn't find bugs, but this changes runtime allocator routing for every BoringSSL allocation on macOS and Windows and drops a linker safeguard, so it's worth a human confirming macOS/Windows CI is green before merge.

What was reviewed:

  • Patch hunks against crypto/mem.cc — each #if BORINGSSL_REQUIRE_MEMORY_HOOKS branch mirrors the existing weak-symbol path (same null/size==0 handling, same goto err on OOM).
  • src/boringssl/lib.rs defines all three hooks as #[no_mangle] extern "C" with matching signatures; OPENSSL_memory_free zeros before freeing per BoringSSL's contract.
  • patches: field on the boringssl dep matches the mechanism used by libarchive/zlib/lshpack; /INCLUDE: removal is safe because the strong undefined refs now force the archive member in.
Extended reasoning...

Overview

Three files: a new patches/boringssl/require-memory-hooks.patch that adds a BORINGSSL_REQUIRE_MEMORY_HOOKS compile-time branch to crypto/mem.cc (declaring the three OPENSSL_memory_* hooks as plain extern "C" and calling them unconditionally in OPENSSL_malloc/OPENSSL_free/OPENSSL_realloc), scripts/build/deps/boringssl.ts wiring the patch and define into the BoringSSL DirectBuild, and scripts/build/flags.ts removing the now-redundant Windows /INCLUDE:OPENSSL_memory_* linker flags.

Security risks

This is allocator plumbing inside the TLS/crypto library. The direct risk is allocator mismatch: memory allocated via OPENSSL_malloc on macOS/Windows now comes from mimalloc instead of the system heap. That is only safe if every consumer frees through OPENSSL_free (BoringSSL's documented contract). Linux has always taken the mimalloc path via the ELF weak-symbol branch, so any shared code that violated the contract would already crash there — but macOS/Windows-specific paths (e.g. platform cert-store integration) haven't been exercised under mimalloc before. I don't see a concrete violation, and the hooks in src/boringssl/lib.rs:210-233 correctly zero-on-free and use mi_usable_size for get_size, but the blast radius is every TLS handshake and crypto allocation on two platforms.

Level of scrutiny

Medium-high. The patch itself is small and each new #if branch is a verbatim copy of the adjacent weak-symbol branch minus the null-function-pointer check, so the diff is mechanically simple to verify. But it's a vendored-dep patch to a crypto library's memory allocator with cross-platform behavioral impact, which per the repo's "Dependencies & vendoring" and "Cross-platform" review guidance warrants a maintainer look. The PR also removes the /INCLUDE: fallback — correct given the strong refs, but worth a human confirming the Windows link still succeeds in CI.

Other factors

  • No automated test asserts the allocator routing (the PR footer says "docs-only change; test-proof not applicable", which undersells it — this is a build/runtime change verified via nm/objdump, not docs).
  • Verification in the description is thorough (nm/objdump on Linux and Darwin arm64) and Linux TLS/crypto tests pass, but macOS/Windows CI results aren't visible in the timeline yet.
  • The patches: mechanism, define plumbing, and extern "C" signatures all check out against existing conventions and src/boringssl/lib.rs.
  • No prior claude[bot] review on this PR; the only prior feedback (CodeRabbit comment-length nit) is resolved.

Comment thread scripts/build/deps/boringssl.ts Outdated
Under ASAN, keep BoringSSL on the intercepted libc heap on Mach-O/COFF so the
sanitizer can track its allocations. Release builds (the ones that ship) still
route to mimalloc.

@claude claude Bot left a comment

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.

No issues found, but this patches BoringSSL's allocator path and changes which heap TLS/crypto allocations land on across macOS/Windows — worth a maintainer sign-off (Jarred is already engaged).

What was reviewed:

  • Patch hunks mirror the existing weak-symbol branches exactly (null-on-nonzero-size → goto err in malloc; null guard in free precedes the hunk; realloc reads get_size for old_size).
  • src/boringssl/lib.rs defines all three hooks with matching extern "C" signatures, and OPENSSL_memory_free zeros before freeing as BoringSSL requires.
  • Under cfg.asan the define is dropped, so behavior is identical to pre-PR on every platform (ELF keeps the weak-symbol bind; Mach-O/COFF fall through to libc); the removed /INCLUDE: flags were dead there anyway since the COFF call sites compile out.
  • patches: usage matches libarchive/zlib/lshpack/etc.; ...(cond && {...}) spread matches the adjacent cfg.windows block.
Extended reasoning...

Overview

Three files: a new patches/boringssl/require-memory-hooks.patch that adds a BORINGSSL_REQUIRE_MEMORY_HOOKS compile-time branch to crypto/mem.cc (declaring the three OPENSSL_memory_* hooks as strong extern "C" and calling them unconditionally); scripts/build/deps/boringssl.ts registering the patch and passing the define when !cfg.asan; and scripts/build/flags.ts removing the now-redundant Windows /INCLUDE:OPENSSL_memory_* linker flags.

Security risks

BoringSSL is the TLS/crypto library, and this changes which allocator its OPENSSL_malloc/OPENSSL_free/OPENSSL_realloc route to on macOS and Windows release builds (libc → mimalloc via src/boringssl/lib.rs). The Rust-side hooks already exist and have been live on ELF via weak linkage; signatures match the patch's declarations, and OPENSSL_memory_free zeroes before mi_free as the BoringSSL contract requires. There is no new parsing of untrusted input and no crypto logic change — only allocator routing. The main hazard class would be an alloc/free mismatch (e.g. something allocated via OPENSSL_malloc freed via raw free), but that hazard already existed on ELF and BoringSSL's own API discipline covers it.

Level of scrutiny

High — vendored-dependency patch to a crypto library affecting every TLS allocation, with cross-platform linker-behavior reasoning (weak vs strong refs on ELF/Mach-O/COFF). This is exactly the kind of change the repo guidelines flag for maintainer review. Jarred has already reviewed and requested the ASAN gate, which was applied in 690058f and marked resolved.

Other factors

  • The /INCLUDE: removal is safe post-ASAN-gate: under non-ASAN Windows the strong undefined refs in mem.cc.o pull the archive member; under ASAN Windows (if that config exists) the COFF weak path compiles the call sites out entirely, so the flags were never load-bearing there — same as before this PR.
  • Under ASAN on ELF, behavior is unchanged from main (weak symbols still bind to the Rust hooks). If the intent was to keep BoringSSL on the ASAN-intercepted libc heap on all platforms under ASAN, that would require a separate change to lib.rs; but Jarred's request was scoped to "this" (the define) and the thread is resolved.
  • CI on the pre-gate commit linked and passed tests on darwin-arm64, windows-x64 (all shards), windows-x64-baseline, and windows-aarch64; the reds were infra (a wedged darwin-x64 runner and Azure VM provisioning). Build 76541 for the ASAN-gated commit is running.
  • No automated test is feasible here (allocator routing is an object-file/link-time property); the PR body's nm/objdump/otool evidence is the appropriate proof.

@Jarred-Sumner
Jarred-Sumner merged commit 3d9617a into main Jul 21, 2026
77 of 79 checks passed
@Jarred-Sumner
Jarred-Sumner deleted the farm/b107465a/boringssl-mimalloc-hooks branch July 21, 2026 03:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants