Fix GH-21639: Keep frameless CV args alive during __toString()

[prateekbhujel]

Fixes GH-21639.

Frameless internal calls can borrow CV operands from the caller frame. If one of those operands is converted through __toString(), userland can overwrite the original CV while the handler is still using the borrowed value.

This protects that case from the reentry side. Before calling __toString(), the engine checks whether the current frame is executing a frameless call and keeps copies of any string/array CV operands for that opcode. The copies are released when the frameless handler returns; executor shutdown still force-cleans anything left during unwinding.

That keeps the copy/dtor work off ordinary frameless calls. The normal path only has the cold pending-copy cleanup check after a frameless handler returns, including the JIT frameless path.

Tests run:

make -j"$(sysctl -n hw.ncpu)"
git diff --check
sapi/cli/php run-tests.php -q -d zend_extension=/private/tmp/php-src-reentry-fix/modules/opcache.so -d opcache.enable_cli=1 -d opcache.jit=tracing -d opcache.protect_memory=1 -d opcache.jit_buffer_size=64M ext/standard/tests/strings/bug22224.phpt ext/standard/tests/strings/implode_variation.phpt Zend/tests/gh21639.phpt ext/pcre/tests/gh21639.phpt
sapi/cli/php run-tests.php -q -d opcache.jit=disable -d opcache.protect_memory=1 -d opcache.jit_buffer_size=64M ext/standard/tests/strings/bug22224.phpt ext/standard/tests/strings/implode_variation.phpt Zend/tests/gh21639.phpt ext/pcre/tests/gh21639.phpt
sapi/cli/php run-tests.php -q -d zend_extension=/private/tmp/php-src-reentry-fix/modules/opcache.so -d opcache.enable_cli=1 -d opcache.jit=tracing -d opcache.protect_memory=1 -d opcache.jit_buffer_size=64M 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/bug22224.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.

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

[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, agreed. That cost belongs on the __toString() side, not on every frameless call.

I pushed a follow-up in that direction. It checks from __toString() whether the parent frame is currently on a frameless opcode, then keeps copies of the string/array CV operands for that opcode.

I did not keep the cleanup on EG(vm_interrupt): in the failing implode() cases it can run before the parent frameless handler has returned, so the parent frame still looks like it is sitting on the same frameless opline and the cleanup can keep treating the copies as live. I moved the release to the frameless handler return instead, with the same cleanup point for JIT frameless calls.

So the copy/dtor work is on actual __toString() reentry. The ordinary frameless path only has the cold pending-copy check after the handler returns, which seemed like the safer version of this approach.