fix(codegen): guard container consumers against the null-container sentinel (chained-read miss segfault)#533
Conversation
…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
|
Opened #554 to track the pre-existing statically- It covers both Keeping this separate lets #533 remain focused on preventing null-container sentinel dereferences while #554 owns the PHP-semantic follow-up. |
|
I opened separate follow-ups for the two write/by-reference cases called out as out of scope here:
This keeps #533 focused on read/by-value consumer safety. #556 also distinguishes the new crash from the separate by-ref loop issue in #345, while #555 links the existing-parent work in #529/#553. |
…s-null-fallback # Conflicts: # src/ir_lower/expr/mod.rs
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-bandNULL_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-guarded0but not the sentinel), the isset array probe, and the foreach length snapshot. Compounding it,IsNullon 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, whatThrowable::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.)emit_branch_if_null_container(src/codegen_support/sentinels.rs): branch onreg == 0 || reg == NULL_SENTINEL, symmetric ARM64/x86_64 arms.lower_array_get_*(jumps to the fallback past the warning, so the Silent variants stay silent automatically), the isset array probes,IsNullforArray/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).??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)$a[7][1](issue repro)x=,done, heap clean$a[1][7](contrast)isset($a[7][1])false, no warning$a[7][1] ?? 'dflt'''— pre-existing gap, see notes)$m['nope']['x']string-keyedNULL, mirrors the working directionNULL×3, one warning per real missforeach ($a[7] ?? [] as $v)/foreach ($a[7] as $v)--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
asshim — the toolchain otherwise insists on assembling with the hostas) and verified the emitted guards verbatim (test/jz+movabssentinel +cmp/je) inarray_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.pyclean on all touched codegen files.Notes for the maintainer
$a[7][1] ?? 'dflt'yields''instead of'dflt'becauseIsNullon a statically-Strslot is constant-false — identical behavior for the already-working$a[1][7] ?? 'dflt'on main. Fixing it means givingStra null representation, which needs an audit of every empty-string producer; follow-up issue candidate.Trying to access array offset on null/foreach() argument … null given) is not emitted — no such warning infrastructure exists, and the already-working miss path omits it too.src/ir_lower/expr/mod.rsin different hunks than fix(ir_lower): release the nullable-access hidden temp after its consuming chained read #531/fix(ir_lower): release owned Mixed sources after string coercion (foreach element leak) #532 (and fix(codegen): release the inner-container temporary of a chained subscript read #524) — expected to auto-merge.