fix(http): guard Buffer/TypedArray in dynamic add ToPrimitive (#5131)#5176
Conversation
A `node:http` server that consumes the request body via
`req.on("data", c => body += c)` on a POST carrying a body segfaulted
(SIGSEGV / exit 139).
The un-typed `data` chunk is a `Buffer`, so `body += c` lowers to the
fully-dynamic add helper `js_dynamic_string_or_number_add` →
`to_primitive_default_for_add`. That function had no Buffer/TypedArray
guard, so it fell through to the `js_url_href_if_url` /
`try_read_as_search_params` / `OrdinaryToPrimitive` probes, all of which
bit-cast the operand pointer to an `ObjectHeader` and read its fields. A
`BufferHeader` carries no `ObjectHeader`/`GcHeader`, so the probe
dereferenced a fake header one word before the data and crashed.
(The same concat with a statically-typed `const c = Buffer.from(...)`
worked: codegen proves `c` non-string and uses the safe
`js_string_concat_value` coerce path, which routes through
`js_jsvalue_to_string`'s existing buffer guard.)
Fix: detect Buffers/TypedArrays via their registries (by-value lookups,
no deref) in `to_primitive_default_for_add` before the ObjectHeader
probes and route them to `js_jsvalue_to_string` — yielding the same
string form as an explicit `.toString()` (Buffer→utf8,
TypedArray→`join(",")`) and matching the guards `js_jsvalue_to_string`
itself runs. This also makes dynamic `+` consistent with `String()` and
template-literal coercion.
Adds regression test `test_issue_5131_http_body_concat` (expected `len:13`).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
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 (3)
📝 WalkthroughWalkthroughAdds a runtime safety guard in ChangesBuffer/TypedArray String Concat Safety Fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Summary
Fixes #5131 — a
node:httpserver that consumes the request body viareq.on("data", c => body += c)on a POST carrying a body segfaulted (SIGSEGV / exit 139).Root cause
The un-typed
datachunk is aBuffer, sobody += c(with both operands dynamic) lowers to the fully-dynamic add helperjs_dynamic_string_or_number_add→to_primitive_default_for_add(crates/perry-runtime/src/value/dynamic_arith.rs).That function had no Buffer/TypedArray guard, so it fell through to the
js_url_href_if_url/try_read_as_search_params/OrdinaryToPrimitiveprobes — all of which bit-cast the operand pointer to anObjectHeaderand read its fields. ABufferHeadercarries noObjectHeader/GcHeader, so the probe dereferenced a fake header one word before the data and crashed.The same concat with a statically-typed
const c = Buffer.from(...)worked, because codegen provescnon-string and uses the safejs_string_concat_valuecoerce path (which routes throughjs_jsvalue_to_string's existing buffer guard). The crash only manifests when the operand is dynamic — exactly the case for an un-typed httpdatachunk.Fix
Detect Buffers/TypedArrays via their registries (by-value lookups, no deref) in
to_primitive_default_for_addbefore the ObjectHeader probes, and route them tojs_jsvalue_to_string. This mirrors the guardsjs_jsvalue_to_stringitself runs before its ordinary-object dispatch, and makes dynamic+consistent withString()and template-literal coercion (Buffer→utf8, TypedArray→join(",")).Verification
len:13(was SIGSEGV).String(x),`${x}`,x.toString(), and dynamica + xall agree for bothBufferandUint8Array.JSON.parseworks.cargo test -p perry-runtime/-p perry-ext-http-serverpass (single-threaded; the one multi-threaded failure,object::tests::builtin_prototype_methods_reject_dynamic_new, is a pre-existing concurrency flake that reproduces identically onmain).test-files/test_issue_5131_http_body_concat.ts(+ expectedlen:13).Note for maintainer
Per the project's PR convention, this branch does not bump
[workspace.package] version, theCurrent Versionline, orCHANGELOG.md— left for the version/changelog fold at merge.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes