http: fix use-after-free of the redirect URL on a retried request - #33242
Conversation
After a redirect, handle_response_metadata repoints client.url at a self-borrow into client.redirect, a Vec the HTTP-thread clone owns and frees in the final-callback teardown (on_async_http_callback_raw). The bitwise copy-back into the JS-thread AsyncHTTP (NetworkTask::notify) kept those dangling slices, so when bun install retried the request after a retryable failure (5xx or connection reset) on the redirect target, the retry connected through freed memory. ASAN: heap-use-after-free READ on the HTTP-client thread in URL::get_port <- HTTPThread::connect. In release builds the retries never reached the server at all and the install failed. Restore the caller-owned pre-redirect URL (and reset connected_url, which is derived from it) in the same teardown that frees the redirect buffer. This also makes a retry restart from the original URL rather than the last redirect hop, which is what the install retry intends.
|
Updated 11:44 PM PT - Jul 1st, 2026
⏳ @robobun, your commit 15c4a1a is still building in
|
|
Warning Review limit reached
Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughUpdates redirect cleanup in async HTTP to restore per-hop client URL state before teardown, and expands bun install retry coverage to verify manifest and tarball redirect chains after transient 500 responses. ChangesRedirect Retry URL Fix
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 `@test/cli/install/bun-install-retry.test.ts`:
- Around line 93-97: The install retry tests are asserting on generated files
before checking the process exit status, which can hide the real failure behind
an ENOENT. In the affected test cases in bun-install-retry.test.ts, move the
exit-code assertion (and any early stdout/stderr status guard) ahead of the
filesystem reads for node_modules/BaR/package.json, keeping the existing
artifact checks afterward. Apply the same ordering in both test blocks so the
install diagnostics from the retry flow are surfaced first via the exitCode
assertion and then the package.json validation runs.
🪄 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: 5488f293-cbe0-43b7-9191-e7ba82795938
📒 Files selected for processing (2)
src/http/AsyncHTTP.rstest/cli/install/bun-install-retry.test.ts
Matches the existing test in this file. A failed install now surfaces the exit code and process output instead of an ENOENT on node_modules.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…t too A cross-origin redirect strips Authorization/Proxy-Authorization/Cookie/Host from client.header_entries in place (handle_response_metadata), and the same bitwise copy-back that leaked the dangling URL leaked the stripped list into the JS-thread AsyncHTTP. A retried request to an authorized registry whose redirect target failed transiently then went out without Authorization and got a 401. header_entries is bitwise-shared with the JS-thread original, so it must not be dropped or reallocated on the HTTP thread. It was cloned from request_headers at init and only ever shrinks, so restore it in place with clear_retaining_capacity + append_list_assume_capacity. Also restore client.method, which the same function may downgrade to GET for a redirect.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/cli/install/bun-install-retry.test.ts (1)
43-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove bug history out of test comments.
These comments document implementation history and ASAN details rather than the durable invariant. Keep the test comment to the expected behavior, and leave the detailed failure narrative in the PR description.
As per coding guidelines, comments should be concise, avoid bug history, and carry only durable non-obvious content.
Also applies to: 100-104
🤖 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 `@test/cli/install/bun-install-retry.test.ts` around lines 43 - 47, The comment in bun-install-retry.test.ts currently records bug history and ASAN internals instead of the lasting test invariant. Update the comment near the retry/redirect scenario in this test to state only the expected behavior: retries after a 302/500 chain must restart from the original URL and still reach the server. Remove the implementation-history narrative and memory-safety details from the test comment, including the similar block referenced elsewhere in the test file.Source: Coding guidelines
🤖 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/AsyncHTTP.rs`:
- Around line 783-791: Replace the release-only safety gap in AsyncHTTP’s header
repopulation path: the capacity invariant checked with debug_assert! before
append_list_assume_capacity must also hold in release builds. Update the code
around client.header_entries and request_headers to use assert! or a checked
append path before calling clear_retaining_capacity and
append_list_assume_capacity, so unchecked mutation cannot proceed if the
invariant is violated.
---
Outside diff comments:
In `@test/cli/install/bun-install-retry.test.ts`:
- Around line 43-47: The comment in bun-install-retry.test.ts currently records
bug history and ASAN internals instead of the lasting test invariant. Update the
comment near the retry/redirect scenario in this test to state only the expected
behavior: retries after a 302/500 chain must restart from the original URL and
still reach the server. Remove the implementation-history narrative and
memory-safety details from the test comment, including the similar block
referenced elsewhere in the test file.
🪄 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: b5416c60-c407-4c35-8bc2-fefb02db07bc
📒 Files selected for processing (2)
src/http/AsyncHTTP.rstest/cli/install/bun-install-retry.test.ts
append_list_assume_capacity writes into the shared header allocation unchecked; a broken capacity invariant there would corrupt memory rather than just misbehave, so the assertion must survive release builds. Also trim the new test comments to the behavior they assert.
|
CI status for this PR, since the red here is not from the diff. The diff is complete: the use-after-free fix, the header/method restore it exposed, the release-safe capacity assertion from review, and 3 regression tests that fail on main (ASAN abort under Every failure in build 67881 traces to something unrelated:
Re-running CI on this branch cannot go green while |
There was a problem hiding this comment.
My earlier header_entries concern was addressed well in 48158ae (and the capacity-invariant hardening in 15c4a1a is the right call). No further issues found, but this restores state into an allocation that's bitwise-shared with the JS-thread original inside an unsafe teardown path — a maintainer should confirm the no-drop/no-realloc contract holds across all copy-back consumers (NetworkTask::notify, FetchTasklet, send_sync, preconnect).
Extended reasoning...
Overview
This PR fixes a use-after-free in src/http/AsyncHTTP.rs (on_async_http_callback_raw's !has_more teardown block) where, after a 3xx redirect, client.url was left as a self-borrow into client.redirect — which the same block frees — and the bitwise copy-back in NetworkTask::notify propagated that dangling slice (plus a stripped header list and a possibly-downgraded method) into the JS-thread original. A retried bun install request would then connect through freed memory. The fix restores client.url/connected_url/method from the untouched AsyncHTTP-level fields and repopulates client.header_entries in-place from request_headers via clear_retaining_capacity() + append_list_assume_capacity(), guarded by a release-mode assert! on the capacity invariant. Three new tests in test/cli/install/bun-install-retry.test.ts cover the manifest, tarball, and cross-origin-authorized variants.
Security risks
None introduced. The change is a memory-safety fix; the cross-origin Authorization strip on the redirect hop itself is preserved (and now explicitly asserted by the third test via cdnAuth = [null, null]). No new attack surface, auth logic, or user-controlled input parsing.
Level of scrutiny
High. This is ~26 lines of new native code inside an unsafe block that manipulates an allocation (header_entries) which is bitwise-shared between the HTTP thread and the JS thread via ptr::read/ptr::write. The correctness of append_list_assume_capacity — an unchecked write — depends on a non-local invariant (the list was cloned from request_headers at init and only ever shrinks via ordered_remove in handle_response_metadata). The author's response to my earlier inline comment demonstrated a solid grasp of why a naive .clone() assignment would double-free here, and the release-mode assert! is the right backstop. But per this repo's own review history, cross-thread bitwise-shared ownership is the single most-blocked category, and the fix's soundness depends on how four separate consumers (NetworkTask::notify, FetchTasklet, send_sync, preconnect) copy back or don't. That's a maintainer-level confirmation, not a bot approval.
Other factors
- My prior inline comment (header restoration gap on cross-origin retry) was addressed in 48158ae with a fix that correctly improved on my suggested diff; the follow-up
debug_assert!→assert!hardening in 15c4a1a is also sound. All review threads are resolved. - Test coverage is thorough: each of the three restores (URL, headers, method) is load-bearing for at least one test, and the PR description confirms all three fail on the unfixed build.
- The bug-hunting system found no issues on the current revision.
- CI build #67881 is in progress; status not yet visible in the timeline.
|
The review summary above asks a maintainer to confirm the no-drop/no-realloc contract across the copy-back consumers. Laying out what I verified so that confirmation is a read rather than a re-derivation. The restore itself never drops or reallocates anything. Per consumer:
The capacity invariant the |
2 participants
What
After a 3xx redirect,
handle_response_metadatarewrites per-hop request state on the HTTP-thread clone of theAsyncHTTP:client.url(andconnected_url) become a self-borrow intoclient.redirect, aVec<u8>the clone owns and frees in the final-callback teardown (AsyncHTTP::on_async_http_callback_raw).Authorization/Proxy-Authorization/Cookie/Hostare removed fromclient.header_entriesin place.NetworkTask::notify's bitwise copy-back (ptr::write(real, ptr::read(async_http))) carries all of that into the JS-threadAsyncHTTP. Whenbun installretries the task after a retryable failure (5xx or a connection reset on the redirect target), the re-scheduled request therefore:Authorization, so an authorized registry answers 401.ASAN (debug build), deterministic on the first try:
On a release build the same sequence does not crash, but the retries never reach the server (each one connects through freed memory) and the install fails.
Repro
A scripted registry where the manifest URL 302-redirects and the redirect target answers a 500 once, then the real packument:
bun installagainst it aborts under ASAN and fails on release. Any 301/302/307/308 and 1- or 2-hop chains hit the same path. With an authorized registry that redirects cross-origin (the common Artifactory / CodeArtifact / GitHub Packages shape), the retry also losesAuthorization; that variant fails withGET <registry>/BaR - 401even once the URL is fixed.Fix
src/http/AsyncHTTP.rs: the!has_moreteardown block already releases every clone-owned allocation. Before freeingclient.redirect, restore the per-hop state that a re-scheduled attempt must not inherit:client.urlback to the caller-owned pre-redirect URL (AsyncHTTP.url, which borrows memory valid for the original's whole lifetime), andclient.connected_url(whichconnectderives from it) to default.client.header_entriesback to the untouchedAsyncHTTP.request_headers. The list is bitwise-shared with the JS-thread original, so it must not be dropped or reallocated on the HTTP thread; it was cloned fromrequest_headersat init and only ever shrinks, soclear_retaining_capacity()+append_list_assume_capacity()restores it in place.client.methodback toAsyncHTTP.method.Nothing that crosses back to the JS thread references clone-freed memory anymore, and a retried request restarts from the original URL with the original headers instead of the last redirect hop's, which is what the install-level retry is meant to do.
Tests
test/cli/install/bun-install-retry.test.ts:retries a manifest whose redirect target 500s onceretries a tarball whose redirect target 500s once(the sibling retry site inrunTasks)retries an authorized manifest whose cross-origin redirect target 500s once(also asserts the cross-origin hop itself still does NOT carryAuthorization, so the spec-mandated strip is unchanged)All three fail on the unfixed build (ASAN abort under
bun bd, install error withUSE_SYSTEM_BUN=1). The third additionally fails with a 401 if only the URL is restored and not the headers, so each restore is load-bearing.test/js/web/fetch/fetch-redirect.test.tsandfetch-url-after-redirect.test.tsstill pass, soresponse.urlafter a redirect is unaffected (it comes from the ownedmetadata.urlcopy, not fromclient.url).