fix(fetch): Headers.forEach/entries on app.fetch() Response no longer SIGSEGVs (#5432)#5442
Merged
Merged
Conversation
|
Caution Review failedPull request was closed or merged during review No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughFixes a SIGSEGV in ChangesFetch Response Headers forEach/entries Fix
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 `@crates/perry-hir/src/destructuring/var_decl.rs`:
- Around line 936-963: The `is_member_fetch_call` function only marks direct
`.fetch(...)` calls, but it does not handle simple variable aliases. When a
variable is initialized with another variable that was already marked as a fetch
response local (like `const alias = res`), the alias should also be added to
`ctx.fetch_call_response_locals`. Add a check after the `is_member_fetch_call`
condition: if `init_expr` is a simple identifier reference and that identifier
is already in `ctx.fetch_call_response_locals`, then insert the current `name`
into the set as well. This ensures aliases to fetch responses maintain the
tracking through the scope.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ae417dd8-1c10-49f4-b1b1-e02ce07e975f
📒 Files selected for processing (6)
crates/perry-hir/src/destructuring/var_decl.rscrates/perry-hir/src/lower/context.rscrates/perry-hir/src/lower/expr_call/array_only_methods.rscrates/perry-hir/src/lower/lowering_context.rscrates/perry-runtime/src/array/header.rstest-files/test_issue_5432_fetch_headers_foreach.ts
Comment on lines
+936
to
+963
| // #5432: `const res = app.fetch(req)` / `await app.fetch(req)` — | ||
| // a member-call `.fetch(...)` is the Fetch-API server-handler | ||
| // convention (Hono `app.fetch`, itty-router, Cloudflare | ||
| // Workers) and yields a native fetch Response. Record it in a | ||
| // narrow set (NOT `register_native_instance`, which would hijack | ||
| // every method on `res`) so only `res.headers.<m>()` bails the | ||
| // array-method fold. See `fetch_call_response_locals`. | ||
| fn is_member_fetch_call(expr: &ast::Expr) -> bool { | ||
| let call = match expr { | ||
| ast::Expr::Await(a) => match a.arg.as_ref() { | ||
| ast::Expr::Call(c) => c, | ||
| _ => return false, | ||
| }, | ||
| ast::Expr::Call(c) => c, | ||
| _ => return false, | ||
| }; | ||
| if let ast::Callee::Expr(callee) = &call.callee { | ||
| if let ast::Expr::Member(member) = callee.as_ref() { | ||
| if let ast::MemberProp::Ident(prop) = &member.prop { | ||
| return prop.sym.as_ref() == "fetch"; | ||
| } | ||
| } | ||
| } | ||
| false | ||
| } | ||
| if is_member_fetch_call(init_expr) { | ||
| ctx.fetch_call_response_locals.insert(name.clone()); | ||
| } |
There was a problem hiding this comment.
Propagate fetch-response local tracking across simple aliases.
fetch_call_response_locals is only set for direct .fetch(...) initializers. A follow-up alias (const alias = res) is not tracked, so alias.headers.forEach/entries can still miss the intended headers path.
Proposed patch
if is_member_fetch_call(init_expr) {
ctx.fetch_call_response_locals.insert(name.clone());
+ } else if let ast::Expr::Ident(src_ident) = init_expr.as_ref() {
+ if ctx
+ .fetch_call_response_locals
+ .contains(src_ident.sym.as_ref())
+ {
+ ctx.fetch_call_response_locals.insert(name.clone());
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // #5432: `const res = app.fetch(req)` / `await app.fetch(req)` — | |
| // a member-call `.fetch(...)` is the Fetch-API server-handler | |
| // convention (Hono `app.fetch`, itty-router, Cloudflare | |
| // Workers) and yields a native fetch Response. Record it in a | |
| // narrow set (NOT `register_native_instance`, which would hijack | |
| // every method on `res`) so only `res.headers.<m>()` bails the | |
| // array-method fold. See `fetch_call_response_locals`. | |
| fn is_member_fetch_call(expr: &ast::Expr) -> bool { | |
| let call = match expr { | |
| ast::Expr::Await(a) => match a.arg.as_ref() { | |
| ast::Expr::Call(c) => c, | |
| _ => return false, | |
| }, | |
| ast::Expr::Call(c) => c, | |
| _ => return false, | |
| }; | |
| if let ast::Callee::Expr(callee) = &call.callee { | |
| if let ast::Expr::Member(member) = callee.as_ref() { | |
| if let ast::MemberProp::Ident(prop) = &member.prop { | |
| return prop.sym.as_ref() == "fetch"; | |
| } | |
| } | |
| } | |
| false | |
| } | |
| if is_member_fetch_call(init_expr) { | |
| ctx.fetch_call_response_locals.insert(name.clone()); | |
| } | |
| // `#5432`: `const res = app.fetch(req)` / `await app.fetch(req)` — | |
| // a member-call `.fetch(...)` is the Fetch-API server-handler | |
| // convention (Hono `app.fetch`, itty-router, Cloudflare | |
| // Workers) and yields a native fetch Response. Record it in a | |
| // narrow set (NOT `register_native_instance`, which would hijack | |
| // every method on `res`) so only `res.headers.<m>()` bails the | |
| // array-method fold. See `fetch_call_response_locals`. | |
| fn is_member_fetch_call(expr: &ast::Expr) -> bool { | |
| let call = match expr { | |
| ast::Expr::Await(a) => match a.arg.as_ref() { | |
| ast::Expr::Call(c) => c, | |
| _ => return false, | |
| }, | |
| ast::Expr::Call(c) => c, | |
| _ => return false, | |
| }; | |
| if let ast::Callee::Expr(callee) = &call.callee { | |
| if let ast::Expr::Member(member) = callee.as_ref() { | |
| if let ast::MemberProp::Ident(prop) = &member.prop { | |
| return prop.sym.as_ref() == "fetch"; | |
| } | |
| } | |
| } | |
| false | |
| } | |
| if is_member_fetch_call(init_expr) { | |
| ctx.fetch_call_response_locals.insert(name.clone()); | |
| } else if let ast::Expr::Ident(src_ident) = init_expr.as_ref() { | |
| if ctx | |
| .fetch_call_response_locals | |
| .contains(src_ident.sym.as_ref()) | |
| { | |
| ctx.fetch_call_response_locals.insert(name.clone()); | |
| } | |
| } |
🤖 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 `@crates/perry-hir/src/destructuring/var_decl.rs` around lines 936 - 963, The
`is_member_fetch_call` function only marks direct `.fetch(...)` calls, but it
does not handle simple variable aliases. When a variable is initialized with
another variable that was already marked as a fetch response local (like `const
alias = res`), the alias should also be added to
`ctx.fetch_call_response_locals`. Add a check after the `is_member_fetch_call`
condition: if `init_expr` is a simple identifier reference and that identifier
is already in `ctx.fetch_call_response_locals`, then insert the current `name`
into the set as well. This ensures aliases to fetch responses maintain the
tracking through the scope.
…ough Headers FFI (#5432) `const res = await app.fetch(req)` — the Fetch-API / WinterCG server-handler shape used by Hono, itty-router, and Cloudflare Workers — is a member-call `.fetch()`, which was never registered as a fetch Response. That left `res` unregistered, so the static array-method fold rewrote `res.headers.forEach(cb)` into `Expr::ArrayForEach` and codegen dispatched `js_array_forEach` directly on the Headers handle id (0x40000 band). Dereferencing `handle - 8` as a GcHeader read unmapped low memory and SIGSEGV'd on the first request of any Hono server. `.entries()` folded the same way (silently yielding 0); `[...res.headers]` used the iterator path that already routed handles correctly, which is why only `.forEach`/`.entries` diverged. Two layers: 1. HIR: a narrow `fetch_call_response_locals` set records locals initialized from a member-call `.fetch(...)`, consulted ONLY by the `is_fetch_headers` guard in `array_only_methods.rs` so `res.headers.<m>()` bails the array fold and routes through the Headers FFI. This deliberately avoids `register_native_instance`, which hijacks every method/property on the local and would mis-route a Bookshelf/Backbone `repo.fetch()` that returns a real array — verified that case stays correct. 2. Runtime (defense-in-depth): `normalize_array_receiver` and `clean_arr_ptr` now reject the small-handle band via `addr_class` predicates before any GcHeader deref (the stale `0x1008` floor was the crash site), turning any stray handle reaching an array helper into a safe empty-result no-op across platforms instead of a crash. Validated against the real hono ^4.10.0 repro (now iterates, no crash), a hono-free regression test (test_issue_5432_fetch_headers_foreach.ts), and the runtime unit suite. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
proggeramlug
force-pushed
the
worktree-fix-5432-headers-foreach
branch
from
b9b7e93 to
3a429cf
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #5432.
Problem
Calling
.forEach()on theHeadersof theResponsereturned by Hono'sapp.fetch()SIGSEGVs injs_array_forEach;.entries()silently yields 0. A plainnew Response().headers.forEach()works. This breaks every server built on Hono (and any WinterCGapp.fetchhandler — itty-router, Cloudflare Workers) on the first request.Root cause
const res = await app.fetch(req)is a member-call.fetch(), which was never registered as a fetchResponse. Withresunregistered, the static array-method fold rewroteres.headers.forEach(cb)intoExpr::ArrayForEach, and codegen dispatchedjs_array_forEachdirectly on the Headers handle id (0x40000 band). Dereferencinghandle - 8as aGcHeaderreads unmapped low memory → SIGSEGV..entries()folded identically (silent 0).[...res.headers]used the iterator path that already routes handles correctly — which is exactly why only.forEach/.entriesdiverged.Fix (two layers)
HIR (functional fix). A narrow
fetch_call_response_localsset records locals initialized from a member-call.fetch(...), consulted only by theis_fetch_headersguard inarray_only_methods.rssores.headers.<m>()bails the array-method fold and routes through the Headers FFI. This deliberately avoidsregister_native_instance, which hijacks every method/property on the local — that broader approach regressed a Bookshelf/Backbone-stylerepo.fetch()returning a real array (.map/.forEachmis-routed). The narrow set only affects<name>.headers.<method>().Runtime (defense-in-depth).
normalize_array_receiverandclean_arr_ptrnow reject the small-handle band viaaddr_classpredicates before anyGcHeaderderef (the stale0x1008floor was the crash site). Any stray handle reaching an array helper becomes a safe empty-result no-op across platforms instead of a crash — matching theaddr_class.rsband-map contract (fix(runtime): honor JSON reviver prototype lookups #4665/fix(runtime): for-of/spread over a Headers handle no longer segfaults #4800).Validation
forEach count: 1, no crash.test-files/test_issue_5432_fetch_headers_foreach.ts(modelsapp.fetch()returning a nativeResponse), byte-for-byte matchingnode --experimental-strip-types.repo.fetch()returningnumber[]still does.map/.forEachcorrectly.cargo test -p perry-runtime -p perry-hirgreen (1 unrelated documented test-isolation flake, passes in isolation); array gap-test spot-check unaffected (the 2 diffs are pre-existingTypedArray.sortarg-validation /toLocaleStringgaps, not in changed code paths).Per repo convention, version bump + CHANGELOG are left to the maintainer.
🤖 Generated with Claude Code
Summary by CodeRabbit
Responseheaders via.forEach()and.entries()(including cases created through member-callfetchpatterns).Response.headersiteration afterfetch()-style member calls.