Fix GH-21639: Protect frameless call args

[prateekbhujel]

Fixes GH-21639.

Frameless calls normally borrow operand zvals for the optimized handler call. That is unsafe for handlers that can re-enter userland while parsing or converting arguments, because the original variables can be overwritten before the handler is done with those operands.

This patch marks those frameless handlers with ZEND_FLF_ARG_COPY in the generated frameless metadata. The VM copies operands only for marked handlers before entering the handler, so unaffected frameless calls keep the existing borrowed-argument fast path.

The JIT direct frameless emission is skipped for marked calls, so those calls use the safe VM handler path instead of emitting a direct call with borrowed operands.

The regression tests cover implode, in_array, strtr, str_replace, min/max, and preg_replace.

Tests run:

./configure --disable-all --enable-cli --enable-opcache --enable-debug --without-pear --enable-address-sanitizer
make
sapi/cli/php run-tests.php -q Zend/tests/gh21639.phpt ext/pcre/tests/gh21639.phpt ext/standard/tests/strings/implode_basic.phpt ext/standard/tests/strings/implode_variation.phpt ext/standard/tests/strings/join_basic.phpt ext/pcre/tests/preg_replace.phpt Zend/tests/frameless_undefined_var.phpt ext/pcre/tests/preg_match_frameless_leak.phpt ext/pcre/tests/preg_match_basic.phpt ext/standard/tests/array/in_array_variation1.phpt ext/standard/tests/strings/str_replace_basic.phpt ext/standard/tests/strings/strtr_basic.phpt

Also smoke-tested the affected calls with opcache JIT enabled.

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

Good point on avoiding unnecessary copies. I don't think checking only the current arg type is enough for this bug though.

The original implode($separator, $pieces) case is the awkward part: $separator is a string and $pieces is an array, but an object inside $pieces can re-enter userland and overwrite both variables while the handler is still using the borrowed operands. class_exists($class) is another non-object top-level case, because autoload can re-enter userland.

So the decision is really about whether the handler can re-enter userland while it still has borrowed operands, not just whether a particular top-level operand is an object. I agree SSA/JIT type info could be used to keep more direct frameless calls when it can prove there is no object/array-of-object path. I avoided doing that in the generic VM handler because it does not have that type info cheaply, and a runtime recursive array scan would likely cost more than the refcount bump this is trying to avoid.

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

Yeah, fair enough. That overhead is worse than I expected.

I'll stop pushing this direction then. If this comes back later, it probably needs a more targeted fix that doesn't touch the hot frameless path.