Skip to content

fix(codegen): guard container consumers against the null-container sentinel (chained-read miss segfault)#533

Open
mirchaemanuel wants to merge 1 commit into
illegalstudio:mainfrom
mirchaemanuel:fix/526-array-get-miss-null-fallback
Open

fix(codegen): guard container consumers against the null-container sentinel (chained-read miss segfault)#533
mirchaemanuel wants to merge 1 commit into
illegalstudio:mainfrom
mirchaemanuel:fix/526-array-get-miss-null-fallback

Conversation

@mirchaemanuel

Copy link
Copy Markdown
Contributor

Fixes #526 — and the crash class turned out wider than the report: on main, all of these segfault (exit 139), not just the chained read: isset($a[7][1]), $a[7][1] ?? …, $m['nope']['x'], $a[9][0][0], $a[0][9][0], foreach ($a[7] ?? [] as $v), foreach ($a[7] as $v).

Root cause

emit_array_get_null_fallback (src/codegen/lower_inst/arrays.rs) materializes the in-band NULL_SENTINEL (0x7fff_ffff_ffff_fffe) for missed reads of refcounted element types, and every downstream container consumer dereferenced it unguarded: lower_array_get_{aarch64,x86_64} loaded the length header straight from it; the same held for __rt_hash_get, __rt_array_get_mixed_key, __rt_hash_iter_next (null-guarded 0 but not the sentinel), the isset array probe, and the foreach length snapshot. Compounding it, IsNull on statically-container-typed slots was hard-coded false, so $a[7] ?? [] evaluated the miss as non-null and propagated the sentinel into foreach.

Design

Consumer-side guards, keeping the sentinel as the established null-container representation — it is already what ??/isset compare against for scalars, what Throwable::getPrevious() uses for null objects, and what incref/decref already skip via heap-range checks, so it is GC-safe by construction. (Changing the fallback to a different representation was considered and rejected: the sentinel-as-null convention is load-bearing across comparisons, so a representation change would ripple much wider.)

  • New shared emitter helper emit_branch_if_null_container (src/codegen_support/sentinels.rs): branch on reg == 0 || reg == NULL_SENTINEL, symmetric ARM64/x86_64 arms.
  • Guarded: lower_array_get_* (jumps to the fallback past the warning, so the Silent variants stay silent automatically), the isset array probes, IsNull for Array/AssocArray/Iterable/Object, the foreach indexed-length snapshot; plus 3-instruction sentinel extensions of the existing null guards in __rt_hash_get, __rt_array_get_mixed_key, __rt_hash_iter_next (both arches each).
  • One ir_lower change so isset/?? silence propagates through the whole subscript chain (isset($a[7][1]) must not warn at any level — PHP semantics).

Before → after (base v0.26.1 93f576168, macOS arm64; all post-fix runs exit 0 and are PHP-compared)

Case before after
$a[7][1] (issue repro) warn + SIGSEGV warn, x=, done, heap clean
$a[1][7] (contrast) OK byte-identical to baseline
isset($a[7][1]) SIGSEGV false, no warning
$a[7][1] ?? 'dflt' SIGSEGV silent, no crash (yields '' — pre-existing gap, see notes)
$m['nope']['x'] string-keyed SIGSEGV NULL, mirrors the working direction
3-level miss at each level 2 of 3 SIGSEGV NULL ×3, one warning per real miss
foreach ($a[7] ?? [] as $v) / foreach ($a[7] as $v) SIGSEGV iterates empty / warns and skips
Mixed-element miss OK unchanged

--heap-debug: the repro and every previously-crashing variant end clean; the leaks remaining on some second-index-miss paths are byte-identical on unmodified main (pre-existing; they belong to the leak family being fixed in #524/#531/#532 — verified composed: on a local merge of those PRs the same programs end clean).

Cross-arch verification

Both arch arms edited symmetrically in every touched emitter (diff-reviewed). Beyond that, I captured the actual x86_64 assembly on this host (user asm via a runtime-cache pre-seed, runtime asm via an as shim — the toolchain otherwise insists on assembling with the host as) and verified the emitted guards verbatim (test/jz + movabs sentinel + cmp/je) in array_get, the isset probe, is_null_container, the iterator length snapshot, __rt_hash_get, __rt_array_get_mixed_key, __rt_hash_iter_next. Executing x86_64 locally is not possible on this host — relying on CI for the run matrix.

Tests

10 new regression tests in tests/codegen/regressions/arrays.rs (crash repro asserting stdout+warning+exit, second-index guard, heap-clean assertion, isset/?? silence, string-key chain, 3-level chain, foreach direct + coalesce, Mixed-element guard); the exact fixtures exit 139 on main. Focused suites, 0 failures: arrays 347 (incl. the 10 new), array_basics 81, isset 21, nested 119, coalesce 54, foreach 105, is_null 18, previous 3, runtime_gc 122. Zero build warnings; scripts/check_asm_comments.py clean on all touched codegen files.

Notes for the maintainer

…llegalstudio#526)

A chained subscript read whose FIRST index misses materialized the
in-band scalar null sentinel (0x7fff_ffff_ffff_fffe) for refcounted
element types, and the consuming outer read then loaded the array
length header from that sentinel address and crashed with SIGSEGV.

Treat a zero pointer or the null sentinel as the in-band PHP-null
container representation on every read surface, on both ARM64 and
x86_64:

- indexed array_get (warn + silent variants) branches to the null
  fallback, skipping the undefined-key warning, when the receiver is
  null/sentinel
- the isset() array-offset probe reports such receivers as missing and
  recognizes ArrayGetSilent producers
- __rt_hash_get, __rt_array_get_mixed_key, and __rt_hash_iter_next
  extend their existing null-table guards with the sentinel pattern
- IsNull on statically typed containers (Array/AssocArray/Iterable/
  Object) now detects the null/sentinel representation so `?? []`
  yields the default instead of propagating the sentinel into foreach
- foreach's indexed length snapshot treats a null/sentinel source as
  length zero so the loop body is skipped instead of crashing
- isset()/`??` suppress undefined-offset warnings across the whole
  subscript chain, matching PHP's silence for every level

Fixes illegalstudio#526

Claude-Session: https://claude.ai/code/session_01Jfn7uqEH5Dog1nBzTM5DCW
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Chained subscript read where the first index misses segfaults (null-fallback sentinel dereferenced as an array pointer)

1 participant