fix(runtime): the unresolved-namespace stub is an ordinary object, not a header-less static (#10821 row 4, fixes #10917) - #10924
proggeramlug wants to merge 4 commits into
Conversation
…t a header-less static (#10821) `js_unresolved_namespace_stub()` and ten dispatch catch-alls handed JS the address of `NULL_OBJECT_BYTES` under `POINTER_TAG` -- a `.rodata` byte array laid out like an `ObjectHeader`. It looked like an object to everything that reads an `ObjectHeader`, and it is not one: it has NO `GcHeader`. `addr_class::try_read_gc_header` accepts any heap-plausible address and returns `&*((addr - 8) as *const GcHeader)`, so every brand probe on the stub read whatever the linker placed before the static. In the v0.5.1631 binary those eight bytes are `6e 74 73 5d 00 00 00 00` -- the tail of a string literal, "nts]" -- so the stub reported `obj_type == 110`, a kind that does not exist. That is observable from a compiled program today, through a value any common-registry handle hands out: const c: any = crypto.createHash("sha256").constructor; // the stub JSON.stringify(c) // "" -- an empty object answers "{}" String(c) // TypeError: Cannot convert object to primitive value // -- an empty object answers "[object Object]" and the answer is BUILD-dependent: a different literal before the static is a different fake kind. This is the hazard the honest-tag invariant (a `POINTER_TAG` value is always a dereferenceable GC cell) exists to forbid, and `native_call_method.rs` already documents the same shape for a `Box`-allocated `SymbolHeader`. The stub is now an ordinary `GC_TYPE_OBJECT` with class id 0 and zero own keys -- exactly what `{}` allocates -- so the header at `addr - 8` is real and the object answers as the empty object it always claimed to be. * ONE object per realm, as before: every stub was the same address, so every stub was `===` every other, and that is kept. Per realm rather than per process because a GC object belongs to the thread whose arena allocated it; the static was shared across threads, which a heap object must not be. * Lazy, so a program that never reaches it pays nothing. All eleven sites return the stub immediately with no raw receiver live across the call, which is what makes allocating from inside the property-read funnels safe here. They now go through one funnel, `object::null_stub_value()`. * Rooted from `object::scan_object_cache_roots_mut`, with a researched `covered_elsewhere` verdict in the root-holder manifest (the gate reddens with the entry removed). * Class id 0, deliberately not a family id: the stub carries no native state, so it stays an ordinary object a worker can deep-copy like any `{}`. Collapsed on the way: an `is_valid_obj_ptr(obj)` branch in `js_native_call_method` whose two arms both returned the stub -- a test that could not change the answer. Its premise was the static's address lying outside the macOS heap window; a re-entrant `stub.raw().all(...)` now takes the ordinary-object path, finds a zero-key shape and reaches the same catch-all. Deleted: `NullObjectBytes`, `NULL_OBJECT_BYTES`, and `is_null_stub_address`, whose only production caller was the receiver-repr ledger arm gate A removes. Gate A: the `null_stub` arm of the receiver-repr fixture is inverted to `assert_fixture_migrated`, and the rendered sink line now witnesses `null_stub=0` in the `observed_old` section (it read 1). Gate B: `the_stub_is_an_ordinary_object_with_a_real_header` -- outside the handle band, a real `GcHeader` with `GC_TYPE_OBJECT`, class id 0, zero own keys. This retires no probe from the receiver-kind cascade: nothing asked about the stub by name. It removes one of the six header-less addresses that keep `try_read_gc_header`'s caller-side screens (`is_plausible_heap_addr`, `try_read_tracked_gc_header`) load-bearing.
…l band ids (#10821) assert_fixture_migrated checked one thing about a migrated producer value: not in the small-handle band. That is one of the TWO dishonest classes (plan 1.1). The other is a pointer-tagged address with NO GcHeader -- the null stub, a Box-allocated SymbolHeader, a SAB or external buffer backing -- and it is not in the band, so for those families gate A could not fail. Measured: with the stub sabotaged back to a header-less block, gate A stayed green while gate B went red. The gate now also requires try_read_tracked_gc_header(value).is_some(), which proves allocator ownership instead of trusting addr - 8. Under the same sabotage it reports: NullStub producer returned 0x39ead543390, which is not an allocator-owned GC cell. Clean, it stays green for every migrated family (text, timer, tui, null_stub: 99 passed).
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
|
Raising the priority of this one: the same header-less stub is also hit on the write side. |
|
Landed on Same two train-side fixes as #10915 (it is stacked on it): The train was validated as one tree: all ratchets, |
…alue() (#10924) Adopted verbatim from merge train 255 (#10950, `train255r`), which found this in its merged tree and fixed it there. The fix was never pushed back to this branch, so the branch stayed broken and the same break was rediscovered from CI. The comment is the train's. #10924 (train 254) replaced the header-less `NullObjectBytes` static with a real GC object and migrated every call site. #10938 was written before that landed and ADDED a new site in the old idiom - the shadowing-scan hand guard at site 3, the one `ShapeObjectKind::Dictionary` does not reach and which was therefore deliberately kept. Main migrated what existed; this branch introduced one more. The merge of the two is textually clean and semantically broken: `main`'s `object/mod.rs` no longer exports `NULL_OBJECT_BYTES`, while this branch's `native_call_method.rs` still spells it. Since CI builds the PR merged with main, every build job failed and all six gap shards were skipped. It was not only a compile break. Had it compiled, a dictionary-mode receiver would have got #10917 back - brand probes reading the `.rodata` bytes in front of a header-less value - which is precisely what #10924 removed. Rebased onto `a022cf2e4` (was train 253, `0fa391529`) in the same push, so the stack restacks once.
…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.
#10938's new dictionary-mode early return still spelled the header-less `NullObjectBytes` static that #10924 replaced with a real GC object, so the branch did not compile and, had it, would have given dictionary-mode receivers the #10917 bug back: brand probes reading the .rodata bytes in front of a header-less value. It returns `null_stub_value()` now, like every other site. #10932's cross-thread test wrapped `buffer::buffer_data` in `unsafe` -- a safe fn on main and unchanged by this train -- which is an `unused_unsafe` warning and therefore a failure under CI's `warnings` gate (--all-targets -D warnings). Also: #10931 added `proto_serial` to the inline `ObjectMeta` while #10938 moved that struct into object/meta_record.rs. The field is ported to the moved module, placed after `dictionary_keys` rather than immediately before `native_state` -- the inline version sat between native_state's doc block and its declaration, which reattached that whole doc ("LAST FIELD ON PURPOSE") to proto_serial and left native_state undocumented.
…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.
#10938's new dictionary-mode early return still spelled the header-less `NullObjectBytes` static that #10924 replaced with a real GC object, so the branch did not compile and, had it, would have given dictionary-mode receivers the #10917 bug back: brand probes reading the .rodata bytes in front of a header-less value. It returns `null_stub_value()` now, like every other site. #10932's cross-thread test wrapped `buffer::buffer_data` in `unsafe` -- a safe fn on main and unchanged by this train -- which is an `unused_unsafe` warning and therefore a failure under CI's `warnings` gate (--all-targets -D warnings). Also: #10931 added `proto_serial` to the inline `ObjectMeta` while #10938 moved that struct into object/meta_record.rs. The field is ported to the moved module, placed after `dictionary_keys` rather than immediately before `native_state` -- the inline version sat between native_state's doc block and its declaration, which reattached that whole doc ("LAST FIELD ON PURPOSE") to proto_serial and left native_state undocumented.
…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.
#10938's new dictionary-mode early return still spelled the header-less `NullObjectBytes` static that #10924 replaced with a real GC object, so the branch did not compile and, had it, would have given dictionary-mode receivers the #10917 bug back: brand probes reading the .rodata bytes in front of a header-less value. It returns `null_stub_value()` now, like every other site. #10932's cross-thread test wrapped `buffer::buffer_data` in `unsafe` -- a safe fn on main and unchanged by this train -- which is an `unused_unsafe` warning and therefore a failure under CI's `warnings` gate (--all-targets -D warnings). Also: #10931 added `proto_serial` to the inline `ObjectMeta` while #10938 moved that struct into object/meta_record.rs. The field is ported to the moved module, placed after `dictionary_keys` rather than immediately before `native_state` -- the inline version sat between native_state's doc block and its declaration, which reattached that whole doc ("LAST FIELD ON PURPOSE") to proto_serial and left native_state undocumented.
Row 4 of the honest-tags tracker. Stacked on #10915 (the tui family); base branch
feat/honest-tags-tui. Fixes #10917.This is a correctness bug as well as a migration row
js_unresolved_namespace_stub()and ten dispatch catch-alls handed JS the address ofNULL_OBJECT_BYTES, a.rodatabyte array laid out like anObjectHeader, underPOINTER_TAG. It has noGcHeader. Every brand probe reads one anyway:try_read_gc_header_known_plausible(crates/perry-runtime/src/value/addr_class.rs:272) returns&*((addr - GC_HEADER_SIZE) as *const GcHeader)for any heap-plausible address, so the probes read the 8 bytes the linker put before the static.In the v0.5.1631 binary (
nm:NULL_OBJECT_BYTESat0x152cf68) those bytes are6e 74 73 5d 00 00 00 00. That is"nts]", the tail of a string literal, so the stub reportsobj_type == 110, a GC kind that does not exist (GC_TYPE_MAXis 21). A compiled program reaches it today throughhandle.constructoron any common-registry handle:const c = crypto.createHash("sha256").constructor{}JSON.stringify(c)""{}{}JSON.stringify({a: c}){"a":{}}{"a":{}}String(c)Cannot convert object to primitive value[object Object][object Object]typeof c,Object.keys(c), brandobject,[],[object Object]c === <another stub>truetruestructuredClone(c){},!== c{},!== cThe answer depends on the build: a different literal before the static gives the stub a different fake kind. Filed separately as #10917 so it can be found and bisected on its own.
The change
The stub is now an ordinary
GC_TYPE_OBJECTwith class id 0 and zero own keys, which is what{}allocates. The header ataddr - 8is real.===every other, and that stays true. It is per realm and not per process because a GC object belongs to the thread whose arena allocated it. The static was shared across threads, and a heap object must not be.object::null_stub_value().object::scan_object_cache_roots_mut, with a researchedcovered_elsewhereverdict in the root-holder manifest. The gate goes red with the entry removed (exit 1, namesNULL_STUB_SLOT [rule T]) and green with it.{}. That covers checklist row 3.Deleted:
NullObjectBytes,NULL_OBJECT_BYTES,is_null_stub_address. The last one's only production caller was the receiver-repr arm that gate A removes.Collapsed: an
is_valid_obj_ptr(obj)branch injs_native_call_methodwhose two arms both returned the stub, so the check could never change the answer. It was written because the static's address lay outside the macOS heap window.Behaviour change: method calls on the stub now throw, like on any
{}c.raw()TypeError: raw is not a functionTypeError: c.raw is not a functionc.raw().all()rawraw({}).raw()TypeErrorTypeErrorTypeErrorThe old silent chain relied on the wild read. The re-entrant call took the
gc_type != GC_TYPE_OBJECTfallback arm only because the fake header said kind 110. On a build where the preceding byte happened to be02, it would already have thrown. Perry had already stopped chaining through real empty objects:test_issue_645_chained_method_on_null_objruns({}).nonExistentMethodA(), throws on both arms, and is listed intest-parity/known_failures.json. This PR makes the stub behave the same way{}already does.Per-family checklist
every_stub_in_a_realm_is_the_same_object; compiled teststable true/map 1(a stub reached through three different handles is oneMapkey){}(node 26.8.1 control run on the host){}.structuredClonegives{},!==, same as nodeassert_fixture_migrated, plus the rendered sink line now witnessesnull_stub=0inobserved_old. Gate A was also strengthened (see below)the_stub_is_an_ordinary_object_with_a_real_header: outside the band, a realGcHeader,GC_TYPE_OBJECT, class id 0, zero own keys, held in the realm slotGate A could not fail for this family, so it was strengthened
The first sabotage run (stub returned as a header-less
Boxblock, the pre-fix shape) turned gate B red (obj_typeread as 0, not 2) but left gate A green.assert_fixture_migratedchecked only "not in the small-handle band", which covers one of the plan's two dishonest classes (§1.1). A header-less address is not in the band. The gate now also requirestry_read_tracked_gc_header(value).is_some(), which proves allocator ownership instead of trustingaddr - 8. Under the same sabotage it now reportsNullStub producer returned 0x39ead543390, which is not an allocator-owned GC cell. On the clean run it stays green for every migrated family (text, timer, tui, null_stub: 99 passed). This also makes gate A usable for the remaining header-less rows (sab, external_buffer, symbol_global, async_hook/async_resource).Verified locally
cargo test --release -p perry-runtime --lib -- --test-threads=1, both arms (the mode matters — see below): baselineupstream/main841b605c94197 passed, 0 failed; the stack tipdffb1e326(this PR on top of feat(runtime): perry/tui handles are ordinary objects (#10821 row 3) #10915) 4205 passed, 0 failed. The +8 are the tests the stack adds — 6 here, 2 in the other. No pre-existing failure, no new failure on either arm.-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.cargo test --release -p perry --test null_stub_is_an_object --test tui_handle_identity: 4 passed.json ""and threw onString(). The new arm prints the table above. The tui differential fixture gives byte-identical output on this stacked branch and on feat(runtime): perry/tui handles are ordinary objects (#10821 row 3) #10915.null_stub::NULL_STUB_SLOTandnull_stub_valueare present, andNULL_OBJECT_BYTESis absent (it is present in the v0.5.1631 archive).scripts/gc_runtime_root_holders.py: exit 0.The baseline arm was another lane's tree (
/root/wt-main247, v0.5.1631), and it was rebuilt away while I worked. For the next family I will build my own baseline.Probe cascade
This PR retires no probe from the receiver-kind cascade, because nothing asked about the stub by name. It removes one of the six header-less addresses that keep the caller-side screens in front of
try_read_gc_header(is_plausible_heap_addr,try_read_tracked_gc_header) load-bearing.