From 7dde4bb27586f9ab5a3830d49adb91e4fcbd55fa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Tue, 1 Sep 2026 18:15:41 +0200 Subject: [PATCH 1/3] fix(runtime): ignore SIGPIPE so a truncating consumer cannot kill the program (#9402) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `claude auto-mode defaults | head -2` exited 141 (128 + SIGPIPE) under Perry and 0 under Node, deterministically. Every pipeline that stops reading early hit it: `| head`, `| grep -q`, `| less` then `q`, a client that closed its socket. A Perry program has its own C `main`, emitted by codegen, so it never runs Rust's `std::rt` startup — and that startup is where an ordinary Rust binary gets SIGPIPE set to SIG_IGN. The compiled program therefore inherited the signal's default disposition and died mid-write, with no JS-visible event and nothing to catch. Node ignores the signal and lets the failing write(2) return EPIPE to the writer instead. `ignore_sigpipe_at_startup()` installs SIG_IGN once per process and only over SIG_DFL, so an embedder's own disposition and a later `process.on('SIGPIPE')` are both untouched. It is called from `js_gc_init`, the first runtime call of every `main` / `perry_module_init`, so every compiled program gets it before a byte can be written. Unix only: Windows has no SIGPIPE. Ignoring the signal alone would have traded exit 141 for exit 134 — `std`'s `println!` turns the resulting EPIPE into a panic and Perry builds with `panic = "abort"`. Node's console is specified never to throw, so the `console.*` family's print macros are shadowed with writers that drop the write error. The shadowing is confined to the `builtins` tree, alongside the existing harmonyos hilog override; diagnostics elsewhere keep `std`'s macros. `fs.writeSync(1, …)` to a closed pipe now throws EPIPE, matching Node — write errors still reach JavaScript rather than being swallowed. test-files/test_gap_9402_sigpipe_truncating_consumer.ts re-runs itself through bash, pipes 50000 lines into `head -2` and reports the WRITER's status. Byte-compared against node 26.5.1: node `writer-status=0`; a compiler built from unfixed origin/main reports `writer-status=141`; with this change, identical to node. --- changelog.d/9402-sigpipe-ignored.md | 45 ++++++++++++ crates/perry-runtime/src/builtins/mod.rs | 71 +++++++++++++++++-- crates/perry-runtime/src/gc/mod.rs | 6 ++ crates/perry-runtime/src/os.rs | 1 + crates/perry-runtime/src/os/signal.rs | 50 +++++++++++++ ...st_gap_9402_sigpipe_truncating_consumer.ts | 34 +++++++++ 6 files changed, 203 insertions(+), 4 deletions(-) create mode 100644 changelog.d/9402-sigpipe-ignored.md create mode 100644 test-files/test_gap_9402_sigpipe_truncating_consumer.ts diff --git a/changelog.d/9402-sigpipe-ignored.md b/changelog.d/9402-sigpipe-ignored.md new file mode 100644 index 0000000000..b108507c92 --- /dev/null +++ b/changelog.d/9402-sigpipe-ignored.md @@ -0,0 +1,45 @@ +### Fixed + +- **A truncating consumer no longer kills a compiled program.** + `claude auto-mode defaults | head -2` exited **141** (128 + SIGPIPE) under + Perry and 0 under node — deterministically, 3 runs out of 3. Every pipeline + that stops reading early hit it: `| head`, `| grep -q`, `| less` followed by + `q`, a client that closed its socket. + + The cause is structural rather than a mistake in any one function. A Perry + program has its **own C `main`**, emitted by codegen, so it never runs Rust's + `std::rt` startup — and that startup is where an ordinary Rust binary gets + `SIGPIPE` set to `SIG_IGN`. A compiled program therefore inherited the + signal's default disposition and died mid-write, with no JavaScript-visible + event and nothing to catch. Node (through libuv) ignores the signal and lets + the failing `write(2)` return `EPIPE` to the writer instead. + + - `crates/perry-runtime/src/os/signal.rs` — `ignore_sigpipe_at_startup()` + installs `SIG_IGN`, once per process, and only over `SIG_DFL`, so an + embedder's own disposition and a later `process.on('SIGPIPE', …)` are both + left alone. Unix only: Windows has no `SIGPIPE`. + - `crates/perry-runtime/src/gc/mod.rs` — called from `js_gc_init`, which is + the first runtime call of every `main` / `perry_module_init`, so every + compiled program gets it before a byte can be written. + + Ignoring the signal alone would have traded exit 141 for exit **134**: + `std`'s `println!` turns the resulting `EPIPE` into a panic, and Perry builds + with `panic = "abort"`. Node's console is specified never to throw + (`node -e 'for(;;) console.log(1)' | head -2` exits 0), so: + + - `crates/perry-runtime/src/builtins/mod.rs` — the `console.*` family's + `println!` / `print!` / `eprintln!` are shadowed with writers that drop the + write error, which is exactly that contract. The shadowing is confined to + the `builtins` tree, alongside the pre-existing harmonyos hilog override; + diagnostics elsewhere in the runtime keep `std`'s macros. + + Validation: `test-files/test_gap_9402_sigpipe_truncating_consumer.ts` + re-runs itself through `bash`, pipes 50 000 lines into `head -2`, and reports + the **writer's** status. Byte-compared against node 26.5.1: node + `writer-status=0`, Perry built from unfixed `origin/main` `writer-status=141`, + Perry with this change `writer-status=0`. + + Known remaining gap, not addressed here: `process.stdout.write` **swallows** + the `EPIPE` (`os_process_streams.rs` has always discarded the write result), + where node emits an `'error'` event on the stream and exits 1 if it is + unhandled. That is a stream-plumbing change, not a signal one. diff --git a/crates/perry-runtime/src/builtins/mod.rs b/crates/perry-runtime/src/builtins/mod.rs index a902e9dc8f..eeedb79b71 100644 --- a/crates/perry-runtime/src/builtins/mod.rs +++ b/crates/perry-runtime/src/builtins/mod.rs @@ -28,10 +28,73 @@ macro_rules! println { }; } -// Make the override visible to the topical submodules below via `use -// super::println;` (each submodule that prints needs the same shadow so -// stdout output on harmonyos routes through hilog the same way it did -// pre-split). +// #9402: with SIGPIPE ignored, a truncated consumer no longer KILLS the +// writer — the failing `write(2)` returns `EPIPE` instead. `std`'s `println!` +// turns that error into a panic ("failed printing to stdout"), and Perry +// builds with `panic = "abort"`, so `prog | head -2` would have traded exit +// 141 for exit 134. Node's console is specified never to throw: it writes +// through a stream whose errors it ignores, and `node -e 'for(;;) +// console.log(1)' | head -2` exits 0. Shadow the print macros for the +// user-facing `console.*` family with writers that drop the error, which is +// exactly that contract. Diagnostics elsewhere in the runtime keep `std`'s +// macros. +#[cfg(not(feature = "ohos-napi"))] +pub(crate) fn console_write_line(args: std::fmt::Arguments<'_>) { + use std::io::Write; + let stdout = std::io::stdout(); + let mut handle = stdout.lock(); + let _ = handle.write_fmt(args); + let _ = handle.write_all(b"\n"); +} + +#[cfg(not(feature = "ohos-napi"))] +pub(crate) fn console_write_fragment(args: std::fmt::Arguments<'_>) { + use std::io::Write; + let stdout = std::io::stdout(); + let mut handle = stdout.lock(); + let _ = handle.write_fmt(args); +} + +#[cfg(not(feature = "ohos-napi"))] +pub(crate) fn console_write_err_line(args: std::fmt::Arguments<'_>) { + use std::io::Write; + let stderr = std::io::stderr(); + let mut handle = stderr.lock(); + let _ = handle.write_fmt(args); + let _ = handle.write_all(b"\n"); +} + +#[cfg(not(feature = "ohos-napi"))] +macro_rules! println { + () => { + $crate::builtins::console_write_line(format_args!("")) + }; + ($($arg:tt)*) => { + $crate::builtins::console_write_line(format_args!($($arg)*)) + }; +} + +#[cfg(not(feature = "ohos-napi"))] +macro_rules! print { + ($($arg:tt)*) => { + $crate::builtins::console_write_fragment(format_args!($($arg)*)) + }; +} + +#[cfg(not(feature = "ohos-napi"))] +macro_rules! eprintln { + () => { + $crate::builtins::console_write_err_line(format_args!("")) + }; + ($($arg:tt)*) => { + $crate::builtins::console_write_err_line(format_args!($($arg)*)) + }; +} + +// `macro_rules!` textual scope already reaches the topical submodules declared +// below, so every `console.*` printer in this tree picks up the shadows above. +// The harmonyos build additionally re-exports `println` because its submodules +// name it explicitly (`use super::println;`). #[cfg(feature = "ohos-napi")] pub(crate) use println; diff --git a/crates/perry-runtime/src/gc/mod.rs b/crates/perry-runtime/src/gc/mod.rs index 8d7e471097..ea794f7e55 100644 --- a/crates/perry-runtime/src/gc/mod.rs +++ b/crates/perry-runtime/src/gc/mod.rs @@ -1165,6 +1165,12 @@ pub extern "C" fn js_gc_init() { // call lands. A host that loads several application images on several // threads gets one image per thread; a plain executable gets one. crate::object::class_image::enter_current_thread_image(); + // #9402: a compiled program has its own `main` and never runs Rust's + // `std::rt` startup, so SIGPIPE arrives with its DEFAULT disposition and + // any truncating consumer (`| head`, `| grep -q`, a closed socket) kills + // the writer mid-write. Node ignores the signal and surfaces `EPIPE` to + // the writer instead; do the same before a single byte can be written. + crate::os::ignore_sigpipe_at_startup(); // Prime the process wall-clock epoch NOW: it lazily initializes on first // read, and the first read is otherwise whatever GC event happens to // consult it — which would make the exit-time GC-share denominator start diff --git a/crates/perry-runtime/src/os.rs b/crates/perry-runtime/src/os.rs index ff8c6fd0c5..700babe28d 100644 --- a/crates/perry-runtime/src/os.rs +++ b/crates/perry-runtime/src/os.rs @@ -834,6 +834,7 @@ pub use chdir::js_process_chdir; // Signal normalization is shared with `util.convertProcessSignalToExitCode`. mod signal; pub use signal::{js_process_kill, js_util_convert_process_signal_to_exit_code}; +pub(crate) use signal::ignore_sigpipe_at_startup; #[path = "os_process_streams.rs"] mod process_streams; diff --git a/crates/perry-runtime/src/os/signal.rs b/crates/perry-runtime/src/os/signal.rs index b2f1becdca..925eb7894c 100644 --- a/crates/perry-runtime/src/os/signal.rs +++ b/crates/perry-runtime/src/os/signal.rs @@ -11,6 +11,56 @@ use std::sync::{LazyLock, Mutex}; static SIGNAL_ASYNC_IDS: LazyLock>> = LazyLock::new(|| Mutex::new(HashMap::new())); +/// Ignore `SIGPIPE` for the life of the process (#9402). +/// +/// A Perry program has its own C `main`, emitted by codegen — it never runs +/// Rust's `std::rt` startup, which is where an ordinary Rust binary gets +/// `SIGPIPE` set to `SIG_IGN`. A compiled program therefore inherited the +/// signal's DEFAULT disposition, so **any** truncating consumer killed it +/// mid-write: `| head`, `| grep -q`, `| less` followed by `q`, a client that +/// closed its socket. `claude auto-mode defaults | head -2` exited 141 under +/// Perry and 0 under Node. +/// +/// Node (through libuv) does exactly this: the signal is ignored and the +/// failing `write(2)` returns `EPIPE` to the writer, which is a value the +/// program can see and act on rather than a death it cannot. +/// +/// Idempotent and safe from any thread — `js_gc_init` calls it once per image +/// entry, and a second entry on another thread re-applies the same +/// disposition. A `SIGPIPE` disposition an embedder installed BEFORE loading +/// the runtime is left alone: only `SIG_DFL` (the state that kills us) is +/// replaced. `process.on('SIGPIPE', …)` still installs its own handler +/// afterwards through [`install_process_signal_handler`], exactly as it did. +/// +/// Not inherited by children: Rust's `Command` restores `SIGPIPE` to +/// `SIG_DFL` in the forked child before `exec`, matching Node. +#[cfg(unix)] +pub(crate) fn ignore_sigpipe_at_startup() { + use std::sync::Once; + static ONCE: Once = Once::new(); + ONCE.call_once(|| unsafe { + // SAFETY: a query (`act` = NULL) followed by a plain disposition + // install. `sigaction` is async-signal-safe and touches no Perry state. + let mut current: libc::sigaction = std::mem::zeroed(); + if libc::sigaction(libc::SIGPIPE, std::ptr::null(), &mut current) != 0 { + return; + } + if current.sa_sigaction != libc::SIG_DFL { + return; + } + let mut sa: libc::sigaction = std::mem::zeroed(); + sa.sa_sigaction = libc::SIG_IGN; + sa.sa_flags = 0; + libc::sigemptyset(&mut sa.sa_mask); + libc::sigaction(libc::SIGPIPE, &sa, std::ptr::null_mut()); + }); +} + +/// Windows has no `SIGPIPE`: a write to a closed pipe already fails with +/// `ERROR_BROKEN_PIPE` instead of raising a signal. +#[cfg(not(unix))] +pub(crate) fn ignore_sigpipe_at_startup() {} + fn signal_number_by_name(name: &str) -> Option { #[cfg(unix)] { diff --git a/test-files/test_gap_9402_sigpipe_truncating_consumer.ts b/test-files/test_gap_9402_sigpipe_truncating_consumer.ts new file mode 100644 index 0000000000..112e027dc1 --- /dev/null +++ b/test-files/test_gap_9402_sigpipe_truncating_consumer.ts @@ -0,0 +1,34 @@ +// #9402: a truncating consumer (`| head -2`, `| grep -q`, `| less` + quit, a +// closed socket) must not kill the process. Node ignores SIGPIPE and lets the +// failing write surface as EPIPE; Perry inherited SIGPIPE's DEFAULT +// disposition, because a compiled program has its own C `main` and never runs +// Rust's `std::rt` startup (which is what installs SIG_IGN for a normal Rust +// binary). Every truncated pipe therefore killed the writer with signal 13. +// +// The witness spawns THIS program again with a marker argument, pipes it into +// `head -2`, and reports the WRITER's exit status (not the pipeline's). +// Pre-fix Perry reports 141 (128 + SIGPIPE); Node reports 0. +import { spawnSync } from "node:child_process"; + +const MARKER = "--emit-many-lines"; + +if (process.argv.indexOf(MARKER) >= 0) { + // Well past a 64 KiB pipe buffer, so `head -2` is guaranteed to have closed + // the read end long before the last line is written. + for (let i = 0; i < 50000; i++) { + console.log("line " + i); + } + console.log("writer finished"); +} else { + // `$1` is the script path. Node needs it on the command line; a compiled + // Perry binary does not, but accepts it as an ignored positional argument — + // the marker is located with indexOf, not by position, so both runtimes read + // the same value. + const script = + '"$0" "$1" ' + MARKER + ' | head -2 >/dev/null; echo "writer-status=${PIPESTATUS[0]}"'; + const child = spawnSync("/bin/bash", ["-c", script, process.argv[0], process.argv[1]], { + encoding: "utf8", + }); + process.stdout.write(child.stdout ?? ""); + console.log("shell-status:", child.status); +} From b457509ca70b3487cb28859a14cb85cf778622b0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Tue, 1 Sep 2026 18:16:00 +0200 Subject: [PATCH 2/3] fix(runtime): a non-UTF-8 argv byte must not abort the process (#9401) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `claude -p $'\xff\xfe\x80abc\xc3\x28'` died with SIGABRT and a raw Rust backtrace — "panicked at library/std/src/env.rs:878:51: called `Result::unwrap()` on an `Err` value" — where Node prints the program's own output. `std::env::args()` panics on an argument that is not valid Unicode, and non-UTF-8 filenames are ordinary on Linux, so anything that passes a path through reached it. Node decodes argv leniently: every invalid byte becomes U+FFFD. Verified against node 26.5.1 — `$'\xff\xfe\x80abc\xc3\x28'` arrives as the eight code points fffd fffd fffd 61 62 63 fffd 28, byte-for-byte `String::from_utf8_lossy`. One `process_args_lossy()` over `std::env::args_os()` now backs every argv reader in the runtime, so a single bad byte cannot resurrect the abort in a path nobody thought to check. There were NINE, all reachable, and the panic was not confined to `process.argv`: - os.rs `js_process_argv` — `process.argv` - node_submodules/trace_events.rs — reads argv from `js_gc_init`, so the process died before a line of JavaScript ran, whatever the program did - process/permission.rs (x3) — the permission-model flag scan - process/report.rs (x2) — `process.report` - process/attributes.rs — `process.title` - cluster.rs (x2) — cluster exec-path defaulting - child_process/options.rs — self-launch detection in `spawn` - process.rs `process_argv0_string` — `process.argv0` / `execPath` Three more outside the runtime, same shape, same fix: perry-stdlib and perry-ext-commander (`program.parse()` with no explicit argv), and the compiler CLI's own arguments in perry/src/{main,update_policy}.rs, so `perry compile` on a non-UTF-8 path reports a diagnostic instead of a backtrace. `std::env::var()` needs no equivalent change: it returns Err for a non-Unicode value rather than panicking, and the runtime has no `env::var(..).unwrap()`. test-files/test_gap_9401_non_utf8_argv.ts re-runs itself through `sh` (which is byte-oriented, so it can build an argument the source file cannot contain) and prints the decoded length, code points and UTF-8 bytes. Byte-compared against node 26.5.1: a compiler built from unfixed origin/main reports `child-status: null / child-signal: SIGABRT`; with this change, identical to node. Not touched, same shape, reported rather than changed: perry-ui-gtk4 src/tray.rs, perry-ui-macos src/app.rs, perry-ui src/bin/styling-matrix.rs. --- changelog.d/9401-non-utf8-argv.md | 61 +++++++++++++++++++ crates/perry-ext-commander/src/lib.rs | 7 ++- .../src/child_process/options.rs | 2 +- crates/perry-runtime/src/cluster.rs | 4 +- .../src/node_submodules/trace_events.rs | 4 +- crates/perry-runtime/src/os.rs | 3 +- crates/perry-runtime/src/process.rs | 22 ++++++- .../perry-runtime/src/process/attributes.rs | 2 +- .../perry-runtime/src/process/permission.rs | 6 +- crates/perry-runtime/src/process/report.rs | 4 +- crates/perry-stdlib/src/commander.rs | 7 ++- crates/perry/src/main.rs | 15 ++++- crates/perry/src/update_policy.rs | 6 +- test-files/test_gap_9401_non_utf8_argv.ts | 42 +++++++++++++ 14 files changed, 168 insertions(+), 17 deletions(-) create mode 100644 changelog.d/9401-non-utf8-argv.md create mode 100644 test-files/test_gap_9401_non_utf8_argv.ts diff --git a/changelog.d/9401-non-utf8-argv.md b/changelog.d/9401-non-utf8-argv.md new file mode 100644 index 0000000000..ddf1f8f40d --- /dev/null +++ b/changelog.d/9401-non-utf8-argv.md @@ -0,0 +1,61 @@ +### Fixed + +- **A non-UTF-8 byte in `argv` no longer aborts the process.** + `claude -p $'\xff\xfe\x80abc\xc3\x28'` died with **SIGABRT** and a raw Rust + backtrace — + + ``` + panicked at library/std/src/env.rs:878:51: + called `Result::unwrap()` on an `Err` value: "\xFF\xFE\x80abc\xC3(" + ``` + + — where node prints the program's own output. `std::env::args()` panics on an + argument that is not valid Unicode, and non-UTF-8 filenames are ordinary on + Linux, so this was trivially reachable by anything that passes a path + through. + + Node decodes `argv` leniently: every invalid byte becomes U+FFFD. Verified + against node 26.5.1 — `$'\xff\xfe\x80abc\xc3\x28'` arrives as the eight code + points `fffd fffd fffd 61 62 63 fffd 28`, which is byte-for-byte + `String::from_utf8_lossy`. + + - `crates/perry-runtime/src/process.rs` — one `process_args_lossy()` over + `std::env::args_os()`, so a single bad byte cannot resurrect the abort in + a path nobody thought to check. + + Every `std::env::args()` reader in the runtime now goes through it. There + were **nine**, all reachable, and the panic was not confined to + `process.argv`: + + - `os.rs` `js_process_argv` — `process.argv`; + - `node_submodules/trace_events.rs` — reads `argv` from **`js_gc_init`**, so + the process died before a line of JavaScript ran, whatever the program did; + - `process/permission.rs` (×3) — the permission-model flag scan; + - `process/report.rs` (×2) — `process.report`; + - `process/attributes.rs` — `process.title`; + - `cluster.rs` (×2) — `cluster` exec-path defaulting; + - `child_process/options.rs` — self-launch detection in `spawn`; + - `process.rs` `process_argv0_string` — `process.argv0` / `execPath`. + + Three more outside the runtime, same shape, same fix: + + - `crates/perry-stdlib/src/commander.rs` and + `crates/perry-ext-commander/src/lib.rs` — `program.parse()` with no + explicit argv; + - `crates/perry/src/main.rs` and `crates/perry/src/update_policy.rs` — the + compiler CLI's own arguments, so `perry compile` on a non-UTF-8 path + reports a diagnostic instead of a backtrace. + + Not touched (UI crates, out of this change's scope): `perry-ui-gtk4` + `src/tray.rs`, `perry-ui-macos` `src/app.rs`, `perry-ui` `src/bin/styling-matrix.rs`. + + `std::env::var()` needs no equivalent change: it *returns* `Err` for a + non-Unicode value rather than panicking, and the runtime has no + `env::var(..).unwrap()`. + + Validation: `test-files/test_gap_9401_non_utf8_argv.ts` re-runs itself + through `sh` (which is byte-oriented, so it can build an argument the source + file cannot contain) and prints the decoded length, code points and UTF-8 + bytes. Byte-compared against node 26.5.1; Perry built from unfixed + `origin/main` reports `child-status: null / child-signal: SIGABRT`, and with + this change is identical to node. diff --git a/crates/perry-ext-commander/src/lib.rs b/crates/perry-ext-commander/src/lib.rs index 8dec1b59ec..ba6eede278 100644 --- a/crates/perry-ext-commander/src/lib.rs +++ b/crates/perry-ext-commander/src/lib.rs @@ -290,7 +290,12 @@ fn resolve_parse_args(argv: f64) -> Vec { return out.into_iter().skip(2).collect(); } } - std::env::args().skip(1).collect() + // #9401: `std::env::args()` panics on a non-UTF-8 argument; Node decodes + // argv leniently (every invalid byte becomes U+FFFD) and so must this. + std::env::args_os() + .skip(1) + .map(|arg| arg.to_string_lossy().into_owned()) + .collect() } /// Top-level parse entry. The second arg is the user's `parse(argv)` diff --git a/crates/perry-runtime/src/child_process/options.rs b/crates/perry-runtime/src/child_process/options.rs index 6b67ea4ab3..41fe9cf91a 100644 --- a/crates/perry-runtime/src/child_process/options.rs +++ b/crates/perry-runtime/src/child_process/options.rs @@ -470,7 +470,7 @@ pub(crate) fn cp_command_for_program(program: &str, opts_val: f64) -> Command { /// Whether a self-launch uses a Node CLI mode that evaluates source text. fn cp_should_use_node_interpreter(cmd: &str, args: &[String]) -> bool { - let is_self = std::env::args().next().as_deref() == Some(cmd) + let is_self = crate::process::process_args_lossy().next().as_deref() == Some(cmd) || std::env::current_exe().is_ok_and(|current| current == std::path::Path::new(cmd)); is_self && args diff --git a/crates/perry-runtime/src/cluster.rs b/crates/perry-runtime/src/cluster.rs index 6f9636f37d..aa4284e025 100644 --- a/crates/perry-runtime/src/cluster.rs +++ b/crates/perry-runtime/src/cluster.rs @@ -945,7 +945,7 @@ fn default_args_array_value() -> f64 { } fn default_exec_path() -> String { - std::env::args() + crate::process::process_args_lossy() .nth(1) .filter(|s| !s.is_empty()) .or_else(|| { @@ -953,7 +953,7 @@ fn default_exec_path() -> String { .ok() .map(|p| p.to_string_lossy().into_owned()) }) - .or_else(|| std::env::args().next()) + .or_else(|| crate::process::process_args_lossy().next()) .unwrap_or_default() } diff --git a/crates/perry-runtime/src/node_submodules/trace_events.rs b/crates/perry-runtime/src/node_submodules/trace_events.rs index e92d718bb6..9b7bfc0e26 100644 --- a/crates/perry-runtime/src/node_submodules/trace_events.rs +++ b/crates/perry-runtime/src/node_submodules/trace_events.rs @@ -542,7 +542,9 @@ pub(crate) fn init_trace_events_runtime() { if slot.borrow().initialized { return; } - let mut output = trace_options_from_args(std::env::args()); + // #9401: this runs from `js_gc_init`, so a raw `std::env::args()` + // aborted the process on a non-UTF-8 argument before any JS ran. + let mut output = trace_options_from_args(crate::process::process_args_lossy()); seed_legacy_categories(&mut output); output.initialized = true; TRACE_ENABLED_COUNTS.with(|counts| { diff --git a/crates/perry-runtime/src/os.rs b/crates/perry-runtime/src/os.rs index 700babe28d..5f3f91619f 100644 --- a/crates/perry-runtime/src/os.rs +++ b/crates/perry-runtime/src/os.rs @@ -583,7 +583,8 @@ pub extern "C" fn js_process_argv() -> *mut ArrayHeader { use crate::array::{js_array_alloc, js_array_push_f64}; use crate::value::js_nanbox_string; - let args: Vec = std::env::args().collect(); + // #9401: never `std::env::args()` — it panics on a non-UTF-8 argument. + let args: Vec = crate::process::process_args_lossy().collect(); // Match Node.js behavior: argv[0] = binary path (like node path), // argv[1] = source entry path (like script path), argv[2+] = user args. // Node.js: ["/usr/bin/node", "/path/to/script.js", ...user_args] diff --git a/crates/perry-runtime/src/process.rs b/crates/perry-runtime/src/process.rs index 995d40da8c..62a472b46f 100644 --- a/crates/perry-runtime/src/process.rs +++ b/crates/perry-runtime/src/process.rs @@ -396,8 +396,28 @@ pub(crate) fn module_set_value(items: &[&str]) -> f64 { crate::value::js_nanbox_pointer(set as i64) } +/// The process's arguments, decoded the way Node decodes them: every byte that +/// is not valid UTF-8 becomes U+FFFD (#9401). +/// +/// `std::env::args()` PANICS on an argument that is not valid Unicode +/// (`library/std/src/env.rs`: `called Result::unwrap() on an Err value`), and +/// with `panic = "abort"` that is a SIGABRT with a raw Rust backtrace where +/// Node prints the program's own output. Non-UTF-8 filenames are ordinary on +/// Linux, so `prog $'\xff\xfe\x80abc'` reached it trivially — and +/// `init_trace_events_runtime` reads argv from `js_gc_init`, so the process +/// died before a line of JavaScript ran. +/// +/// Node's decoding is byte-for-byte `String::from_utf8_lossy`: verified +/// against Node 26.5.1, `$'\xff\xfe\x80abc\xc3\x28'` arrives as the eight +/// code points `fffd fffd fffd 61 62 63 fffd 28`. Every argv reader in the +/// runtime goes through here so one bad byte cannot resurrect the abort in a +/// path nobody thought to check. +pub(crate) fn process_args_lossy() -> impl Iterator { + std::env::args_os().map(|arg| arg.to_string_lossy().into_owned()) +} + pub(crate) fn process_argv0_string() -> String { - std::env::args().next().unwrap_or_default() + process_args_lossy().next().unwrap_or_default() } pub(crate) fn node_arch_name() -> &'static str { diff --git a/crates/perry-runtime/src/process/attributes.rs b/crates/perry-runtime/src/process/attributes.rs index 9ba0f319c1..2f62761abb 100644 --- a/crates/perry-runtime/src/process/attributes.rs +++ b/crates/perry-runtime/src/process/attributes.rs @@ -109,7 +109,7 @@ pub extern "C" fn js_process_resource_usage() -> f64 { pub extern "C" fn js_process_title() -> f64 { use crate::value::JSValue; let stored: Option = PROCESS_TITLE.with(|c| c.borrow().clone()); - let s = stored.unwrap_or_else(|| std::env::args().next().unwrap_or_default()); + let s = stored.unwrap_or_else(super::process_argv0_string); let bytes = s.as_bytes(); let ptr = js_string_from_bytes(bytes.as_ptr(), bytes.len() as u32); f64::from_bits(JSValue::string_ptr(ptr).bits()) diff --git a/crates/perry-runtime/src/process/permission.rs b/crates/perry-runtime/src/process/permission.rs index be0e5174e7..e8b2fb5eb6 100644 --- a/crates/perry-runtime/src/process/permission.rs +++ b/crates/perry-runtime/src/process/permission.rs @@ -7,7 +7,7 @@ use crate::value::JSValue; pub(crate) fn process_permission_enabled() -> bool { let mut enabled = false; - for arg in std::env::args().skip(1) { + for arg in super::process_args_lossy().skip(1) { match arg.as_str() { "--permission" => enabled = true, "--no-permission" => enabled = false, @@ -20,7 +20,7 @@ pub(crate) fn process_permission_enabled() -> bool { fn process_permission_flag_values(flag: &str) -> Vec { let mut values = Vec::new(); let prefix = format!("{flag}="); - let mut args = std::env::args().skip(1).peekable(); + let mut args = super::process_args_lossy().skip(1).peekable(); while let Some(arg) = args.next() { if let Some(value) = arg.strip_prefix(&prefix) { values.extend( @@ -52,7 +52,7 @@ fn process_permission_flag_values(flag: &str) -> Vec { } fn process_permission_has_flag(flag: &str) -> bool { - std::env::args().skip(1).any(|arg| arg == flag) + super::process_args_lossy().skip(1).any(|arg| arg == flag) } fn permission_canonical_path(path: &str) -> Option { diff --git a/crates/perry-runtime/src/process/report.rs b/crates/perry-runtime/src/process/report.rs index a6f1290407..e0e995b59c 100644 --- a/crates/perry-runtime/src/process/report.rs +++ b/crates/perry-runtime/src/process/report.rs @@ -313,7 +313,7 @@ fn process_report_user_limits_object() -> f64 { } fn process_report_command_line_array() -> f64 { - let args: Vec = std::env::args().collect(); + let args: Vec = super::process_args_lossy().collect(); let items = if args.is_empty() { vec![process_argv0_string()] } else { @@ -344,7 +344,7 @@ fn process_report_unix_time_ms() -> f64 { #[cfg(feature = "diagnostics")] fn process_report_json_string(trigger: &str, filename: Option<&str>) -> String { - let args: Vec = std::env::args().collect(); + let args: Vec = super::process_args_lossy().collect(); let command_line = if args.is_empty() { vec![process_argv0_string()] } else { diff --git a/crates/perry-stdlib/src/commander.rs b/crates/perry-stdlib/src/commander.rs index 8e94b6edc0..8be1558a61 100644 --- a/crates/perry-stdlib/src/commander.rs +++ b/crates/perry-stdlib/src/commander.rs @@ -318,7 +318,12 @@ unsafe fn resolve_parse_args(argv: f64) -> Vec { return out.into_iter().skip(2).collect(); } } - std::env::args().skip(1).collect() + // #9401: `std::env::args()` panics on a non-UTF-8 argument; Node decodes + // argv leniently (every invalid byte becomes U+FFFD) and so must this. + std::env::args_os() + .skip(1) + .map(|arg| arg.to_string_lossy().into_owned()) + .collect() } /// Top-level parse entry. The second arg is the user's `parse(argv)` diff --git a/crates/perry/src/main.rs b/crates/perry/src/main.rs index 80ec75f1d8..f900d5ee4b 100644 --- a/crates/perry/src/main.rs +++ b/crates/perry/src/main.rs @@ -359,11 +359,22 @@ fn install_panic_hook() { })); } +/// The CLI's own arguments, decoded leniently (#9401). +/// +/// `std::env::args()` panics on an argument that is not valid Unicode, so +/// `perry compile $'\xff'.ts` aborted with a raw Rust backtrace instead of a +/// diagnostic. A non-UTF-8 path is an ordinary Linux filename. +fn argv_lossy() -> Vec { + std::env::args_os() + .map(|arg| arg.to_string_lossy().into_owned()) + .collect() +} + fn main_inner() -> Result<()> { env_logger::init(); #[cfg(windows)] { - let raw_args: Vec = std::env::args().collect(); + let raw_args: Vec = argv_lossy(); if let Some(result) = update_checker::maybe_run_windows_update_helper(&raw_args) { return result; } @@ -371,7 +382,7 @@ fn main_inner() -> Result<()> { update_checker::recover_interrupted_self_update()?; // Handle legacy invocation (perry file.ts -o out) - let args: Vec = std::env::args().collect(); + let args: Vec = argv_lossy(); let effective_args = if is_legacy_invocation(&args) { transform_legacy_args(args) } else { diff --git a/crates/perry/src/update_policy.rs b/crates/perry/src/update_policy.rs index 79012afc73..bc58c4360b 100644 --- a/crates/perry/src/update_policy.rs +++ b/crates/perry/src/update_policy.rs @@ -326,7 +326,11 @@ impl UpdatePolicy { /// because the policy is resolved before dispatch and this is the one input /// that has to be right on the very first line of output. fn structured_output_selected() -> bool { - let mut args = std::env::args().skip(1); + // #9401: a non-UTF-8 argument must not abort the CLI before it can + // report anything. + let mut args = std::env::args_os() + .skip(1) + .map(|arg| arg.to_string_lossy().into_owned()); while let Some(arg) = args.next() { if let Some(value) = arg.strip_prefix("--format=") { return !value.eq_ignore_ascii_case("text"); diff --git a/test-files/test_gap_9401_non_utf8_argv.ts b/test-files/test_gap_9401_non_utf8_argv.ts new file mode 100644 index 0000000000..de240bb23a --- /dev/null +++ b/test-files/test_gap_9401_non_utf8_argv.ts @@ -0,0 +1,42 @@ +// #9401: a non-UTF-8 byte in argv must not abort the process. +// +// `std::env::args()` PANICS on an argument that is not valid Unicode, and +// `js_process_argv` collected through it — so `claude -p $'\xff\xfe\x80abc\xc3\x28'` +// died with SIGABRT and a raw Rust backtrace +// (`library/std/src/env.rs: called Result::unwrap() on an Err value`). +// Non-UTF-8 filenames are ordinary on Linux, so this is trivially reachable. +// Node decodes argv leniently: every invalid byte becomes U+FFFD. +// +// The witness re-runs THIS program through `sh`, which is byte-oriented and +// can therefore construct an argument the source file itself cannot contain. +import { spawnSync } from "node:child_process"; + +const MARKER = "--decode-argv"; +const at = process.argv.indexOf(MARKER); + +if (at >= 0) { + const bad = process.argv[at + 1]; + console.log("typeof:", typeof bad); + console.log("length:", bad.length); + console.log( + "codepoints:", + Array.from(bad) + .map((c) => c.codePointAt(0)!.toString(16)) + .join(","), + ); + console.log("utf8-hex:", Buffer.from(bad, "utf8").toString("hex")); + console.log("next-arg:", process.argv[at + 2]); +} else { + // \377\376\200 abc \303 \050 — three lone invalid bytes, ASCII "abc", a + // truncated 2-byte lead byte, then '('. + const script = + 'exec "$0" "$1" ' + + MARKER + + ' "$(printf \'\\377\\376\\200abc\\303\\050\')" tail-marker'; + const child = spawnSync("/bin/sh", ["-c", script, process.argv[0], process.argv[1]], { + encoding: "utf8", + }); + process.stdout.write(child.stdout ?? ""); + console.log("child-status:", child.status); + console.log("child-signal:", child.signal); +} From 0f3192c1491aeb4a2df4afab134d7dbebd0a6d49 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Tue, 1 Sep 2026 18:16:22 +0200 Subject: [PATCH 3/3] fix(runtime): a rejected array element write throws only in strict mode (#9394) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit const a = [1]; Object.freeze(a); a[0] = 9; // node silent, Perry TypeError const a2 = [1]; Object.freeze(a2); a2[5] = 9; // node silent, Perry TypeError Object.defineProperty(a3, 0, {writable:false}); a3[0]=9; // node silent, Perry TypeError Object.preventExtensions(a4); a4[5] = 9; // node silent, Perry TypeError const o = {x:1}; Object.freeze(o); o.x = 9; // node silent, Perry silent (correct) ES2024 6.2.5.7 (PutValue) calls Set(O, P, V, Throw) with Throw = IsStrictReference, so a failed [[Set]] throws ONLY in strict mode — for an Array exactly as for the ordinary object that was already right. A CommonJS bundle is sloppy code from top to bottom, which is where this surfaced. Introduced by #9326 (the merge of #9297, live again via #9370). That change is right about what it set out to fix — an inherited accessor must run, an inherited non-writable index must reject — but it reached the rejection by routing the cold element-store continuation through the STRICT runtime entry unconditionally. The inline store guard declines exactly the receivers whose write can be rejected (frozen, sealed, non-extensible, descriptor-bearing, prototype-sensitive), so every one of those shapes arrived there and threw. The fix carries the assignment's own Throw flag, which codegen already had and already passes to the ordinary-object [[Set]] and to `js_dyn_index_set_strict`. Finding the target is unchanged in both modes — the #9220 inherited-descriptor walk still runs, so a prototype setter still fires on a sloppy assignment; only the rejection differs. - codegen: `assignment_strict` reaches `js_typed_feedback_array_index_set_fallback_boxed` and `js_typed_feedback_array_set_index_or_string` (one new trailing i32 each). - array/indexing.rs: the strict entry's body is strictness-parameterised (`js_array_set_f64_extend_sloppy` is the sloppy twin); `array_spec_set` takes Throw and returns the receiver unchanged instead of throwing when it is false. Array mutators keep Throw = true: their own algorithms specify it regardless of the calling code. - value/dyn_index.rs: `js_dyn_index_set_strict` already carried the flag and its array arm forced true; it now uses it. The realloc arm in expr/index.rs deliberately keeps the strict entry: it runs only for a receiver the guard already accepted, which cannot reject. test-files/test_gap_9394_array_element_store_strictness.cts is a `.cts`, so it is a CommonJS script in BOTH runtimes, with a sloppy arm and a "use strict" arm. BOTH ARMS ARE ASSERTED. Asserting only the throw is precisely what let this through: #9326 shipped with a 64-check differential and a 205-line gap fixture, all green, none of it sloppy code. Byte-compared against node 26.5.1; a compiler built from unfixed origin/main reports TypeError for six sloppy cases where node is silent, and with this change is identical to node. #9326's own fixture (test_gap_9220_9221_array_proto_paths.ts, an ES module and therefore strict) is unchanged and still byte-identical to node. Unit tests assert both arms too: `element_store_rejection_throws_only_in_strict_mode`, and #9326's `typed_feedback_array_set_guards_reject_frozen_arrays`, which now asserts the silent sloppy call alongside the strict throw. Both were confirmed to FAIL with the sloppy entry rewired to the strict one. Three pieces of test infrastructure had to admit a `.cts` fixture at all, each of which would have made it a DARK TEST: the suite's `find … -name '*.ts'` does not match `foo.cts`, so the harness never selected it (`--filter test_gap_9394` selected 0 tests before, and PASSes after); `basename … .ts` named it `…strictness.c`; and `.gitignore` re-included only `.ts`/`.tsx` under test-files/, so it could not be committed. Not addressed here, found while writing the fixture: Perry emits `js_put_value_set(..., strict = 0)` at EVERY property-set site, so a rejected strict ordinary-object write is silent where Node throws — the mirror-image gap on the object path. --- .gitignore | 8 + .../9394-sloppy-array-element-store.md | 89 ++++++ crates/perry-codegen/src/expr/index.rs | 21 +- crates/perry-codegen/src/expr/index_set.rs | 11 + .../src/runtime_decls/objects.rs | 6 +- crates/perry-runtime/src/array/indexing.rs | 74 ++++- .../perry-runtime/src/array/indexing_keyed.rs | 19 +- crates/perry-runtime/src/array/mod.rs | 3 + crates/perry-runtime/src/array/push_pop.rs | 7 +- .../src/array/strict_store_tests.rs | 78 +++++ crates/perry-runtime/src/typed_feedback.rs | 44 ++- .../perry-runtime/src/typed_feedback/tests.rs | 28 +- .../perry-runtime/src/typed_feedback/trace.rs | 4 +- crates/perry-runtime/src/value/dyn_index.rs | 10 +- run_parity_tests.sh | 16 +- ...ap_9394_array_element_store_strictness.cts | 299 ++++++++++++++++++ 16 files changed, 669 insertions(+), 48 deletions(-) create mode 100644 changelog.d/9394-sloppy-array-element-store.md create mode 100644 test-files/test_gap_9394_array_element_store_strictness.cts diff --git a/.gitignore b/.gitignore index ee0230acfa..4856dbcdc9 100644 --- a/.gitignore +++ b/.gitignore @@ -65,6 +65,14 @@ test-files/test-* !test-files/test_*.tsx !test-files/test-*.ts !test-files/test-*.tsx +# A fixture that must be CommonJS in BOTH runtimes is a `.cts` (this repo's +# package is `"type": "module"`, so a `.ts` is strict-mode ESM for Node and for +# Perry alike). Without these it would be an ignored file — a DARK TEST, the +# exact failure mode `scripts/check_test_registration.py` exists to prevent. +!test-files/test_*.cts +!test-files/test_*.mts +!test-files/test-*.cts +!test-files/test-*.mts !test-files/*/ tests/test_* tests/test-* diff --git a/changelog.d/9394-sloppy-array-element-store.md b/changelog.d/9394-sloppy-array-element-store.md new file mode 100644 index 0000000000..2642dc17ac --- /dev/null +++ b/changelog.d/9394-sloppy-array-element-store.md @@ -0,0 +1,89 @@ +### Fixed + +- **A rejected array element write no longer throws in sloppy code.** + + ```js + const a = [1]; Object.freeze(a); a[0] = 9; // node: silent Perry: TypeError + const a2 = [1]; Object.freeze(a2); a2[5] = 9; // node: silent Perry: TypeError + Object.defineProperty(a3, 0, {writable:false}); a3[0]=9; // node: silent Perry: TypeError + Object.preventExtensions(a4); a4[5] = 9; // node: silent Perry: TypeError + const o = {x:1}; Object.freeze(o); o.x = 9; // node: silent Perry: silent (correct) + ``` + + ES2024 §6.2.5.7 (`PutValue`) calls `Set(O, P, V, Throw)` with + `Throw = IsStrictReference`, so a failed `[[Set]]` throws **only in strict + mode** — for an Array exactly as for the ordinary object that was already + right. A CommonJS bundle is sloppy code from top to bottom, which is where + this surfaced. + + Introduced by #9326 (the merge of #9297, live again on `main` via #9370). + That change is right about what it set out to fix — an inherited accessor + must run, an inherited non-writable index must reject — but it reached the + rejection by routing the cold element-store continuation through the STRICT + runtime entry unconditionally. The inline store guard declines exactly the + receivers whose write can be rejected (frozen, sealed, non-extensible, + descriptor-bearing, prototype-sensitive), so every one of those shapes + arrived at that continuation and threw. + + The fix carries the assignment's own `Throw` flag, which codegen already had + and already passes to the ordinary-object `[[Set]]` and to + `js_dyn_index_set_strict`. Finding the target is unchanged in both modes — + the #9220 inherited-descriptor walk still runs, so a prototype setter still + fires on a sloppy assignment; only the rejection differs. + + - `crates/perry-codegen/src/expr/index.rs`, + `crates/perry-codegen/src/expr/index_set.rs`, + `crates/perry-codegen/src/runtime_decls/objects.rs` — pass the site's + `assignment_strict` to `js_typed_feedback_array_index_set_fallback_boxed` + and `js_typed_feedback_array_set_index_or_string` (one new trailing `i32` + each). + - `crates/perry-runtime/src/typed_feedback.rs` — both helpers take that flag + and dispatch on it. + - `crates/perry-runtime/src/array/indexing.rs` — the strict entry's body + becomes strictness-parameterised (`js_array_set_f64_extend_sloppy` is the + sloppy twin); `array_spec_set` takes `Throw` and returns the receiver + unchanged instead of throwing when it is false. Array mutators keep + `Throw = true`: their own algorithms specify it regardless of the calling + code. + - `crates/perry-runtime/src/array/indexing_keyed.rs` — the same for the + numeric/string-key dispatcher. + - `crates/perry-runtime/src/value/dyn_index.rs` — `js_dyn_index_set_strict` + already carried the flag and its array arm forced `true`; it now uses it. + + The realloc arm in `expr/index.rs` deliberately keeps the strict entry: it + runs only for a receiver the guard already accepted, which cannot reject. + + Validation: `test-files/test_gap_9394_array_element_store_strictness.cts` + — a `.cts` file, so it is a CommonJS script in **both** runtimes, with a + sloppy arm and a `"use strict"` arm. **Both arms are asserted.** Asserting + only the throw is precisely what let this through: #9326 shipped with a + 64-check differential and a 205-line gap fixture, all green, none of it + sloppy code. Byte-compared against node 26.5.1; Perry built from unfixed + `origin/main` reports `TypeError` for six sloppy cases where node is silent, + and with this change is identical to node. The #9326 fixture + (`test_gap_9220_9221_array_proto_paths.ts`, an ES module and therefore + strict) is unchanged and still byte-identical to node. + + Unit tests, both arms: `array/strict_store_tests.rs` + `element_store_rejection_throws_only_in_strict_mode`, and #9326's own + `typed_feedback_array_set_guards_reject_frozen_arrays`, which now asserts the + silent sloppy call alongside the strict throw. + + Three pieces of test infrastructure had to admit a `.cts` fixture at all — + each of which would have made it a **dark test**, green because it never ran: + + - `run_parity_tests.sh` discovered the suite with `find … -name '*.ts'`, + which does **not** match `foo.cts` (the suffix is `.cts`). The fixture was + invisible to the harness — confirmed empirically: `--filter test_gap_9394` + selected 0 tests before the change and reports + `PASS test_gap_9394_array_element_store_strictness` after it. + - the same script derived a test's name with `basename … .ts`, which left + such a file called `…strictness.c`. + - `.gitignore` ignores `test-files/test_*` (compiled test binaries) and + re-included only `.ts` / `.tsx`, so the fixture could not be committed. + + Not addressed here, found while writing the fixture: Perry emits + `js_put_value_set(..., strict = 0)` at **every** property-set site, so a + rejected *strict* ordinary-object write (`"use strict"; Object.freeze(o); + o.x = 9`) is silent where node throws. That is the mirror-image gap on the + object path and is out of scope for #9394. diff --git a/crates/perry-codegen/src/expr/index.rs b/crates/perry-codegen/src/expr/index.rs index 2a94aed43b..0d975282bc 100644 --- a/crates/perry-codegen/src/expr/index.rs +++ b/crates/perry-codegen/src/expr/index.rs @@ -134,6 +134,12 @@ pub(crate) fn lower_index_set_fast( // `expr_produces_canonical_raw_f64` — the slot store may skip the // `js_array_numeric_value_to_raw_f64` canonicalization call entirely. value_is_canonical_raw_f64: bool, + // #9394: the assignment's own `Throw` flag (ES2024 §6.2.5.7). The guard + // declines exactly the receivers whose element write can be REJECTED + // (frozen, sealed, descriptor-bearing, prototype-sensitive), so this is + // the flag the fallback continuation needs to decide between a TypeError + // and a silent no-op. + assignment_strict: bool, feedback_site_id: &str, ) -> Result<()> { // #8583-followup: if evaluating an operand diverged — a throwing @@ -389,6 +395,7 @@ pub(crate) fn lower_index_set_fast( ctx.current_block = guard_fallback_idx; { + let strict_flag = if assignment_strict { "1" } else { "0" }; let fallback_box = ctx.block().call( DOUBLE, "js_typed_feedback_array_index_set_fallback_boxed", @@ -397,6 +404,7 @@ pub(crate) fn lower_index_set_fast( (DOUBLE, arr_box), (DOUBLE, idx_double), (DOUBLE, val_double), + (I32, strict_flag), ], ); ctx.block().store(DOUBLE, &fallback_box, &slot); @@ -754,11 +762,14 @@ pub(crate) fn lower_index_set_fast( "js_typed_feedback_record_fallback_call", &[(I64, feedback_site_id)], ); - // Strict `arr[i] = v`: a frozen array's element is non-writable and a - // non-extensible array rejects a new index, so route to the throwing - // variant. (The inline fast/medium paths above are only reached for - // arrays with a proven dense-numeric layout, which excludes frozen / - // sealed / non-extensible arrays — those always fall to this call.) + // Growth for a receiver the guard already ACCEPTED. That guard + // (`plain_array_index_set_guard`) declines frozen, sealed and + // non-extensible arrays, descriptor-bearing arrays, and every + // prototype-sensitive shape — all of which take the `fallback` edge + // above instead — so no store reaching here can be rejected and the + // entry's `Throw` argument is unobservable. The strict entry is kept + // because it is the one that carries the fused key/policy/store + // path (#9394 left this arm alone deliberately). let new_handle = blk.call( I64, "js_array_set_f64_extend_strict", diff --git a/crates/perry-codegen/src/expr/index_set.rs b/crates/perry-codegen/src/expr/index_set.rs index 015d21fb41..1792aab463 100644 --- a/crates/perry-codegen/src/expr/index_set.rs +++ b/crates/perry-codegen/src/expr/index_set.rs @@ -284,6 +284,9 @@ fn lower_array_index_set_via_runtime_key( index: &Expr, value: &Expr, source_label: &str, + // #9394: the assignment's own `Throw` flag, carried to the runtime helper + // so a rejected element write is a TypeError only in strict code. + assignment_strict: bool, ) -> Result { // #7341, same hazard as the packed path: the receiver is live across both // `index` and `value` lowering, and an allocating RHS is a collection @@ -321,6 +324,7 @@ fn lower_array_index_set_via_runtime_key( source_label, TypedFeedbackContract::array_set_index_or_string(), ); + let strict_flag = if assignment_strict { "1" } else { "0" }; let new_handle = ctx.block().call( I64, "js_typed_feedback_array_set_index_or_string", @@ -329,6 +333,7 @@ fn lower_array_index_set_via_runtime_key( (I64, &arr_handle), (DOUBLE, &idx_double), (DOUBLE, &val_double), + (I32, strict_flag), ], ); if let Expr::LocalGet(id) = object { @@ -778,6 +783,7 @@ pub(crate) fn lower( index.as_ref(), value.as_ref(), "array[dynamic_numeric_index]", + assignment_strict, ); } // Same dispatch tree as IndexGet: known array → fast inline, @@ -880,6 +886,7 @@ pub(crate) fn lower( index.as_ref(), value.as_ref(), "array[dynamic_numeric_index]", + assignment_strict, ); }; let layout_note_needed = array_store_needs_layout_note(ctx, object, value); @@ -944,6 +951,8 @@ pub(crate) fn lower( ctx.current_block = fallback_idx; { + let strict_flag = + if assignment_strict { "1" } else { "0" }; let fallback_box = ctx.block().call( DOUBLE, "js_typed_feedback_array_index_set_fallback_boxed", @@ -952,6 +961,7 @@ pub(crate) fn lower( (DOUBLE, &arr_box), (DOUBLE, &idx_double), (DOUBLE, &val_double), + (I32, strict_flag), ], ); if let Some(slot) = ctx.locals.get(arr_id).cloned() { @@ -1180,6 +1190,7 @@ pub(crate) fn lower( value_is_numeric, require_numeric_layout, value_is_canonical_raw_f64, + assignment_strict, &feedback_site_id, )?; } else if let Some(global_name) = ctx.module_globals.get(&id).cloned() { diff --git a/crates/perry-codegen/src/runtime_decls/objects.rs b/crates/perry-codegen/src/runtime_decls/objects.rs index c9efe40066..645665f81d 100644 --- a/crates/perry-codegen/src/runtime_decls/objects.rs +++ b/crates/perry-codegen/src/runtime_decls/objects.rs @@ -462,10 +462,11 @@ pub fn declare_phase_b_objects(module: &mut LlModule) { I32, &[I64, DOUBLE, DOUBLE], ); + // Trailing I32: the assignment's own strict/`Throw` flag (#9394). module.declare_function( "js_typed_feedback_array_index_set_fallback_boxed", DOUBLE, - &[I64, DOUBLE, DOUBLE, DOUBLE], + &[I64, DOUBLE, DOUBLE, DOUBLE, I32], ); module.declare_function( "js_typed_feedback_observe_array_element", @@ -477,10 +478,11 @@ pub fn declare_phase_b_objects(module: &mut LlModule) { I64, &[I64, I64, I64, DOUBLE], ); + // Trailing I32: the assignment's own strict/`Throw` flag (#9394). module.declare_function( "js_typed_feedback_array_set_index_or_string", I64, - &[I64, I64, DOUBLE, DOUBLE], + &[I64, I64, DOUBLE, DOUBLE, I32], ); module.declare_function( "js_typed_feedback_object_set_index_polymorphic", diff --git a/crates/perry-runtime/src/array/indexing.rs b/crates/perry-runtime/src/array/indexing.rs index f08344312a..24ce713be2 100644 --- a/crates/perry-runtime/src/array/indexing.rs +++ b/crates/perry-runtime/src/array/indexing.rs @@ -12,6 +12,7 @@ pub use keyed::{ js_array_get_index_or_string, js_array_set_index_or_string, js_array_set_index_or_string_strict, js_array_set_string_key, }; +pub(crate) use keyed::js_array_set_index_or_string_with_strictness; use proto_chain::array_oob_prototype_get; pub(crate) use proto_chain::{array_custom_prototype, array_spec_get, array_spec_has_index}; use proto_chain::{array_object_proto_index_owner, ArrayCustomProto}; @@ -1185,18 +1186,37 @@ pub extern "C" fn js_array_set_f64_extend_strict( index: u32, value: f64, ) -> *mut ArrayHeader { - js_array_set_f64_extend_strict_impl(arr, index, value, false) + js_array_set_f64_extend_strict_impl(arr, index, value, true, false) } -/// Strict indexed assignment after optionally completing the inherited -/// descriptor walk. `prototype_already_checked` is true only for the callback -/// from [`array_spec_set`]; it prevents a writable inherited data property -/// from recursing when the spec walk proceeds to create the receiver's own -/// element. +/// The same element assignment for a SLOPPY `arr[i] = v` (#9394). +/// +/// ES2024 §6.2.5.7 calls `Set(O, P, V, Throw)` with `Throw = +/// IsStrictReference`, so a rejected element write is a silent no-op outside +/// strict code — for an Array exactly as for an ordinary object. Everything +/// the strict entry does to *find* the right target still runs (the dense +/// lanes, and the #9220 inherited-descriptor walk, so a prototype setter is +/// still invoked with this array as its receiver); only the rejection +/// changes from a TypeError to returning the receiver unchanged. +pub(crate) fn js_array_set_f64_extend_sloppy( + arr: *mut ArrayHeader, + index: u32, + value: f64, +) -> *mut ArrayHeader { + js_array_set_f64_extend_strict_impl(arr, index, value, false, false) +} + +/// Indexed assignment after optionally completing the inherited descriptor +/// walk. `strict` is the assignment's own `Throw` flag (see +/// [`js_array_set_f64_extend_sloppy`]). `prototype_already_checked` is true +/// only for the callback from [`array_spec_set`]; it prevents a writable +/// inherited data property from recursing when the spec walk proceeds to +/// create the receiver's own element. fn js_array_set_f64_extend_strict_impl( arr: *mut ArrayHeader, index: u32, value: f64, + strict: bool, prototype_already_checked: bool, ) -> *mut ArrayHeader { // Two exact fast lanes, each storing only what the general path below @@ -1223,8 +1243,12 @@ fn js_array_set_f64_extend_strict_impl( { // Preserve the existing polymorphic/subclass behavior on receivers // that are not live plain arrays. These are cold and cannot use the - // resolved-header contract below. - array_strict_index_write_guard(arr, index); + // resolved-header contract below. The guard is the *throwing* half of + // the policy, so a sloppy assignment skips it and keeps + // `js_array_set_f64_extend`'s silent contract. + if strict { + array_strict_index_write_guard(arr, index); + } return js_array_set_f64_extend(arr, index, value); } @@ -1248,14 +1272,16 @@ fn js_array_set_f64_extend_strict_impl( && unsafe { array_custom_prototype(clean).is_some() })) && unsafe { !array_has_own_index(clean, index) } { - return array_spec_set(clean, index, value); + return array_spec_set(clean, index, value, strict); } // SAFETY: the clean above resolved this exact live plain-array head. The // guard performs no Perry allocation/safepoint, so the proof remains live // for the store core. let flags = unsafe { array_object_flags_resolved(clean) }; - array_strict_index_write_guard_resolved(clean, index, flags); + if strict { + array_strict_index_write_guard_resolved(clean, index, flags); + } crate::string::js_string_addref_if_heap_string(value); unsafe { js_array_set_f64_extend_resolved(clean, index, value, flags) } } @@ -1610,12 +1636,24 @@ unsafe fn js_array_set_f64_extend_resolved( // to `raw_handle_debt.py --no-raise-vs` as debt appearing in a module that // did not exist at the merge base, which is a raise it refuses by design // (#7659) even though the repository total is unchanged. -/// Spec `Set(O, ToString(index), value, true)` for an Array receiver. Unlike +/// Spec `Set(O, ToString(index), value, Throw)` for an Array receiver. Unlike /// the internal dense setter, this observes an inherited indexed accessor /// before creating an own element. Array mutators use it on their exotic path /// because a prototype setter may mutate the receiver (including freezing it /// or making `length` non-writable) before the mutator's final length Set. -pub(crate) fn array_spec_set(arr: *mut ArrayHeader, index: u32, value: f64) -> *mut ArrayHeader { +/// +/// `strict` is the assignment's `Throw` argument (#9394): finding the right +/// target is the same work in both modes — an inherited setter runs either +/// way — and only a REJECTION differs, throwing in strict code and returning +/// the receiver unchanged in sloppy code. A spec-internal caller (an array +/// mutator's own `Set`) passes `true`, because those algorithms specify +/// `Throw = true` regardless of the calling code's strictness. +pub(crate) fn array_spec_set( + arr: *mut ArrayHeader, + index: u32, + value: f64, + strict: bool, +) -> *mut ArrayHeader { let arr = clean_arr_ptr_mut(arr); if arr.is_null() { return arr; @@ -1633,6 +1671,7 @@ pub(crate) fn array_spec_set(arr: *mut ArrayHeader, index: u32, value: f64) -> * arr_handle.get_raw_mut_ptr::(), index, value_handle.get_nanbox_f64(), + strict, true, ); } @@ -1675,6 +1714,11 @@ pub(crate) fn array_spec_set(arr: *mut ArrayHeader, index: u32, value: f64) -> * if inherited_owner != 0 { if let Some(accessor) = crate::object::get_accessor_descriptor(inherited_owner, &key) { if accessor.set == 0 { + // A getter-only inherited index rejects the Set. Sloppy + // code observes that as a no-op (#9394). + if !strict { + return arr_handle.get_raw_mut_ptr::(); + } crate::collection_iter::throw_type_error(&format!( "Cannot set property {index} which has only a getter" )); @@ -1689,6 +1733,11 @@ pub(crate) fn array_spec_set(arr: *mut ArrayHeader, index: u32, value: f64) -> * if crate::object::get_property_attrs(inherited_owner, &key) .is_some_and(|attrs| !attrs.writable()) { + // A non-writable inherited data property rejects the Set + // without creating an own element. Silent in sloppy code. + if !strict { + return arr_handle.get_raw_mut_ptr::(); + } throw_frozen_array_index_write(index); } } @@ -1697,6 +1746,7 @@ pub(crate) fn array_spec_set(arr: *mut ArrayHeader, index: u32, value: f64) -> * arr_handle.get_raw_mut_ptr::(), index, value_handle.get_nanbox_f64(), + strict, true, ) } diff --git a/crates/perry-runtime/src/array/indexing_keyed.rs b/crates/perry-runtime/src/array/indexing_keyed.rs index b812fe4b6a..a80ab873e1 100644 --- a/crates/perry-runtime/src/array/indexing_keyed.rs +++ b/crates/perry-runtime/src/array/indexing_keyed.rs @@ -352,6 +352,19 @@ pub extern "C" fn js_array_set_index_or_string_strict( arr: *mut ArrayHeader, idx: f64, value: f64, +) -> *mut ArrayHeader { + js_array_set_index_or_string_with_strictness(arr, idx, value, true) +} + +/// [`js_array_set_index_or_string_strict`] with the assignment's own `Throw` +/// flag (#9394). A sloppy `arr[k] = v` still takes the fused element path — +/// same key canonicalization, same inherited-descriptor walk — but a rejected +/// write is a silent no-op instead of a TypeError. +pub(crate) fn js_array_set_index_or_string_with_strictness( + arr: *mut ArrayHeader, + idx: f64, + value: f64, + strict: bool, ) -> *mut ArrayHeader { if !arr.is_null() { // Resolve the canonical array-index interpretation of the key (mirrors @@ -365,7 +378,11 @@ pub extern "C" fn js_array_set_index_or_string_strict( // canonical index is already proved here, so use the fused strict // element path and share one receiver resolution across policy // and store. - return js_array_set_f64_extend_strict(arr, i, value); + return if strict { + js_array_set_f64_extend_strict(arr, i, value) + } else { + crate::array::js_array_set_f64_extend_sloppy(arr, i, value) + }; } } js_array_set_index_or_string(arr, idx, value) diff --git a/crates/perry-runtime/src/array/mod.rs b/crates/perry-runtime/src/array/mod.rs index 5fb029c72a..65f77ac221 100644 --- a/crates/perry-runtime/src/array/mod.rs +++ b/crates/perry-runtime/src/array/mod.rs @@ -149,6 +149,9 @@ pub use self::indexing::{ js_array_set_f64_extend, js_array_set_f64_extend_strict, js_array_set_f64_unchecked, js_array_set_index_or_string, js_array_set_index_or_string_strict, js_array_set_string_key, }; +pub(crate) use self::indexing::{ + js_array_set_f64_extend_sloppy, js_array_set_index_or_string_with_strictness, +}; #[cfg(test)] pub(crate) use self::indexing_support::test_keys_array_slot_fallbacks; pub(crate) use self::indexing_support::{ diff --git a/crates/perry-runtime/src/array/push_pop.rs b/crates/perry-runtime/src/array/push_pop.rs index 260b4457ff..5dffd08002 100644 --- a/crates/perry-runtime/src/array/push_pop.rs +++ b/crates/perry-runtime/src/array/push_pop.rs @@ -896,10 +896,13 @@ fn push_array_spec_path(arr: *mut ArrayHeader, value: f64) -> *mut ArrayHeader { crate::array::array_length_range_error(); } + // `Array.prototype.push` step 4.d specifies `Set(O, …, true)` — the + // mutator's own Throw, independent of the caller's strictness (#9394). let next = crate::array::array_spec_set( arr_handle.get_raw_mut_ptr::(), length, value_handle.get_nanbox_f64(), + true, ); let next = clean_arr_ptr_mut(next); if !next.is_null() { @@ -1575,7 +1578,9 @@ fn shift_array_spec_set( ) { let (next, post_gc) = arr_handle.across_mut::(|| { let value = value_handle.get_nanbox_f64(); - arr_handle.with_mut_ptr(|current| crate::array::array_spec_set(current, index, value)) + // The mutators specify `Set(O, …, true)` regardless of the calling + // code's strictness (#9394). + arr_handle.with_mut_ptr(|current| crate::array::array_spec_set(current, index, value, true)) }); let next = clean_arr_ptr_mut(next); let current = if next.is_null() { diff --git a/crates/perry-runtime/src/array/strict_store_tests.rs b/crates/perry-runtime/src/array/strict_store_tests.rs index 3f3f3c092a..e2ed0450c0 100644 --- a/crates/perry-runtime/src/array/strict_store_tests.rs +++ b/crates/perry-runtime/src/array/strict_store_tests.rs @@ -153,3 +153,81 @@ fn strict_dense_number_store_fast_lane_matches_the_general_path() { crate::object::prototype_chain::test_swap_array_static_proto_recorded(latch_was); } } + +/// Whether `f` threw a runtime exception. +fn catch_runtime_throw(f: impl FnOnce()) -> bool { + let env = crate::exception::js_try_push(); + let jumped = unsafe { crate::ffi::setjmp::setjmp(env as *mut std::os::raw::c_int) }; + if jumped == 0 { + f(); + crate::exception::js_try_end(); + false + } else { + crate::exception::js_try_end(); + crate::exception::js_clear_exception(); + true + } +} + +/// #9394: a rejected element write throws only in STRICT code. +/// +/// ES2024 §6.2.5.7 calls `Set(O, P, V, Throw)` with `Throw = +/// IsStrictReference`, so a sloppy `arr[i] = v` against a frozen / +/// non-extensible / non-writable target is a silent no-op — exactly as it is +/// for an ordinary object, and exactly as Node behaves. +/// +/// BOTH arms are asserted here on purpose. #9326 shipped the throw-only half +/// with a 64-check differential and a 205-line gap fixture, all of it module +/// (strict) code, and every one of them stayed green while sloppy code — the +/// whole of a CommonJS bundle — started throwing. +#[test] +fn element_store_rejection_throws_only_in_strict_mode() { + // SAFETY: plain array construction plus the public element setters; every + // pointer below is a live head this test allocated. + unsafe { + let values = [1.0, 2.0, 3.0]; + + let frozen = js_array_from_f64(values.as_ptr(), values.len() as u32); + crate::object::js_object_freeze(crate::value::js_nanbox_pointer(frozen as i64)); + + assert!( + catch_runtime_throw(|| { + js_array_set_f64_extend_strict(frozen, 0, 9.0); + }), + "strict: a frozen element is non-writable" + ); + assert_eq!(js_array_get_f64(frozen, 0), 1.0); + + assert!( + !catch_runtime_throw(|| { + crate::array::js_array_set_f64_extend_sloppy(frozen, 0, 9.0); + }), + "sloppy: the same rejection is silent" + ); + assert_eq!(js_array_get_f64(frozen, 0), 1.0); + + // A new index on a non-extensible array is the other rejection shape. + assert!( + catch_runtime_throw(|| { + js_array_set_f64_extend_strict(frozen, 7, 9.0); + }), + "strict: a frozen array cannot gain an element" + ); + assert!( + !catch_runtime_throw(|| { + crate::array::js_array_set_f64_extend_sloppy(frozen, 7, 9.0); + }), + "sloppy: the same rejection is silent" + ); + assert_eq!((*frozen).length, 3); + + // A writable element is stored in both modes — the sloppy entry is a + // no-op only where the strict one would have thrown. + let open = js_array_from_f64(values.as_ptr(), values.len() as u32); + let out = crate::array::js_array_set_f64_extend_sloppy(open, 0, 42.0); + assert_eq!(js_array_get_f64(out, 0), 42.0); + let out = crate::array::js_array_set_f64_extend_sloppy(out, 3, 4.0); + assert_eq!((*out).length, 4); + assert_eq!(js_array_get_f64(out, 3), 4.0); + } +} diff --git a/crates/perry-runtime/src/typed_feedback.rs b/crates/perry-runtime/src/typed_feedback.rs index 97e74a4c09..04be9adad4 100644 --- a/crates/perry-runtime/src/typed_feedback.rs +++ b/crates/perry-runtime/src/typed_feedback.rs @@ -2545,12 +2545,22 @@ pub extern "C" fn js_typed_feedback_numeric_array_push_guard( } } +/// Cold continuation of a source-level `arr[index] = value` after the inline +/// guard declines the receiver. +/// +/// `strict` is the assignment's own `Throw` flag (ES2024 §6.2.5.7): codegen +/// passes `1` from strict code and `0` from sloppy code. #9394: this helper +/// used the strict element entry unconditionally, so a rejected write (frozen +/// array, non-writable own or inherited index, non-extensible receiver) threw +/// a TypeError in sloppy code where Node is silent — the guard declines +/// exactly those shapes, so every one of them arrived here. #[no_mangle] pub extern "C" fn js_typed_feedback_array_index_set_fallback_boxed( site_id: u64, receiver: f64, index: f64, value: f64, + strict: i32, ) -> f64 { record_fallback_call(site_id); @@ -2598,17 +2608,18 @@ pub extern "C" fn js_typed_feedback_array_index_set_fallback_boxed( (raw_addr as *const u8).sub(crate::gc::GC_HEADER_SIZE) as *const crate::gc::GcHeader; match (*gc_header).obj_type { crate::gc::GC_TYPE_ARRAY | crate::gc::GC_TYPE_LAZY_ARRAY => { - // #9220: this is the cold continuation of a source-level - // `arr[index] = value` after the inline guard rejects a - // retargeted/prototype-sensitive array. It must preserve the - // assignment's strict Set semantics; the non-strict helper - // bypassed `js_array_set_f64_extend_strict` entirely, so an - // inherited setter/non-writable index was silently replaced - // by a new own element. - let new_arr = crate::array::js_array_set_index_or_string_strict( + // #9220: the inherited-descriptor walk must run in BOTH + // modes — a prototype setter fires on a sloppy assignment + // too, and the pre-#9220 non-strict helper bypassed it + // entirely, silently replacing an inherited setter / + // non-writable index with a new own element. #9394: only the + // REJECTION is strictness-dependent, so carry the + // assignment's own `Throw` rather than forcing `true`. + let new_arr = crate::array::js_array_set_index_or_string_with_strictness( raw_addr as *mut ArrayHeader, index, value, + strict != 0, ); crate::value::js_nanbox_pointer(new_arr as i64) } @@ -2680,19 +2691,27 @@ pub extern "C" fn js_typed_feedback_array_set_string_key( crate::array::js_array_set_string_key(arr, key, value) } +/// Assignment-site wrapper for `arr[] = value`. `strict` is the +/// assignment's own `Throw` flag (#9394). #[no_mangle] pub extern "C" fn js_typed_feedback_array_set_index_or_string( site_id: u64, arr: *mut ArrayHeader, idx: f64, value: f64, + strict: i32, ) -> *mut ArrayHeader { // #5094 for the assignment site: with recording off (the default) every // helper below early-returns, but the index conversion and two // out-of-line calls to reach those returns were 1.5% of an ECS frame on // `column[index] = record`. One flag test, then the store. if !typed_feedback_enabled() { - return crate::array::js_array_set_index_or_string_strict(arr, idx, value); + return crate::array::js_array_set_index_or_string_with_strictness( + arr, + idx, + value, + strict != 0, + ); } let index = finite_nonnegative_u32_index(idx).unwrap_or(u32::MAX); observe_array(site_id, arr, index); @@ -2702,9 +2721,10 @@ pub extern "C" fn js_typed_feedback_array_set_index_or_string( } else { record_guard_pass(site_id); } - // Assignment-site wrapper → strict `Set` with `Throw = true` (frozen / - // non-extensible array element write throws a TypeError). - crate::array::js_array_set_index_or_string_strict(arr, idx, value) + // Assignment-site wrapper → spec `Set` with the reference's own `Throw`: + // a frozen / non-extensible array element write throws a TypeError in + // strict code and is a silent no-op in sloppy code. + crate::array::js_array_set_index_or_string_with_strictness(arr, idx, value, strict != 0) } #[no_mangle] diff --git a/crates/perry-runtime/src/typed_feedback/tests.rs b/crates/perry-runtime/src/typed_feedback/tests.rs index 58cabd8bab..4cd407a81c 100644 --- a/crates/perry-runtime/src/typed_feedback/tests.rs +++ b/crates/perry-runtime/src/typed_feedback/tests.rs @@ -552,7 +552,7 @@ fn typed_feedback_non_bounded_array_set_guard_failure_uses_jsvalue_object_fallba let guard = js_typed_feedback_plain_array_index_set_guard(24, obj_box, 0, 99.0, 0); assert_eq!(guard, 0); - let returned = js_typed_feedback_array_index_set_fallback_boxed(24, obj_box, 0.0, 99.0); + let returned = js_typed_feedback_array_index_set_fallback_boxed(24, obj_box, 0.0, 99.0, 1); assert_eq!(returned.to_bits(), obj_box.to_bits()); let key = crate::string::js_string_from_bytes(b"0".as_ptr(), 1); @@ -586,8 +586,18 @@ fn typed_feedback_array_set_guards_reject_frozen_arrays() { 0 ); + // #9394: BOTH arms. A rejected element write throws only in strict code — + // asserting the throw alone is exactly what let the sloppy regression + // through a 64-check differential and a 205-line gap fixture. assert!(catch_runtime_throw(|| { - js_typed_feedback_array_index_set_fallback_boxed(70, arr_box, 0.0, 99.0); + js_typed_feedback_array_index_set_fallback_boxed(70, arr_box, 0.0, 99.0, 1); + })); + assert_eq!( + crate::array::js_array_get_f64(arr, 0).to_bits(), + 1.0f64.to_bits() + ); + assert!(!catch_runtime_throw(|| { + js_typed_feedback_array_index_set_fallback_boxed(70, arr_box, 0.0, 99.0, 0); })); assert_eq!( crate::array::js_array_get_f64(arr, 0).to_bits(), @@ -610,7 +620,7 @@ fn typed_feedback_array_set_boxed_fallback_preserves_original_index_value() { let key = crate::string::js_string_from_bytes(b"foo".as_ptr(), 3); let key_value = crate::value::js_nanbox_string(key as i64); - let returned = js_typed_feedback_array_index_set_fallback_boxed(72, obj_box, key_value, 77.0); + let returned = js_typed_feedback_array_index_set_fallback_boxed(72, obj_box, key_value, 77.0, 1); assert_eq!(returned.to_bits(), obj_box.to_bits()); assert_eq!( crate::object::js_object_get_field_by_name_f64(obj, key).to_bits(), @@ -693,24 +703,24 @@ fn typed_feedback_boxed_set_fallback_does_not_truncate_fractional_array_like_key let buf = crate::buffer::js_buffer_alloc(3, 0); crate::buffer::js_buffer_set(buf, 1, 22); let buf_box = crate::value::js_nanbox_pointer(buf as i64); - js_typed_feedback_array_index_set_fallback_boxed(74, buf_box, 1.5, 99.0); + js_typed_feedback_array_index_set_fallback_boxed(74, buf_box, 1.5, 99.0, 1); assert_eq!(crate::buffer::js_buffer_get(buf, 1), 22); - js_typed_feedback_array_index_set_fallback_boxed(74, buf_box, 1.0, 99.0); + js_typed_feedback_array_index_set_fallback_boxed(74, buf_box, 1.0, 99.0, 1); assert_eq!(crate::buffer::js_buffer_get(buf, 1), 99); let ta = crate::typedarray::js_typed_array_new_empty(crate::typedarray::KIND_UINT8 as i32, 3); crate::typedarray::js_typed_array_set(ta, 1, 33.0); let ta_box = crate::value::js_nanbox_pointer(ta as i64); - js_typed_feedback_array_index_set_fallback_boxed(74, ta_box, 1.5, 88.0); + js_typed_feedback_array_index_set_fallback_boxed(74, ta_box, 1.5, 88.0, 1); assert_eq!(crate::typedarray::js_typed_array_get(ta, 1), 33.0); - js_typed_feedback_array_index_set_fallback_boxed(74, ta_box, 1.0, 88.0); + js_typed_feedback_array_index_set_fallback_boxed(74, ta_box, 1.0, 88.0, 1); assert_eq!(crate::typedarray::js_typed_array_get(ta, 1), 88.0); let set = crate::set::js_set_alloc(4); crate::set::js_set_add(set, 10.0); crate::set::js_set_add(set, 20.0); let set_box = crate::value::js_nanbox_pointer(set as i64); - js_typed_feedback_array_index_set_fallback_boxed(74, set_box, 1.5, 77.0); + js_typed_feedback_array_index_set_fallback_boxed(74, set_box, 1.5, 77.0, 1); assert_eq!(crate::set::js_set_size(set), 2); assert_eq!(crate::set::js_set_value_at(set, 1), 20.0); @@ -718,7 +728,7 @@ fn typed_feedback_boxed_set_fallback_does_not_truncate_fractional_array_like_key crate::map::js_map_set(map, 10.0, 100.0); crate::map::js_map_set(map, 20.0, 200.0); let map_box = crate::value::js_nanbox_pointer(map as i64); - js_typed_feedback_array_index_set_fallback_boxed(74, map_box, 1.5, 66.0); + js_typed_feedback_array_index_set_fallback_boxed(74, map_box, 1.5, 66.0, 1); assert_eq!(crate::map::js_map_size(map), 2); assert_eq!(crate::map::js_map_entry_key_at(map, 1), 20.0); diff --git a/crates/perry-runtime/src/typed_feedback/trace.rs b/crates/perry-runtime/src/typed_feedback/trace.rs index bfdf1b79dd..370d4a5ee2 100644 --- a/crates/perry-runtime/src/typed_feedback/trace.rs +++ b/crates/perry-runtime/src/typed_feedback/trace.rs @@ -430,13 +430,13 @@ mod keep_typed_feedback { #[cfg(feature = "keepalive-anchors")] #[used] static K21: extern "C" fn(u64, f64, f64) -> i32 = js_typed_feedback_numeric_array_push_guard; #[cfg(feature = "keepalive-anchors")] -#[used] static K22: extern "C" fn(u64, f64, f64, f64) -> f64 = js_typed_feedback_array_index_set_fallback_boxed; +#[used] static K22: extern "C" fn(u64, f64, f64, f64, i32) -> f64 = js_typed_feedback_array_index_set_fallback_boxed; #[cfg(feature = "keepalive-anchors")] #[used] static K23: extern "C" fn(u64, *const ArrayHeader, u32) = js_typed_feedback_observe_array_element; #[cfg(feature = "keepalive-anchors")] #[used] static K24: extern "C" fn(u64, *mut ArrayHeader, *const crate::StringHeader, f64) -> *mut ArrayHeader = js_typed_feedback_array_set_string_key; #[cfg(feature = "keepalive-anchors")] -#[used] static K25: extern "C" fn(u64, *mut ArrayHeader, f64, f64) -> *mut ArrayHeader = js_typed_feedback_array_set_index_or_string; +#[used] static K25: extern "C" fn(u64, *mut ArrayHeader, f64, f64, i32) -> *mut ArrayHeader = js_typed_feedback_array_set_index_or_string; #[cfg(feature = "keepalive-anchors")] #[used] static K26: extern "C" fn(u64, i64, f64, f64) = js_typed_feedback_object_set_index_polymorphic; #[cfg(feature = "keepalive-anchors")] diff --git a/crates/perry-runtime/src/value/dyn_index.rs b/crates/perry-runtime/src/value/dyn_index.rs index 857df74a48..9d94ab7329 100644 --- a/crates/perry-runtime/src/value/dyn_index.rs +++ b/crates/perry-runtime/src/value/dyn_index.rs @@ -529,7 +529,8 @@ pub extern "C" fn js_dyn_index_get(value: f64, index: f64) -> f64 { /// computed assignments use [`js_dyn_index_set_strict`] below. /// /// Routes by the receiver's `gc_type` byte: arrays go through -/// `js_array_set_index_or_string_strict` (numeric/string-key spec dispatch); +/// `js_array_set_index_or_string_with_strictness` (numeric/string-key spec +/// dispatch, carrying this entry's `strict` flag); /// ordinary objects retain receiver-aware property `[[Set]]` semantics. /// Strings are immutable — no-op (matches /// strict-mode `s[i] = x` semantics, close enough for the `++result[key]` @@ -767,10 +768,15 @@ pub extern "C" fn js_dyn_index_set_strict(obj: f64, index: f64, value: f64, stri } let is_array = receiver_tag.is_some_and(|(obj_type, _)| obj_type == crate::gc::GC_TYPE_ARRAY); if is_array { - crate::array::js_array_set_index_or_string_strict( + // #9394: this entry already carries the assignment's own `Throw` + // flag (`js_dyn_index_set` passes 0 for the sloppy runtime callers); + // the array arm forced `true` and threw on a frozen / non-writable / + // non-extensible element write that Node silently drops. + crate::array::js_array_set_index_or_string_with_strictness( raw_ptr as *mut crate::array::ArrayHeader, index, value, + strict != 0, ); return value; } diff --git a/run_parity_tests.sh b/run_parity_tests.sh index 2014dd2f1d..5c0039d079 100755 --- a/run_parity_tests.sh +++ b/run_parity_tests.sh @@ -1187,7 +1187,14 @@ case "$TEST_SUITE" in all) while IFS= read -r test_file; do TEST_FILES+=("$test_file") - done < <(find "$TEST_DIR" -maxdepth 1 -type f -name '*.ts' | sort) + # `.cts` / `.mts` as well as `.ts`: a fixture whose semantics depend on + # the module goal has to name it in the extension, because this repo's + # package is `"type": "module"` and a plain `.ts` is therefore + # strict-mode ESM for Node and for Perry alike. `-name '*.ts'` does NOT + # match `foo.cts` (the suffix is `.cts`), so such a fixture was invisible + # to the suite — a dark test, green because it never ran. + done < <(find "$TEST_DIR" -maxdepth 1 -type f \ + \( -name '*.ts' -o -name '*.cts' -o -name '*.mts' \) | sort) ;; parity|smoke) while IFS= read -r test_file; do @@ -1232,7 +1239,12 @@ for test_file in "${TEST_FILES[@]}"; do # Skip directories (multi/ folder) [[ -d "$test_file" ]] && continue - test_name=$(basename "$test_file" .ts) + # Strip any TypeScript extension, not just `.ts`: a fixture that has to be + # CommonJS in BOTH runtimes is a `.cts` (see + # test_gap_9394_array_element_store_strictness.cts, which needs sloppy-mode + # semantics that a `.ts` under this repo's `"type": "module"` cannot have). + # `basename … .ts` left such a file named `…strictness.c`. + test_name=$(basename "$test_file" | sed -E 's/\.(m|c)?ts$//') if [[ "$test_file" == "$NODE_SUITE_DIR"/* ]]; then test_rel="${test_file#"$NODE_SUITE_DIR"/}" test_id="node-suite/${test_rel%.ts}" diff --git a/test-files/test_gap_9394_array_element_store_strictness.cts b/test-files/test_gap_9394_array_element_store_strictness.cts new file mode 100644 index 0000000000..d80398bf8a --- /dev/null +++ b/test-files/test_gap_9394_array_element_store_strictness.cts @@ -0,0 +1,299 @@ +// #9394: a failed indexed [[Set]] on an Array throws ONLY in strict mode. +// +// ES2024 SS6.2.5.7 (PutValue) calls `Set(O, P, V, Throw)` with +// Throw = IsStrictReference(ref). A rejected write is therefore a silent no-op +// in sloppy code and a TypeError in strict code -- for arrays exactly as for +// ordinary objects. #9326 ("indexed writes honour a custom array prototype", +// live again via #9370) routed the cold element-store continuation through the +// STRICT runtime entry unconditionally, so every rejected array element write +// began throwing regardless of the assignment's own strictness. Plain objects +// were unaffected, which is why a 64-check differential and a 205-line gap +// fixture -- all of it module (strict) code -- stayed green. +// +// This file is `.cts`, so it is a CommonJS script in BOTH runtimes: `sloppyArm` +// is sloppy code and `strictArm` opts in with its own directive prologue. +// ASSERTING ONLY THE THROW IS WHAT LET THIS THROUGH, so every case below is +// asserted in both modes. +// +// The ordinary-object control appears in the sloppy arm only. Perry emits +// `js_put_value_set(..., strict = 0)` at every property-set site, so a rejected +// STRICT ordinary-object write is silent too -- a separate, pre-existing gap +// that is not what #9394 is about and is not fixed here. +// +// The two arms are textual duplicates on purpose: a function inherits the +// strictness of the code it is DEFINED in, never its caller's, so a shared +// helper would test sloppy twice. Only the mode prefix differs. + +function report(name: string, threw: boolean, ...rest: unknown[]): void { + console.log(name, threw ? "TypeError" : "silent", ...rest); +} + +function hasOwn(value: any, key: PropertyKey): boolean { + return Object.prototype.hasOwnProperty.call(value, key); +} + +function lockedProto(): any { + const proto: any = {}; + Object.defineProperty(proto, "6", { + configurable: true, + enumerable: true, + value: "lockedSix", + writable: false, + }); + return proto; +} + +function getterOnlyProto(): any { + const proto: any = {}; + Object.defineProperty(proto, "4", { + configurable: true, + get() { + return "getterOnly4"; + }, + }); + return proto; +} + +function setterProto(calls: any[]): any { + const proto: any = {}; + Object.defineProperty(proto, "4", { + configurable: true, + get() { + return "acc4"; + }, + set(value: any) { + calls.push(value); + }, + }); + return proto; +} + +function sloppyArm(): void { + let threw = false; + + const frozen: any[] = [1, 2, 3]; + Object.freeze(frozen); + threw = false; + try { + frozen[0] = 9; + } catch { + threw = true; + } + report("sloppy frozen in-bounds:", threw, frozen[0], frozen.length); + + const frozenOob: any[] = [1]; + Object.freeze(frozenOob); + threw = false; + try { + frozenOob[5] = 9; + } catch { + threw = true; + } + report("sloppy frozen new-index:", threw, frozenOob[5], frozenOob.length); + + const readOnly: any[] = [1]; + Object.defineProperty(readOnly, 0, { writable: false }); + threw = false; + try { + readOnly[0] = 9; + } catch { + threw = true; + } + report("sloppy non-writable own index:", threw, readOnly[0], readOnly.length); + + const noExtend: any[] = [1]; + Object.preventExtensions(noExtend); + threw = false; + try { + noExtend[5] = 9; + } catch { + threw = true; + } + report("sloppy preventExtensions new-index:", threw, noExtend[5], noExtend.length); + + // Sealed leaves existing elements writable, so this succeeds in both modes. + const sealed: any[] = [1]; + Object.seal(sealed); + threw = false; + try { + sealed[0] = 9; + } catch { + threw = true; + } + report("sloppy sealed in-bounds:", threw, sealed[0], sealed.length); + + // Control: the ordinary-object [[Set]] path, which already honours Throw. + const obj: any = { x: 1 }; + Object.freeze(obj); + threw = false; + try { + obj.x = 9; + } catch { + threw = true; + } + report("sloppy frozen plain object:", threw, obj.x); + + // #9220 shapes: an inherited index is consulted before an own element is + // created. Rejecting it is exactly what has to become mode-sensitive. + const lockedTarget: any = [1]; + Object.setPrototypeOf(lockedTarget, lockedProto()); + threw = false; + try { + lockedTarget[6] = "changed"; + } catch { + threw = true; + } + report( + "sloppy inherited non-writable:", + threw, + hasOwn(lockedTarget, 6), + lockedTarget[6], + lockedTarget.length, + ); + + const getterOnlyTarget: any = [1]; + Object.setPrototypeOf(getterOnlyTarget, getterOnlyProto()); + threw = false; + try { + getterOnlyTarget[4] = "changed"; + } catch { + threw = true; + } + report( + "sloppy inherited getter-only:", + threw, + hasOwn(getterOnlyTarget, 4), + getterOnlyTarget[4], + getterOnlyTarget.length, + ); + + // An inherited setter runs in both modes and creates no own element. + const calls: any[] = []; + const setterTarget: any = [1]; + Object.setPrototypeOf(setterTarget, setterProto(calls)); + threw = false; + try { + setterTarget[4] = 11; + } catch { + threw = true; + } + report( + "sloppy inherited setter:", + threw, + calls.join(","), + hasOwn(setterTarget, 4), + setterTarget.length, + ); +} + +function strictArm(): void { + "use strict"; + + let threw = false; + + const frozen: any[] = [1, 2, 3]; + Object.freeze(frozen); + threw = false; + try { + frozen[0] = 9; + } catch { + threw = true; + } + report("strict frozen in-bounds:", threw, frozen[0], frozen.length); + + const frozenOob: any[] = [1]; + Object.freeze(frozenOob); + threw = false; + try { + frozenOob[5] = 9; + } catch { + threw = true; + } + report("strict frozen new-index:", threw, frozenOob[5], frozenOob.length); + + const readOnly: any[] = [1]; + Object.defineProperty(readOnly, 0, { writable: false }); + threw = false; + try { + readOnly[0] = 9; + } catch { + threw = true; + } + report("strict non-writable own index:", threw, readOnly[0], readOnly.length); + + const noExtend: any[] = [1]; + Object.preventExtensions(noExtend); + threw = false; + try { + noExtend[5] = 9; + } catch { + threw = true; + } + report("strict preventExtensions new-index:", threw, noExtend[5], noExtend.length); + + // Sealed leaves existing elements writable, so this succeeds in both modes. + const sealed: any[] = [1]; + Object.seal(sealed); + threw = false; + try { + sealed[0] = 9; + } catch { + threw = true; + } + report("strict sealed in-bounds:", threw, sealed[0], sealed.length); + + // #9220 shapes: an inherited index is consulted before an own element is + // created. Rejecting it is exactly what has to become mode-sensitive. + const lockedTarget: any = [1]; + Object.setPrototypeOf(lockedTarget, lockedProto()); + threw = false; + try { + lockedTarget[6] = "changed"; + } catch { + threw = true; + } + report( + "strict inherited non-writable:", + threw, + hasOwn(lockedTarget, 6), + lockedTarget[6], + lockedTarget.length, + ); + + const getterOnlyTarget: any = [1]; + Object.setPrototypeOf(getterOnlyTarget, getterOnlyProto()); + threw = false; + try { + getterOnlyTarget[4] = "changed"; + } catch { + threw = true; + } + report( + "strict inherited getter-only:", + threw, + hasOwn(getterOnlyTarget, 4), + getterOnlyTarget[4], + getterOnlyTarget.length, + ); + + // An inherited setter runs in both modes and creates no own element. + const calls: any[] = []; + const setterTarget: any = [1]; + Object.setPrototypeOf(setterTarget, setterProto(calls)); + threw = false; + try { + setterTarget[4] = 11; + } catch { + threw = true; + } + report( + "strict inherited setter:", + threw, + calls.join(","), + hasOwn(setterTarget, 4), + setterTarget.length, + ); +} + +sloppyArm(); +strictArm();