Skip to content

fix(codegen): guard print_r's container walk against the null-container sentinel (#647) - #649

Open
mirchaemanuel wants to merge 1 commit into
illegalstudio:mainfrom
mirchaemanuel:fix/647-printr-null-container-sentinel
Open

fix(codegen): guard print_r's container walk against the null-container sentinel (#647)#649
mirchaemanuel wants to merge 1 commit into
illegalstudio:mainfrom
mirchaemanuel:fix/647-printr-null-container-sentinel

Conversation

@mirchaemanuel

Copy link
Copy Markdown
Contributor

Fixes #647. Based on current main 9f74f5300.

Sibling of #646 (same file, same sentinel family) but a different function and different PHP semantics — see below. The two are independent; see the conflict note at the end.

Root cause

emit_print_r_array (src/codegen/lower_inst/builtins/debug.rs) had no null branch at all — not even the zero-pointer check that emit_var_dump_array carried. So a missed element read wrote Array\n and passed the null-container sentinel straight to the walker as a live container.

Pre-fix assembly for the repro:

    ; print_r()
    ldur x0, [x29, #-96]          <- loads the sentinel
    str  x0, [sp, #-16]!
    ...                           <- writes "Array\n"
    bl   __rt_print_r_indexed     <- sentinel handed to the walker

No cbz, no comparison — so unlike #581 the crash happens inside the walker rather than at an inline header load.

Fix, and why it is not #646's fix

The guard uses the shared sentinels::emit_branch_if_null_container (zero or sentinel) and jumps to a label placed after the walker call, skipping both the Array\n write and the walk.

It deliberately does not reuse the branch target added in #646, because PHP's semantics differ and I checked them before implementing:

php -r 'echo "["; print_r(null); echo "]";'                          -> []
php -r '$s = print_r(null, true); echo strlen($s);'                  -> 0
php -r 'var_dump(print_r(null));'                                    -> bool(true)

print_r(null) prints nothing, where var_dump(null) prints NULL. Skipping every write is also what makes all three call modes correct with one branch: echo mode prints nothing and still returns true (the immediate is loaded later, in lower_print_r), and the capture modes leave the buffer empty so __rt_pr_finish yields "".

Tests

Eight regressions in tests/codegen/io/printing.rs — the natural home, next to the 20+ existing test_print_r_* cases. Six are behavioural and fail on the pre-fix build with program crashed; one guards the shapes that already worked; one asserts the emitted guard directly.

The assembly test was checked with an explicit negative control: reverting the source makes it fail on the host and under ELEPHC_TEST_TARGET=linux-x86_64 and linux-aarch64, with missing print_r null-container skip branch. So x86_64 is genuinely verified, not assumed.

Sibling shapes, all matching PHP after the fix: hash receiver with an array value, hash receiver with a hash value (the __rt_print_r_hash walker — it crashed too), a miss through ?? null, the return mode (print_r($x, true)), and the runtime-flag mode (print_r($x, $flag)). Return mode was the least obvious: it crashed without printing Array, because the text went into the capture buffer first, so its test asserts len=0 rather than merely the absence of a crash.

A miss inside a boxed mixed was already correct before this change — the boxing-boundary normalization from #585/#591 covers it — and is left alone rather than claimed.

Verification (macOS ARM64, 78a6a6590)

filter result
print_r (incl. the 8 new) 42 passed, 0 failed
same, ELEPHC_IR_OPT=off 37 passed, 0 failed
asm test, ELEPHC_TEST_TARGET=linux-x86_64 1 passed, 0 failed
asm test, ELEPHC_TEST_TARGET=linux-aarch64 1 passed, 0 failed
output_buffering (print_r writes through ob) 44 passed, 0 failed
var_dump 28 passed, 0 failed
sentinel 41 passed, 0 failed, 3 ignored
array_miss (#526) 20 passed, 0 failed
null_container (#556/#592) 2 passed, 0 failed
autovivify (#600/#592) 26 passed, 0 failed
all three call modes under --heap-debug leak summary: clean, exit 0
cargo build / git diff --check clean, zero warnings

Narrow filters only, per the project's test policy; CI owns the full matrix. Linux not executed locally (no Docker), but the x86_64 lowering is covered by the assembly test above.

Conflict note with #646

Checked with git merge-tree rather than assumed:

…er sentinel

emit_print_r_array had no null branch at all -- not even the zero-pointer
check var_dump carried -- so a missed element read wrote "Array\n" and
handed the null-container sentinel to the walker as a live container,
segfaulting mid-output.

The guard uses the shared sentinels helper, recognizing both the zero
pointer and the sentinel, and jumps past BOTH the "Array\n" write and the
walker call. Skipping every write is what makes all three call modes
correct at once: echo mode prints nothing and still returns true, and the
capture modes leave the buffer empty so __rt_pr_finish yields "".

Note this is deliberately not var_dump's behaviour: PHP prints nothing for
print_r(null), where var_dump(null) prints NULL, so the branch target
differs from the one added for issue illegalstudio#581.
@github-actions github-actions Bot added area:builtins Touches PHP builtin declarations or emitters. area:codegen Touches target-aware assembly or backend lowering. size:s Small pull request. type:fix Corrects broken or incompatible behavior. labels Jul 29, 2026
@greptile-apps

greptile-apps Bot commented Jul 29, 2026

Copy link
Copy Markdown

Greptile Summary

This PR fixes issue #647 by adding the null-container guard that was missing entirely from emit_print_r_array in src/codegen/lower_inst/builtins/debug.rs. Without the guard, a null-container sentinel (materialised by a missed array element read) was handed directly to the runtime walker as a live pointer, causing a crash.

  • Core fix: emit_branch_if_null_container is inserted before the Array\ header write and the walker call in emit_print_r_array, using the same shared sentinel helper that other null-container guards in the codebase use. The skip label lands after the walker call so both writes and the walk are entirely skipped, matching PHP's empty output for print_r(null).
  • Tests: Eight regression tests are added in tests/codegen/io/printing.rs covering the indexed walker path, the hash walker path, the ?? null coalescing path, the return-mode capture path, and the runtime-flag mode, plus a negative guard and a target-portable assembly test that verifies ordering of the sentinel comparison relative to the walker call.

Confidence Score: 5/5

Safe to merge; the change is a targeted one-function guard addition with no impact on existing code paths and thorough test coverage.

The guard is inserted in exactly the right place (before the header write and walker call), uses the shared sentinel helper that every other null-container site in the codebase uses, and the skip label is placed after the walker call so both writes are skipped together. All three call modes (echo, capture, runtime-flag) are covered by behavioural tests, and an assembly-ordering test confirms the guard precedes the walker on both supported architectures. The negative test verifies that genuine null locals and live arrays continue to render correctly. No existing behaviour is disturbed.

Files Needing Attention: No files require special attention.

Important Files Changed

Filename Overview
src/codegen/lower_inst/builtins/debug.rs Adds null-container guard to emit_print_r_array using the shared emit_branch_if_null_container helper; skip label is placed correctly after the walker call, matching PHP semantics for both walkers (indexed and hash). Docblock updated to explain the fix.
tests/codegen/io/printing.rs Adds 8 regression tests: 6 behavioural crash tests, 1 negative guard for pre-existing shapes, and 1 assembly-ordering test that verifies the sentinel comparison precedes the walker call on both AArch64 and x86_64.
CHANGELOG.md Adds one [Unreleased] bullet describing the fix, consistent with the project changelog policy.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["emit_print_r_array(ctx, walker)"] --> B["result_reg = int_result_reg\nskip_label = next_label\nscratch_reg = secondary_scratch_reg"]
    B --> C["emit_branch_if_null_container\n(zero ptr OR NULL_SENTINEL)"]
    C -->|null / sentinel| D["skip_label:\n(no output — PHP empty)"]
    C -->|live container| E["emit_push_reg(result_reg)\nemit_write_literal 'Array\\n'\nemit_pop_reg(result_reg)"]
    E --> F{"arch?"}
    F -->|AArch64| G["mov x1, #0\n(base indent = 0)"]
    F -->|x86_64| H["mov rdi, rax\nmov esi, 0"]
    G --> I["emit_call_label(walker)\n__rt_print_r_indexed or\n__rt_print_r_hash"]
    H --> I
    I --> D
Loading

Reviews (1): Last reviewed commit: "fix(codegen): guard print_r's container ..." | Re-trigger Greptile

@mirchaemanuel

Copy link
Copy Markdown
Contributor Author

CI note — the three Non-Codegen Tests failures are not caused by this change.

All three targets fail on the same unrelated test:

test eval_can_name_enum_cases_matches_php_case_numbering ... FAILED
  panicked at tests/eval_object_handle_tests.rs:112:5
  elephc compile failed:
  native project error: regex support requires managed native package pcre2

It is an eval/enum fixture failing on a missing managed native package — nothing to do with print_r or the sentinel guard. This branch is based on 9f74f5300, which predates the commits that introduced and then fixed that dependency:

a69cd5c89 feat: add managed native dependencies
eec7496f7 ci: initialize native cache at step runtime
ac0cbd507 test: seed managed pcre2 for eval CLI fixtures

None of those is in 9f74f5300 (checked), and all three are in current main. So rebasing onto current main should clear it. I'm leaving the rebase to you rather than force-pushing.

For what it is worth locally, on this branch: print_r_return_mode_heap_tests 6/6, builtin_parity_tests 8/8, var_export_and_strstr_result_tests 20/20, plus the filters listed in the PR body.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:builtins Touches PHP builtin declarations or emitters. area:codegen Touches target-aware assembly or backend lowering. size:s Small pull request. type:fix Corrects broken or incompatible behavior.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

print_r() on a null-container sentinel segfaults (no null guard in its lowering at all)

1 participant