Skip to content

Commit 678cf76

Browse files
TimWollaTimWolla
committed
opcache: Do not disable userland error handlers in opcache_compile_file()
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 php#17422.
1 parent c4b678f commit 678cf76

File tree

9 files changed

+186
-4
lines changed

9 files changed

+186
-4
lines changed

Diff for: ‎ext/opcache/ZendAccelerator.c

-4
Original file line numberDiff line numberDiff line change
@@ -1728,7 +1728,6 @@ static zend_persistent_script *opcache_compile_file(zend_file_handle *file_handl
17281728
zend_persistent_script *new_persistent_script;
17291729
uint32_t orig_functions_count, orig_class_count;
17301730
zend_op_array *orig_active_op_array;
1731-
zval orig_user_error_handler;
17321731
zend_op_array *op_array;
17331732
bool do_bailout = false;
17341733
accel_time_t timestamp = 0;
@@ -1796,10 +1795,8 @@ static zend_persistent_script *opcache_compile_file(zend_file_handle *file_handl
17961795
orig_active_op_array = CG(active_op_array);
17971796
orig_functions_count = EG(function_table)->nNumUsed;
17981797
orig_class_count = EG(class_table)->nNumUsed;
1799-
ZVAL_COPY_VALUE(&orig_user_error_handler, &EG(user_error_handler));
18001798

18011799
/* Override them with ours */
1802-
ZVAL_UNDEF(&EG(user_error_handler));
18031800
if (ZCG(accel_directives).record_warnings) {
18041801
zend_begin_record_errors();
18051802
}
@@ -1825,7 +1822,6 @@ static zend_persistent_script *opcache_compile_file(zend_file_handle *file_handl
18251822

18261823
/* Restore originals */
18271824
CG(active_op_array) = orig_active_op_array;
1828-
EG(user_error_handler) = orig_user_error_handler;
18291825
EG(record_errors) = 0;
18301826

18311827
if (!op_array) {

Diff for: ‎ext/opcache/tests/gh17422/001.phpt

+32
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
--TEST--
2+
GH-17422 (OPcache bypasses the user-defined error handler for deprecations)
3+
--INI--
4+
opcache.enable=1
5+
opcache.enable_cli=1
6+
--EXTENSIONS--
7+
opcache
8+
--FILE--
9+
<?php
10+
11+
require __DIR__ . "/shutdown.inc";
12+
13+
set_error_handler(static function (int $errno, string $errstr, string $errfile, int $errline) {
14+
echo "set_error_handler: {$errstr}", PHP_EOL;
15+
});
16+
17+
require __DIR__ . "/warning.inc";
18+
19+
warning();
20+
21+
?>
22+
--EXPECT--
23+
set_error_handler: "continue" targeting switch is equivalent to "break"
24+
OK: warning
25+
array(3) {
26+
[0]=>
27+
string(7) "001.php"
28+
[1]=>
29+
string(12) "shutdown.inc"
30+
[2]=>
31+
string(11) "warning.inc"
32+
}

Diff for: ‎ext/opcache/tests/gh17422/002.phpt

+36
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
--TEST--
2+
GH-17422 (OPcache bypasses the user-defined error handler for deprecations) - Throwing error handler
3+
--INI--
4+
opcache.enable=1
5+
opcache.enable_cli=1
6+
--EXTENSIONS--
7+
opcache
8+
--FILE--
9+
<?php
10+
11+
require __DIR__ . "/shutdown.inc";
12+
13+
set_error_handler(static function (int $errno, string $errstr, string $errfile, int $errline) {
14+
throw new \ErrorException($errstr, 0, $errno, $errfile, $errline);
15+
});
16+
17+
try {
18+
require __DIR__ . "/warning.inc";
19+
} catch (\Exception $e) {
20+
echo "Caught: ", $e->getMessage(), PHP_EOL;
21+
}
22+
23+
warning();
24+
25+
?>
26+
--EXPECT--
27+
Caught: "continue" targeting switch is equivalent to "break"
28+
OK: warning
29+
array(3) {
30+
[0]=>
31+
string(7) "002.php"
32+
[1]=>
33+
string(12) "shutdown.inc"
34+
[2]=>
35+
string(11) "warning.inc"
36+
}

Diff for: ‎ext/opcache/tests/gh17422/003.phpt

+30
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
--TEST--
2+
GH-17422 (OPcache bypasses the user-defined error handler for deprecations) - Fatal Error
3+
--INI--
4+
opcache.enable=1
5+
opcache.enable_cli=1
6+
memory_limit=2M
7+
--EXTENSIONS--
8+
opcache
9+
--FILE--
10+
<?php
11+
12+
require __DIR__ . "/shutdown.inc";
13+
14+
set_error_handler(static function (int $errno, string $errstr, string $errfile, int $errline) {
15+
str_repeat('x', 1024 * 1024 * 1024);
16+
});
17+
18+
require __DIR__ . "/warning.inc";
19+
20+
warning();
21+
22+
?>
23+
--EXPECTF--
24+
Fatal error: Allowed memory size of 2097152 bytes exhausted %s on line 6
25+
array(2) {
26+
[0]=>
27+
string(7) "003.php"
28+
[1]=>
29+
string(12) "shutdown.inc"
30+
}

Diff for: ‎ext/opcache/tests/gh17422/004.phpt

+36
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
--TEST--
2+
GH-17422 (OPcache bypasses the user-defined error handler for deprecations) - eval
3+
--INI--
4+
opcache.enable=1
5+
opcache.enable_cli=1
6+
memory_limit=2M
7+
--EXTENSIONS--
8+
opcache
9+
--FILE--
10+
<?php
11+
12+
require __DIR__ . "/shutdown.inc";
13+
14+
set_error_handler(static function (int $errno, string $errstr, string $errfile, int $errline) {
15+
eval(
16+
<<<'PHP'
17+
function warning() {
18+
echo "NOK", PHP_EOL;
19+
}
20+
PHP
21+
);
22+
});
23+
24+
require __DIR__ . "/warning.inc";
25+
26+
warning();
27+
28+
?>
29+
--EXPECTF--
30+
Fatal error: Cannot redeclare function warning() (previously declared in %s(8) : eval()'d code:1) in %swarning.inc on line 2
31+
array(2) {
32+
[0]=>
33+
string(7) "004.php"
34+
[1]=>
35+
string(12) "shutdown.inc"
36+
}

Diff for: ‎ext/opcache/tests/gh17422/005.phpt

+34
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
--TEST--
2+
GH-17422 (OPcache bypasses the user-defined error handler for deprecations) - require
3+
--INI--
4+
opcache.enable=1
5+
opcache.enable_cli=1
6+
memory_limit=2M
7+
--EXTENSIONS--
8+
opcache
9+
--FILE--
10+
<?php
11+
12+
require __DIR__ . "/shutdown.inc";
13+
14+
set_error_handler(static function (int $errno, string $errstr, string $errfile, int $errline) {
15+
require_once __DIR__ . "/dummy.inc";
16+
});
17+
18+
require __DIR__ . "/warning.inc";
19+
20+
dummy();
21+
22+
?>
23+
--EXPECT--
24+
OK: dummy
25+
array(4) {
26+
[0]=>
27+
string(7) "005.php"
28+
[1]=>
29+
string(9) "dummy.inc"
30+
[2]=>
31+
string(12) "shutdown.inc"
32+
[3]=>
33+
string(11) "warning.inc"
34+
}

Diff for: ‎ext/opcache/tests/gh17422/dummy.inc

+4
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
<?php
2+
function dummy() {
3+
echo "OK: ", __FUNCTION__, PHP_EOL;
4+
}

Diff for: ‎ext/opcache/tests/gh17422/shutdown.inc

+6
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
<?php
2+
register_shutdown_function(static function() {
3+
$scripts = array_map(basename(...), array_keys(opcache_get_status()['scripts'] ?? []));
4+
sort($scripts);
5+
var_dump($scripts);
6+
});

Diff for: ‎ext/opcache/tests/gh17422/warning.inc

+8
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
<?php
2+
function warning() {
3+
switch (1) {
4+
case 1:
5+
echo "OK: ", __FUNCTION__, PHP_EOL;
6+
continue;
7+
}
8+
}

0 commit comments

Comments
 (0)