Skip to content

picohttp: add lifetime parameter to Header#31179

Open
Jarred-Sumner wants to merge 4 commits into
mainfrom
claude/s0-1-picohttp-header-lifetime
Open

picohttp: add lifetime parameter to Header#31179
Jarred-Sumner wants to merge 4 commits into
mainfrom
claude/s0-1-picohttp-header-lifetime

Conversation

@Jarred-Sumner
Copy link
Copy Markdown
Collaborator

What

bun_picohttp::Header stored name/value as raw (ptr, len) pairs into the caller's parse buffer with no lifetime parameter, and name() / value() were safe functions returning slices tied to &self instead of to the buffer. Safe code could drop the buffer, keep the (Copy) Header, and read a dangling pointer:

let h = { let v = b"x".to_vec(); Header::new(&v, &v) }; // compiles today
h.name(); // use-after-free, no `unsafe` anywhere

Header is now Header<'buf>:

  • Header::new(name: &'buf [u8], value: &'buf [u8]) -> Header<'buf> ties the result to its inputs.
  • name() / value() return &'buf [u8].
  • Request::parse / Response::parse / Headers::parse return headers borrowed from their input slice.
  • The lifetime lives in a PhantomData<&'buf [u8]> field; the repr(C) layout and the static asserts against phr_header are unchanged, so the FFI surface is identical.

The example above is now a compile error.

Sites converted / left unsafe

  • Converted: Header::new, Header::name, Header::value — the safe surface can no longer produce a dangling read. The unsafe { ffi::slice(...) } blocks inside name()/value() remain (raw-pointer deref) but their invariant is now carried by the type.
  • Now unsafe fn (were unsound safe fns): Header::clone, Request::clone, Response::clone. They return data aliasing the StringBuilder's heap buffer — a precondition the compiler cannot check — so they become unsafe fn with # Safety contracts instead of safe wrappers.
  • New unsafe fn: Header::detach_lifetime, mirroring the existing Request::detach_lifetime / Response::detach_lifetime, for the call sites that intentionally store headers in 'static scratch buffers or self-referential structs.

Unsafe count (rg 'unsafe \{|unsafe fn|unsafe extern|unsafe impl|unsafe trait')

file before after
src/picohttp/lib.rs 22 29
src/http/** 201 208
src/http_jsc/.../WebSocketUpgradeClient.rs 178 181
src/s3_signing/credentials.rs 5 18

The textual count goes up: every place that was already erasing the buffer lifetime implicitly (by storing a lifetime-less Header in a 'static scratch buffer or a struct field) now does so through an explicit detach_lifetime with a SAFETY: comment. The safe surface is what got smaller — none of those erasures are reachable from safe code anymore.

Cost

Zero-cost. PhantomData is a ZST; the generated code for Header is unchanged (layout asserts still pass). The one behavioral reshuffle is in fetch.rs's S3 header mixing, where the replacement Headers is now built before the old value is overwritten instead of inside set_headers — same allocations, same order of operations.

Files outside src/picohttp / src/http

Adding the lifetime parameter forced mechanical edits in four files that name the Header type in struct fields or function signatures:

  • src/http_jsc/websocket_client/WebSocketUpgradeClient.rsheaders_buf field annotation + explicit widening of the response buffer at the 3 parse sites.
  • src/s3_signing/credentials.rsSignResult::_headers field annotation, mix_with_header lifetimes, and pico_header_new becomes unsafe fn (it builds the self-referential headers that point into SignResult's own Box<[u8]> fields).
  • src/runtime/webcore/fetch.rs — the S3 Content-Type mix-in borrows the Headers value it replaces; restructured to deep-copy before overwriting.
  • scripts/rust-miri.ts — adds bun_picohttp to the Miri allowlist.

Callers found holding headers past the buffer (pre-existing, not fixed here)

  • SignResult::_headers[0] stores a header whose value is sign_options.content_hash — a caller-borrowed slice that is not required to outlive the returned SignResult. The conversion documents this as the # Safety precondition of pico_header_new but does not change the behavior.
  • The h2/h3 Stream::decoded_headers vectors and the WebSocket headers_buf field retain stale pointers after their backing buffers are reused; all are documented as "read only synchronously before the buffer is touched" and now carry that invariant in their SAFETY: comments.

Tests

  • cargo test -p bun_picohttp — 5 new unit tests covering Header::new/name/value/ZERO, HeaderList lookup, Header::clone through StringBuilder, and a parser-shaped header array pointing into a request buffer.
  • bun run rust:miri -p bun_picohttp passes (-Zmiri-tree-borrows). The phr_* C entry points cannot be linked by cargo test or interpreted by Miri (they are compiled by the CMake build), so the C parse round-trip itself is covered by the JS HTTP suites, not the Rust unit tests.
  • bun bd test test/js/web/fetch/fetch.test.ts — same pass/fail set as an unmodified debug build of the merge-base (the failures are root-user permission tests, external-network tests, and 5s-timeout GC stress tests; verified by rebuilding the baseline and diffing the failure lists).
  • WebSocket client suites (websocket.test.js, upgrade, utf16-headers, subprotocol, accept-header-validation, short-read, custom-headers) pass; the one failure ("connect many times over https") is a 300-TLS-handshake stress test that exceeds the 5s timeout in debug builds.

Header stored name/value as raw (ptr, len) pairs into the caller's parse
buffer with no lifetime parameter, and name()/value() were safe functions
returning slices tied to &self instead of to the buffer. Safe code could
drop the buffer, keep the (Copy) Header, and read a dangling pointer.

Header is now Header<'buf>: new() ties the result to its inputs,
name()/value() return &'buf [u8], and the parse functions return headers
borrowed from their input slice, so the use-after-free becomes a compile
error. A PhantomData field carries the lifetime; the repr(C) layout (and
the static asserts against phr_header) are unchanged.

Call sites that intentionally store headers past a borrow (the 'static
scratch buffers in the HTTP client, the self-referential SignResult and
h2/h3 stream header vectors, the WebSocket upgrade client's scratch
array) now go through explicit detach_lifetime calls with SAFETY
comments instead of erasing the lifetime silently through the
unparameterized type. Header::clone / Request::clone / Response::clone
return data aliasing the StringBuilder's heap buffer, a precondition the
compiler cannot check, so they are now unsafe fn with documented
contracts.

Adds unit tests for the Header pointer round-trips and registers
bun_picohttp in the Miri crate allowlist.
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 21, 2026

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 6048d465-529b-462e-9394-842bd0c96f65

📥 Commits

Reviewing files that changed from the base of the PR and between c58e837 and 9160ea0.

📒 Files selected for processing (2)
  • src/http/lib.rs
  • src/s3_signing/credentials.rs

Walkthrough

This PR systematically migrates HTTP header and request/response structures from implicitly unbounded lifetimes to explicit lifetime-parameterized types, enabling Rust's borrow checker to enforce that header slices remain valid against their backing buffers. The core Header<'buf> type is introduced with lifetime-aware APIs, and callers explicitly widen lifetimes to 'static at parse/callback boundaries using bun_ptr::detach_lifetime.

Changes

HTTP Header Lifetime Safety

Layer / File(s) Summary
Core picohttp Header lifetime parameterization
src/picohttp/lib.rs
Header becomes Header<'buf> with PhantomData. Request, Response, HeaderList, and Headers now carry header lifetimes. clone and detach_lifetime are explicit unsafe/layout-preserving operations. Unit tests added for header behavior.
HTTP library parsing, scratch buffers, and request building
src/http/lib.rs
Shared scratch buffers and header constants retyped to picohttp::Header<'static>. build_request widens user header/name/value slices and Content-Length/Host to 'static via bun_ptr::detach_lifetime. Response parsing widens the parse input to &'static [u8] before Response::parse_parts. clone_metadata uses an explicit unsafe block.
HTTP/2 and HTTP/3 stream decoded header storage
src/http/h2_client/Stream.rs, src/http/h3_client/Stream.rs
decoded_headers fields changed to Vec<picohttp::Header<'static>> with doc comments about pointer validity relative to decoded buffers.
HTTP/3 callback lifetime detachment
src/http/h3_client/callbacks.rs
on_stream_headers now detaches lsquic-provided header name/value slices to &'static [u8] via bun_ptr::detach_lifetime and documents safety invariants for callback-scoped buffers and deep-copying.
WebSocket upgrade response parsing
src/http_jsc/websocket_client/WebSocketUpgradeClient.rs
headers_buf retyped to picohttp::Header<'static>. Response parsing paths detach the response buffer lifetime to &'static [u8] before calling picohttp::Response::parse.
S3 signing header construction
src/s3_signing/credentials.rs
Introduce unsafe pico_header_new(...) -> PicoHeader<'static> and use it to populate SignResult._headers. SignResult._headers now stores PicoHeader<'static> and public APIs adjust lifetimes; safety documentation added.
Fetch response header mixing fix
src/runtime/webcore/fetch.rs
Avoid self-borrowing when mixing response headers by deep-copying mixed headers into a new Headers instance and assigning it directly when necessary.
Rust miri test coverage
scripts/rust-miri.ts
Add bun_picohttp to the MIRI_CRATES default array for cargo miri test runs without -p arguments.

Possibly related PRs

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'picohttp: add lifetime parameter to Header' accurately describes the main change—adding a lifetime parameter to the Header struct to prevent use-after-free bugs.
Description check ✅ Passed The description comprehensively covers the problem (safe use-after-free), the solution (Header<'buf>), API changes, impact analysis, testing, and known limitations with detailed examples and tables.
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.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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

@robobun
Copy link
Copy Markdown
Collaborator

robobun commented May 21, 2026

Updated 6:42 AM PT - May 21st, 2026

@Jarred-Sumner, your commit 9160ea0 has 1 failures in Build #56634 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 31179

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

bun-31179 --bun

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 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 `@src/http/lib.rs`:
- Around line 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.
- Around line 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.
🪄 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: 84899f0d-869e-40b3-a552-03d6d1768069

📥 Commits

Reviewing files that changed from the base of the PR and between 0b20408 and c58e837.

📒 Files selected for processing (9)
  • scripts/rust-miri.ts
  • src/http/h2_client/Stream.rs
  • src/http/h3_client/Stream.rs
  • src/http/h3_client/callbacks.rs
  • src/http/lib.rs
  • src/http_jsc/websocket_client/WebSocketUpgradeClient.rs
  • src/picohttp/lib.rs
  • src/runtime/webcore/fetch.rs
  • src/s3_signing/credentials.rs

Comment thread src/http/lib.rs
Comment on lines +2070 to +2074
// 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)) };
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.

⚠️ 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

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

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`.

Comment thread src/http/lib.rs
Comment on lines +3115 to 3125
// 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, which is
// deep-copied by `clone_metadata()` before this fn returns. Widened
// to `'static` so it can be parsed into the `'static` scratch
// buffer and stored in `state.pending_response`.
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,
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.

⚠️ 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

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

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`.

Comment thread src/s3_signing/credentials.rs Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants