-
-
Notifications
You must be signed in to change notification settings - Fork 161
fix(runtime): SIGPIPE, non-UTF-8 argv, and sloppy-mode array store strictness (#9402, #9401, #9394) #9418
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(runtime): SIGPIPE, non-UTF-8 argv, and sloppy-mode array store strictness (#9402, #9401, #9394) #9418
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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). | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win Reduce this fragment to the shipped behavior. Remove the implementation history, PR chronology, validation transcript, and unrelated out-of-scope defect discussion. Keep one concise release-note entry that describes the sloppy array-write fix. Based on learnings, changelog fragments must describe final shipped behavior as one coherent release-note entry and must not include separate development-slice narratives. 🤖 Prompt for AI AgentsSource: Learnings |
||
| 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. | ||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
| @@ -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 — | ||||
|
|
||||
| ``` | ||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win Add a language identifier to the fenced block. The supplied markdownlint MD040 warning applies to Line 7. Mark the stack-trace block as Proposed fix- ```
+ ```text📝 Committable suggestion
Suggested change
🧰 Tools🪛 markdownlint-cli2 (0.23.2)[warning] 7-7: Fenced code blocks should have a language specified (MD040, fenced-code-language) 🤖 Prompt for AI AgentsSource: Linters/SAST tools |
||||
| 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 | ||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win Correct the reader counts. Line 27 says there are nine runtime readers, but the listed multiplicities total twelve sites. Lines 40-47 say there are three readers outside the runtime, but the list names four files. Remove the numeric claims or update them to match the inventory. Proposed wording-There were **nine**, all reachable, and the panic was not confined to `process.argv`:
+The following reachable runtime readers used the same panic-prone path, and the panic was not confined to `process.argv`:
...
-Three more outside the runtime, same shape, same fix:
+Additional readers outside the runtime use the same fix:Also applies to: 40-47 🤖 Prompt for AI Agents |
||||
| `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. | ||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Declare the example arrays before mutating them.
a3anda4are undeclared. Each example throwsReferenceErrorbefore it tests a rejected array write.Proposed fix
📝 Committable suggestion
🤖 Prompt for AI Agents