Fix bug #76895

[nikic]

There are two parts here: First, opcache should not be blocking signals while invoking compile_file. This both means that signals may be blocked for a very long time (compile_file may execute PHP code) and that we will not unblock signals if a bailout occurs, which happens on any compile error.

Second, we should have noticed this issue a long time ago, but apparently we don't use zend_signal_deactivate as part of the standard shutdown sequence. In addition to calling deactivate, also make sure that we always check for zero depth, independent of SIGG(check), which is currently not enabled.

Fixes bug #76895.

[nikic]

@dstogov Can you please check this?

[plmnikulin]

Could not comment the code, just tested in my environment with daf0c95 and b62c6e5 instead of #4460 patch.

In addition to usual

[24-Jul-2019 11:18:49] WARNING: [pool php72.test] child 19034 said into stderr: "NOTICE: PHP message: PHP Fatal error:  'break' not in the 'loop' or 'switch' context in /var/www/vhosts/php72.test/httpdocs/bad.php on line 7"

a new line appears in logs while processing request to the buggy file

[24-Jul-2019 11:18:49] WARNING: [pool php72.test] child 19034 said into stderr: "NOTICE: PHP message: PHP Warning:  zend_signal: shutdown with non-zero blocking depth (1) in Unknown on line 0"

Reload does not stuck any more.

[nikic]

a new line appears in logs while processing request to the buggy file

[24-Jul-2019 11:18:49] WARNING: [pool php72.test] child 19034 said into stderr: "NOTICE: PHP message: PHP Warning:  zend_signal: shutdown with non-zero blocking depth (1) in Unknown on line 0"

Hm, this part isn't good. That means that some codepath can still leave signals blocked, which the opcache change was intended to avoid (and did for the compilation error case in my tests).

[plmnikulin]

I hope, that build was without other patches, so SIGTERM was not blocked. Reload a couple of seconds later completed with no problem.

[dstogov]

I think, it's not a good idea to add several syscalls on each request. May be enable this only in DEBUG build?

[nikic]

Maybe just drop the SIGNAL_BEGIN_CRITICAL in zend_signal_deactivate? Then there shouldn't be any syscalls, and I don't see a reason why this needs to block signals during the reset, as long as active=0 is set first.

[dstogov]

agree

[dstogov]

@nikic This looks fine, except for overhead of zend_signal_deactivate().

[nikic]

@dstogov I've pushed a few additional changes to zend_signal_deactivate. The use of a critical section is removed, the signal queue is cleaned up and SIGG(check) is now enabled based on ZEND_DEBUG.

[dstogov]

looks fine

[nikic]

Merged as 38f1288 into 7.2.

[plmnikulin]

Thank you for the fix, I do not see additional errors mentioned above (in #4461 (comment)) any more.

I have tried a test for this bug #76895 with changes to tester.inc prepared by @bukka and suggested in #4471 (comment) (discussed earlier in bug #76601). Unfortunately at least for an out of source build directory and tests prior to make install step, I have to put absolute path to opcache.so in my build directory. I have no idea how to get build directory from the test script. Environment is clean, working directory is the top of the source tree. If there is a way to overcome the difficulty with extension load path, the test could be used for the bug #76895 and the test passes with changes introduced in this pull request.