Fix GH-21639: Guard frameless handler reentry

[prateekbhujel]

Fixes GH-21639.

Frameless handlers borrow their arguments from the caller frame. That is fine while the handler stays in C, but it becomes unsafe when the handler can re-enter userland before it is done with another borrowed argument. The original zval can be overwritten during string conversion, loose comparison, or array element conversion.

This keeps the frameless VM/JIT dispatch path unchanged. The guards are local to the handler paths that can re-enter:

• parse-time string conversions protect only the later sibling arguments they still need
• implode() and preg_replace() protect arrays/values only around element conversions
• in_array(), min(), and max() keep their scalar fast paths and only pin arguments before the zend_compare() fallback

Tests run:

make -j"$(sysctl -n hw.ncpu)"
git diff --check
TEST_PHP_JOBS=1 sapi/cli/php run-tests.php -q Zend/tests/gh21639.phpt Zend/tests/gh21639_parse_time.phpt ext/pcre/tests/gh21639.phpt Zend/tests/frameless_undefined_var.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

[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.

[mvorisek]

This has high overhead even if not needed - type analysis should be used, when the arg is non-object, copying/protecting the argument is useless.

[prateekbhujel]

Agreed. I ended up dropping this VM-level copy path entirely.

The current branch keeps the normal frameless dispatch/JIT path untouched and only pins arguments inside the handler paths that can actually re-enter, such as parse-time sibling conversion, array element string conversion, and the zend_compare() fallback. So this overhead should no longer apply to the updated patch.

[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...?