From 980b9f267224d031fda903bc36625a74504d0b7f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Fri, 28 Aug 2026 15:25:22 +0000 Subject: [PATCH 1/4] fix(codegen): an imported class no longer installs its private brand twice (#8962) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `import { Hono } from "hono"; new Hono()` compiled and linked, then threw `TypeError: Cannot initialize private elements twice on the same object` during construction. It reduces to two files and no inheritance at all: // base.ts export class BaseX { #m(): number { return 1; } call(): number { return this.#m(); } } // main.ts import { BaseX } from "./base"; new BaseX().call(); The importing module sees the class only as the metadata-only stub `compile_module` synthesizes for an import (`codegen/mod.rs`, "Build a stub Class with the minimum fields the codegen needs"). A stub is a name table: it carries member names so dispatch symbols resolve, and carries no bodies, no initializers and no constructor. Everything construction actually *does* is baked into the defining module's standalone `___constructor` instead — `codegen/method.rs` says so where it emits them, "At the `new ImportedClass(...)` call site, `lower_new` applies initializers against the imported class stub — which has none". That premise held for FIELDS, because the stub flattens every field to `is_private: false` with `init: None`: the worst `apply_field_initializers_ recursive` could do at the `new` site was write `undefined` into a slot the real constructor overwrote moments later. It did not hold for the private BRAND. The stub copies private METHOD and accessor names verbatim, and `has_private_instance_brand` is defined purely over `#`-prefixed member names, so a stub answered `true` and the `new` site emitted `js_private_brand_add` on top of the one the defining module's constructor emits. Installing a class's brand twice on one object is the error PrivateMethodOrAccessorAdd requires, so the runtime threw — correctly, at the second install. Fix: `apply_field_initializers_recursive` skips the private-element decision for a chain entry that is an imported stub. The duplicate check itself is untouched: exactly one `js_private_brand_add` survives, in the defining module's constructor (verified with objdump — the importing module's object now has none, the defining module's still has one). Reached both spellings: the class constructed directly (`new BaseX()`), and the class reached as an ANCESTOR through the `AncestorsOnly` walk, where the leaf is a local subclass. hono hits the second — `class Hono extends HonoBase` with `#path`, `#notFoundHandler`, `#clone`, `#addRoute`, `#dispatch` on the base. Only classes with a private method or accessor were affected; a private field alone never was, since the stub does not mark fields private. Tests: `crates/perry/tests/issue_8962_imported_class_private_brand.rs`. Every case calls the private member after constructing, so a fix that dropped the second install without leaving the first standing fails them too — the brand check throws when no brand is present. Two guard cases pin the boundaries: same-module construction still installs the brand at the `new` site, and a genuine double initialization (a base ctor returning an object the derived class already branded) still throws. Verified: `new Hono()` runs (routing, `route()`, `basePath()`, `fetch`); `cargo test -p perry --bin perry` 1049/1049; `cargo test -p perry-hir -p perry-codegen` all green; mb24's `packages/db/src/migrate.ts` still compiles. Claude-Session: https://claude.ai/code/session_0145yUtx1jiWHf66QEZh6DzY --- .../src/lower_call/field_init.rs | 29 ++ crates/perry-hir/src/ir/decl.rs | 21 ++ ...issue_8962_imported_class_private_brand.rs | 284 ++++++++++++++++++ 3 files changed, 334 insertions(+) create mode 100644 crates/perry/tests/issue_8962_imported_class_private_brand.rs diff --git a/crates/perry-codegen/src/lower_call/field_init.rs b/crates/perry-codegen/src/lower_call/field_init.rs index 9090685aaa..6615765a10 100644 --- a/crates/perry-codegen/src/lower_call/field_init.rs +++ b/crates/perry-codegen/src/lower_call/field_init.rs @@ -693,10 +693,39 @@ pub(crate) fn apply_field_initializers_recursive( None => init_pairs.push((field.name.clone(), init, field.is_private)), } } + // #8962: an IMPORTED class installs nothing here. Its whole + // field-initializer phase — public field writes, private-field adds AND + // the shared private brand — is baked into the defining module's + // standalone `___constructor`, which `codegen/method.rs` + // emits for exactly that reason ("At the `new ImportedClass(...)` call + // site, `lower_new` applies initializers against the imported class + // stub — which has none"). That premise holds for FIELDS because the + // stub flattens every field to `is_private: false` with `init: None`, + // so the worst this loop could do was write `undefined` into a slot the + // real constructor overwrites moments later. + // + // It does NOT hold for the private BRAND. The stub copies private + // METHOD and accessor names verbatim (it needs them to resolve dispatch + // symbols), and `has_private_instance_brand` is defined purely over + // `#`-prefixed method/getter/setter names — so a stub answers `true` and + // this site emitted `js_private_brand_add` at the importing module's + // `new`, on top of the one the defining module's constructor emits. + // Installing a class's brand twice on one object is the observable + // error PrivateMethodOrAccessorAdd requires, so the runtime threw + // "Cannot initialize private elements twice on the same object" out of + // `new Hono()` — any imported class with a private method or accessor, + // whether constructed directly or reached as an ancestor through + // `AncestorsOnly`. + // + // Suppressing BOTH flags (not just the brand) is what restores the + // `continue` below for a stub whose only private elements are methods: + // for a stub the two predicates are the same question, since its fields + // are never private. let (class_has_private_elements, class_has_private_brand) = ctx .classes .get(&class_name_in_chain) .copied() + .filter(|class| !class.is_imported_stub()) .map(|class| { ( class.has_private_instance_elements(), diff --git a/crates/perry-hir/src/ir/decl.rs b/crates/perry-hir/src/ir/decl.rs index 4683d1b220..dbbe74e3cc 100644 --- a/crates/perry-hir/src/ir/decl.rs +++ b/crates/perry-hir/src/ir/decl.rs @@ -284,6 +284,27 @@ pub struct Class { } impl Class { + /// True for the metadata-only stub `compile_module` synthesizes for a class + /// IMPORTED from another module (`perry-codegen/src/codegen/mod.rs`, "Build + /// a stub Class with the minimum fields the codegen needs"). + /// + /// A stub is a NAME TABLE, not a class: it carries member names so the + /// importing module can resolve dispatch symbols, and carries no bodies, no + /// field initializers and no constructor. Everything a construction + /// actually *does* — field initializers, private-field adds, the private + /// brand — is baked into the defining module's standalone + /// `___constructor` instead (`codegen/method.rs`, + /// `is_constructor_method`), precisely because the stub has none of it. + /// + /// `id == 0` is the marker: the driver hands out class ids from 1 + /// (`run_pipeline.rs`: "Start at 1, 0 is reserved for \"no parent\"") and + /// every local class takes its id from `LoweringContext::fresh_class`, so + /// the stub built at `codegen/mod.rs` ("id: 0, // imported — no local + /// ClassId") is the only `Class` in a module's class table with id 0. + pub fn is_imported_stub(&self) -> bool { + self.id == 0 + } + /// Whether construction installs any instance-private element. pub fn has_private_instance_elements(&self) -> bool { self.fields.iter().any(|field| field.is_private) diff --git a/crates/perry/tests/issue_8962_imported_class_private_brand.rs b/crates/perry/tests/issue_8962_imported_class_private_brand.rs new file mode 100644 index 0000000000..58eea312a1 --- /dev/null +++ b/crates/perry/tests/issue_8962_imported_class_private_brand.rs @@ -0,0 +1,284 @@ +//! Regression for #8962: constructing a class IMPORTED from another module +//! threw `TypeError: Cannot initialize private elements twice on the same +//! object` when that class declared a private method or accessor. +//! +//! `import { Hono } from "hono"; new Hono()` was the report. The importing +//! module sees the class only as the metadata-only stub `compile_module` +//! builds — a name table with no bodies and no initializers — but the stub +//! copies private METHOD names verbatim (it needs them to resolve dispatch +//! symbols), and `Class::has_private_instance_brand` is defined purely over +//! `#`-prefixed member names. So the `new` site emitted `js_private_brand_add` +//! for a brand it does not own, on top of the one the DEFINING module's +//! standalone `___constructor` emits — and installing a class's +//! brand twice on one object is the error PrivateMethodOrAccessorAdd requires. +//! +//! Every case here calls the private member after construction, so a fix that +//! merely dropped the second install without leaving the first one standing +//! would fail these too: the brand check inside the private-member access +//! throws when no brand is present. + +use std::path::PathBuf; +use std::process::Command; + +fn perry_bin() -> PathBuf { + PathBuf::from(env!("CARGO_BIN_EXE_perry")) +} + +/// Compile `files` (relative path -> source) with `entry` as the entry point +/// and return the binary's stdout. Panics with the compiler's or the program's +/// output on any failure. +fn compile_and_run(files: &[(&str, &str)], entry: &str) -> String { + let dir = tempfile::tempdir().expect("tempdir"); + let root = dir.path(); + for (name, source) in files { + let path = root.join(name); + if let Some(parent) = path.parent() { + std::fs::create_dir_all(parent).expect("mkdir"); + } + std::fs::write(&path, source).expect("write source"); + } + let entry_path = root.join(entry); + let output = root.join("main_bin"); + let compile = Command::new(perry_bin()) + .current_dir(root) + .arg("compile") + .arg(&entry_path) + .arg("-o") + .arg(&output) + .env("PERRY_NO_CACHE", "1") + .output() + .expect("run perry compile"); + assert!( + compile.status.success(), + "perry compile failed\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&compile.stdout), + String::from_utf8_lossy(&compile.stderr) + ); + let run = Command::new(&output).output().expect("run binary"); + assert!( + run.status.success(), + "binary failed (status {:?})\nstdout:\n{}\nstderr:\n{}", + run.status.code(), + String::from_utf8_lossy(&run.stdout), + String::from_utf8_lossy(&run.stderr) + ); + String::from_utf8_lossy(&run.stdout).trim().to_string() +} + +/// The reduced `new Hono()`: a class with a private METHOD, declared in one +/// module and constructed in another. No inheritance is needed to trigger it. +#[test] +fn imported_class_with_private_method_constructs_once() { + let stdout = compile_and_run( + &[ + ( + "base.ts", + r#"export class BaseX { + #m(): number { return 41; } + call(): number { return this.#m() + 1; } +} +"#, + ), + ( + "main.ts", + r#"import { BaseX } from "./base"; +console.log(new BaseX().call()); +"#, + ), + ], + "main.ts", + ); + assert_eq!(stdout, "42"); +} + +/// A private ACCESSOR carries the same brand as a private method, and the stub +/// copies getter/setter names the same way. +#[test] +fn imported_class_with_private_getter_constructs_once() { + let stdout = compile_and_run( + &[ + ( + "base.ts", + r#"export class BaseX { + v = 41; + get #g(): number { return this.v + 1; } + call(): number { return this.#g; } +} +"#, + ), + ( + "main.ts", + r#"import { BaseX } from "./base"; +console.log(new BaseX().call()); +"#, + ), + ], + "main.ts", + ); + assert_eq!(stdout, "42"); +} + +/// A private-method class reached as an ANCESTOR: the leaf is local, so the +/// brand came from the `AncestorsOnly` walk at the `new` site rather than from +/// the leaf's own entry. Both spellings of the subclass — with and without an +/// explicit constructor — take different paths through `lower_new`. +#[test] +fn local_subclass_of_imported_private_method_class() { + let base = r#"export class BaseX { + #m(): number { return 41; } + call(): number { return this.#m() + 1; } +} +"#; + let with_ctor = compile_and_run( + &[ + ("base.ts", base), + ( + "main.ts", + r#"import { BaseX } from "./base"; +class D extends BaseX { constructor() { super(); } } +console.log(new D().call()); +"#, + ), + ], + "main.ts", + ); + assert_eq!(with_ctor, "42"); + + let without_ctor = compile_and_run( + &[ + ("base.ts", base), + ( + "main.ts", + r#"import { BaseX } from "./base"; +class D extends BaseX {} +console.log(new D().call()); +"#, + ), + ], + "main.ts", + ); + assert_eq!(without_ctor, "42"); +} + +/// hono's own shape: the base with the private members is in one module, the +/// subclass that `super()`s into it is an anonymous class expression in a +/// second, and the `new` is in a third. Every link in the chain is an imported +/// stub at the site that constructs it. +#[test] +fn imported_subclass_of_imported_private_method_class() { + let stdout = compile_and_run( + &[ + ( + "base.ts", + r#"const notFound = (x: string): string => "nf:" + x; +export class BaseX { + pub: number; + #path = "/"; + #nf = notFound; + constructor(options: any = {}) { + this.pub = 1; + } + #addRoute(m: string): string { return m + this.#path; } + route(m: string): string { return this.#addRoute(m) + this.#nf("!"); } +} +"#, + ), + ( + "mid.ts", + r#"import { BaseX } from "./base"; +export const DerivedX = class extends BaseX { + constructor(options: any = {}) { super(options); } +}; +"#, + ), + ( + "main.ts", + r#"import { DerivedX } from "./mid"; +const a = new DerivedX(); +console.log(a.route("GET") + "|" + a.pub); +"#, + ), + ], + "main.ts", + ); + assert_eq!(stdout, "GET/nf:!|1"); +} + +/// The same class constructed INSIDE its defining module never had the bug — +/// there the `new` site owns the field-initializer phase and installs the +/// brand itself. Pin it, so a fix that suppressed the install unconditionally +/// (rather than only where another module already performs it) fails here. +#[test] +fn same_module_construction_still_installs_the_brand() { + let stdout = compile_and_run( + &[ + ( + "base.ts", + r#"export class BaseX { + #m(): number { return 41; } + call(): number { return this.#m() + 1; } +} +export function make(): BaseX { return new BaseX(); } +"#, + ), + ( + "main.ts", + r#"import { make } from "./base"; +console.log(make().call()); +"#, + ), + ], + "main.ts", + ); + assert_eq!(stdout, "42"); +} + +/// Double initialization must still be observable where the spec requires it: +/// a base constructor that returns an object the derived class has already +/// branded. This is the case `js_private_brand_add`'s duplicate check exists +/// for, and #8962's fix must not silence it. +#[test] +fn genuine_double_initialization_still_throws() { + let dir = tempfile::tempdir().expect("tempdir"); + let root = dir.path(); + std::fs::write( + root.join("main.ts"), + r#"const recycled: any = {}; +class Base { + constructor() { return recycled; } +} +class Derived extends Base { + #m(): number { return 1; } + call(): number { return this.#m(); } +} +new Derived(); +new Derived(); +console.log("no throw"); +"#, + ) + .expect("write entry"); + let output = root.join("main_bin"); + let compile = Command::new(perry_bin()) + .current_dir(root) + .arg("compile") + .arg(root.join("main.ts")) + .arg("-o") + .arg(&output) + .env("PERRY_NO_CACHE", "1") + .output() + .expect("run perry compile"); + assert!( + compile.status.success(), + "perry compile failed\nstderr:\n{}", + String::from_utf8_lossy(&compile.stderr) + ); + let run = Command::new(&output).output().expect("run binary"); + let stderr = String::from_utf8_lossy(&run.stderr); + let stdout = String::from_utf8_lossy(&run.stdout); + assert!( + !run.status.success() && stderr.contains("private elements twice"), + "expected the second construction to throw the duplicate-brand \ + TypeError\nstatus: {:?}\nstdout:\n{stdout}\nstderr:\n{stderr}", + run.status.code() + ); +} From ace855bd601f161c76afee642bf36558dd05169e Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 28 Aug 2026 11:31:26 +0000 Subject: [PATCH 2/4] fix(link): group the ELF archive block so ld can resolve back into stdlib (#8930) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A `bundled-streams` build died with `undefined reference to ::notify` out of `libperry_ext_http.a`, even though the `libperry_stdlib.a` on the same command line exports that symbol from its own `futures_channel` member. The mechanism is archive order, but not the obvious one. The wrapper archives already appear twice — once before perry-stdlib, once after — so at a glance every reference has somewhere to go. What the #8930 link map shows is that the FIRST listing pulls nothing at all: the user objects reference only `js_*` symbols that perry-stdlib provides, so nothing in the wrapper is undefined yet when `ld` walks it. All 92 wrapper members that end up in the executable are pulled from the SECOND listing, on references that stdlib's own members had just opened (`js_node_http_*`, …) — and the references those members carry back INTO stdlib have nowhere to go, because GNU `ld` scans each archive once and never revisits one it has passed. That was harmless while each wrapper bundled its own copy of everything it closed over. It becomes a hard error the moment `strip_bundled_shared_deps_from_well_known_lib` drops a bundled member because stdlib provides it — a correct decision by the archive index, but one that only holds if `ld` can still get back to stdlib. That is what `bundled-streams` changes: it is the feature (enabled by a `fs/promises` or `stream/web` import, per `stdlib_features::module_to_features`) that pulls futures_channel into perry-stdlib's own graph, so stdlib starts bundling a name-matching `futures_channel-*.rcgu.o`, so the wrapper's copy becomes eligible to drop. A stdlib built without it carries no futures_channel member at all, the wrapper keeps its copy, and the link stays self-contained. #8939 tightened the pruning rule to the name-matched stdlib member's own exports; here that member does export the symbol, so the rule fires correctly and the link still fails. The bug is on the link line, not in the pruning. Wrap the perry archive block in `-Wl,--start-group` / `-Wl,--end-group` on ELF targets so `ld` re-scans it to a fixed point — the guarantee a mutually recursive archive set needs. Repeating one archive (the codebase's usual "archive twice" trick) only covers a one-step cycle, and this graph is LTO-partitioned across hundreds of codegen units on both sides. Members are still pulled left to right, so the wrappers-before-stdlib and localized-runtime-last first-definition-wins ordering is unaffected; Mach-O `ld64` resolves archives to a fixed point already (and rejects the flag) and `lld-link` / MSVC have no group concept, so both are left alone. Verified against both reported repros in mb24 — `apps/landing` (hono plus a compiled `@skelpo/cms-client`) and `apps/api` (hono + ws + mysql2, four wrapper archives, no `perry.compilePackages` at all). Both fail before and link after; replaying either final link line with only the two group flags removed reproduces the exact undefined reference. A case that already linked (`packages/db/src/migrate.ts`) produces a byte-identical executable with and without the flags. The link/strip-dedup unit tests pass (68), as do the two archive-ordering integration tests — `issue_5920_wrapper_bundled_runtime_async_starvation` (the two-runtime-copies hazard) and `issue_6715_native_wrapper_precedence`. `native_link_cache` fails identically on the unpatched parent commit. --- changelog.d/8930-elf-archive-group.md | 15 +++ .../commands/compile/link/build_and_run.rs | 8 ++ crates/perry/src/commands/compile/link/mod.rs | 113 ++++++++++++++++++ 3 files changed, 136 insertions(+) create mode 100644 changelog.d/8930-elf-archive-group.md diff --git a/changelog.d/8930-elf-archive-group.md b/changelog.d/8930-elf-archive-group.md new file mode 100644 index 0000000000..3956ec184d --- /dev/null +++ b/changelog.d/8930-elf-archive-group.md @@ -0,0 +1,15 @@ +Fixed ELF links that died with `undefined reference to +::notify` out of a well-known wrapper +archive when a compiled package pulled in `bundled-streams`. + +GNU `ld` scans each archive on the command line exactly once, left to right, +and the perry archive block is mutually recursive: the wrapper archives listed +*before* perry-stdlib resolve nothing (the user objects reference only stdlib +symbols), every wrapper member is pulled from the repeat *after* stdlib on +references stdlib itself opened, and those members then reference back into +stdlib — which `ld` has already walked past. That stayed invisible until +shared-dependency pruning correctly dropped a bundled member because stdlib +exports it. The archive block is now wrapped in `-Wl,--start-group` / +`-Wl,--end-group` on ELF targets so the linker re-scans it to a fixed point. +Symbol precedence is unchanged and non-ELF link lines are untouched; a link +that already resolved produces a byte-identical executable. diff --git a/crates/perry/src/commands/compile/link/build_and_run.rs b/crates/perry/src/commands/compile/link/build_and_run.rs index 2f0b316baf..6da6bd6cbd 100644 --- a/crates/perry/src/commands/compile/link/build_and_run.rs +++ b/crates/perry/src/commands/compile/link/build_and_run.rs @@ -437,6 +437,13 @@ pub(crate) fn build_and_run_link( // Multiple specifiers can route to the same native archive (`http` and // `https` both select perry_ext_http). They were de-duplicated before the // archive-preparation pass so the expensive transform also runs once. + // #8930: the archives below reference each other in BOTH directions and + // ELF `ld` scans each once — bracket them (see `ElfArchiveGroup`). + let archive_group = ElfArchiveGroup::open( + &mut cmd, + !is_windows && (is_linux || is_android || is_harmonyos), + !skip_runtime || ctx.needs_stdlib, + ); if !skip_runtime { if ctx.needs_stdlib || is_windows { // On Windows/MSVC, always try to link stdlib because codegen unconditionally @@ -550,6 +557,7 @@ pub(crate) fn build_and_run_link( eprintln!("Warning: stdlib required but libperry_stdlib.a not found"); } } + archive_group.close(&mut cmd); // Issue #76 — wasmi host runtime, opt-in via `--enable-wasm-runtime`. // Append after stdlib so the linker can resolve `perry_wasm_host_*` diff --git a/crates/perry/src/commands/compile/link/mod.rs b/crates/perry/src/commands/compile/link/mod.rs index 3c0b1d3413..a988b8b28e 100644 --- a/crates/perry/src/commands/compile/link/mod.rs +++ b/crates/perry/src/commands/compile/link/mod.rs @@ -648,6 +648,119 @@ fn rewrite_link_with_response_file(cmd: &Command, msvc: bool) -> Option<(Command Some((new_cmd, rsp)) } +/// `--start-group` / `--end-group` around the perry static-archive block on +/// ELF links (#8930). +/// +/// GNU `ld` scans each archive on the command line exactly once, left to +/// right, and pulls only the members that resolve symbols undefined *at that +/// moment*. The perry archive block is mutually recursive, and the direction +/// of that recursion is the opposite of the command-line order: +/// +/// * the user objects reference `js_*` symbols that **perry-stdlib** +/// provides, so the well-known wrapper archives listed BEFORE stdlib +/// resolve nothing at all — in the #8930 repro not one of the 92 +/// eventually-linked wrapper members was pulled from that first listing; +/// * every wrapper member came from the repeat AFTER stdlib, on references +/// that stdlib's own members had just opened (`js_node_http_*`, …); +/// * those wrapper members then reference back INTO stdlib, which `ld` has +/// already walked past and never revisits. +/// +/// While a wrapper archive bundled its own copy of everything it closed over, +/// the third step was invisible. It turns into a hard error the moment +/// `strip_bundled_shared_deps_from_well_known_lib` drops a bundled member +/// *because stdlib provides it* — a correct decision by the archive index, +/// but one that only holds if `ld` can still get back to stdlib. #8930 is +/// exactly that shape: `futures_channel::mpsc::SenderTask::notify`, exported +/// by the linked `libperry_stdlib.a`'s own `futures_channel` member and +/// referenced by a kept `perry_ext_http` codegen unit, with no path from the +/// one to the other. +/// +/// A group makes `ld` re-scan the block until it reaches a fixed point, which +/// is the guarantee a mutually recursive archive set needs. Repeating a single +/// archive (the codebase's usual "archive twice" trick — see the GTK4 stdlib +/// re-link) only covers a one-step cycle, and this graph is LTO-partitioned +/// across hundreds of codegen units on both sides. Symbol precedence is +/// unchanged: members are still pulled left to right, so the +/// wrappers-before-stdlib and localized-runtime-last first-definition-wins +/// ordering keeps working. +/// +/// ELF only. Mach-O's `ld64` already resolves archives to a fixed point (and +/// rejects the flag); `lld-link` / MSVC `link.exe` have no group concept. +pub(super) struct ElfArchiveGroup { + opened: bool, +} + +impl ElfArchiveGroup { + /// Emit `-Wl,--start-group` when this is an ELF link that will actually + /// receive archives. `has_archives` keeps an empty group off the command + /// line for the link shapes that add none. + pub(super) fn open(cmd: &mut Command, is_elf: bool, has_archives: bool) -> Self { + let opened = is_elf && has_archives; + if opened { + cmd.arg("-Wl,--start-group"); + } + Self { opened } + } + + /// Close a group opened by [`ElfArchiveGroup::open`]. Takes `self` by + /// value so the marker cannot be emitted twice. + pub(super) fn close(self, cmd: &mut Command) { + if self.opened { + cmd.arg("-Wl,--end-group"); + } + } +} + +#[cfg(test)] +mod elf_archive_group_tests { + use super::ElfArchiveGroup; + use std::process::Command; + + fn args(is_elf: bool, archives: &[&str]) -> Vec { + let mut cmd = Command::new("cc"); + let group = ElfArchiveGroup::open(&mut cmd, is_elf, !archives.is_empty()); + for archive in archives { + cmd.arg(archive); + } + group.close(&mut cmd); + cmd.get_args() + .map(|arg| arg.to_string_lossy().into_owned()) + .collect() + } + + /// #8930 — an ELF link brackets the archive block so `ld` re-scans it to a + /// fixed point instead of dying on a stdlib symbol it already walked past. + #[test] + fn elf_link_brackets_the_archive_block() { + assert_eq!( + args(true, &["wrapper.a", "libperry_stdlib.a", "wrapper.a"]), + vec![ + "-Wl,--start-group", + "wrapper.a", + "libperry_stdlib.a", + "wrapper.a", + "-Wl,--end-group", + ] + ); + } + + /// Mach-O's `ld64` rejects the flag and needs no group; `lld-link` / MSVC + /// have no group concept. Those command lines must come out unchanged. + #[test] + fn non_elf_link_is_untouched() { + assert_eq!( + args(false, &["libperry_stdlib.a"]), + vec!["libperry_stdlib.a"] + ); + } + + /// A link shape that contributes no archive must not emit an empty group. + #[test] + fn empty_archive_block_emits_no_group() { + assert!(args(true, &[]).is_empty()); + } +} + #[cfg(test)] mod optional_framework_dir_tests; From 362a1a743bfc24ce52c014cbcb73c8ca75911591 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Mon, 31 Aug 2026 11:11:51 +0000 Subject: [PATCH 3/4] fix: preserve private compound reads and response headers (#8968) Lower the read half of private compound assignments through the class-mangled storage key and the normal private brand guard. Parse dynamic HeadersInit values in Response constructors and carry string-body metadata explicitly through the Response ABI so default content types survive without mutable side-channel state. --- .../perry-codegen/src/lower_call/builtin.rs | 9 +- crates/perry-codegen/src/runtime_decls/mod.rs | 1 + .../src/runtime_decls/strings_part2.rs | 4 +- crates/perry-ext-fetch/src/lib.rs | 14 +- crates/perry-ext-fetch/src/tests.rs | 11 + crates/perry-hir/src/lower/mod.rs | 4 +- crates/perry-hir/src/lower_patterns.rs | 48 +++- .../perry-runtime/src/object/global_fetch.rs | 32 ++- .../src/object/global_this/fetch_globals.rs | 6 + crates/perry-runtime/src/stdlib_stubs.rs | 1 + crates/perry-runtime/src/value/mod.rs | 5 +- crates/perry-runtime/src/value/nanbox.rs | 11 + .../perry-stdlib/src/common/dispatch/init.rs | 1 + crates/perry-stdlib/src/fetch/headers.rs | 40 +++ crates/perry-stdlib/src/fetch/mod.rs | 21 +- .../perry-stdlib/src/fetch/response_ctor.rs | 55 +++- crates/perry-stdlib/src/fetch/tests.rs | 1 + .../issue_8968_private_compound_assignment.rs | 272 ++++++++++++++++++ .../tests/issue_8968_response_headers.rs | 143 +++++++++ 19 files changed, 633 insertions(+), 46 deletions(-) create mode 100644 crates/perry/tests/issue_8968_private_compound_assignment.rs create mode 100644 crates/perry/tests/issue_8968_response_headers.rs diff --git a/crates/perry-codegen/src/lower_call/builtin.rs b/crates/perry-codegen/src/lower_call/builtin.rs index 41c766ed3f..266b4749f9 100644 --- a/crates/perry-codegen/src/lower_call/builtin.rs +++ b/crates/perry-codegen/src/lower_call/builtin.rs @@ -1061,12 +1061,14 @@ pub(super) fn lower_builtin_new<'a>( // `new Response(res.body, res)` header re-wrap — is drained to its // bytes instead of stringified to its numeric stream handle. // Non-stream bodies coerce exactly as get_raw_string_ptr did. - let body_ptr = if !args.is_empty() { + let (body_ptr, body_is_string) = if !args.is_empty() { let v = lower_expr(ctx, &args[0])?; let blk = ctx.block(); - blk.call(I64, "js_response_body_init_ptr", &[(DOUBLE, &v)]) + let is_string = blk.call(I32, "js_nanbox_is_any_string", &[(DOUBLE, &v)]); + let body_ptr = blk.call(I64, "js_response_body_init_ptr", &[(DOUBLE, &v)]); + (body_ptr, is_string) } else { - "0".to_string() + ("0".to_string(), "0".to_string()) }; // Default init: status=200, statusText=null, headers=0 @@ -1164,6 +1166,7 @@ pub(super) fn lower_builtin_new<'a>( (DOUBLE, &status_val), (I64, &status_text_ptr), (DOUBLE, &headers_handle), + (I32, &body_is_string), ], ); // Response handle is a plain numeric f64 (response-registry id). diff --git a/crates/perry-codegen/src/runtime_decls/mod.rs b/crates/perry-codegen/src/runtime_decls/mod.rs index a0781361f5..c3eddac932 100644 --- a/crates/perry-codegen/src/runtime_decls/mod.rs +++ b/crates/perry-codegen/src/runtime_decls/mod.rs @@ -111,6 +111,7 @@ pub fn declare_phase1(module: &mut LlModule) { module.declare_function("js_nanbox_string", DOUBLE, &[I64]); module.declare_function("js_nanbox_pointer", DOUBLE, &[I64]); module.declare_function("js_nanbox_get_pointer", I64, &[DOUBLE]); + module.declare_function("js_nanbox_is_any_string", I32, &[DOUBLE]); module.declare_function( "js_native_handle_new_owned", DOUBLE, diff --git a/crates/perry-codegen/src/runtime_decls/strings_part2.rs b/crates/perry-codegen/src/runtime_decls/strings_part2.rs index c8d9417808..df8307afa2 100644 --- a/crates/perry-codegen/src/runtime_decls/strings_part2.rs +++ b/crates/perry-codegen/src/runtime_decls/strings_part2.rs @@ -944,8 +944,8 @@ pub(crate) fn declare_phase_b_strings_part2(module: &mut LlModule) { // property access (`request.url` where `request: any`) routes through // `js_object_get_field_by_name`'s strip-tag path → `HANDLE_PROPERTY_DISPATCH`. // ────────────────────────────────────────────────────────────────── - // new Response(body_ptr, status, status_text_ptr, headers_handle) -> f64 - module.declare_function("js_response_new", DOUBLE, &[I64, DOUBLE, I64, DOUBLE]); + // new Response(body_ptr, status, status_text_ptr, headers, body_is_string) -> f64 + module.declare_function("js_response_new", DOUBLE, &[I64, DOUBLE, I64, DOUBLE, I32]); // Normalize a Request/Response subclass object (for example NextResponse) // to its native Fetch registry handle; bare handles pass through. module.declare_function("js_fetch_unwrap_handle", DOUBLE, &[DOUBLE]); diff --git a/crates/perry-ext-fetch/src/lib.rs b/crates/perry-ext-fetch/src/lib.rs index b765794f01..03f94a9d7f 100644 --- a/crates/perry-ext-fetch/src/lib.rs +++ b/crates/perry-ext-fetch/src/lib.rs @@ -1179,21 +1179,22 @@ pub extern "C" fn js_headers_entries(handle: f64) -> f64 { // ── Response advanced ───────────────────────────────────────────── /// `new Response(body, init)` — stores body string + status + statusText -/// + headers. The `headers_handle` arg matches perry-stdlib's 4-arg shape -/// (declared in `crates/perry-codegen/src/runtime_decls.rs:1045`); a -/// 3-arg version dropped the codegen-supplied headers handle on the +/// + headers. The arguments match perry-stdlib's constructor ABI +/// (declared in `crates/perry-codegen/src/runtime_decls/strings_part2.rs`); +/// a 3-arg version dropped the codegen-supplied headers handle on the /// floor — `fetchRes.headers.forEach(...)` then iterated an empty map. /// /// # Safety /// All string pointers must be null or Perry-runtime `StringHeader`s; /// `headers_handle` must be 0.0 / TAG_UNDEFINED or a valid handle id -/// returned by `js_headers_new`. +/// returned by `js_headers_new`; `body_is_string` is a boolean i32. #[no_mangle] pub unsafe extern "C" fn js_response_new( body_ptr: *const StringHeader, status: f64, status_text_ptr: *const StringHeader, headers_handle: f64, + body_is_string: i32, ) -> f64 { let body_opt = read_str(body_ptr); let body_present = body_opt.is_some(); @@ -1224,7 +1225,7 @@ pub unsafe extern "C" fn js_response_new( )); } let headers_id = handle_id(headers_handle); - let headers = if headers_id != 0 { + let mut headers = if headers_id != 0 { HEADERS_HANDLES .lock() .unwrap() @@ -1234,6 +1235,9 @@ pub unsafe extern "C" fn js_response_new( } else { HeadersStore::default() }; + if body_is_string != 0 && !headers.has("content-type") { + headers.set("content-type", "text/plain;charset=UTF-8"); + } store_response(FetchResponse { status, status_text, diff --git a/crates/perry-ext-fetch/src/tests.rs b/crates/perry-ext-fetch/src/tests.rs index 53bc342569..5d18deb89f 100644 --- a/crates/perry-ext-fetch/src/tests.rs +++ b/crates/perry-ext-fetch/src/tests.rs @@ -109,6 +109,17 @@ fn response_static_json() { assert_eq!(status, 200.0); } +#[test] +fn response_string_body_gets_default_content_type() { + let body = alloc_string("hello"); + let response = unsafe { js_response_new(body.as_raw(), 0.0, std::ptr::null(), 0.0, 1) }; + let headers = js_response_get_headers(response); + let key = alloc_string("content-type"); + let value_ptr = unsafe { js_headers_get(headers, key.as_raw()) }; + let value = perry_ffi::read_string(unsafe { JsString::from_raw(value_ptr) }).expect("header"); + assert_eq!(value, "text/plain;charset=UTF-8"); +} + // #1688: request.text()/.json()/.arrayBuffer() were unimplemented. The // FFIs build a JsPromise (runtime symbols unavailable in the unittest // binary, as with every other promise-returning fetch FFI), so this diff --git a/crates/perry-hir/src/lower/mod.rs b/crates/perry-hir/src/lower/mod.rs index 6ce65947ac..7a2cc5d62c 100644 --- a/crates/perry-hir/src/lower/mod.rs +++ b/crates/perry-hir/src/lower/mod.rs @@ -42,7 +42,9 @@ mod expr_call; pub(crate) mod expr_function; pub(crate) use expr_function::capture_function_source; mod expr_member; -pub(crate) use expr_member::{private_storage_property, wrap_private_guard, PRIV_OP_WRITE}; +pub(crate) use expr_member::{ + private_storage_property, wrap_private_guard, PRIV_OP_READ, PRIV_OP_WRITE, +}; mod expr_misc; mod expr_new; mod expr_new_builtins; diff --git a/crates/perry-hir/src/lower_patterns.rs b/crates/perry-hir/src/lower_patterns.rs index 217044e333..2ade3096e2 100644 --- a/crates/perry-hir/src/lower_patterns.rs +++ b/crates/perry-hir/src/lower_patterns.rs @@ -4,7 +4,9 @@ //! parameter destructuring, and other pattern-related utilities. use crate::ir::*; -use crate::lower::{lower_expr, LoweringContext}; +use crate::lower::{ + lower_expr, private_storage_property, wrap_private_guard, LoweringContext, PRIV_OP_READ, +}; use crate::lower_types::*; use crate::types::{LocalId, Type}; use anyhow::{anyhow, Result}; @@ -244,9 +246,49 @@ pub(crate) fn lower_assign_target_to_expr( Ok(Expr::IndexGet { object, index }) } ast::MemberProp::PrivateName(private) => { - let property = format!("#{}", private.name); + // #8968: the READ half of a compound / logical assignment + // to a private member must lower exactly like an ordinary + // `this.#x` read in `expr_member/member_tail.rs` — brand- + // guard the receiver, and address the slot by its MANGLED + // storage key. + // + // This arm used to build `PropertyGet { property: "#x" }` + // from the source spelling. A private FIELD does not live + // under that key: `private_storage_property` stores it as + // `#`, a mangling that + // exists precisely so a private field cannot collide with + // an ordinary computed property such as `obj["#x"]`. So + // the read missed every time and produced `undefined`, + // while the WRITE half — lowered through + // `lower_expr_assignment`, which does use the mangled key + // — landed correctly. Nothing threw; the value was simply + // wrong: + // + // this.#n += 1 // NaN, not n + 1 + // this.#s += "b" // "undefinedb" + // this.#v ||= d // ALWAYS stored d (read was falsy) + // this.#v ??= d // ALWAYS stored d (read was nullish) + // this.#v &&= d // NEVER stored (read was falsy) + // + // hono is the reported case (#8968): its + // `get res() { return this.#res ||= new Response(null, …) }` + // meant every read of `c.res` replaced the finalized + // response with a fresh empty 200, so an unmatched route + // answered `200 ""` and a registered `app.notFound()` + // handler had its 404 thrown away after it had already run. + // + // The guard is also what makes the mangled key resolvable + // for a private METHOD or ACCESSOR: `js_private_guard` + // pushes the access hint that the runtime + // `private_member_get_by_name` consumes. It restores the + // spec-required TypeError for the read of a set-only + // private accessor (`set #x(v) {}` then `obj.#x ||= 1`) + // and for a compound assignment on a foreign receiver. + let private_name = format!("#{}", private.name); + let object = wrap_private_guard(ctx, object, &private_name, PRIV_OP_READ); + let property = private_storage_property(ctx, &private_name); Ok(Expr::PropertyGet { - byte_offset: 0, + byte_offset: member.span.lo.0, object, property, }) diff --git a/crates/perry-runtime/src/object/global_fetch.rs b/crates/perry-runtime/src/object/global_fetch.rs index a4284fc977..313025a15c 100644 --- a/crates/perry-runtime/src/object/global_fetch.rs +++ b/crates/perry-runtime/src/object/global_fetch.rs @@ -86,8 +86,13 @@ type FetchRequestNewFn = unsafe extern "C" fn( *const crate::StringHeader, f64, ) -> f64; -type FetchResponseNewFn = - unsafe extern "C" fn(*const crate::StringHeader, f64, *const crate::StringHeader, f64) -> f64; +type FetchResponseNewFn = unsafe extern "C" fn( + *const crate::StringHeader, + f64, + *const crate::StringHeader, + f64, + i32, +) -> f64; type FetchResponseStaticJsonFn = unsafe extern "C" fn(f64, f64, *const crate::StringHeader, f64) -> f64; type FetchResponseStaticRedirectFn = unsafe extern "C" fn(*const crate::StringHeader, f64) -> f64; @@ -296,6 +301,7 @@ unsafe extern "C" { status: f64, status_text_ptr: *const crate::StringHeader, headers_handle: f64, + body_is_string: i32, ) -> f64; fn js_response_static_json( value: f64, @@ -572,8 +578,17 @@ pub(super) fn call_global_response_new( status: f64, status_text_ptr: *const crate::StringHeader, headers_handle: f64, + body_is_string: i32, ) -> f64 { - unsafe { js_response_new(body_ptr, status, status_text_ptr, headers_handle) } + unsafe { + js_response_new( + body_ptr, + status, + status_text_ptr, + headers_handle, + body_is_string, + ) + } } #[cfg(not(feature = "external-fetch-symbols"))] @@ -582,11 +597,20 @@ pub(super) fn call_global_response_new( status: f64, status_text_ptr: *const crate::StringHeader, headers_handle: f64, + body_is_string: i32, ) -> f64 { let f = GLOBAL_FETCH_RESPONSE_NEW.load(Ordering::Acquire); if !f.is_null() { let func: FetchResponseNewFn = unsafe { std::mem::transmute(f) }; - return unsafe { func(body_ptr, status, status_text_ptr, headers_handle) }; + return unsafe { + func( + body_ptr, + status, + status_text_ptr, + headers_handle, + body_is_string, + ) + }; } warn_unregistered_fetch_symbol("js_response_new") } diff --git a/crates/perry-runtime/src/object/global_this/fetch_globals.rs b/crates/perry-runtime/src/object/global_this/fetch_globals.rs index 0140e9987d..699febda70 100644 --- a/crates/perry-runtime/src/object/global_this/fetch_globals.rs +++ b/crates/perry-runtime/src/object/global_this/fetch_globals.rs @@ -261,6 +261,11 @@ pub(crate) extern "C" fn global_this_response_thunk( body: f64, init: f64, ) -> f64 { + let body_is_string = if crate::value::JSValue::from_bits(body.to_bits()).is_any_string() { + 1 + } else { + 0 + }; // Route the body through the registered body-init helper (stdlib // `js_response_body_init_ptr`) so a binary body — Buffer / Uint8Array / // ArrayBuffer — copies its raw bytes instead of being stringified to a @@ -287,6 +292,7 @@ pub(crate) extern "C" fn global_this_response_thunk( status, status_text_ptr, headers_handle, + body_is_string, ) } diff --git a/crates/perry-runtime/src/stdlib_stubs.rs b/crates/perry-runtime/src/stdlib_stubs.rs index 20ae4684c4..7bd7bc88a1 100644 --- a/crates/perry-runtime/src/stdlib_stubs.rs +++ b/crates/perry-runtime/src/stdlib_stubs.rs @@ -222,6 +222,7 @@ pub extern "C" fn js_response_new( _status: f64, _status_text_ptr: *const crate::string::StringHeader, _headers_handle: f64, + _body_is_string: i32, ) -> f64 { perry_stub_warn("js_response_new", FETCH_REASON, None); f64::from_bits(crate::value::TAG_UNDEFINED) diff --git a/crates/perry-runtime/src/value/mod.rs b/crates/perry-runtime/src/value/mod.rs index b6b83020ed..ef1718a46d 100644 --- a/crates/perry-runtime/src/value/mod.rs +++ b/crates/perry-runtime/src/value/mod.rs @@ -98,8 +98,9 @@ pub use handle::{ pub(crate) use nanbox::nanbox_string_key; pub use nanbox::{ js_checkpoint, js_debug_val, js_get_string_pointer_unified, js_nanbox_bigint, - js_nanbox_get_bigint, js_nanbox_get_pointer, js_nanbox_get_string_pointer, js_nanbox_is_bigint, - js_nanbox_is_pointer, js_nanbox_is_string, js_nanbox_pointer, js_nanbox_string, + js_nanbox_get_bigint, js_nanbox_get_pointer, js_nanbox_get_string_pointer, + js_nanbox_is_any_string, js_nanbox_is_bigint, js_nanbox_is_pointer, js_nanbox_is_string, + js_nanbox_pointer, js_nanbox_string, }; // ----- Dynamic arithmetic dispatch (BigInt vs float) ----- diff --git a/crates/perry-runtime/src/value/nanbox.rs b/crates/perry-runtime/src/value/nanbox.rs index e9cdd95451..a2183ee86a 100644 --- a/crates/perry-runtime/src/value/nanbox.rs +++ b/crates/perry-runtime/src/value/nanbox.rs @@ -389,3 +389,14 @@ pub extern "C" fn js_nanbox_is_string(value: f64) -> i32 { 0 } } + +/// Check if a NaN-boxed f64 value is either a heap or inline string. +#[no_mangle] +pub extern "C" fn js_nanbox_is_any_string(value: f64) -> i32 { + let jsval = JSValue::from_bits(value.to_bits()); + if jsval.is_any_string() { + 1 + } else { + 0 + } +} diff --git a/crates/perry-stdlib/src/common/dispatch/init.rs b/crates/perry-stdlib/src/common/dispatch/init.rs index 1cf933d6e4..9fa2f78358 100644 --- a/crates/perry-stdlib/src/common/dispatch/init.rs +++ b/crates/perry-stdlib/src/common/dispatch/init.rs @@ -494,6 +494,7 @@ pub unsafe extern "C" fn js_stdlib_init_dispatch() { f64, *const perry_runtime::StringHeader, f64, + i32, ) -> f64, response_static_json: unsafe extern "C" fn( f64, diff --git a/crates/perry-stdlib/src/fetch/headers.rs b/crates/perry-stdlib/src/fetch/headers.rs index 9ef2b4cf82..657af5eeee 100644 --- a/crates/perry-stdlib/src/fetch/headers.rs +++ b/crates/perry-stdlib/src/fetch/headers.rs @@ -100,6 +100,46 @@ fn is_headers_init_iterable(value: f64) -> bool { || has_sync_iterator(value) } +/// Build a standalone store from any `HeadersInit` representation: an existing +/// Headers handle, a record object, or an iterable of `[name, value]` pairs. +/// +/// `0.0` is the internal "no headers" ABI sentinel and `undefined` means the +/// same at a dynamic call site. Other invalid values follow the existing +/// Headers constructor validation instead of being silently discarded. +pub(super) unsafe fn headers_store_from_init_value(init: f64) -> Option { + if init == 0.0 { + return None; + } + let init_value = JSValue::from_bits(init.to_bits()); + if init_value.is_undefined() { + return None; + } + if init_value.is_null() { + headers_init_type_error("Headers constructor: init must not be null"); + } + + let source_id = handle_id(init); + if let Some(store) = HEADERS_REGISTRY.lock().unwrap().get(&source_id).cloned() { + return Some(store); + } + + let scope = perry_runtime::gc::RuntimeHandleScope::new(); + let rooted = scope.root_nanbox_f64(init); + let init_now = rooted.get_nanbox_f64(); + let entries = match read_headers_record_entries(init_now, &scope) { + Some(entries) => entries, + None => { + let array = materialize_headers_init_iterable(init_now, &scope); + read_headers_iterable_entries(array, &scope) + } + }; + let mut store = HeadersStore::default(); + for (key, value) in entries { + store.append(&key, &value); + } + Some(store) +} + fn read_headers_record_entries( value: f64, scope: &perry_runtime::gc::RuntimeHandleScope, diff --git a/crates/perry-stdlib/src/fetch/mod.rs b/crates/perry-stdlib/src/fetch/mod.rs index 81bc085ce3..17b4a99e0a 100644 --- a/crates/perry-stdlib/src/fetch/mod.rs +++ b/crates/perry-stdlib/src/fetch/mod.rs @@ -275,6 +275,11 @@ thread_local! { static PENDING_FETCH_BODY_STREAM_ID: Cell = const { Cell::new(0) }; } +/// `text/plain;charset=UTF-8` — the exact spelling the Fetch standard gives a +/// body extracted from a string (no space after the semicolon; node emits it +/// verbatim, and hono's `c.text()` relies on `new Response(str)` producing it). +pub(super) const BODY_CONTENT_TYPE_TEXT_PLAIN: &str = "text/plain;charset=UTF-8"; + fn take_pending_fetch_body_stream_id() -> Option { PENDING_FETCH_BODY_STREAM_ID.with(|pending| { let id = pending.get(); @@ -1629,19 +1634,9 @@ pub unsafe extern "C" fn js_response_static_json( // Node's `Response.json` leaves statusText "" when not provided — it does // not fall back to the status reason phrase. let status_text = string_from_header(init_status_text_ptr).unwrap_or_default(); - // Start from any user-provided headers, then add the default content-type - // only if the init headers didn't already set one. - let headers_id = handle_id(headers_handle); - let mut headers = if headers_id != 0 { - HEADERS_REGISTRY - .lock() - .unwrap() - .get(&headers_id) - .cloned() - .unwrap_or_default() - } else { - HeadersStore::default() - }; + // Accept the complete HeadersInit surface, including a runtime record or + // iterable that codegen could not pre-materialize as a Headers handle. + let mut headers = headers_store_from_init_value(headers_handle).unwrap_or_default(); if !headers.has("content-type") { headers.set("content-type", "application/json"); } diff --git a/crates/perry-stdlib/src/fetch/response_ctor.rs b/crates/perry-stdlib/src/fetch/response_ctor.rs index 67453a62a7..b56d3af7f7 100644 --- a/crates/perry-stdlib/src/fetch/response_ctor.rs +++ b/crates/perry-stdlib/src/fetch/response_ctor.rs @@ -32,13 +32,15 @@ pub(super) fn alloc_response( /// - body_ptr: StringHeader for the body, or null for "" /// - status: f64 (200 default) /// - status_text_ptr: StringHeader for statusText, or null for "" -/// - headers_handle: f64 numeric handle from js_headers_new, or 0 +/// - headers_handle: Headers handle, raw HeadersInit value, or 0 +/// - body_is_string: nonzero when the original body JS value was a string #[no_mangle] pub unsafe extern "C" fn js_response_new( body_ptr: *const StringHeader, status: f64, status_text_ptr: *const StringHeader, headers_handle: f64, + body_is_string: i32, ) -> f64 { let body_stream_id = take_pending_fetch_body_stream_id(); // Lossless raw-byte read so binary bodies survive byte-for-byte (#5435). @@ -76,21 +78,48 @@ pub unsafe extern "C" fn js_response_new( "Response constructor: Invalid response status code {status_u16}" )); } - let headers_id = handle_id(headers_handle); - let headers = if headers_id != 0 { - HEADERS_REGISTRY - .lock() - .unwrap() - .get(&headers_id) - .cloned() - .unwrap_or_default() - } else { - HeadersStore::default() - }; + // `headers_handle` is documented as "an f64 handle from `js_headers_new`", + // and codegen produces one whenever it can SEE the header object: an inline + // `{ ... }` literal, or a local it tracked as an options object. When it + // cannot — a `??` expression, a spread-built object, a call result, or the + // `headers` field read off a runtime options object — it passed the plain + // NaN-boxed JS value straight through instead. That value is not a registry + // id, so this lookup missed and the response was built with NO headers at + // all. No throw, no warning. + // + // hono lost the `Content-Type` of every `c.json()` / `c.html()` that way: + // `#newResponse` ends in `new Response(data, { status, headers: + // responseHeaders ?? headers })`, handed to `createResponseInstance = + // (body, init) => new Response(body, init)` — so BOTH the init object and + // its `headers` value reach codegen as opaque expressions. Fixing it here + // rather than in codegen covers those two call shapes with one change. + // + // A value that is not already a registry handle is parsed through the same + // complete HeadersInit path as `new Headers(init)`: records and pair + // iterables are preserved, while malformed values raise the corresponding + // constructor TypeError instead of being silently dropped. + let headers_init = headers_store_from_init_value(headers_handle); + let had_headers_init = headers_init.is_some(); + let mut headers = headers_init.unwrap_or_default(); + // Fetch §"extract a body" + the `Response` constructor: the extracted + // body's Content-Type is installed only when `init.headers` did not + // already carry one (an explicit header always wins, and never gets a + // second value appended). Codegen/runtime captured `body_is_string` before + // `js_response_body_init_ptr` erased that distinction by coercing the body + // to a byte string. Carrying the bit explicitly keeps nested constructors + // independent and cannot retain state when init evaluation throws. + // + // Missing this made `new Response("hello")` answer with NO content type + // where node answers `text/plain;charset=UTF-8` — the whole of hono's + // `c.text()`, which short-circuits to exactly that constructor call when + // no header, status or prepared header has been set on the context. + if body_is_string != 0 && headers.get("content-type").is_none() { + headers.set("content-type", BODY_CONTENT_TYPE_TEXT_PLAIN); + } // A Response owns a private Headers list. The constructor input may be an // existing Headers object, so retaining its registry id would make // mutations alias in both directions instead of copying the initializer. - let response_headers_id = (headers_id != 0).then(|| alloc_headers(headers.clone())); + let response_headers_id = had_headers_init.then(|| alloc_headers(headers.clone())); let id = alloc_response(status_u16, status_text, headers, body, body_present); if response_headers_id.is_some() || body_stream_id.is_some() { if let Some(resp) = FETCH_RESPONSES.lock().unwrap().get_mut(&id) { diff --git a/crates/perry-stdlib/src/fetch/tests.rs b/crates/perry-stdlib/src/fetch/tests.rs index 11b5fa23e6..9525a2da5e 100644 --- a/crates/perry-stdlib/src/fetch/tests.rs +++ b/crates/perry-stdlib/src/fetch/tests.rs @@ -89,6 +89,7 @@ fn response_constructor_copies_headers_initializer() { 200.0, std::ptr::null(), handle_to_f64(source_id), + 0, ) }; let response_id = handle_id(response); diff --git a/crates/perry/tests/issue_8968_private_compound_assignment.rs b/crates/perry/tests/issue_8968_private_compound_assignment.rs new file mode 100644 index 0000000000..447e8060fa --- /dev/null +++ b/crates/perry/tests/issue_8968_private_compound_assignment.rs @@ -0,0 +1,272 @@ +//! Regression for #8968: a compound or logical assignment to a PRIVATE member +//! read the wrong slot, and answered `undefined` every time. +//! +//! `lower_assign_target_to_expr` builds the READ half of `a op= b`. Its +//! private-name arm addressed the field by its SOURCE spelling +//! (`PropertyGet { property: "#x" }`) while every other private access — the +//! ordinary read in `expr_member/member_tail.rs` and the write half in +//! `lower_expr_assignment` — addresses it by the MANGLED storage key +//! `private_storage_property` produces (`#`). So +//! the read missed, silently: +//! +//! this.#n += 1 // NaN +//! this.#s += "b" // "undefinedb" +//! this.#v ||= d // ALWAYS stored d +//! this.#v ??= d // ALWAYS stored d +//! this.#v &&= d // NEVER stored +//! +//! The report was hono answering an unmatched route with `200 ""` and never +//! invoking a registered `app.notFound()` handler. hono's `Context` memoizes +//! its response as `get res() { return this.#res ||= new Response(null, …) }`, +//! so every read of `c.res` threw away the finalized response — including the +//! 404 the not-found handler had already produced — and replaced it with a +//! fresh empty 200. A matched route never reads `c.res` (the single-handler +//! fast path returns the handler's response directly), which is exactly why +//! only the miss path was visibly wrong. +//! +//! Nothing here asserts on an error: every case is a WRONG ANSWER, so each +//! test pins the value, not the absence of a throw. + +use std::path::PathBuf; +use std::process::Command; + +fn perry_bin() -> PathBuf { + PathBuf::from(env!("CARGO_BIN_EXE_perry")) +} + +/// Compile `source` as a single entry module and return its stdout. +fn run_source(source: &str) -> String { + let dir = tempfile::tempdir().expect("tempdir"); + let root = dir.path(); + let entry = root.join("main.ts"); + std::fs::write(&entry, source).expect("write source"); + let output = root.join("main_bin"); + let compile = Command::new(perry_bin()) + .current_dir(root) + .arg("compile") + .arg(&entry) + .arg("-o") + .arg(&output) + .env("PERRY_NO_CACHE", "1") + .output() + .expect("run perry compile"); + assert!( + compile.status.success(), + "perry compile failed\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&compile.stdout), + String::from_utf8_lossy(&compile.stderr) + ); + let run = Command::new(&output).output().expect("run binary"); + assert!( + run.status.success(), + "binary failed (status {:?})\nstdout:\n{}\nstderr:\n{}", + run.status.code(), + String::from_utf8_lossy(&run.stdout), + String::from_utf8_lossy(&run.stderr) + ); + String::from_utf8_lossy(&run.stdout).trim().to_string() +} + +/// `||=` and `??=` must SHORT-CIRCUIT on a private field that already holds a +/// truthy / non-nullish value — no store, and the existing value is the result. +/// `&&=` is the mirror: it must store there, and skip on a falsy value. +#[test] +fn logical_assignment_reads_the_private_field_it_writes() { + let stdout = run_source( + r#"class Box { + #or: any; + #and: any; + #qq: any; + set(a: any, b: any, c: any): void { this.#or = a; this.#and = b; this.#qq = c; } + ops(): string { + const r1 = this.#or ||= "OR"; + const r2 = this.#and &&= "AND"; + const r3 = this.#qq ??= "QQ"; + return JSON.stringify([r1, r2, r3]); + } + peek(): string { return JSON.stringify([this.#or, this.#and, this.#qq]); } +} +function probe(a: any, b: any, c: any): void { + const box = new Box(); + box.set(a, b, c); + console.log(box.ops() + " " + box.peek()); +} +probe("T", "T", "T"); +probe(0, 0, 0); +probe(undefined, undefined, undefined); +"#, + ); + assert_eq!( + stdout.lines().collect::>(), + vec![ + // truthy / truthy / non-nullish: only `&&=` stores. + r#"["T","AND","T"] ["T","AND","T"]"#, + // falsy 0: `||=` stores, `&&=` short-circuits to 0, `??=` keeps 0 + // (0 is not nullish). + r#"["OR",0,0] ["OR",0,0]"#, + // undefined: `||=` and `??=` store, `&&=` short-circuits. + r#"["OR",null,"QQ"] ["OR",null,"QQ"]"#, + ] + ); +} + +/// The arithmetic and string compound operators read the same slot. `+=` on a +/// number gave NaN and on a string gave `"undefined…"` — the clearest evidence +/// that the read, not the short-circuit, was what broke. +#[test] +fn arithmetic_compound_assignment_reads_the_private_field() { + let stdout = run_source( + r#"class Counter { + #n = 10; + #s = "a"; + run(): string { + this.#n += 1; + this.#n *= 2; + this.#n -= 2; + this.#s += "b"; + return this.#n + " " + this.#s; + } +} +console.log(new Counter().run()); +"#, + ); + assert_eq!(stdout, "20 ab"); +} + +/// A STATIC private field goes through the same lowering. +#[test] +fn static_private_field_compound_assignment_reads_the_field() { + let stdout = run_source( + r#"class Acc { + static #total = 5; + static bump(): number { Acc.#total += 3; return Acc.#total; } +} +console.log(Acc.bump() + " " + Acc.bump()); +"#, + ); + assert_eq!(stdout, "8 11"); +} + +/// The exact hono shape: a private field memoized behind a getter with `||=`. +/// Reading the getter twice must yield the SAME object, and must not overwrite +/// a value the setter had already installed. +#[test] +fn private_field_memoized_by_a_getter_is_computed_once() { + let stdout = run_source( + r#"let built = 0; +class Ctx { + #res: any; + get res(): any { + return this.#res ||= { tag: "fresh-" + (++built) }; + } + set res(v: any) { this.#res = v; } +} +const c = new Ctx(); +c.res = { tag: "finalized" }; +const first = c.res; +const second = c.res; +console.log(first.tag + " " + second.tag + " " + (first === second) + " built=" + built); +const d = new Ctx(); +console.log(d.res.tag + " " + d.res.tag + " built=" + built); +"#, + ); + assert_eq!( + stdout.lines().collect::>(), + vec![ + "finalized finalized true built=0", + "fresh-1 fresh-1 built=1", + ] + ); +} + +/// A private ACCESSOR pair must run its getter for the read half and its setter +/// for the write half — the guard the fix installs is what carries the runtime +/// access hint that resolves an accessor by its mangled name. +#[test] +fn private_accessor_compound_assignment_runs_getter_and_setter() { + let stdout = run_source( + r#"class G { + #raw = 7; + #gets = 0; + #sets = 0; + get #val(): any { this.#gets += 1; return this.#raw; } + set #val(v: any) { this.#sets += 1; this.#raw = v; } + run(): string { + this.#val += 1; + this.#val ||= 99; + return this.#raw + " gets=" + this.#gets + " sets=" + this.#sets; + } +} +console.log(new G().run()); +"#, + ); + // `+= 1` is one get + one set; `||= 99` short-circuits on the truthy 8, so + // it is one more get and NO set. + assert_eq!(stdout, "8 gets=2 sets=1"); +} + +/// The read half is now brand-guarded like every other private access, so a +/// compound assignment against a receiver whose class did not declare the +/// member throws `TypeError` instead of silently reading `undefined` and +/// writing an ordinary `"#x"` string property onto the stranger. +#[test] +fn compound_assignment_on_a_foreign_receiver_throws() { + let dir = tempfile::tempdir().expect("tempdir"); + let root = dir.path(); + let entry = root.join("main.ts"); + std::fs::write( + &entry, + r#"class Holder { + #n = 1; + static bump(target: any): void { target.#n += 1; } +} +try { + Holder.bump({}); + console.log("no throw"); +} catch (e: any) { + console.log("threw: " + e.message); +} +"#, + ) + .expect("write source"); + let output = root.join("main_bin"); + let compile = Command::new(perry_bin()) + .current_dir(root) + .arg("compile") + .arg(&entry) + .arg("-o") + .arg(&output) + .env("PERRY_NO_CACHE", "1") + .output() + .expect("run perry compile"); + assert!( + compile.status.success(), + "perry compile failed\nstderr:\n{}", + String::from_utf8_lossy(&compile.stderr) + ); + let run = Command::new(&output).output().expect("run binary"); + let stdout = String::from_utf8_lossy(&run.stdout); + let stderr = String::from_utf8_lossy(&run.stderr); + assert!( + stdout.contains("threw:") || stderr.contains("private member"), + "expected a TypeError for the foreign receiver\nstatus: {:?}\n\ + stdout:\n{stdout}\nstderr:\n{stderr}", + run.status.code() + ); +} + +/// A PUBLIC field never had the bug — it is the control that shows the fix is +/// scoped to the private-name arm. +#[test] +fn public_field_compound_assignment_is_unchanged() { + let stdout = run_source( + r#"class P { + v: any = "KEPT"; + n = 10; + run(): string { this.v ||= "FRESH"; this.n += 5; return this.v + " " + this.n; } +} +console.log(new P().run()); +"#, + ); + assert_eq!(stdout, "KEPT 15"); +} diff --git a/crates/perry/tests/issue_8968_response_headers.rs b/crates/perry/tests/issue_8968_response_headers.rs new file mode 100644 index 0000000000..a88778fd5d --- /dev/null +++ b/crates/perry/tests/issue_8968_response_headers.rs @@ -0,0 +1,143 @@ +//! Regression for the two `Response` header defects found alongside #8968. +//! Both are silent wrong answers in the Web Fetch surface, not codegen bugs, +//! and neither is caused by the private-member miss the sibling test covers. +//! +//! 1. `js_response_new` took its `headers` parameter as "an f64 handle from +//! `js_headers_new`". Codegen only produces one when it can SEE the header +//! object — an inline `{ … }` literal or a local it tracked as an options +//! object. For anything else (a `??` expression, a spread-built object, a +//! call result, or the `headers` field read off a runtime options object) it +//! passed the plain NaN-boxed JS value through, the registry lookup missed, +//! and the response was built with NO headers. +//! +//! 2. Extracting a body from a STRING contributes +//! `Content-Type: text/plain;charset=UTF-8` per the Fetch standard, and +//! `new Response("hello")` did not set it. +//! +//! Together they cost hono the content type of every `c.text()` / `c.json()` / +//! `c.html()`. + +use std::path::PathBuf; +use std::process::Command; + +fn perry_bin() -> PathBuf { + PathBuf::from(env!("CARGO_BIN_EXE_perry")) +} + +fn run_source(source: &str) -> String { + let dir = tempfile::tempdir().expect("tempdir"); + let root = dir.path(); + let entry = root.join("main.ts"); + std::fs::write(&entry, source).expect("write source"); + let output = root.join("main_bin"); + let compile = Command::new(perry_bin()) + .current_dir(root) + .arg("compile") + .arg(&entry) + .arg("-o") + .arg(&output) + .env("PERRY_NO_CACHE", "1") + .output() + .expect("run perry compile"); + assert!( + compile.status.success(), + "perry compile failed\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&compile.stdout), + String::from_utf8_lossy(&compile.stderr) + ); + let run = Command::new(&output).output().expect("run binary"); + assert!( + run.status.success(), + "binary failed (status {:?})\nstdout:\n{}\nstderr:\n{}", + run.status.code(), + String::from_utf8_lossy(&run.stdout), + String::from_utf8_lossy(&run.stderr) + ); + String::from_utf8_lossy(&run.stdout).trim().to_string() +} + +/// Every spelling of a `headers` init that codegen cannot fold to a literal. +/// The inline-literal cases are the controls: they already worked, and must +/// keep working. Expected values are node 26 verbatim. +#[test] +fn response_headers_survive_a_non_literal_init() { + let stdout = run_source( + r#"function ct(r: Response): any { return r.headers.get("content-type"); } +function show(label: string, r: Response): void { + console.log(label + " " + r.status + " " + JSON.stringify(ct(r))); +} +const literalLocal = { "content-type": "application/json" }; +const spread: any = { "Content-Type": "application/json", ...(undefined as any) }; +function fromCall(): any { return { "content-type": "application/json" }; } +show("inline", new Response("x", { headers: { "content-type": "application/json" } })); +function fromPairs(): any { return [["content-type", "application/x-pairs"]]; } +function fromCustomCall(): any { return { "content-type": "application/problem+json" }; } +show("local", new Response("x", { headers: literalLocal })); +show("spread", new Response("x", { headers: spread })); +show("coalesce", new Response("x", { headers: (undefined as any) ?? literalLocal })); +show("call", new Response("x", { headers: fromCall() })); +show("headers-obj", new Response("x", { headers: new Headers({ "content-type": "application/json" }) })); +show("pairs", new Response("x", { headers: fromPairs() })); +show("static-json-call", Response.json({ ok: true }, { headers: fromCustomCall() })); +const runtimeInit: any = { status: 201, headers: fromCall() }; +show("runtime-init", new Response("x", runtimeInit)); +"#, + ); + assert_eq!( + stdout.lines().collect::>(), + vec![ + r#"inline 200 "application/json""#, + r#"local 200 "application/json""#, + // `setDefaultContentType` in hono builds exactly this shape, and + // with a capital `Content-Type` — the store lower-cases it. + r#"spread 200 "application/json""#, + r#"coalesce 200 "application/json""#, + r#"call 200 "application/json""#, + r#"headers-obj 200 "application/json""#, + r#"pairs 200 "application/x-pairs""#, + r#"static-json-call 200 "application/problem+json""#, + r#"runtime-init 201 "application/json""#, + ] + ); +} + +/// Fetch §"extract a body": a string body contributes +/// `text/plain;charset=UTF-8` (that exact spelling, no space), and an explicit +/// header always wins over it. Body types that contribute nothing must keep +/// contributing nothing. +#[test] +fn string_body_contributes_the_default_content_type() { + let stdout = run_source( + r#"function show(label: string, r: Response): void { + console.log(label + " " + JSON.stringify(r.headers.get("content-type"))); +} +const nul: any = null; +show("string", new Response("hello")); +show("empty-init", new Response("hello", {})); +show("explicit-wins", new Response("hello", { headers: { "content-type": "application/json" } })); +show("bytes", new Response(new Uint8Array([1, 2, 3]) as any)); +show("no-body", new Response()); +function afterBodylessResponse(): any { new Response(); return {}; } +function afterBodiedResponse(): any { new Response("hello"); return {}; } +function afterRequest(): any { new Request("https://example.com", { method: "POST", body: "outer" }); return {}; } +show("nested-bodyless", new Response("outer", afterBodylessResponse())); +show("nested-bodied", new Response("outer", afterBodiedResponse())); +show("nested-request", new Response("outer", afterRequest())); +show("null-body", new Response(nul)); +"#, + ); + assert_eq!( + stdout.lines().collect::>(), + vec![ + r#"string "text/plain;charset=UTF-8""#, + r#"empty-init "text/plain;charset=UTF-8""#, + r#"explicit-wins "application/json""#, + "bytes null", + "no-body null", + r#"nested-bodyless "text/plain;charset=UTF-8""#, + r#"nested-bodied "text/plain;charset=UTF-8""#, + r#"nested-request "text/plain;charset=UTF-8""#, + "null-body null", + ] + ); +} From 328d14814022765f754cce799b2da02003433f7b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Mon, 31 Aug 2026 14:40:12 +0000 Subject: [PATCH 4/4] fix(crypto): honor scrypt cost parameters (#9289) --- changelog.d/9289-scrypt-params.md | 6 + .../src/expr/calls/crypto_kdf.rs | 21 +- .../runtime_decls/stdlib_ffi/data_stores.rs | 5 +- .../src/runtime_decls/strings_part2.rs | 4 +- crates/perry-stdlib/src/crypto/kdf.rs | 326 +++++++++++++++--- crates/perry-stdlib/src/crypto/random.rs | 7 +- .../node-suite/crypto/scrypt/options.ts | 109 ++++++ 7 files changed, 405 insertions(+), 73 deletions(-) create mode 100644 changelog.d/9289-scrypt-params.md create mode 100644 test-parity/node-suite/crypto/scrypt/options.ts diff --git a/changelog.d/9289-scrypt-params.md b/changelog.d/9289-scrypt-params.md new file mode 100644 index 0000000000..95ce61a13d --- /dev/null +++ b/changelog.d/9289-scrypt-params.md @@ -0,0 +1,6 @@ +Fix a silent security downgrade in `node:crypto` scrypt: the callback form now +forwards `N`/`cost`, `r`/`blockSize`, `p`/`parallelization`, and `maxmem` +instead of always deriving with Node's defaults. Both callback and synchronous +forms now reject invalid combinations and insufficient `maxmem` with a +Node-compatible `RangeError`, rather than silently substituting the weaker +default work factor. diff --git a/crates/perry-codegen/src/expr/calls/crypto_kdf.rs b/crates/perry-codegen/src/expr/calls/crypto_kdf.rs index 6b5a7c568b..8fbc2e4f18 100644 --- a/crates/perry-codegen/src/expr/calls/crypto_kdf.rs +++ b/crates/perry-codegen/src/expr/calls/crypto_kdf.rs @@ -127,11 +127,13 @@ pub(crate) fn arm_crypto_scrypt( let pwd_box = lower_expr(ctx, &args[0])?; let salt_box = lower_expr(ctx, &args[1])?; let len_box = lower_expr(ctx, &args[2])?; - let cb_expr = if args.len() >= 5 { - let _ = lower_expr(ctx, &args[3])?; - &args[4] + let (opts_box, cb_expr) = if args.len() >= 5 { + (lower_expr(ctx, &args[3])?, &args[4]) } else { - &args[3] + ( + double_literal(f64::from_bits(crate::nanbox::TAG_UNDEFINED)), + &args[3], + ) }; let cb_box = lower_expr(ctx, cb_expr)?; let blk = ctx.block(); @@ -144,6 +146,7 @@ pub(crate) fn arm_crypto_scrypt( (I64, &pwd_handle), (I64, &salt_handle), (DOUBLE, &len_box), + (DOUBLE, &opts_box), (DOUBLE, &cb_box), ], )) @@ -243,19 +246,15 @@ pub(crate) fn arm_crypto_scrypt_sync( let salt_box = lower_expr(ctx, &args[1])?; let keylen_box = lower_expr(ctx, &args[2])?; let opts_box = if args.len() >= 4 { - Some(lower_expr(ctx, &args[3])?) + lower_expr(ctx, &args[3])? } else { - None + double_literal(f64::from_bits(crate::nanbox::TAG_UNDEFINED)) }; // #2013/#3146: node validates keylen as an integer in [0, 2^31-1]. emit_validate_integer_arg(ctx, &keylen_box, "keylen", 0.0, i32::MAX as f64); let blk = ctx.block(); let pwd_handle = unbox_to_i64(blk, &pwd_box); let salt_handle = unbox_to_i64(blk, &salt_box); - let opts_handle = match &opts_box { - Some(b) => unbox_to_i64(blk, b), - None => "0".to_string(), - }; let buf_handle = blk.call( I64, "js_crypto_scrypt_bytes", @@ -263,7 +262,7 @@ pub(crate) fn arm_crypto_scrypt_sync( (I64, &pwd_handle), (I64, &salt_handle), (DOUBLE, &keylen_box), - (I64, &opts_handle), + (DOUBLE, &opts_box), ], ); Ok(nanbox_pointer_inline(blk, &buf_handle)) diff --git a/crates/perry-codegen/src/runtime_decls/stdlib_ffi/data_stores.rs b/crates/perry-codegen/src/runtime_decls/stdlib_ffi/data_stores.rs index cfdaa4c587..d9da94b533 100644 --- a/crates/perry-codegen/src/runtime_decls/stdlib_ffi/data_stores.rs +++ b/crates/perry-codegen/src/runtime_decls/stdlib_ffi/data_stores.rs @@ -274,8 +274,9 @@ pub(crate) fn declare_data_stores(module: &mut LlModule) { module.declare_function("js_crypto_random_nonce", I64, &[]); module.declare_function("js_crypto_scrypt", I64, &[I64, I64, DOUBLE]); // crypto.scryptSync(password, salt, keylen, options?) -> Buffer. The 4th - // arg is the NaN-unboxed options-object pointer (0 = none). - module.declare_function("js_crypto_scrypt_bytes", I64, &[I64, I64, DOUBLE, I64]); + // arg is the full NaN-boxed options value so validation can distinguish + // objects, primitives, and undefined. + module.declare_function("js_crypto_scrypt_bytes", I64, &[I64, I64, DOUBLE, DOUBLE]); // crypto.generateKeyPairSync(type, options) -> { publicKey, privateKey }. module.declare_function("js_crypto_generate_key_pair_sync", DOUBLE, &[I64, I64]); module.declare_function( diff --git a/crates/perry-codegen/src/runtime_decls/strings_part2.rs b/crates/perry-codegen/src/runtime_decls/strings_part2.rs index df8307afa2..f73d9d4141 100644 --- a/crates/perry-codegen/src/runtime_decls/strings_part2.rs +++ b/crates/perry-codegen/src/runtime_decls/strings_part2.rs @@ -491,11 +491,11 @@ pub(crate) fn declare_phase_b_strings_part2(module: &mut LlModule) { DOUBLE, &[I64, I64, I64, I64, DOUBLE, DOUBLE], ); - module.declare_function("js_crypto_scrypt_bytes", I64, &[I64, I64, DOUBLE, I64]); + module.declare_function("js_crypto_scrypt_bytes", I64, &[I64, I64, DOUBLE, DOUBLE]); module.declare_function( "js_crypto_scrypt_async", DOUBLE, - &[I64, I64, DOUBLE, DOUBLE], + &[I64, I64, DOUBLE, DOUBLE, DOUBLE], ); module.declare_function("js_crypto_sign_rsa_sha256", I64, &[I64, I64, DOUBLE]); module.declare_function("js_crypto_sign_async", DOUBLE, &[I64, I64, DOUBLE, DOUBLE]); diff --git a/crates/perry-stdlib/src/crypto/kdf.rs b/crates/perry-stdlib/src/crypto/kdf.rs index 9194ff7788..b52b2a08d8 100644 --- a/crates/perry-stdlib/src/crypto/kdf.rs +++ b/crates/perry-stdlib/src/crypto/kdf.rs @@ -149,11 +149,10 @@ pub unsafe extern "C" fn js_crypto_scrypt_async( password_ptr: i64, salt_ptr: i64, keylen: f64, + options_bits: f64, callback_bits: f64, ) -> f64 { - // Routes to the 4-arg scryptSync (defined below) with no options - // object — same default cost parameters as Node. - let buf = js_crypto_scrypt_bytes(password_ptr, salt_ptr, keylen, 0); + let buf = js_crypto_scrypt_bytes(password_ptr, salt_ptr, keylen, options_bits); let value = if buf.is_null() { f64::from_bits(JSValue::undefined().bits()) } else { @@ -883,72 +882,209 @@ pub unsafe extern "C" fn js_crypto_scrypt_custom( js_string_from_bytes(hex_str.as_ptr(), hex_str.len() as u32) } +const SCRYPT_DEFAULT_N: u64 = 16_384; +const SCRYPT_DEFAULT_R: u32 = 8; +const SCRYPT_DEFAULT_P: u32 = 1; +const SCRYPT_DEFAULT_MAXMEM: u64 = 32 << 20; +const MAX_SAFE_INTEGER: u64 = 9_007_199_254_740_991; + +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +enum ScryptParamError { + Invalid, + MemoryLimit, +} + +fn scrypt_numeric_value(value: JSValue, name: &str, max: u64) -> u64 { + let bits = f64::from_bits(value.bits()); + if !perry_runtime::fs::validate::is_numeric(value) { + let message = format!( + "The \"{name}\" argument must be of type number. Received {}", + perry_runtime::fs::validate::describe_received(bits) + ); + perry_runtime::fs::validate::throw_type_error_with_code(&message, "ERR_INVALID_ARG_TYPE"); + } + let number = if value.is_int32() { + value.as_int32() as f64 + } else { + value.as_number() + }; + if !number.is_finite() || number.fract() != 0.0 { + let message = format!( + "The value of \"{name}\" is out of range. It must be an integer. Received {}", + perry_runtime::fs::validate::format_received_number(number) + ); + perry_runtime::fs::validate::throw_range_error_with_code(&message); + } + if number < 0.0 || number > max as f64 { + let message = format!( + "The value of \"{name}\" is out of range. It must be >= 0 && <= {max}. Received {}", + perry_runtime::fs::validate::format_received_number(number) + ); + perry_runtime::fs::validate::throw_range_error_with_code(&message); + } + number as u64 +} + +unsafe fn scrypt_options_object(options_bits: f64) -> Option<*const ObjectHeader> { + let value = JSValue::from_bits(options_bits.to_bits()); + if !value.is_pointer() { + return None; + } + let ptr = value.as_pointer::(); + if ptr.is_null() || (ptr as usize) < perry_runtime::gc::GC_HEADER_SIZE + 0x1000 { + return None; + } + let header = + &*(ptr.sub(perry_runtime::gc::GC_HEADER_SIZE) as *const perry_runtime::gc::GcHeader); + if header.obj_type != perry_runtime::gc::GC_TYPE_OBJECT { + return None; + } + Some(ptr as *const ObjectHeader) +} + +unsafe fn read_scrypt_option(obj: *const ObjectHeader, name: &str) -> Option { + let key = js_string_from_bytes(name.as_ptr(), name.len() as u32); + let value = js_object_get_field_by_name(obj, key); + (!value.is_undefined()).then_some(value) +} + +fn throw_incompatible_scrypt_options(primary: &str, alias: &str) -> ! { + let message = + format!("Option \"{primary}\" cannot be used in combination with option \"{alias}\""); + perry_runtime::fs::validate::throw_type_error_with_code( + &message, + "ERR_INCOMPATIBLE_OPTION_PAIR", + ) +} + +unsafe fn read_scrypt_options(options_bits: f64) -> (u64, u32, u32, u64) { + let Some(obj) = scrypt_options_object(options_bits) else { + return ( + SCRYPT_DEFAULT_N, + SCRYPT_DEFAULT_R, + SCRYPT_DEFAULT_P, + SCRYPT_DEFAULT_MAXMEM, + ); + }; + + let n = read_scrypt_option(obj, "N"); + let cost = read_scrypt_option(obj, "cost"); + if n.is_some() && cost.is_some() { + throw_incompatible_scrypt_options("N", "cost"); + } + let r = read_scrypt_option(obj, "r"); + let block_size = read_scrypt_option(obj, "blockSize"); + if r.is_some() && block_size.is_some() { + throw_incompatible_scrypt_options("r", "blockSize"); + } + let p = read_scrypt_option(obj, "p"); + let parallelization = read_scrypt_option(obj, "parallelization"); + if p.is_some() && parallelization.is_some() { + throw_incompatible_scrypt_options("p", "parallelization"); + } + let maxmem = read_scrypt_option(obj, "maxmem"); + + let n_name = if n.is_some() { "N" } else { "cost" }; + let r_name = if r.is_some() { "r" } else { "blockSize" }; + let p_name = if p.is_some() { "p" } else { "parallelization" }; + let mut n = n + .or(cost) + .map(|value| scrypt_numeric_value(value, n_name, u32::MAX as u64)) + .unwrap_or(SCRYPT_DEFAULT_N); + let mut r = r + .or(block_size) + .map(|value| scrypt_numeric_value(value, r_name, u32::MAX as u64) as u32) + .unwrap_or(SCRYPT_DEFAULT_R); + let mut p = p + .or(parallelization) + .map(|value| scrypt_numeric_value(value, p_name, u32::MAX as u64) as u32) + .unwrap_or(SCRYPT_DEFAULT_P); + let mut maxmem = maxmem + .map(|value| scrypt_numeric_value(value, "maxmem", MAX_SAFE_INTEGER)) + .unwrap_or(SCRYPT_DEFAULT_MAXMEM); + + // Node treats explicit zeroes as requests for the corresponding default. + if n == 0 { + n = SCRYPT_DEFAULT_N; + } + if r == 0 { + r = SCRYPT_DEFAULT_R; + } + if p == 0 { + p = SCRYPT_DEFAULT_P; + } + if maxmem == 0 { + maxmem = SCRYPT_DEFAULT_MAXMEM; + } + (n, r, p, maxmem) +} + +fn checked_scrypt_params( + n: u64, + r: u32, + p: u32, + maxmem: u64, +) -> Result { + if n <= 1 || !n.is_power_of_two() { + return Err(ScryptParamError::Invalid); + } + let log_n = n.trailing_zeros() as u8; + // Params::len belongs to the crate's PHC-string facade, not its low-level + // `scrypt` function. Keep it valid independently of Node's requested + // output length, which may be any int32-sized byte count. + let params = scrypt::Params::new(log_n, r, p, 32).map_err(|_| ScryptParamError::Invalid)?; + + // OpenSSL's SCRYPT_MAX_MEM check includes B, V, and XY: + // 128 * r * (N + p + 2). This is stricter than the dominant 128*N*r + // term and matches Node at the exact acceptance boundary. + let workspace = 128u64 + .checked_mul(r as u64) + .and_then(|block| n.checked_add(p as u64)?.checked_add(2)?.checked_mul(block)) + .ok_or(ScryptParamError::Invalid)?; + if workspace > maxmem { + return Err(ScryptParamError::MemoryLimit); + } + Ok(params) +} + +fn throw_scrypt_param_error(error: ScryptParamError) -> ! { + let message = match error { + ScryptParamError::Invalid => "Invalid scrypt params", + ScryptParamError::MemoryLimit => { + "Invalid scrypt params: error:030000AC:digital envelope routines::memory limit exceeded" + } + }; + perry_runtime::fs::validate::throw_range_error_named( + message, + "ERR_CRYPTO_INVALID_SCRYPT_PARAMS", + ) +} + /// `crypto.scryptSync(password, salt, keylen[, options])` → Buffer. /// -/// Unlike `js_crypto_scrypt` (which returns a hex string), this returns a -/// Buffer to match Node's `scryptSync`, and reads password/salt via -/// `bytes_from_ptr` so Buffer inputs hash correctly. Optional cost -/// parameters are read from `options_ptr` (a NaN-unboxed object pointer, or -/// a null/sentinel for none): `N`/`cost`, `r`/`blockSize`, `p`/ -/// `parallelization`. Defaults match Node: N=16384, r=8, p=1. +/// The same helper backs the callback form. It receives the full NaN-boxed +/// options value, validates Node's primary/alias names, and never substitutes +/// defaults for an invalid or unsupported parameter combination. #[no_mangle] pub unsafe extern "C" fn js_crypto_scrypt_bytes( password_ptr: i64, salt_ptr: i64, key_length: f64, - options_ptr: i64, + options_bits: f64, ) -> *mut perry_runtime::buffer::BufferHeader { - use perry_runtime::{js_object_get_field_by_name, ObjectHeader}; let password = bytes_from_ptr(password_ptr); let salt = bytes_from_ptr(salt_ptr); - let klen = key_length as usize; - if klen == 0 || klen > 1024 { + let keylen_value = JSValue::from_bits(key_length.to_bits()); + let klen = scrypt_numeric_value(keylen_value, "keylen", i32::MAX as u64) as usize; + let (n, r, p, maxmem) = read_scrypt_options(options_bits); + let params = checked_scrypt_params(n, r, p, maxmem) + .unwrap_or_else(|error| throw_scrypt_param_error(error)); + if klen == 0 { return alloc_buffer_from_slice(&[]); } - // Node defaults: N=16384 (cost), r=8 (blockSize), p=1 (parallelization). - let (mut n, mut r, mut p) = (16384u64, 8u32, 1u32); - if (options_ptr as usize) >= 0x1000 { - let obj = options_ptr as *const ObjectHeader; - // Read a numeric option by primary or alias name; None if absent. - let read = |primary: &str, alias: &str| -> Option { - let pk = js_string_from_bytes(primary.as_ptr(), primary.len() as u32); - let v = js_object_get_field_by_name(obj, pk); - if !v.is_undefined() { - return Some(v.to_number()); - } - let ak = js_string_from_bytes(alias.as_ptr(), alias.len() as u32); - let v = js_object_get_field_by_name(obj, ak); - if v.is_undefined() { - None - } else { - Some(v.to_number()) - } - }; - if let Some(x) = read("N", "cost") { - if x >= 1.0 { - n = x as u64; - } - } - if let Some(x) = read("r", "blockSize") { - if x >= 1.0 { - r = x as u32; - } - } - if let Some(x) = read("p", "parallelization") { - if x >= 1.0 { - p = x as u32; - } - } - } - // `scrypt::Params` takes log2(N); Node requires N to be a power of two, - // so trailing_zeros gives the exact exponent. A non-power-of-two or an - // otherwise-invalid combo falls back to the Node defaults. - let log_n = n.trailing_zeros() as u8; - let params = scrypt::Params::new(log_n, r, p, klen) - .unwrap_or_else(|_| scrypt::Params::new(14, 8, 1, klen).unwrap()); let mut out = vec![0u8; klen]; if scrypt::scrypt(&password, &salt, ¶ms, &mut out).is_err() { - return alloc_buffer_from_slice(&[]); + throw_scrypt_param_error(ScryptParamError::Invalid); } alloc_buffer_from_slice(&out) } @@ -1100,3 +1236,83 @@ pub(super) unsafe fn build_key_pair_object(pub_pem: &str, priv_pem: &str) -> f64 js_object_set_keys(obj, keys); nanbox_pointer_f64(obj as usize) } + +#[cfg(test)] +mod scrypt_tests { + use super::*; + + const TEST_MAXMEM: u64 = 512 * 1024 * 1024; + + fn digest(n: u64, r: u32, p: u32, keylen: usize) -> String { + let password = b"correct horse battery staple"; + let salt = hex::decode("0123456789abcdef0123456789abcdef").unwrap(); + let params = checked_scrypt_params(n, r, p, TEST_MAXMEM).unwrap(); + let mut output = vec![0; keylen]; + scrypt::scrypt(password, &salt, ¶ms, &mut output).unwrap(); + hex::encode(output) + } + + #[test] + fn scrypt_cost_parameters_produce_exact_node_digests() { + let vectors = [ + ( + 1 << 12, + "05f47c22e65fc21d3e11a92222323c577271be35cea9b34b4a6970e2fa3ebf48", + ), + ( + 1 << 13, + "a2d6acfcc30e33aa067080845b9f4790427da3d8f992e651765c095abb7cd276", + ), + ( + 1 << 14, + "33e39503baad99447708713ceec3f39bb876329254d4f5cd93da92a65d983f01", + ), + ( + 1 << 15, + "cb18e56f62485654eba440e7cc2fcaff9f92102d944dab86b13642272dfeb1f4", + ), + ( + 1 << 16, + "9d12077809f9271ef0dd063bff62817d49d53c7f1acb8e42c67516c5b0287cf1", + ), + ( + 1 << 17, + "e5581239883361913bc8cd281ef2a9d7e5bf2171f4b6c8eb0292e99077483208", + ), + ]; + for (n, expected) in vectors { + assert_eq!(digest(n, 8, 1, 32), expected, "N={n}"); + } + assert_eq!( + digest(1 << 14, 4, 1, 32), + "cd4b3b2db0af02f65f71c999901e07abba66fd088d65268928a7e971a50651f0" + ); + assert_eq!( + digest(1 << 14, 8, 2, 32), + "77636ab3d38dd67285438f5694c64bfa9fe81144034a49e7599511136bcfb071" + ); + assert_eq!( + digest(1 << 12, 8, 1, 64), + "05f47c22e65fc21d3e11a92222323c577271be35cea9b34b4a6970e2fa3ebf48\ + 1399c1db6db41b7c49fcc677fbe03319ec8e9c37a73734dd9eec71bd1d9ed212" + .replace(' ', "") + ); + } + + #[test] + fn scrypt_rejects_invalid_parameters_and_enforces_exact_memory_boundary() { + assert_eq!( + checked_scrypt_params(3, 8, 1, TEST_MAXMEM).unwrap_err(), + ScryptParamError::Invalid + ); + assert_eq!( + checked_scrypt_params(1, 8, 1, TEST_MAXMEM).unwrap_err(), + ScryptParamError::Invalid + ); + assert_eq!( + checked_scrypt_params(4096, 8, 1, 4_197_375).unwrap_err(), + ScryptParamError::MemoryLimit + ); + assert!(checked_scrypt_params(4096, 8, 1, 4_197_376).is_ok()); + } +} diff --git a/crates/perry-stdlib/src/crypto/random.rs b/crates/perry-stdlib/src/crypto/random.rs index 8e9d0f94d5..3c846dc049 100644 --- a/crates/perry-stdlib/src/crypto/random.rs +++ b/crates/perry-stdlib/src/crypto/random.rs @@ -434,13 +434,14 @@ pub unsafe extern "C" fn js_crypto_native_dispatch( js_crypto_pbkdf2_async_alg(bytes_ptr(0), bytes_ptr(1), arg(2), arg(3), digest, callback) } "scrypt" => { + let options = if args_len >= 5 { arg(3) } else { undefined }; let callback = if args_len >= 5 { arg(4) } else { arg(3) }; - js_crypto_scrypt_async(bytes_ptr(0), bytes_ptr(1), arg(2), callback) + js_crypto_scrypt_async(bytes_ptr(0), bytes_ptr(1), arg(2), options, callback) } "scryptSync" => { - let options_ptr = if args_len >= 4 { bytes_ptr(3) } else { 0 }; + let options = if args_len >= 4 { arg(3) } else { undefined }; pointer_value( - js_crypto_scrypt_bytes(bytes_ptr(0), bytes_ptr(1), arg(2), options_ptr) as *mut u8, + js_crypto_scrypt_bytes(bytes_ptr(0), bytes_ptr(1), arg(2), options) as *mut u8, ) } // Node callback forms are randomInt(max, callback) and diff --git a/test-parity/node-suite/crypto/scrypt/options.ts b/test-parity/node-suite/crypto/scrypt/options.ts new file mode 100644 index 0000000000..dfb430db8c --- /dev/null +++ b/test-parity/node-suite/crypto/scrypt/options.ts @@ -0,0 +1,109 @@ +import * as crypto from "node:crypto"; +import { promisify } from "node:util"; + +const password = "correct horse battery staple"; +const salt = Buffer.from("0123456789abcdef0123456789abcdef", "hex"); +const maxmem = 512 * 1024 * 1024; +const vectors: Array<[number, string]> = [ + [1 << 12, "05f47c22e65fc21d3e11a92222323c577271be35cea9b34b4a6970e2fa3ebf48"], + [1 << 13, "a2d6acfcc30e33aa067080845b9f4790427da3d8f992e651765c095abb7cd276"], + [1 << 14, "33e39503baad99447708713ceec3f39bb876329254d4f5cd93da92a65d983f01"], + [1 << 15, "cb18e56f62485654eba440e7cc2fcaff9f92102d944dab86b13642272dfeb1f4"], + [1 << 16, "9d12077809f9271ef0dd063bff62817d49d53c7f1acb8e42c67516c5b0287cf1"], + [1 << 17, "e5581239883361913bc8cd281ef2a9d7e5bf2171f4b6c8eb0292e99077483208"], +]; + +function equal(actual: string, expected: string, label: string): void { + if (actual !== expected) { + throw new Error(label + ": expected " + expected + ", got " + actual); + } +} + +function requireScryptRangeError(error: any, fragment: string): void { + if (error?.name !== "RangeError") { + throw new Error("expected RangeError, got " + String(error)); + } + if (error?.code !== "ERR_CRYPTO_INVALID_SCRYPT_PARAMS") { + throw new Error("unexpected error code: " + String(error?.code)); + } + if (!String(error).includes(fragment)) { + throw new Error("unexpected error message: " + String(error)); + } +} + +for (const [N, expected] of vectors) { + equal( + crypto.scryptSync(password, salt, 32, { N, r: 8, p: 1, maxmem }).toString("hex"), + expected, + "scryptSync N=" + N, + ); +} +equal( + crypto.scryptSync(password, salt, 32, { N: 1 << 14, r: 4, p: 1, maxmem }).toString("hex"), + "cd4b3b2db0af02f65f71c999901e07abba66fd088d65268928a7e971a50651f0", + "scryptSync r=4", +); +equal( + crypto.scryptSync(password, salt, 32, { N: 1 << 14, r: 8, p: 2, maxmem }).toString("hex"), + "77636ab3d38dd67285438f5694c64bfa9fe81144034a49e7599511136bcfb071", + "scryptSync p=2", +); +equal( + crypto.scryptSync(password, salt, 64, { N: 1 << 12, r: 8, p: 1, maxmem }).toString("hex"), + "05f47c22e65fc21d3e11a92222323c577271be35cea9b34b4a6970e2fa3ebf48" + + "1399c1db6db41b7c49fcc677fbe03319ec8e9c37a73734dd9eec71bd1d9ed212", + "scryptSync keylen=64", +); +equal( + crypto.scryptSync(password, salt, 32, { + cost: 1 << 12, + blockSize: 8, + parallelization: 1, + maxmem, + }).toString("hex"), + vectors[0][1], + "scryptSync aliases", +); +console.log("scryptSync parameter vectors: ok"); + +const scrypt = promisify(crypto.scrypt); +for (const [N, expected] of vectors) { + const result: any = await scrypt(password, salt, 32, { N, r: 8, p: 1, maxmem }); + equal(result.toString("hex"), expected, "scrypt async N=" + N); +} +console.log("scrypt async parameter vectors: ok"); + +await new Promise((resolve, reject) => { + crypto.scrypt(password, salt, 32, { N: 1 << 14, r: 4, p: 1, maxmem }, (error, key) => { + if (error) { + reject(error); + return; + } + try { + equal( + key.toString("hex"), + "cd4b3b2db0af02f65f71c999901e07abba66fd088d65268928a7e971a50651f0", + "scrypt callback r=4", + ); + resolve(); + } catch (caught) { + reject(caught); + } + }); +}); +console.log("scrypt direct callback options: ok"); + +try { + crypto.scryptSync(password, salt, 32, { N: 3, r: 8, p: 1, maxmem }); + throw new Error("invalid N did not throw"); +} catch (error: any) { + requireScryptRangeError(error, "Invalid scrypt params"); +} + +try { + await scrypt(password, salt, 32, { N: 1 << 17, r: 8, p: 1, maxmem: 1024 }); + throw new Error("insufficient maxmem did not throw"); +} catch (error: any) { + requireScryptRangeError(error, "memory limit exceeded"); +} +console.log("scrypt invalid parameter errors: ok");