Repository navigation
Named arguments in CTE functions cause a segfault #10801
Description
Description
The following code:
<?php
$h0 = hash("murmur3a", "Two hashes meet in a bar.", options: ["seed" => 2345]);and hash function marked as CTE (like in https://github.com/php/php-src/blob/f16c3e1265/ext/hash/hash.stub.php#L12)
Resulted in this output:
========DIFF========
95855f9be0db784a5c37e878c4a4dcee
002+ Segmentation fault (core dumped)
003+
004+ Termsig=11
002- 95855f9be0db784a5c37e878c4a4dcee
003- f64c9eb40287fa686575163893e283b2
004- f64c9eb40287fa686575163893e283b2
005- 7f7ec59b
006- 7f7ec59b
========DONE========
FAIL Hash: MurmurHash3 seed [ext/hash/tests/murmurhash3_seed.phpt]
But I expected this output instead:
discovered in #10771, based on #10771 (comment) hash will be not supported as CTE, but I belive the segfault should be investigated, as it can be used from an extension on compile time for example
PHP Version
master
Operating System
any
Activity
ndossche commentedon Mar 7, 2023
I can confirm that this crashes. In fact, any compile-time-evaluated function where the programmer uses named arguments will crash because
EX(call)is NULL.For example, even something like:
will crash after marking that function as CTE (like you did in your recent PR).
And it is even triggerable with functions which are CTE right now already:
This definitely needs fixing, I'll do that tonight.
[-]`hash` as CTE leads to segfault[/-][+]Named arguments in CTE functions cause a segfault (was: `hash` as CTE leads to segfault)[/+][-]Named arguments in CTE functions cause a segfault (was: `hash` as CTE leads to segfault)[/-][+]Named arguments in CTE functions cause a segfault[/+]ndossche commentedon Mar 7, 2023
I edited the title so the root cause is in the title. Others can then also more easily find these issues.
Old title was: "
hashas CTE leads to segfault"ndossche commentedon Mar 7, 2023
Interesting bug. I can't always reproduce it with the simple reproducer I gave.
It's actually not during the constant evaluation that it crashes, it's during script execution but only if the constant evaluation code ran.
Furthermore, I found 2 other weird behaviours.
If you use ASAN, you'll find that the following works correctly without opcache:
but if you enable opcache: it crashes with a segfault on an invalid pointer. I added a printf to show what types of zvals get destroyed and I see that the crash happens when a zval of type "32" gets destroyed. This is during execution and is not a valid type. It tries to destroy a reference which of course fails and crashes... It appears this is the argument sent to print_r which gets destroyed due to an exception. And the argument probably happens because the zval is invalid... It this destruction were not to happen we get the EX(call) failure from earlier again. Probably also caused by the invalid zval in the first place.
Also worth noting, if I first assign the return value of
array_keysto a CV then the crashes do not happen.If I change the
print_rin the above code fragment tovar_dump, I get a different error instead (but no ASAN failure):I'm out of time for today so I'll have to resume some other time.
ndossche commentedon Mar 8, 2023
It's an optimizer bug in
remove_callin sccp. Looking at the SSA it removes too fewSEND_VALoplines. That's becausenum_argsequals 1 in the reproducer I gave in my previous post. If I don't use named arguments then thenum_argsvariable equals 3 as I (and the optimizer) had expected. So perhaps thenum_argsvariable just simply doesn't count named arguments, which could very well be intended (I don't know yet).A simple fix I just tried is to change the loop condition to check the opcode nr instead.
I'll try to figure out now if the
num_argsbehaviour is expected...EDIT: according to https://wiki.php.net/rfc/named_params this is expected. So I'll proceed with my opcode nr fix, unless I find something smart
ndossche commentedon Mar 8, 2023
So I fixed the crash and random fatal error bug locally, but I found another bug in the process.
It appears the named arguments are actually ignored for the constant evaluation call. This causes the hash to differ between regular execution vs CTE. I'll check that one as well...
ndossche commentedon Mar 8, 2023
Adding support for named parameters in CTE is not trivial, requires tens of lines of code extra instead of the few lines I had to change for the crash fix. So therefore I think that would be master-only.
For now, I'll fix the crash (still useful for variadic functions), and disable the CTE for named parameters on stable branches as it doesn't work properly (results are wrong or it crashes). Note however that named parameters in the function declaration order will still work because internally it'll use the regular argument passing mechanism it seems.
Fix GH-10801: Named arguments in CTE functions cause a segfault
Merge branch 'PHP-8.1' into PHP-8.2
Merge branch 'PHP-8.2'