fix(http): res.setHeaders no longer SIGSEGVs on Headers; correct Headers/Map handling (#4965)#5175
Merged
Merged
Conversation
|
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 (5)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthrough
Changesres.setHeaders SIGSEGV fix and normalization pipeline
Sequence Diagram(s)sequenceDiagram
rect rgba(30, 100, 200, 0.5)
note over dispatch,stdlib: Startup: producer registration
participant dispatch as js_stdlib_init_dispatch
participant reg as js_register_global_headers_entries_json
participant global as GLOBAL_HEADERS_ENTRIES_JSON
dispatch->>reg: register(js_headers_setheaders_entries_json)
reg->>global: store fn pointer
end
rect rgba(200, 80, 30, 0.5)
note over jscaller,sr: Runtime: res.setHeaders(headers)
participant jscaller as JS caller
participant setHeaders as js_node_http_res_set_headers
participant norm as js_node_setheaders_entries_json
participant stdlib as js_headers_setheaders_entries_json
participant sr as ServerResponse
jscaller->>setHeaders: setHeaders(value)
setHeaders->>sr: check headers_sent || header_committed
alt committed
setHeaders-->>jscaller: throw ERR_HTTP_HEADERS_SENT
else not committed
setHeaders->>norm: normalize(value)
norm->>global: load fn pointer
global->>stdlib: call(handle)
stdlib-->>norm: [name,value] JSON entries
norm-->>setHeaders: *mut StringHeader
setHeaders->>sr: apply_headers_entries(entries)
setHeaders-->>jscaller: ok
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
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: 3
🤖 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-ext-http-server/src/response.rs`:
- Around line 1014-1018: The `header_committed` guard that prevents mutations
after writeHead() is only applied in the writeHead() method itself, but the
related header mutation methods (setHeader, removeHeader, appendHeader) and
status/statusMessage property setters can still modify state after the head is
committed. Add the same header_committed check to each of these four header
mutator methods and property setters, ensuring they throw an error consistent
with Node.js behavior (ERR_HTTP_HEADERS_SENT) when attempting to mutate after
the head has been committed.
- Around line 793-819: Header name validation is missing in the bulk-applied
header methods. In both apply_headers_entries (shown in the diff at lines
793-819) and apply_headers_flat_array (at lines 1033-1059), add a check using
the same http_is_valid_token validation that is applied in setHeader before
inserting any header names into sr.headers, sr.header_value_lists, and
sr.raw_header_names. If a header name fails validation, the methods should throw
ERR_INVALID_HTTP_TOKEN instead of silently accepting the malformed name. Apply
this validation guard consistently in both functions before any map insertions
occur.
In `@crates/perry-runtime/src/object/global_fetch.rs`:
- Around line 275-278: The code is directly passing Map entries obtained from
js_map_entries(map) to js_json_stringify without validating that the entries
don't contain fetch-band handles as keys or values, which can cause the JSON
walker to recursively process those handles incorrectly. Before calling
js_json_stringify on the boxed entries pointer, either validate and normalize
the Map entries with an address-band-aware header normalizer to safely handle
any fetch-band handles, or explicitly reject Map entries that contain
unsupported key/value shapes (such as fetch-band handles) before the
stringification step.
🪄 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: 89b8afd2-79bf-4c0b-8711-2a11c27ff4ef
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
CHANGELOG.mdCLAUDE.mdCargo.tomlcrates/perry-ext-http-server/src/response.rscrates/perry-ext-http-server/src/types.rscrates/perry-runtime/src/object/global_fetch.rscrates/perry-stdlib/src/common/dispatch.rscrates/perry-stdlib/src/fetch/headers.rs
Comment on lines
+793
to
+819
| let name = match name_v { | ||
| serde_json::Value::String(s) => s, | ||
| other => other.to_string(), | ||
| }; | ||
| if name.is_empty() { | ||
| continue; | ||
| } | ||
| let lower = name.to_lowercase(); | ||
| if let serde_json::Value::Array(elems) = value_v { | ||
| let elems: Vec<String> = elems | ||
| .into_iter() | ||
| .map(|item| match item { | ||
| serde_json::Value::String(s) => s, | ||
| other => other.to_string(), | ||
| }) | ||
| .collect(); | ||
| sr.headers.insert(lower.clone(), elems.join(", ")); | ||
| sr.header_value_lists.insert(lower.clone(), elems); | ||
| } else { | ||
| let value = match value_v { | ||
| serde_json::Value::String(s) => s, | ||
| other => other.to_string(), | ||
| }; | ||
| sr.headers.insert(lower.clone(), value); | ||
| sr.header_value_lists.remove(&lower); | ||
| } | ||
| sr.raw_header_names.insert(lower, name); |
There was a problem hiding this comment.
Validate header names before storing bulk-applied entries.
setHeaders(new Map(...)) and writeHead([...]) now bypass the http_is_valid_token check used by setHeader, so malformed names can enter sr.headers instead of throwing ERR_INVALID_HTTP_TOKEN. Add the same validation in both new appliers before inserting into the maps.
Suggested guard
if name.is_empty() {
continue;
}
+ if !http_is_valid_token(&name) {
+ perry_ffi::throw_with_code(
+ &format!("Header name must be a valid HTTP token [\"{name}\"]"),
+ "ERR_INVALID_HTTP_TOKEN",
+ perry_ffi::ErrorKind::TypeError,
+ );
+ }
let lower = name.to_lowercase();Apply the same guard in apply_headers_entries and apply_headers_flat_array.
Also applies to: 1033-1059
🤖 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-ext-http-server/src/response.rs` around lines 793 - 819, Header
name validation is missing in the bulk-applied header methods. In both
apply_headers_entries (shown in the diff at lines 793-819) and
apply_headers_flat_array (at lines 1033-1059), add a check using the same
http_is_valid_token validation that is applied in setHeader before inserting any
header names into sr.headers, sr.header_value_lists, and sr.raw_header_names. If
a header name fails validation, the methods should throw ERR_INVALID_HTTP_TOKEN
instead of silently accepting the malformed name. Apply this validation guard
consistently in both functions before any map insertions occur.
Comment on lines
+1014
to
+1018
| // Mark the head committed (Node's `_header`) so a later | ||
| // `res.setHeaders(...)` throws `ERR_HTTP_HEADERS_SENT`. The actual wire | ||
| // flush still happens lazily at `write`/`end` (`headers_sent`), so the | ||
| // deferred-send path is unchanged (#4965). | ||
| sr.header_committed = true; |
There was a problem hiding this comment.
Apply header_committed to every head mutator.
After writeHead() sets header_committed but leaves headers_sent false, setHeader, removeHeader, appendHeader, and status/statusMessage setters can still mutate state that will later be snapshotted to the wire. Use a shared “head committed” guard for those paths, not only for setHeaders.
🤖 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-ext-http-server/src/response.rs` around lines 1014 - 1018, The
`header_committed` guard that prevents mutations after writeHead() is only
applied in the writeHead() method itself, but the related header mutation
methods (setHeader, removeHeader, appendHeader) and status/statusMessage
property setters can still modify state after the head is committed. Add the
same header_committed check to each of these four header mutator methods and
property setters, ensuring they throw an error consistent with Node.js behavior
(ERR_HTTP_HEADERS_SENT) when attempting to mutate after the head has been
committed.
Comment on lines
+275
to
+278
| if let Some(map) = crate::map::map_ptr_from_receiver_bits(bits) { | ||
| let entries = crate::map::js_map_entries(map); | ||
| let boxed = crate::value::js_nanbox_pointer(entries as i64); | ||
| return unsafe { crate::json::js_json_stringify(f64::from_bits(boxed.to_bits()), 0) }; |
There was a problem hiding this comment.
Avoid recursively stringifying arbitrary Map entries.
The top-level Map receiver is heap-backed, but its keys/values can still be fetch-band handles; js_map_entries(map) preserves those values inside heap arrays, and the generic JSON walker can then recurse into a handle and recreate the same handle-as-GcHeader crash class. Serialize Map entries with an address-band-aware header normalizer, or reject unsupported key/value shapes before calling js_json_stringify.
🤖 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-runtime/src/object/global_fetch.rs` around lines 275 - 278, The
code is directly passing Map entries obtained from js_map_entries(map) to
js_json_stringify without validating that the entries don't contain fetch-band
handles as keys or values, which can cause the JSON walker to recursively
process those handles incorrectly. Before calling js_json_stringify on the boxed
entries pointer, either validate and normalize the Map entries with an
address-band-aware header normalizer to safely handle any fetch-band handles, or
explicitly reject Map entries that contain unsupported key/value shapes (such as
fetch-band handles) before the stringification step.
…ers/Map handling (#4965) setHeaders JSON-stringified its argument directly. A Headers value is a fetch-band registry handle (first id 0x40000), not a heap pointer, so the generic stringify walker dereferenced id-8 as a GcHeader and segfaulted nondeterministically (~6/8 runs of test-http-response-setheaders.js). - New runtime js_node_setheaders_entries_json classifies by address band before any deref: Map read directly, Headers delegated to a registered perry-stdlib producer; else null -> ERR_INVALID_ARG_TYPE. - perry-stdlib js_headers_setheaders_entries_json emits sorted [name,value] entries, Set-Cookie as an array; routed via the always-linked runtime so http-server keeps no direct stdlib symbol dep. - setHeaders throws ERR_HTTP_HEADERS_SENT once the head is committed (new header_committed flag set by writeHead, distinct from headers_sent), and writeHead now accepts the flat-array headers form. Separate pre-existing blocker filed as #5174 (Headers + in-process http server/client hang). Unit tests added for the entries/flat-array appliers.
proggeramlug
force-pushed
the
worktree-fix-4965-setheaders-segv
branch
from
036d12f to
7dc687d
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.
Fixes #4965.
Root cause
res.setHeaders(headers)JSON-stringified its argument directly. AHeadersvalue is a fetch-band registry handle (its first id is0x40000, pervalue::addr_class), not a heap pointer — so the genericJSON.stringifywalker dereferencedid - 8as aGcHeaderand segfaulted. Because the page at that low address is sometimes mapped (mimalloc retention) and sometimes not, the crash was nondeterministic:test-http-response-setheaders.jsexited 139 (SIGSEGV) ~6/8 runs and 1 ("Missing expected exception") otherwise (the no-op fallback also never threw Node's validation errors).Fix
js_node_setheaders_entries_json(value)(perry-runtime/src/object/global_fetch.rs) classifies the argument by address band before any dereference: aMap(real heapMapHeader) is read directly viajs_map_entries; aHeadershandle is delegated to a registered perry-stdlib producer; everything else returns null. No path ever dereferences a registry handle.js_headers_setheaders_entries_json(fetch/headers.rs, registered fromjs_stdlib_init_dispatch) emits WHATWG sorted-by-name[name, value]entries, collapsingSet-Cookieinto a["set-cookie", [c1, c2, …]]array. Routed through the always-linked runtime soperry-ext-http-serverkeeps no direct perry-stdlib symbol dependency (avoids the fix(runtime,codegen,hir): Next.js standalone walls 31-35 #5112 link-break class).perry-ext-http-serverjs_node_http_res_set_headersrewritten to Node's contract:ERR_HTTP_HEADERS_SENTwhen the head is committed, thenERR_INVALID_ARG_TYPEfor non-Headers/Mapargs, else apply. A newheader_committedflag is set bywriteHead(distinct from the lazyheaders_sentwire-flush flag, so the deferred-send path is unchanged).writeHeadalso now applies the flat-array headers form ([name, value, …]).Verification
ERR_INVALID_ARG_TYPEfires for all six invalid arg shapes;ERR_HTTP_HEADERS_SENTfires afterwriteHead.MapandHeadersentries both apply (confirmed viares.getHeaders()).cargo test -p perry-ext-http-server --lib: 27 passed (4 new unit tests for the entries/flat-array appliers).Known separate blocker (NOT this issue)
Filed as #5174: instantiating a
globalThis.Headersobject in a program that runs both anhttp.Serverand an in-process http client hangs the response pump — even without callingsetHeaders. This is why the full upstream test still can't complete end-to-end; the SIGSEGV it reported is fixed.Summary by CodeRabbit
Release Notes
New Features
writeHead()now accepts headers in both object and array formats for greater flexibility.Bug Fixes
Tests