opcache: Do not disable userland error handlers in opcache_compile_file()

[TimWolla]

The logic to disable userland error handlers existed since the first commit including OPcache in php-src. The reason unfortunately is not explained. No existing tests require that userland error handlers do not trigger in opcache_compile_file(). The newly added tests are intended to exercise all possible kinds of edge-cases that might arise and cause issues (e.g. throwing Exceptions, performing a bailout, or loading a different file from within the error handler).

They all pass for both --repeat 1 and --repeat 2, as well as with OPcache enabled and without OPcache enabled (in the latter case with the exception of the opcache_get_status() verification). The list of cached files at the end of execution also matches my expectations.

Therefore it seems that disabling the userland error handler when compiling a file with OPcache is not (or at least no longer) necessary.

Fixes #17422.

[iluuu1994]

It might be there to 1. avoid running code that may include more files while shm is locked (which will just be compiled and discarded) and 2. keeping the lock open for a long time, during the execution of handlers.

Also, I think you would need to set EG(record_errors) to false to avoid recording warnings thrown in the handler itself and being replayed when including the file in subsequent requests.

Edit: I'm also not sure if it's possible to run into issues when relying on functions and classes declared in the compiling file. E.g. when calling them, you might be priming your cache slot, but then they will be relocated when persisting them to shm.

[arnaud-lb]

Error handlers executed during the compilation of a file that's going to be persisted could create dangling references to classes/functions declared by the file (e.g. class instances, closures, generators, fci/fcc). These are on the heap at this point, but will be moved to SHM, so references to them would be dangling after persistence:

// test.php

set_error_handler(function ($code, $msg) use (&$f) {
    $f = f(...);
});

opcache_compile_file(__DIR__.'/b.php');

$f(); // SIGSEGV

// b.php

function f($x){}
function deprecated($a = 1, $b){}

ZEND_COMPILE_WITHOUT_EXECUTION makes this less likely for classes, but not impossible as classes are still added to the class_table under a rtd key.

Also, classes/functions are mutable when they are not persisted yet, so they could be changed by user code. E.g. linking changes the zend_class_entry in-place.

An other issue is that opcache_compile_file() changes CG(compiler_options), which would be inherited by any file compiled in the error handler.

I think that delaying/recording errors is fine, but we need to fix how they are replayed.

[dstogov]

I guess, user error handlers were initially disabled to prevent different behavior on first and following (restoring from cache) requests. After error recording, this shouldn't be a problem any more, but @arnaud-lb showed that the behavior in the user error handlers may be not exactly the same. Even worse, error handlers may be exploited to do something destructive.

I would prefer not to merge this, or at least target this to master only.

[TimWolla]

Thank you for your review and pointing out how my tests were insufficient to prove that enabling userland error handlers is unsafe.

I think that delaying/recording errors is fine, but we need to fix how they are replayed.

Do you mean that userland error handlers should be disabled while actually loading the file, error recorded and then userland error handlers re-enabled and the errors replayed (as would happen for follow-up requests)?

It should probably be safe to do that after efree(op_array); /* we have valid persistent_script, so it's safe to free op_array */? I would try to adjust the patch to do that then.