fix(runtime): Object.freeze/seal/preventExtensions check ownership before writing header flags — a wild write that segfaults on one receiver (fixes #10933) - #10935
Closed
proggeramlug wants to merge 2 commits into
Conversation
added 2 commits
September 21, 2026 22:25
…ront of header-less values Committed BEFORE the gate. On v0.5.1633, 30 of 32 registered symbols have the word at sym - 8 change under freeze/seal/preventExtensions: 0x...0000 -> 0x...00070000, i.e. FROZEN|SEALED|NO_EXTEND landing in _reserved six bytes in front of the symbol. The second test pins that the ops still mark a REAL object, so the gate cannot pass by becoming a blanket no-op.
…iting header flags (#10933) `Object.freeze` / `Object.seal` / `Object.preventExtensions` wrote `OBJ_FLAG_FROZEN | SEALED | NO_EXTEND` into `(value - 8) + 2` -- a real object's `GcHeader._reserved` -- for ANY pointer-tagged value above the handle band, with nothing establishing that the value HAS a header. `extract_obj_ptr` admits every such value, and several that perry hands to JS have no header at all, so the write landed in memory belonging to something else. Every earlier finding in this class (#10917, #10925, #10926) was a wild READ. This is the write side of the same hole, and on one value it is fatal: import * as crypto from "node:crypto"; Object.freeze(crypto.createHash("sha256").constructor); // SIGSEGV, 3/3 That receiver is the unresolved-namespace stub, a `.rodata` static, so the store faults. On a registered symbol -- a `Box::into_raw`'d `SymbolHeader` -- it does not fault, it just corrupts. Measured over 32 of them, reading the word at `sym - 8` before and after: pre[0] 0x8000000000000000 -> 0x8000000000070000 pre[2] 0x0000000000000004 -> 0x0000000000070004 pre_header_words_changed=30 of 32 `0x7` is the three flags landing in `_reserved`, six bytes in front of each symbol. Under the sabotage run below one of them reads `0x0000583129dbb9f0 -> 0x0000583129dfb9f0`: the write went into a POINTER-shaped value in an unrelated live allocation. THE GUARDS WERE THE WRONG QUESTION. `freeze` tested `is_above_handle_band(obj)`; `seal` (twice) and `preventExtensions` tested a bare `(obj as usize) > 0x10000`. Both keep small registry ids out -- which is why they were written -- and neither can tell whether `value - 8` is a header. The question is OWNERSHIP, and `try_read_tracked_gc_header` is the funnel that answers it: it proves the allocator owns this address on THIS thread (arena membership or the gc_malloc registry) instead of trusting `addr - 8`. All four write sites now go through one `integrity_flags_are_writable` helper. Behaviour for a rejected receiver is unchanged: the op is a no-op that returns the value, exactly as `Object.freeze(handle)` already was (`test_gap_handle_band_object_ops`). `Object.isFrozen` on the stub still answers `true`, matching node. This is narrower than the honest-tag migration and does not wait on it. The migration removes the header-less populations (#10924 stub, #10932 SAB, row 13 async, symbols later); this removes the ability to write through ANY of them, including ones not yet found. Tests, must-fail committed BEFORE the gate (63e44af): * `integrity_ops_do_not_write_in_front_of_a_header_less_value` -- 32 registered symbols, word at `sym - 8` before and after all three ops. Sabotaged by restoring the old band predicate: 31 of 32 corrupted. * `integrity_ops_still_apply_to_a_real_object` -- the gate must not pass by becoming a blanket no-op. * The compiled `Object.freeze(stub)` program segfaults on v0.5.1633 and returns normally here, WITHOUT #10924 -- the gate alone is sufficient. Note a fresh `Symbol("x")` goes through `gc_malloc` and DOES carry a header; only the leaked registered / well-known symbols are header-less.
|
Understand this PR’s impact Explore downstream dependencies and potential security impact with Blast Radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe integrity operations now verify tracked GC ownership before writing object flags. Header-less values remain unchanged. Regression tests cover symbols without GC headers and valid allocated objects. ChangesIntegrity operation ownership gate
Priority: ⬆️ High Estimated code review effort: 2 (Simple) | ~15 minutes Change: Bug fix · Severity of issue fixed: High 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
proggeramlug
pushed a commit
that referenced
this pull request
Sep 22, 2026
Three new bare GcHeader casts failed `scripts/addr_class_inventory.py`, a required `lint` gate. One is expressible through the canonical predicate and is converted; two are not, and carry written justifications rather than a blanket silence: * shared_sab.rs (test read) -> `try_read_gc_header`, exactly as object::tombstone_tests reads a keys array's flags. * shared_sab.rs (header INITIALISATION) -> allowlisted. Every addr_class predicate is a read-side check on an address of unknown provenance; none can express writing a header onto a block this function just alloc_zeroed'd. * keys_front_offset_tests.rs (test flag WRITE) -> allowlisted. `try_read_gc_header` returns a shared reference and cannot express the write; same discipline as the box/release_tests.rs entry. Also lowers the ratchet baseline by 10 sites across 8 entries, which the audit asked for: #10935's ownership gate, #10948's keys-array fix, this train's null_stub reconciliation and the earlier binding removals all deleted sites. Verified mechanically that no entry rose and none was added -- the ratchet only tightened.
proggeramlug
pushed a commit
that referenced
this pull request
Sep 22, 2026
proggeramlug
pushed a commit
that referenced
this pull request
Sep 22, 2026
Three new bare GcHeader casts failed `scripts/addr_class_inventory.py`, a required `lint` gate. One is expressible through the canonical predicate and is converted; two are not, and carry written justifications rather than a blanket silence: * shared_sab.rs (test read) -> `try_read_gc_header`, exactly as object::tombstone_tests reads a keys array's flags. * shared_sab.rs (header INITIALISATION) -> allowlisted. Every addr_class predicate is a read-side check on an address of unknown provenance; none can express writing a header onto a block this function just alloc_zeroed'd. * keys_front_offset_tests.rs (test flag WRITE) -> allowlisted. `try_read_gc_header` returns a shared reference and cannot express the write; same discipline as the box/release_tests.rs entry. Also lowers the ratchet baseline by 10 sites across 8 entries, which the audit asked for: #10935's ownership gate, #10948's keys-array fix, this train's null_stub reconciliation and the earlier binding removals all deleted sites. Verified mechanically that no entry rose and none was added -- the ratchet only tightened.
proggeramlug
pushed a commit
that referenced
this pull request
Sep 22, 2026
proggeramlug
pushed a commit
that referenced
this pull request
Sep 22, 2026
Three new bare GcHeader casts failed `scripts/addr_class_inventory.py`, a required `lint` gate. One is expressible through the canonical predicate and is converted; two are not, and carry written justifications rather than a blanket silence: * shared_sab.rs (test read) -> `try_read_gc_header`, exactly as object::tombstone_tests reads a keys array's flags. * shared_sab.rs (header INITIALISATION) -> allowlisted. Every addr_class predicate is a read-side check on an address of unknown provenance; none can express writing a header onto a block this function just alloc_zeroed'd. * keys_front_offset_tests.rs (test flag WRITE) -> allowlisted. `try_read_gc_header` returns a shared reference and cannot express the write; same discipline as the box/release_tests.rs entry. Also lowers the ratchet baseline by 10 sites across 8 entries, which the audit asked for: #10935's ownership gate, #10948's keys-array fix, this train's null_stub reconciliation and the earlier binding removals all deleted sites. Verified mechanically that no entry rose and none was added -- the ratchet only tightened.
proggeramlug
pushed a commit
that referenced
this pull request
Sep 22, 2026
proggeramlug
pushed a commit
that referenced
this pull request
Sep 22, 2026
Three new bare GcHeader casts failed `scripts/addr_class_inventory.py`, a required `lint` gate. One is expressible through the canonical predicate and is converted; two are not, and carry written justifications rather than a blanket silence: * shared_sab.rs (test read) -> `try_read_gc_header`, exactly as object::tombstone_tests reads a keys array's flags. * shared_sab.rs (header INITIALISATION) -> allowlisted. Every addr_class predicate is a read-side check on an address of unknown provenance; none can express writing a header onto a block this function just alloc_zeroed'd. * keys_front_offset_tests.rs (test flag WRITE) -> allowlisted. `try_read_gc_header` returns a shared reference and cannot express the write; same discipline as the box/release_tests.rs entry. Also lowers the ratchet baseline by 10 sites across 8 entries, which the audit asked for: #10935's ownership gate, #10948's keys-array fix, this train's null_stub reconciliation and the earlier binding removals all deleted sites. Verified mechanically that no entry rose and none was added -- the ratchet only tightened.
Contributor
Author
|
Landed on main in merge train 255 (#10950, v0.5.1636), main The train carried this PR at head |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #10933. Off
upstream/mainv0.5.1633, independent of the honest-tag PRs.The write side of the same hole
Object.freeze/Object.seal/Object.preventExtensionswroteOBJ_FLAG_FROZEN | SEALED | NO_EXTENDinto(value - 8) + 2— a real object'sGcHeader._reserved— for any pointer-tagged value above the handle band, with nothing establishing that the value has a header.extract_obj_ptradmits every such value, and several perry hands to JS have no header, so the write landed in memory belonging to something else.#10917, #10925 and #10926 were all wild reads. This is the write side, and on one value it is fatal:
That receiver is the unresolved-namespace stub, a
.rodatastatic, so the store faults. On a registered symbol — aBox::into_raw'dSymbolHeader— it doesn't fault, it corrupts. Measured over 32 of them, reading the word atsym - 8before and after all three ops:0x7is the three flags landing in_reserved, six bytes in front of each symbol. In the sabotage run one reads0x0000583129dbb9f0 -> 0x0000583129dfb9f0— the write went into a pointer-shaped value in an unrelated live allocation.The guards were asking the wrong question
Object.freezeis_above_handle_band(obj)Object.seal(×2)(obj as usize) > 0x10000Object.preventExtensions(obj as usize) > 0x10000Both keep small registry ids out — which is why they were written — and neither can tell whether
value - 8is a header. The question is ownership, andtry_read_tracked_gc_headeris the funnel that answers it: it proves the allocator owns this address on this thread (arena membership or the gc_malloc registry) instead of trustingaddr - 8. All four write sites now go through oneintegrity_flags_are_writablehelper.Behaviour for a rejected receiver is unchanged — a no-op returning the value, exactly as
Object.freeze(handle)already was (test_gap_handle_band_object_ops).Object.isFrozen(stub)still answerstrue, matching node.Why this is separate from the honest-tag migration
The migration removes the header-less populations one at a time (#10924 stub, #10932 SAB, row 13 async, symbols later). This removes the ability to write through any of them, including ones not yet found, and it doesn't wait on that work. The two are complementary; I'd take both.
Worth noting for the migration: a fresh
Symbol("x")goes throughgc_mallocand does carry a header. Only the leaked registered / well-known symbols are header-less, which narrows that row.Tests — must-fail committed BEFORE the gate (
63e44afb6)integrity_ops_do_not_write_in_front_of_a_header_less_valueintegrity_ops_still_apply_to_a_real_objectObject.freeze(stub)isFrozentruelike nodeSabotage: restoring the old band predicate behind an env var reddens the first test with
an integrity op wrote in front of a header-less value (31 of 32)and the byte-level before/after. Restored; no sabotage code remains.The stub program is fixed by this gate alone, without #10924 — verified on a build of this branch only.
Verified locally (CI runners are unreliable)
cargo test --release -p perry-runtime --lib -- --test-threads=1, both arms (the mode matters — see below): baselineupstream/main0fa3915294215 passed, 0 failed; this branch 4217 passed, 0 failed. The +2 are this PR's twoheader_gate_tests. No pre-existing failure, no new failure.-p perry-runtimecannot attribute a regression in parallel mode, because memo-counter assertions share process-global state in one binary: pristine main fails 13, a change fails 14, and the failing sets differ in BOTH directions. Three lanes got 0, 11 and 13 failures on comparable trees the same night. Quote the single-threaded numbers above; the parallel counts mean nothing either way.typed_feedback_class_field_set_guard_fails_for_frozen_object,typed_feedback_array_set_guards_reject_frozen_arrays,numeric_range_add_rejects_frozen_arrays_without_writing.clippy --all-targetshas ~12 pre-existing errors in unrelated files.Summary by CodeRabbit