Skip to content

picohttp: add lifetime parameter to Header #31179

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Jarred-Sumner wants to merge 4 commits into
base: main
Choose a base branch
from
Open

picohttp: add lifetime parameter to Header #31179

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5457ab3
picohttp: add lifetime parameter to Header
Jarred-Sumner May 21, 2026
c58e837
[autofix.ci] apply automated fixes
autofix-ci[bot] May 21, 2026
ac52c0b
http: clarify pending_response lifetime invariant in SAFETY comment
Jarred-Sumner May 21, 2026
9160ea0
s3: narrow SignResult::headers() borrow to &self
Jarred-Sumner May 21, 2026
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
1 change: 1 addition & 0 deletions scripts/rust-miri.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ const MIRI_CRATES = [
"bun_http_types",
"bun_md",
"bun_paths",
"bun_picohttp",
"bun_ptr",
"bun_resolve_builtins",
"bun_shell_parser",
Expand Down
4 changes: 3 additions & 1 deletion src/http/h2_client/Stream.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,9 @@ pub struct Stream {
/// consistent across multiple HEADERS in one read; the resulting strings
/// land here until `deliverStream` hands them to handleResponseMetadata.
pub decoded_bytes: Vec<u8>,
pub decoded_headers: Vec<picohttp::Header>,
/// Self-referential: each `Header`'s name/value point into `decoded_bytes`
/// (lifetime-erased to `'static`); valid until `decoded_bytes` is mutated.
pub decoded_headers: Vec<picohttp::Header<'static>>,
/// Final (non-1xx) status code; 0 until the response HEADERS arrive.
pub status_code: u32,

Expand Down
10 changes: 5 additions & 5 deletions src/http/h3_client/Stream.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,11 @@ pub struct Stream {
// FFI handle into lsquic; bound from `callbacks.onStreamOpen`, closed via `abort`.
pub qstream: Option<NonNull<quic::Stream>>,

/// Slices into the lsquic-owned hset buffer; valid only for the duration
/// of the `onStreamHeaders` callback that populated it. `cloneMetadata`
/// deep-copies synchronously inside that callback, so nothing reads these
/// after they go stale.
pub decoded_headers: Vec<picohttp::Header>,
/// Slices into the lsquic-owned hset buffer (lifetime-erased to
/// `'static`); valid only for the duration of the `onStreamHeaders`
/// callback that populated it. `cloneMetadata` deep-copies synchronously
/// inside that callback, so nothing reads these after they go stale.
pub decoded_headers: Vec<picohttp::Header<'static>>,
pub body_buffer: Vec<u8>,
pub status_code: u16,

Expand Down
11 changes: 9 additions & 2 deletions src/http/h3_client/callbacks.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,8 +216,15 @@ extern "C" fn on_stream_headers(s: *mut quic::Stream) {
i += 1;
continue;
};
let name = h.name_bytes();
let value = h.value_bytes();
// SAFETY: `name`/`value` point into the lsquic-owned hset buffer,
// which stays alive for the duration of this callback;
// `decoded_headers` is documented as valid only within this callback
// and is deep-copied (`cloneMetadata`) before the callback returns.
// Widened to `'static` so the headers can be stored in
// `Stream::decoded_headers`.
let name: &'static [u8] = unsafe { bun_ptr::detach_lifetime(h.name_bytes()) };
// SAFETY: same hset-buffer invariant as `name` above.
let value: &'static [u8] = unsafe { bun_ptr::detach_lifetime(h.value_bytes()) };
if name.first() == Some(&b':') {
if name == b":status" {
status = bun_core::fmt::parse_int::<u16>(value, 10).unwrap_or(0);
Expand Down
93 changes: 61 additions & 32 deletions src/http/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -794,26 +794,27 @@ pub type GenHttpContext<const SSL: bool> = http_context::HTTPContext<SSL>;
// ── header constants ────────────────────────────────────────────────────
const HOST_HEADER_NAME: &[u8] = b"Host";
const CONTENT_LENGTH_HEADER_NAME: &[u8] = b"Content-Length";
const CHUNKED_ENCODED_HEADER: picohttp::Header =
const CHUNKED_ENCODED_HEADER: picohttp::Header<'static> =
picohttp::Header::new(b"Transfer-Encoding", b"chunked");
const CONNECTION_HEADER: picohttp::Header = picohttp::Header::new(b"Connection", b"keep-alive");
const ACCEPT_HEADER: picohttp::Header = picohttp::Header::new(b"Accept", b"*/*");
const CONNECTION_HEADER: picohttp::Header<'static> =
picohttp::Header::new(b"Connection", b"keep-alive");
const ACCEPT_HEADER: picohttp::Header<'static> = picohttp::Header::new(b"Accept", b"*/*");

const ACCEPT_ENCODING_NO_COMPRESSION: &[u8] = b"identity";
const ACCEPT_ENCODING_COMPRESSION: &[u8] = b"gzip, deflate, br, zstd";
const ACCEPT_ENCODING_HEADER_COMPRESSION: picohttp::Header =
const ACCEPT_ENCODING_HEADER_COMPRESSION: picohttp::Header<'static> =
picohttp::Header::new(b"Accept-Encoding", ACCEPT_ENCODING_COMPRESSION);
const ACCEPT_ENCODING_HEADER_NO_COMPRESSION: picohttp::Header =
const ACCEPT_ENCODING_HEADER_NO_COMPRESSION: picohttp::Header<'static> =
picohttp::Header::new(b"Accept-Encoding", ACCEPT_ENCODING_NO_COMPRESSION);

const ACCEPT_ENCODING_HEADER: picohttp::Header = if FeatureFlags::DISABLE_COMPRESSION_IN_HTTP_CLIENT
{
ACCEPT_ENCODING_HEADER_NO_COMPRESSION
} else {
ACCEPT_ENCODING_HEADER_COMPRESSION
};
const ACCEPT_ENCODING_HEADER: picohttp::Header<'static> =
if FeatureFlags::DISABLE_COMPRESSION_IN_HTTP_CLIENT {
ACCEPT_ENCODING_HEADER_NO_COMPRESSION
} else {
ACCEPT_ENCODING_HEADER_COMPRESSION
};

fn get_user_agent_header() -> picohttp::Header {
fn get_user_agent_header() -> picohttp::Header<'static> {
let ua = OVERRIDDEN_DEFAULT_USER_AGENT.get().copied().unwrap_or(b"");
picohttp::Header::new(
b"User-Agent",
Expand Down Expand Up @@ -852,11 +853,12 @@ static PRINT_EVERY_I: AtomicUsize = AtomicUsize::new(0);
// we always rewrite the entire HTTP request when write() returns EAGAIN
// so we can reuse this buffer
const MAX_REQUEST_HEADERS: usize = 256;
static SHARED_REQUEST_HEADERS_BUF: bun_core::RacyCell<[picohttp::Header; MAX_REQUEST_HEADERS]> =
bun_core::RacyCell::new([picohttp::Header::ZERO; MAX_REQUEST_HEADERS]);
static SHARED_REQUEST_HEADERS_BUF: bun_core::RacyCell<
[picohttp::Header<'static>; MAX_REQUEST_HEADERS],
> = bun_core::RacyCell::new([picohttp::Header::ZERO; MAX_REQUEST_HEADERS]);

// this doesn't need to be stack memory because it is immediately cloned after use
static SHARED_RESPONSE_HEADERS_BUF: bun_core::RacyCell<[picohttp::Header; 256]> =
static SHARED_RESPONSE_HEADERS_BUF: bun_core::RacyCell<[picohttp::Header<'static>; 256]> =
bun_core::RacyCell::new([picohttp::Header::ZERO; 256]);

// the first packet for Transfer-Encoding: chunked
Expand All @@ -875,12 +877,13 @@ static SINGLE_PACKET_SMALL_BUFFER: bun_core::RacyCell<[u8; 16 * 1024]> =
mod scratch {
use super::*;
#[inline]
pub(super) fn request_headers() -> &'static mut [picohttp::Header; MAX_REQUEST_HEADERS] {
pub(super) fn request_headers() -> &'static mut [picohttp::Header<'static>; MAX_REQUEST_HEADERS]
{
// SAFETY: see module-level INVARIANT.
unsafe { &mut *SHARED_REQUEST_HEADERS_BUF.get() }
}
#[inline]
pub(super) fn response_headers() -> &'static mut [picohttp::Header; 256] {
pub(super) fn response_headers() -> &'static mut [picohttp::Header<'static>; 256] {
// SAFETY: see module-level INVARIANT.
unsafe { &mut *SHARED_RESPONSE_HEADERS_BUF.get() }
}
Expand Down Expand Up @@ -2056,15 +2059,19 @@ impl<'a> HTTPClient<'a> {
let mut override_connection_header = false;
let mut override_user_agent = false;
let mut add_transfer_encoding = true;
let mut original_content_length: Option<&[u8]> = None;
let mut original_content_length: Option<&'static [u8]> = None;

// Reserve slots for default headers that may be appended after user headers
// (Connection, User-Agent, Accept, Host, Accept-Encoding, Content-Length/Transfer-Encoding).
const MAX_DEFAULT_HEADERS: usize = 6;
const MAX_USER_HEADERS: usize = MAX_REQUEST_HEADERS - MAX_DEFAULT_HEADERS;

for (i, head) in header_names.iter().enumerate() {
let name = self.header_str(*head);
// SAFETY: `header_str` returns a slice into `self.header_buf`, which
// outlives the returned `Request` (see the SAFETY block on the
// `Request` literal at the end of this fn). Widened to `'static` so
// the `Header` can be stored in the `'static` scratch buffer.
let name: &'static [u8] = unsafe { bun_ptr::detach_lifetime(self.header_str(*head)) };
Comment on lines +2070 to +2074
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 21, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== build_request signature and lifetime erasure =="
rg -n -C3 "pub fn build_request|Request<'static>|detach_lifetime" src/http/lib.rs

echo
echo "== build_request call sites =="
rg -n -C2 '\.build_request\(' -g '*.rs'

Repository: oven-sh/bun

Length of output: 7175

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Inspect send_initial_request_payload =="
rg -n "send_initial_request_payload" src/http/lib.rs src/http -S

echo
echo "== Inspect lib.rs call site around line ~2550 =="
sed -n '2520,2605p' src/http/lib.rs

echo
echo "== Inspect lib.rs call site around line ~2920 =="
sed -n '2890,2995p' src/http/lib.rs

echo
echo "== Inspect H2Client build_request call =="
sed -n '110,190p' src/http/H2Client.rs

echo
echo "== Inspect h3_client encode build_request usage =="
sed -n '1,120p' src/http/h3_client/encode.rs

Repository: oven-sh/bun

Length of output: 16784

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Inspect send_initial_request_payload =="
rg -n "send_initial_request_payload" src/http/lib.rs src/http -S

echo
echo "== Inspect lib.rs call site around line ~2550 =="
sed -n '2520,2605p' src/http/lib.rs

echo
echo "== Inspect lib.rs call site around line ~2920 =="
sed -n '2890,2995p' src/http/lib.rs

echo
echo "== Inspect H2Client build_request call =="
sed -n '110,190p' src/http/H2Client.rs

echo
echo "== Inspect h3_client encode build_request usage =="
sed -n '1,120p' src/http/h3_client/encode.rs

Repository: oven-sh/bun

Length of output: 16784

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Inspect HTTPClient struct fields around header_buf / url =="
rg -n "struct HTTPClient|pub struct HTTPClient" src/http/lib.rs

echo
# Print a window around the first match
line=$(rg -n "struct HTTPClient|pub struct HTTPClient" src/http/lib.rs | head -n1 | cut -d: -f1)
start=$((line-80)); end=$((line+120))
sed -n "${start},${end}p" src/http/lib.rs

echo
echo "== Inspect build_request impl block (around 2040-2270) =="
sed -n '2025,2275p' src/http/lib.rs

echo
echo "== Search for any storage/escape of picohttp::Request<'static> from build_request =="
rg -n "picohttp::Request<'static>" src/http -S

echo
rg -n "build_request\\(.*\\)\\.detach_lifetime|detach_lifetime\\(\\).*build_request|store.*Request|pending_.*Request|state\\..*Request" src/http/lib.rs src/http -S

Repository: oven-sh/bun

Length of output: 27047

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Inspect AsyncHTTP struct and where .request is assigned =="
sed -n '1,120p' src/http/AsyncHTTP.rs
rg -n "\.request\s*=" src/http/AsyncHTTP.rs src/http/lib.rs src/http -S

echo
echo "== Inspect HTTPClient struct definition to see what header_buf/url actually are =="
sed -n '560,760p' src/http/lib.rs

echo
echo "== Find definitions of header_buf and request_buffers/scratch used by build_request =="
rg -n "header_buf|SHARED_REQUEST_HEADERS_BUF|scratch::request_headers" src/http/lib.rs src/http -S

echo
echo "== Inspect where build_request result is used/stored in lib.rs beyond local scope =="
rg -n "AsyncHTTP.*request|state\..*request|pending.*request|Request<'static>" src/http/lib.rs src/http -S

Repository: oven-sh/bun

Length of output: 48900

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Inspect AsyncHTTP.rs around where self.request is assigned =="
rg -n "self\.request\s*=" src/http/AsyncHTTP.rs
sed -n '300,740p' src/http/AsyncHTTP.rs

echo
echo "== Inspect lib.rs for public methods returning picohttp::Request =="
rg -n "pub (fn|type).*picohttp::Request" src/http/lib.rs src/http -S

echo
echo "== Inspect HTTPClient::build_request usage storage in HTTPClient/AsyncHTTP =="
rg -n "build_request\\(.*\\).*self\\.request|self\\.request\\s*=.*build_request|request: Some\\(.*build_request" src/http -S

Repository: oven-sh/bun

Length of output: 18668

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Search for storage of AsyncHTTP.request = Some(...) =="
rg -n "\.request\s*=\s*Some\(" src/http -S

echo
echo "== Search for uses of AsyncHTTP.request (take/unwrap/borrow) =="
rg -n "async_http\.request|self\.request\.|\.request\.take\(\)|\.request\.as_ref\(\)" src/http -S

echo
echo "== List all occurrences of 'AsyncHTTP.request' field =="
rg -n "AsyncHTTP<'[^>]*>\s*\{|\brequest:\s*Option<picohttp::Request<'static>>\b|pub request: Option<picohttp::Request<'static>>" src/http -S

Repository: oven-sh/bun

Length of output: 113

Avoid lifetime escape from HTTPClient::build_request returning picohttp::Request<'static>
HTTPClient::build_request widens borrows from self.header_buf / self.url to 'static via bun_ptr::detach_lifetime(...) and returns picohttp::Request<'static> from a safe pub fn. Even if current send paths use the Request only transiently to write headers/body, the signature still lets a caller stash and use it after self/backing buffers are gone. Tie the return lifetime to &mut self (e.g., picohttp::Request<'a> / picohttp::Request<'_>) or move the 'static widening behind a private/unsafe boundary so it can’t escape as a safe API.

🤖 Prompt for 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.

In `@src/http/lib.rs` around lines 2070 - 2074, The function
HTTPClient::build_request currently widens borrows from self.header_buf and
self.url to 'static via bun_ptr::detach_lifetime and returns
picohttp::Request<'static>, which lets callers retain references after self is
dropped; change the API to tie the Request lifetime to &mut self (e.g., make
build_request return picohttp::Request<'a> or picohttp::Request<'_>) so the
header/url borrows cannot escape, remove the use of bun_ptr::detach_lifetime on
values produced from self.header_str and self.url, and adjust call sites to
accept the shorter lifetime; alternatively, if 'static is truly required
internally, move the unsafe bun_ptr::detach_lifetime widening behind a private
unsafe helper (not exposed from the public fn) and keep the public
HTTPClient::build_request signature borrowing from &mut self to prevent safe
lifetime escape.

Copy link
Copy Markdown
Collaborator Author

@Jarred-Sumner Jarred-Sumner May 21, 2026

Choose a reason for hiding this comment

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

This is the pre-existing shape of build_request, not something this PR introduced: on main the function already returns picohttp::Request<'static> and already widens path and the headers slice with bun_ptr::detach_lifetime (see the SAFETY block on the Request literal at the end of the fn). The signature, the 'static return, and the callers (AsyncHTTP.request: Option<Request<'static>>, h2_build_request) are unchanged here.

What changed is only where the widening happens: now that Header carries a lifetime, the name/value/host slices have to be widened before Header::new instead of implicitly losing their lifetime inside a lifetime-less Header. The erasure is the same; it is just visible (and documented) now.

Re-tying build_request's return to &mut self is a real improvement but it is a separate refactor of the detach_lifetime family (it ripples into AsyncHTTP's stored request, the EAGAIN re-write path, and the h2/h3 bridges), and is out of scope for this PR, which is specifically about giving Header a lifetime parameter.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 21, 2026

Choose a reason for hiding this comment

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

@Jarred-Sumner, understood — the 'static return and the detach_lifetime usage in build_request are pre-existing, and this PR's only change is surfacing the widening before Header::new (because Header<'buf> now requires it to be explicit). The safety contract is identical to what was there before.

I'll note this context for future reviews. Would you like me to open a follow-up issue to track the longer-term refactor of tying build_request's return lifetime to &mut self (which would ripple into AsyncHTTP.request, the EAGAIN re-write path, and the h2/h3 bridges)?

✏️ Learnings added 
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 31179
File: src/http/lib.rs:2070-2074
Timestamp: 2026-05-21T10:05:03.090Z
Learning: In oven-sh/bun `src/http/lib.rs`, `HTTPClient::build_request` intentionally returns `picohttp::Request<'static>` and widens `self.header_buf`/`self.url` slices via `bun_ptr::detach_lifetime` before constructing `picohttp::Header` values. This is pre-existing design: callers store the result in `AsyncHTTP.request: Option<Request<'static>>` and reuse it on EAGAIN re-write / h2/h3 bridges. Do NOT flag this as a lifetime-escape issue. The `'static` widening is safe under the documented invariant (header_buf/url live for the client's lifetime), and tying the return to `&mut self` is a separate, explicitly deferred refactor.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

🧠 Learnings used 
Learnt from: robobun
Repo: oven-sh/bun PR: 30522
File: src/runtime/cli/publish_command.rs:914-916
Timestamp: 2026-05-15T11:58:16.766Z
Learning: In this repo’s Rust code, do not flag a “typed accessor missing” or similar violation when reading `npm_config_*` environment variables. For `npm_config_*` keys (e.g. `NPM_CONFIG_REGISTRY`, `NPM_CONFIG_PROVENANCE`), the accepted pattern is to read them via `bun_core::getenv_z` using both upper-case and lower-case forms combined with `.or_else(...)`. This dual `getenv_z` approach is intentional and consistent even when `src/bun_core/env_var.rs` does not provide typed accessors for specific `npm_config_*` variables (e.g., in `PackageManagerOptions::load` and `publish_command.rs`).

Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 31176
File: src/io/uv_handle.rs:92-100
Timestamp: 2026-05-21T09:34:31.660Z
Learning: In the oven-sh/bun repo, review allocator-mismatch/safety warnings carefully around raw `Box` round-trips. `bun_core::heap::{into_raw, take, destroy, alloc}` are `#[inline(always)]` thin aliases with identical machine code to `Box::into_raw`/`Box::from_raw` and do not provide additional allocator-mismatch protection or ASAN-risk reduction.

Therefore, when a value is wrapped in a *typed owner* that owns both halves of the raw round-trip (e.g., similar to patterns like `UvHandle<H,T>`, `WorkPool::schedule_owned`, etc.), it is the correct idiomatic form to use `Box::leak` and then `Box::from_raw` (open-coded) inside that wrapper. In this case, do not flag `Box::leak`/`Box::from_raw` usage as an allocator-mismatch risk and do not suggest replacing it with `heap::into_raw`/`take`.

// Hash it as lowercase
let hash = hash_header_name(name);

Expand All @@ -2079,7 +2086,10 @@ impl<'a> HTTPClient<'a> {
match hash {
h if h == hash_header_const(b"Content-Length") => {
// Content-Length is always consumed (never written to the buffer).
original_content_length = Some(self.header_str(header_values[i]));
// SAFETY: same `header_buf` widening as `name` above.
original_content_length = Some(unsafe {
bun_ptr::detach_lifetime(self.header_str(header_values[i]))
});
continue;
}
h if h == hash_header_const(b"Connection") => {
Expand Down Expand Up @@ -2154,8 +2164,10 @@ impl<'a> HTTPClient<'a> {
continue;
}

request_headers_buf[header_count] =
picohttp::Header::new(name, self.header_str(header_values[i]));
// SAFETY: same `header_buf` widening as `name` above.
let value: &'static [u8] =
unsafe { bun_ptr::detach_lifetime(self.header_str(header_values[i])) };
request_headers_buf[header_count] = picohttp::Header::new(name, value);

header_count += 1;
}
Expand All @@ -2176,8 +2188,11 @@ impl<'a> HTTPClient<'a> {
}

if !override_host_header {
request_headers_buf[header_count] =
picohttp::Header::new(HOST_HEADER_NAME, self.url.host);
// SAFETY: `url.host` borrows `self.url`, which outlives the
// returned `Request` (see the SAFETY block on the `Request` literal
// at the end of this fn).
let host: &'static [u8] = unsafe { bun_ptr::detach_lifetime(self.url.host) };
request_headers_buf[header_count] = picohttp::Header::new(HOST_HEADER_NAME, host);
header_count += 1;
}

Expand Down Expand Up @@ -3097,8 +3112,23 @@ impl<'a> HTTPClient<'a> {
}

let shared_resp = scratch::response_headers();
// SAFETY: the parsed response borrows `SHARED_RESPONSE_HEADERS_BUF`
// and the bytes behind `to_read` (`incoming_data` for the duration
// of this callback, or `response_message_buffer` which lives on
// `self.state`). Both outlive every *read* of the response: it is
// only read within this callback (`handle_response_metadata`) and
// by `clone_metadata()`, which deep-copies it. The early-return
// paths that skip `clone_metadata()` (redirect, proxy CONNECT)
// overwrite or clear `state.pending_response` before the next read
// (`Response::default()` at the top of the next parse;
// `ProxyTunnel` sets it to `None`). Widened to `'static` so it can
// be parsed into the `'static` scratch buffer and stored in
// `state.pending_response`. This matches the pre-existing
// `detach_lifetime()` that was applied to the parsed response
// here before `Header` carried a lifetime.
let parse_buf: &'static [u8] = unsafe { bun_ptr::detach_lifetime(to_read!()) };
let response = match picohttp::Response::parse_parts(
to_read!(),
parse_buf,
shared_resp,
Comment on lines +3115 to 3132
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 21, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

pending_response can outlive incoming_data on the CONNECT path.

This detached parse_buf is stored in self.state.pending_response before we know clone_metadata() will run. On the successful proxy CONNECT branch, handle_on_data_headers returns via start_proxy_handshake() without cloning or clearing that field, so pending_response can keep a Response<'static> pointing at callback-owned bytes after this callback exits. Clear pending_response before that return, or deep-copy on that path too.

🤖 Prompt for 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.

In `@src/http/lib.rs` around lines 3115 - 3125, The parse_buf created via unsafe
detach_lifetime is stored into self.state.pending_response after
Response::parse_parts, but on the successful CONNECT branch in
handle_on_data_headers the code returns through start_proxy_handshake() without
calling clone_metadata(), so pending_response may hold a Response<'static> that
points at callback-owned bytes that will be freed; to fix, ensure
pending_response is either cleared or deep-copied before that early return:
update handle_on_data_headers to call clone_metadata() (or explicitly clone the
Response metadata) or set self.state.pending_response = None immediately before
calling start_proxy_handshake(), so no borrowed data from parse_buf can outlive
the callback.

Copy link
Copy Markdown
Collaborator Author

@Jarred-Sumner Jarred-Sumner May 21, 2026

Choose a reason for hiding this comment

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

Verified — the stored Response<'static> on the proxy CONNECT path is pre-existing behavior, not introduced here. On main the same path already does let response = unsafe { response.detach_lifetime() }; self.state.pending_response = Some(response); and then takes the same start_proxy_handshake() early return without clone_metadata(). This PR only moves the lifetime widening from the parsed response to the parse buffer; the value stored in pending_response is bit-identical.

It is also not a live use-after-free: the stale CONNECT response is never read after the callback returns. ProxyTunnel explicitly sets this.state.pending_response = None (ProxyTunnel.rs:354) without reading the contents, and the next handle_on_data_headers pass overwrites it with Response::default() before any read. The only reader of the stored value is clone_metadata(), which runs within the same callback that parsed it on the non-proxy paths.

The new SAFETY comment did overclaim ("deep-copied by clone_metadata() before this fn returns"), though — fixed in ac52c0b to describe the actual invariant including the redirect/proxy early returns.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 21, 2026

Choose a reason for hiding this comment

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

@Jarred-Sumner, thank you for the thorough explanation. You're right on all counts:

  1. The behavior is pre-existing — the same detach_lifetime + start_proxy_handshake pattern without clone_metadata existed on main before this PR, so this change is bit-identical in what gets stored.
  2. There's no live use-after-free: ProxyTunnel clears pending_response at line 354 of ProxyTunnel.rs before any read, and the next parse loop overwrites it with Response::default() up front.
  3. The updated SAFETY comment in ac52c0b accurately describes the actual invariant (covering the redirect/proxy early-return paths) — that's a good improvement in documentation clarity.

I was wrong to flag this as a critical issue. Apologies for the noise.

🧠 Learnings used 
Learnt from: robobun
Repo: oven-sh/bun PR: 30522
File: src/runtime/cli/publish_command.rs:914-916
Timestamp: 2026-05-15T11:58:16.766Z
Learning: In this repo’s Rust code, do not flag a “typed accessor missing” or similar violation when reading `npm_config_*` environment variables. For `npm_config_*` keys (e.g. `NPM_CONFIG_REGISTRY`, `NPM_CONFIG_PROVENANCE`), the accepted pattern is to read them via `bun_core::getenv_z` using both upper-case and lower-case forms combined with `.or_else(...)`. This dual `getenv_z` approach is intentional and consistent even when `src/bun_core/env_var.rs` does not provide typed accessors for specific `npm_config_*` variables (e.g., in `PackageManagerOptions::load` and `publish_command.rs`).

Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 31176
File: src/io/uv_handle.rs:92-100
Timestamp: 2026-05-21T09:34:31.660Z
Learning: In the oven-sh/bun repo, review allocator-mismatch/safety warnings carefully around raw `Box` round-trips. `bun_core::heap::{into_raw, take, destroy, alloc}` are `#[inline(always)]` thin aliases with identical machine code to `Box::into_raw`/`Box::from_raw` and do not provide additional allocator-mismatch protection or ASAN-risk reduction.

Therefore, when a value is wrapped in a *typed owner* that owns both halves of the raw round-trip (e.g., similar to patterns like `UvHandle<H,T>`, `WorkPool::schedule_owned`, etc.), it is the correct idiomatic form to use `Box::leak` and then `Box::from_raw` (open-coded) inside that wrapper. In this case, do not flag `Box::leak`/`Box::from_raw` usage as an allocator-mismatch risk and do not suggest replacing it with `heap::into_raw`/`take`.

Some(&mut amount_read),
) {
Expand All @@ -3125,12 +3155,8 @@ impl<'a> HTTPClient<'a> {
};

// we save the successful parsed response
// SAFETY: response borrows SHARED_RESPONSE_HEADERS_BUF / response_message_buffer,
// both of which outlive this fn; widen to 'static for storage.
// Rebind `response` to the detached `'static` copy so it no longer
// borrows `to_read` (lets the `to_read` reassignment below pass
// borrowck — `RawSlice::slice` ties output to `&to_read`).
let response = unsafe { response.detach_lifetime() };
// (`response` is already `Response<'static>` — `parse_buf` and the
// scratch buffer were both widened to `'static` above.)
self.state.pending_response = Some(response);

let bytes_read =
Expand Down Expand Up @@ -3429,7 +3455,10 @@ impl<'a> HTTPClient<'a> {
let headers_buf = bun_core::heap::release(
vec![picohttp::Header::ZERO; response.headers.list.len()].into_boxed_slice(),
);
let cloned_response = response.clone(headers_buf, &mut builder);
// SAFETY: the cloned response's slices alias `builder`'s heap
// buffer, whose ownership is transferred to `owned_buf` below and
// stored alongside the response in `HTTPResponseMetadata`.
let cloned_response = unsafe { response.clone(headers_buf, &mut builder) };

// we clean the temporary response since cloned_metadata is now the owner
self.state.pending_response = None;
Expand Down
26 changes: 22 additions & 4 deletions src/http_jsc/websocket_client/WebSocketUpgradeClient.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,11 @@ pub struct HTTPClient<const SSL: bool> {
// suffix of `input_body_buf`; stored here as the suffix length so we don't
// hold a self-referential slice.
to_send_len: usize,
headers_buf: [picohttp::Header; 128],
/// Scratch storage for `Response::parse`. The parsed headers point into
/// the response bytes (`data` / `self.body`), are lifetime-erased to
/// `'static`, and are only read synchronously inside the receive callback
/// that parsed them.
headers_buf: [picohttp::Header<'static>; 128],
body: Vec<u8>,
/// Owned NUL-terminated hostname for SNI; empty when unset.
hostname: ZBox,
Expand Down Expand Up @@ -952,7 +956,13 @@ impl<const SSL: bool> HTTPClient<SSL> {
}
}

let response = match picohttp::Response::parse(body, &mut me.headers_buf) {
// SAFETY: `body` aliases `data` (live for this callback) or `me.body`;
// the parsed response (and `me.headers_buf`, which its headers point
// into alongside `body`) is only read synchronously below, before
// either buffer is mutated or freed. Widened to `'static` to match the
// `'static` element type of the `me.headers_buf` scratch array.
let parse_buf: &'static [u8] = unsafe { bun_ptr::detach_lifetime(body) };
let response = match picohttp::Response::parse(parse_buf, &mut me.headers_buf) {
Ok(r) => r,
Err(picohttp::ParseResponseError::MalformedHttpResponse) => {
// SAFETY: `me`'s last use is above; no `&mut Self` spans this call.
Expand Down Expand Up @@ -1011,7 +1021,11 @@ impl<const SSL: bool> HTTPClient<SSL> {
}

// Parse the response to find the end of headers
let response = match picohttp::Response::parse(body, &mut me.headers_buf) {
// SAFETY: same invariant as the `handle_data` parse — `body` and
// `me.headers_buf` are only read synchronously below, before either is
// mutated or freed.
let parse_buf: &'static [u8] = unsafe { bun_ptr::detach_lifetime(body) };
let response = match picohttp::Response::parse(parse_buf, &mut me.headers_buf) {
Ok(r) => r,
Err(picohttp::ParseResponseError::MalformedHttpResponse) => {
// SAFETY: `me`'s last use is above; no `&mut Self` spans this call.
Expand Down Expand Up @@ -1248,7 +1262,11 @@ impl<const SSL: bool> HTTPClient<SSL> {
}
}

let response = match picohttp::Response::parse(body, &mut me.headers_buf) {
// SAFETY: same invariant as the `handle_data` parse — `body` and
// `me.headers_buf` are only read synchronously below, before either is
// mutated or freed.
let parse_buf: &'static [u8] = unsafe { bun_ptr::detach_lifetime(body) };
let response = match picohttp::Response::parse(parse_buf, &mut me.headers_buf) {
Ok(r) => r,
Err(picohttp::ParseResponseError::MalformedHttpResponse) => {
// SAFETY: `me`'s last use is above; no `&mut Self` spans this call.
Expand Down
Loading
Loading