Skip to content

fix(ir_lower): release owned Mixed sources after string coercion (foreach element leak)#532

Open
mirchaemanuel wants to merge 1 commit into
illegalstudio:mainfrom
mirchaemanuel:fix/527-foreach-value-copy-release
Open

fix(ir_lower): release owned Mixed sources after string coercion (foreach element leak)#532
mirchaemanuel wants to merge 1 commit into
illegalstudio:mainfrom
mirchaemanuel:fix/527-foreach-value-copy-release

Conversation

@mirchaemanuel

Copy link
Copy Markdown
Contributor

Fixes #527 — with a corrected diagnosis: the foreach rebind machinery was innocent.

Root cause (differs from the issue's hypothesis — verified with controlled probes)

The by-value foreach rebind on main is already balanced: IterCurrentValue/IterCurrentKey are listed as owning temporaries, and the emitted IR shows the full release-old → acquire-new → store sequence per iteration. Probes prove it: a loop body of {}, { $t = $row[1]; }, or { echo $row[1]; } is heap-clean. Only { echo $row[1] . "\n"; } leaks.

The real bug: release_stringified_source_if_owned (src/ir_lower/expr/mod.rs) skipped boxed Mixed sources. $row[1] on a Mixed row produces an owned fresh Mixed box; the implicit string coercion for the concat emits cast … Str and released the source only for Object/Array/AssocArray reprs — Mixed fell through to the no-op arm, leaking the box plus the persisted string payload it owns: exactly the 2 blocks/iteration in the issue. The same hole leaked every owned boxed-Mixed temporary flowing into a string coercion — explicit (string) casts and interpolation included, foreach or not.

The fix

One functional line: add PhpType::Mixed to the released-source match (Union codegen-reprs to Mixed, so it is covered too), with an explanatory comment. Safety: the backend lowers cast Mixed→Str via __rt_mixed_cast_string, which detaches the result from the box (string payloads persisted into an independent copy; int/float/bool rendered into fresh buffers) — the produced string never aliases the released cell. Ownership is still gated by release_if_owned (borrowed/non-heap/moved sources untouched).

Evidence (macOS arm64, base v0.26.1 93f576168; every output byte-identical pre/post and equal to PHP 8.4.23)

Case before after
issue repro (4 rows) 8 leaked blocks clean (allocs=35 frees=35)
20 rows (scaling) 40 blocks clean
$row used after the loop (PHP semantics) 2 blocks clean, prints last element like PHP
break mid-loop + use after 2 blocks clean
by-ref as &$row incl. mutation visibility 8 blocks clean, semantics unchanged
as $k => $row with string keys 6 blocks clean
explicit (string) cast / interpolation / array-literal element probes leaked clean, no double-release anywhere

Tests

3 regression tests in tests/codegen/runtime_gc/regressions.rs (issue repro; post-loop $row survival + clean heap; assoc key+value form) — proven fail-pre-fix by running the identical fixtures through a preserved pre-fix compiler (8/2/6 leaked blocks), pass post-fix. Focused suites all green, 0 failures: foreach 106 (103 + 3 new), arrays 337, nested 119, runtime_gc 125, concat 59, cast 106, string 705, mixed 221, iterator 122, echo 63, optimizer 251. Zero warnings; --no-ir-opt run also clean.

Notes for the maintainer

A string coercion of an owned boxed Mixed temporary (cast Mixed -> Str)
never released the source box: release_stringified_source_if_owned only
covered Object/Array/AssocArray sources. Every owned Mixed temp flowing
into a string coercion leaked its box plus the payload the box owned --
most visibly 2 heap blocks per iteration for the element read in
'foreach ($rows as $row) { echo $row[1] . PHP_EOL; }' over an array
of arrays.

The backend lowers the cast through __rt_mixed_cast_string, which
detaches the result (persisted string copy / fresh itoa-ftoa buffers),
so the produced string never aliases the released cell and the release
is safe.

Fixes illegalstudio#527

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.

foreach by-value over an array of arrays leaks two blocks per iteration (element copy never released)

1 participant