Skip to content

fix(ir_lower): release owned condition operands after truthiness coercion#593

Open
mirchaemanuel wants to merge 1 commit into
illegalstudio:mainfrom
mirchaemanuel:fix/586-match-merge-coalesce-leak
Open

fix(ir_lower): release owned condition operands after truthiness coercion#593
mirchaemanuel wants to merge 1 commit into
illegalstudio:mainfrom
mirchaemanuel:fix/586-match-merge-coalesce-leak

Conversation

@mirchaemanuel

Copy link
Copy Markdown
Contributor

Summary

Reading a value through the null-coalescing form ($x[0] ?? default) and using the result directly as a condition leaked the boxed Mixed coalesce temporary on every evaluation (issue #586). The issue's loop repro leaks one heap block per iteration; stdout is correct.

Bisection shows the leak is not tied to the match merge, to ternary merges, or to element-type widening (issue #549): a plain indexed-array local read through ?? default leaks just the same, and the trigger is that the coalesce result is consumed as a condition. Storing it in a local first is already heap-clean, and varying the array size keeps the leaked block at the fixed box size — so the leaked allocation is the coalesce box, not the container.

Root cause

LoweringContext::truthy emits Op::IsTruthy, which reads its operand and yields a fresh, non-aliasing boolean, but it never releases the operand. When the operand is an owned temporary — most visibly the boxed Mixed produced by lower_null_coalesce's take_owned_temp — nothing releases it after the truthiness test, so it leaks. Every condition site that fully consumes its operand was affected: if, ternary, while/do-while/for, short-circuit &&/||, negation !, and magic __isset.

Fix

Add LoweringContext::truthy_consuming, which computes truthiness and, when this path owns the input (value_is_owning_temporary), releases it through the type-gated release_if_owned — the same ownership contract the scalar-coercion casts already use for (bool)/(int)/(float). Route every condition site that discards its operand through it. lower_short_ternary keeps using truthy because PHP ?: forwards the original value when truthy, so releasing there would double-free.

Test plan

New heap-debug regression tests in tests/codegen/runtime_gc/regressions.rs ($i/$argc to defeat constant folding):

  • the exact issue repro (match arms + ternary), plus ternary-merge and plain-array siblings;
  • if, &&, and ! condition siblings;
  • controls: coalesce result stored in a local then tested, and a direct element read in a ternary condition, guarding against double frees.

6 leaking shapes were red before the fix and the 2 controls were already green; all 8 pass after. Focused slices green: coalesce/ternary/logical/truthy/isset (154), match/heap/leak/mixed/ownership/nulls (546), ir_lower lib (28), loop/control-flow (445), negation/switch/magic_isset/bool (114).

Fixes #586

https://claude.ai/code/session_01HRvbcjPHa3kAojdCjNZL5L

…cion

Reading a value through `?? default` and feeding the result directly into
a condition leaked the boxed coalesce temporary on every evaluation
(issue illegalstudio#586). `IsTruthy` reads its operand and yields a fresh,
non-aliasing boolean, but the owned `Mixed` box produced by the coalesce
was never released once its truthiness had been tested, so a loop like
`($r[0] ?? 0 ? 1 : 0)` leaked one block per iteration.

The leak is not specific to `match`/ternary merges or to element-type
widening: any owned temporary consumed purely as a branch condition
(`if`, ternary, `while`/`do-while`/`for`, short-circuit `&&`/`||`,
negation `!`, magic `__isset`) was orphaned.

Add `LoweringContext::truthy_consuming`, which computes truthiness and
releases the input when this path owns it (type-gated through
`value_is_owning_temporary` + `release_if_owned`, the same contract the
scalar-coercion casts use). Route every condition site that discards its
operand through it; `lower_short_ternary` keeps using `truthy` because
PHP `?:` forwards the original value.

Regression tests cover the issue repro plus ternary-merge, plain-array,
`if`, `&&`, and `!` siblings, with controls (store-then-test, direct
read) guarding against double frees.

Claude-Session: https://claude.ai/code/session_01HRvbcjPHa3kAojdCjNZL5L
@mirchaemanuel

Copy link
Copy Markdown
Contributor Author

While bisecting this I found two adjacent pre-existing bugs (both reverified on current main, independent of this fix — no ?? involved): #594 (self-referential array literal reassignment reads freed storage → garbage values / non-terminating while) and #595 (assoc-array local rebound to a fresh literal in a loop leaks the previous hash each iteration; the indexed sibling is clean).

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

Labels

area:eir Touches EIR definitions, lowering, validation, or passes. 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.

Reading a match-merge result through ?? default leaks the merged container on every iteration

1 participant