Fix GH-23725: use-after-free when __toString() frees a frameless argument

[iliaal]

Frameless calls hand the handler the caller's operand zvals without taking a reference, so an argument freed by user code the handler triggers leaves it reading freed memory. Array arguments are now pinned in the Z_FLF_PARAM_ARRAY macros, replacing the per-function guards from 8ce7f7f as suggested on #23221, which this supersedes. String arguments are pinned only when user code can still run after they are read, and str_replace()/preg_replace() skip pinning when all three arguments are strings.

Fixes #23725

[arnaud-lb]

This looks good to me in general: With this change, frameless functions can have the same expectations about their arguments than normal ones. But I would like to see @iluuu1994's opinion on this.

so #21639 stays open

Is this correct? I believe that this branch fixes that issue as well

Z_FLF_PARAM_STR still reads its string directly on the fast path

Unless something is blocking, I think that we should update Z_FLF_PARAM_STR as well. Test case:

$a = 'foo' . time();
$b = new class {
    function __toString() {
        global $a;
        $a[0] = '!';
        return '0';
    }
};
var_dump(strtr($a, 'o', $b)); // expected: f00..., actual: !00

[iluuu1994]

Wordpress i-count goes up by 0.44%, that seems like more than just noise... The reason I was hesitant with these fixes is that frameless calls have a modest performance improvement to begin with. Full correctness for artificial code might make the benefit negligible. I don't know how else to address this, the fix does look technically reasonable, but I'm not sure is worth it. At least not considering how many other memory issues can be triggered with malicious code.

The other option would be to disallow CVs and always send a copy using QM_ASSIGN. At least then we can drop all the checks, including the string checks. I don't know if that's actually faster, somewhat doubtful given the additional VM overhead. I don't have any other ideas.

[iliaal]

Agreed, almost .5% regression for a fix is too much, I am not sure the alternative you suggested would be faster. A little frustrating TBH, needs more thought, or just won't fix

[iliaal]

Reworked: str_replace() and preg_replace() skip pinning when all three arguments are strings, the helper-level array pins from 8ce7f7f are gone because the frameless handlers pin the arrays themselves, and an argument that is already a string is pinned only when a later argument needs conversion or the handler can run user code afterwards, which covers the strtr() case too. Locally that takes WordPress from +0.46% to -0.07% against the merge base; the CI benchmark will confirm.

[ndossche]

I have a completely other idea that may generalize nicer.

Instead of trying to protect against malicious code: detect when a malicious change happened and tell the programmer to f- off. We can add that detection code to the slow path so that it probably doesn't affect real-world code; and if it affects such code anyway then the code was already slow.

The PoC linked here is a bit specific to frameless+__toString but may be generalised. Maybe this is a bad idea, but maybe this is a way to go forward with this, or maybe it brings inspiration to fix these classes of issues (I think the same conceptual idea can work for error handlers).

PoC: https://gist.github.com/ndossche/ecba0a86b9f284a80382d064318a5e8f