Fix GH-21639: Protect frameless args during __toString reentry

[prateekbhujel]

Fixes GH-21639.

Frameless calls borrow CV operands from the caller frame. If an argument conversion reaches __toString(), userland can overwrite the variables behind those operands while the C handler is still using borrowed strings or arrays.

This moves the protection to the __toString() reentry point: before entering userland, the engine checks whether the active opcode is a frameless call and keeps copies of any array/string CV operands. Those copies are released after the frame has moved past that opcode, or during executor shutdown if the request unwinds first.

That keeps the individual frameless handlers and normal dispatch path unchanged; the cost is paid only when __toString() is actually entered during a frameless call.

Tests run:

make -j"$(sysctl -n hw.ncpu)"
git diff --check
sapi/cli/php run-tests.php -q Zend/tests/gh21639.phpt ext/pcre/tests/gh21639.phpt
sapi/cli/php run-tests.php -q Zend/tests/frameless_undefined_var.phpt ext/standard/tests/general_functions/gettype_settype_basic.phpt ext/standard/tests/array/in_array_variation1.phpt ext/standard/tests/strings/implode_basic.phpt ext/standard/tests/strings/implode_variation.phpt ext/standard/tests/strings/join_basic.phpt ext/standard/tests/strings/strtr_basic.phpt ext/standard/tests/strings/str_replace_basic.phpt ext/pcre/tests/preg_match_basic.phpt ext/pcre/tests/preg_replace.phpt Zend/tests/gh21639.phpt ext/pcre/tests/gh21639.phpt

[iluuu1994]

Thanks for the PR. implode() is just one example, this applies to:

• preg_replace
• in_array (with strict: false)
• strtr
• str_replace

I'm afraid this will need a more general solution. My hope was to avoid overhead in the VM, but it might be unavoidable for a proper fix, even if this is a largely artificial issue.

[prateekbhujel]

Thanks @iluuu1994. I reworked this so it no longer copies every frameless call.

The updated patch adds a ZEND_FLF_ARG_COPY bit to the existing frameless metadata and only marks handlers that need by-value lifetime protection. Unmarked handlers keep the current fast path. I also made the JIT skip direct frameless emission for marked calls, so those go through the VM handler path instead of borrowing operands directly.

While checking the frameless list I also included min()/max(), which can hit the same issue through comparison. The GH-21639 test now covers those along with implode, in_array, strtr, str_replace, and preg_replace.

[iluuu1994]

The Symfony benchmark IC increases by 0.64%, the Wordpress one by 1.15%. This removes much of the benefit of frameless calls, so I'm not to keen on solving this issue in that way. As long as other engine bugs aren't solved (#20001 and #20018) I don't think this needs to be rushed.

[prateekbhujel]

@iluuu1994 I pushed a different version that drops the VM/metadata/JIT approach.

The current patch keeps frameless dispatch as-is and guards only the local reentry boundaries in the affected handlers. In particular, in_array() keeps the existing scalar fast path and only pins before the zend_compare() fallback, and the string/PCRE wrappers keep pure-string fast paths before doing any protection work.

I reran the focused tests and checked the hot cases from the earlier benchmark locally. This should address the main issue with the previous version without turning the bug fix into a broader engine-level change.

[bwoebi]

The other alternative would be checking inside the tostring handler whether the parent frame is currently at a frameless opcode and then safely copy its CV args to a buffer, set EG(vm_interrupt) and free them on the next EG(vm_interrupt), completely moving the overhead off the main paths and be a truly generic solution.
(Not necessarily vm_interrupt, it can rather be like the delayed error handler solution.)

Obviously comes at a small tostring handler cost, but I'd really rather see overhead there...?

[prateekbhujel]

@bwoebi Yeah, that sounds like a better direction. I went with the local handler guards here because it kept the patch contained, but I agree that paying the cost at the actual __toString() reentry point would be cleaner if we can make the lifetime handling safe.

I'll look into that approach instead of growing the per-handler guards further.

[prateekbhujel]

@bwoebi I pushed a version of that direction.

It copies the active frameless call's array/string CV operands before entering __toString(), then releases those copies once the frame has moved past that opcode. That keeps the normal frameless handlers unchanged and puts the cost on the __toString() reentry path instead.

I kept the tests focused on the lifetime issue from GH-21639 and the related cases @iluuu1994 listed. I did not try to fold in the separate parse-time sibling-argument mutation case here; that would need the VM to pass copied operands into the handler, not only keep borrowed payloads alive.