assertion failure spl_fixedarray

[YuanchengJiang]

Description

The following code:

<?php
set_error_handler('func_get_args');
function foo() {
for ($cnt = 0; $cnt < 6; $cnt++) {
$b = new SplFixedArray(1);
$b[0] = $a;
}
}
@foo();

Resulted in this output:

php: /home/phpfuzz/WorkSpace/flowfusion/php-src/ext/spl/spl_fixedarray.c:357: zend_ulong spl_offset_convert_to_ulong(const zval *): Assertion `!(((zend_executor_globals *) (((char*) _tsrm_ls_cache)+(executor_globals_offset)))->exception)' failed.
Aborted (core dumped)

To reproduce:

./php-src/sapi/cli/php  ./test.php

Commit:

ac9392b855cc4db6792a8379223e1fd1bbebd157

Configurations:

CC="clang-12" CXX="clang++-12" CFLAGS="-DZEND_VERIFY_TYPE_INFERENCE" CXXFLAGS="-DZEND_VERIFY_TYPE_INFERENCE" ./configure --enable-debug --enable-address-sanitizer --enable-undefined-sanitizer --enable-re2c-cgoto --enable-fpm --enable-litespeed --enable-phpdbg-debug --enable-zts --enable-bcmath --enable-calendar --enable-dba --enable-dl-test --enable-exif --enable-ftp --enable-gd --enable-gd-jis-conv --enable-mbstring --enable-pcntl --enable-shmop --enable-soap --enable-sockets --enable-sysvmsg --enable-zend-test --with-zlib --with-bz2 --with-curl --with-enchant --with-gettext --with-gmp --with-mhash --with-ldap --with-libedit --with-readline --with-snmp --with-sodium --with-xsl --with-zip --with-mysqli --with-pdo-mysql --with-pdo-pgsql --with-pgsql --with-sqlite3 --with-pdo-sqlite --with-webp --with-jpeg --with-freetype --enable-sigchild --with-readline --with-pcre-jit --with-iconv

Operating System:

Ubuntu 20.04 Host, Docker 0599jiangyc/flowfusion:latest

This report is automatically generated by FlowFusion

PHP Version

ac9392b

Operating System

No response

[ndossche]

This feels like a VM problem to me. The point is that after getting the zval pointer for the dimension CV or the value CV, we don't check for an exception. So that means the dimension handlers are executed regardless of there being an exception, and then the assertion in SplFixedArray fails of course.

That also means that the following code:

<?php
set_error_handler(fn ($_,$str) => throw new Error($str));
$b = [];
try {
    $b[0] = $a;
} catch(Error $e) {
    echo $e->getMessage(), "\n";
}
var_dump($b);

results in

Undefined variable $a
array(1) {
  [0]=>
  NULL
}

despite the access to $a already throwing an exception. So I expected an empty array at the end honestly.
This can be fixed in the VM, and maybe should be to be 100% correct, but then we have to add a few more checks for something that should likely never happen anyway. So I'm reluctant do that. Especially since we'll need to revise CV accesses for PHP9 anyway.
Another fix is to just comment out the assertion in SplFixedArray. The reason it was added in the first place is to avoid an exception check for the common case (the compiler was able to utilize this assertion/assume).
cc @arnaud-lb @iluuu1994

[arnaud-lb]

Interestingly the following snippet works as expected (offsetSet() is not called) because zend_call_function() returns early when EG(exception) is set:

set_error_handler(fn ($_,$str) => throw new Error($str));

$b = new class implements ArrayAccess {
    function offsetSet($name, $value): void {
        var_dump($name, $value);
    }
    function offsetGet($value): mixed {
        var_dump($value);
    }
    function offsetExists($name): bool {
        return true;
    }
    function offsetUnset($name): void {
    }
};

$b[0] = $a;

However this doesn't feel right. I agree the VM should check for exceptions here, after calling zval_undefined_cv() (which calls the error handler). The extra check should have minimal overhead for being in a slow path, and will be eliminated from non-OP_DATA_CV handlers.

Possibly we have the same issue for ASSIGN_OBJ and all op handlers that use zval_undefined_cv() before interacting with other zvals.

The right long term fix is probably #12805. IMHO we should delay error handlers until the next interrupt check, just like signals and timeouts.

[iluuu1994]

I think this is the default behavior for pretty much all handlers. E.g. GET_OP1_ZVAL_PTR() can call zval_undefined_cv(). However, it won't actually end execution, it will just use NULL (&EG(uninitialized_zval)) for the execution of the current handler. There might be other exception checks in the handler which will stop execution. Changing this might take quite a bit of refactoring, given that many get_zval_ptr functions are affected. They are forcefully inlined, so converting them to macros that can return from the caller using HANDLE_EXCEPTION() might be an option. This should only slightly increase assembly size, namely by inserting returns for these error paths.

The right long term fix is probably #12805. IMHO we should delay error handlers until the next interrupt check, just like signals and timeouts.

I think this will be the wrong solution again once we promote access on undeclared variables to errors.

[iluuu1994]

I just realized this won't work well either, because we very often need to clean up on exit, e.g. FREE_OP1();. An option would be to make the get_zval_ptr functions that potentially operate on CVs macros, and add a {} block that injects some cleanup code into the zval_undefined_cv() branch, but this means a lot of code churn. The alternative would be to just accept execution can continue for the remainder of the handler, with null used as a value. This doesn't matter much for some handlers, but more for others. E.g. $object + $undef will call do_operation() with null.

I'm not sure yet what the best approach is.

[arnaud-lb]

We could move the undefined check to a separate opcode, emit the opcode when a CV is accessed, and rely on the optimizer to remove the unnecessary ones (when we can determine that a CV is always defined before a use).

This may have a performance impact, but if most uses can be determined to be defined, this may be positive.

[ndossche]

We could move the undefined check to a separate opcode

@Girgias got this idea before and talked to me about that briefly. I was skeptical because it's an extra dispatch and handler overhead. If the optimizer can eliminate most of them then this can be positive indeed, but I don't know.

Anyway, I'm leaning towards just fixing this in SPL (by refactoring the code in such a way we don't need the exception assertion as an optimization hint anyway).

[iluuu1994]

This should indeed work well with opcache. I wonder if we can have very simple liveliness tracking during compilation to avoid a penalty without opcache. This shouldn't be too hard.

[Girgias]

I would imagine that any function body would be able to elide them at compile time, as it seems very unlikely that an undefined variable is used in such a body.

The annoying thing for global variables is that it is possible to unset them via $GLOBALS https://3v4l.org/JtuOa

[iluuu1994]

Main functions will need special care in general due to access to globals in error handlers. They can probably compile to FETCH_R instead if we want to drop the check from all other handlers.