Skip to content

fetch.preconnect: let the Preconnect record own the href - #47023

Draft
robobun wants to merge 1 commit into
mainfrom
robobun/45170a85/preconnect-owned-href
Draft

robobun wants to merge 1 commit into
mainfrom
robobun/45170a85/preconnect-owned-href

Conversation

@robobun

@robobun robobun commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator

Draft until #46528 and #46522 are merged. See "Landing order" in Notes.

Behaviour change: none

Problem

  • free_owned_href(href: &'static [u8]) (src/http/AsyncHTTP.rs:93) frees the Box<[u8]> behind its own reference parameter. mordant reports this call to destroy frees the value behind href while href is still borrowed. Miri rejects the free. No crash is known.
  • No type owns the href of fetch.preconnect(url). fetch.rs leaks the box and frees it by hand on four early returns.

Fix

  • preconnect() takes a bun_url::OwnedURL. The Preconnect record holds it after async_http, so the href drops after the request that views it.
  • fetch.rs borrows the OwnedURL to validate. --fetch-preconnect passes a copy of its argument. free_owned_href, is_url_owned, reclaim_href! and the baseline line "freed_while_borrowed:src/http/AsyncHTTP.rs" = 1 are gone.
  • bun_http joins the Miri crates with one test of the record. On main's shape the test fails: Undefined Behavior: deallocation through <tag> ... is forbidden.
  • Verified: bun run rust:miri -p bun_http, test/js/web/fetch/fetch-preconnect.test.ts, mordant, clippy. Self-reviewed: 15 concerns raised, 15 addressed.

Background

Downsides

  • A scheduled fetch.preconnect() parses the href twice: +697 instructions (1,103 to 1,799, release build). A --fetch-preconnect flag copies and parses its URL once more: +716 instructions, one allocation, one free.
  • The Miri job builds six more crates: 42 s of build and 8 s of run in my container.
Notes

Measurements (main 449877f against this branch)

  • Miri, the repo's flags (bun run rust:miri -p bun_http, Tree Borrows): the record test passes on this branch. The same test written for main's record (url: URL<'static>, is_url_owned: true) fails with Undefined Behavior: deallocation through <tag> at alloc[0x0] is forbidden. The conflicting tag is Frozen, created at src/http/AsyncHTTP.rs:99:42 (core::ptr::from_ref(href)).
  • A standalone reduction of the two callers, the record and the HTTP-thread callback gives the same result under Stacked Borrows (trying to retag ... for Unique permission ..., but that tag only grants SharedReadOnly permission). In that reduction a raw-pointer parameter for free_owned_href, which is what the lint suggests, still fails under both models.
  • URL::parse per scheduled fetch.preconnect: 1 -> 2 (gdb hit count over 20 calls). Per throwing call: 1 -> 1. Per --fetch-preconnect flag: 1 -> 2 (the instruction count of URL::parse_host goes from 304 to 608 per flag).
  • Instructions, release bun-profile, qemu-x86_64 -one-insn-per-tb exact count in Bun__Bindgen__Fetch__preconnect, async_http::preconnect, Preconnect::on_result, RunCommand::do_preconnect and the bun_url::URL parse functions:
    • scheduled call: 1,103 -> 1,799 (200 calls). The second parse is +787. async_http::preconnect no longer clones the 280-byte view.
    • throwing call: 859 -> 857 (200 calls).
    • --fetch-preconnect: 1,039 -> 1,755 per flag (1 and 21 flags). This count does not include the allocator.
    • perf is not installed in the container, so there is no perf stat line.
  • Allocator calls of the whole process (entries of the mimalloc functions, same qemu method): per scheduled fetch.preconnect 4.06 -> 4.06 mi_malloc and 4.03 -> 4.03 mi_free (200 calls). Per --fetch-preconnect flag 5.2 -> 6.25 mi_malloc and 3.6 -> 4.65 mi_free (1 and 21 flags): one more of each, for the copy of the href.
  • size_of::<Preconnect>(): 2496 -> 2224 bytes (gdb, debug builds).
  • Release binary: .text 82,164,617 -> 82,163,849 bytes, .rodata the same (size -A; bloaty is not installed). async_http::preconnect 1,333 -> 633 bytes, Preconnect::on_result 166 -> 96, do_preconnect 1,089 -> 1,208.
  • mordant 564c8da, three targets: exit 0 with the line deleted. Linux listing with no baseline: 684 -> 683 findings, src/http 35 -> 34, new findings 0.
  • Miri job: bun_http adds six crates to its build (bun_http, bun_picohttp, bun_dns, bun_brotli, bun_brotli_sys, bun_libdeflate_sys). In my container that is 42 s of build after the 146 s for the other 18 crates, and 8 s to run.
  • cargo clippy -p bun_http -p bun_runtime --no-deps and cargo check -p bun_http --all-targets: no errors.
  • fetch-session.test.ts ("fetch.preconnect does not dial an origin the environment proxies") passes on the debug ASAN build.

Landing order

What does not change

  • Validation order, error codes and messages of fetch.preconnect() and --fetch-preconnect.
  • fetch.preconnect() still allocates the href before it validates it, and frees it on an early return.
  • The href is freed in Preconnect::on_result as before. It is now freed after the AsyncHTTP drop, not before. Nothing reads it in between.
  • do_preconnect keeps its signature. The CLI strings stay in the context and are never freed. The record owns a copy, so the from_raw_parts forge of a 'static slice in do_preconnect is gone.

Not in this change

  • Two other frees rebuild a Box from a shared slice: impl Drop for HTTPResponseMetadata (src/http/lib.rs:185) and TranspilerJob::reset_for_pool (src/jsc/RuntimeTranspilerStore.rs:506). mordant does not report them, because the reference is a field and not a parameter. They need their own owner types and are being handled separately.
  • HTTPClient::dispatch_preconnect_result(&mut self) still ends in a free of the HTTP-thread request copy that embeds self. It is the &mut self teardown class that fetch: release the FetchTasklet through its raw pointer, not a &mut receiver #37703 addresses for another type.
  • For the ratchet: the lint's help for freed_while_borrowed ("take a raw pointer in place of the reference") makes mordant silent and leaves the free rejected by Miri. A line of this lint is only done when the owner frees through the pointer or value it holds.

Self-review

`free_owned_href` freed a `Box<[u8]>` through its own `&'static [u8]`
parameter, and a `bool` beside a `URL<'static>` said who owned the
bytes. mordant reports the free as `freed_while_borrowed`, and Miri
rejects it under Tree Borrows.

`preconnect()` now takes a `bun_url::OwnedURL`. The `Preconnect` record
holds it in a field declared after `async_http`, so the href drops
after the request that views it. `fetch.rs` validates through an
ordinary borrow of the `OwnedURL`: the leak, the forged `&'static [u8]`
and the four hand-written frees are gone. `--fetch-preconnect` passes a
copy of its argument, so the forged `'static` in `do_preconnect` goes
too.

`bun_http` joins the Miri crates with one test of the record. The test
is undefined behaviour on the old shape.

Removes the `freed_while_borrowed:src/http/AsyncHTTP.rs` line from
mordant-baseline.toml.
@robobun

robobun commented Oct 26, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 6:33 PM PT - Oct 26th, 2026

✅ @robobun, your commit b75f4fc2876412a4722311525cfea29dd837d5bf passed in Build #133314! 🎉


🧪   To try this PR locally:

bunx bun-pr 47023

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

bun-47023 --bun

@robobun

robobun commented Oct 26, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status: draft. It waits for #46528 and #46522, then it is cut again on top of them. The branch conflicts with main in src/runtime/cli/run_command.rs since #45631 and #47382 changed do_preconnect. The new cut resolves that.

  • Reproduced: cargo mordant at the pinned MORDANT_REV reports this call to destroy frees the value behind href while href is still borrowed at src/http/AsyncHTTP.rs:99 on main. The record test of this PR, written for main's record, fails under bun run rust:miri -p bun_http with Undefined Behavior: deallocation through <tag> ... is forbidden.
  • Checked: bun run rust:miri passes with bun_http added (19 crates). bun run rust:mordant exits 0 on the three targets with the baseline line deleted. bun bd test test/js/web/fetch/fetch-preconnect.test.ts passes (14 tests, ASAN build). The mordant, cargo miri test and cargo clippy jobs passed on b75f4fc.
  • PR: fetch.preconnect: let the Preconnect record own the href #47023

@robobun

robobun commented Oct 27, 2026

Copy link
Copy Markdown
Collaborator Author

#47098 changes two things that this PR meets when it is cut again.

  • It deletes AsyncHTTP::clear_data() and the call in Preconnect::on_result. AsyncHTTP.response is gone there, and the method cleared nothing else. git merge-tree reports one conflict region: the deleted call is directly above the lines that this PR rewrites. The resolution takes both deletions.
  • It extends test/internal/source-lints/self-receiver-reclaim.test.ts to ban a reclaim through ptr::from_ref(..). free_owned_href is allowed there by an exact count, "src/http/AsyncHTTP.rs": 1. This PR deletes the function, so it must delete that entry too. If it does not, the test allowlisted files still carry exactly their documented count fails.

Both PRs add the same "bun_http" line to scripts/rust-miri.ts. That line merges with no conflict.

This branch has not been deployed

No deployments
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