Fixed bug in zend_accel_error() and cleaned up kill_all_lockers()

1. zend_accel_error was only executing clean up if log_verbosity_level is high enough to log
2. Cleaned up kill_all_lockers function and fixed comments.

diff --git a/ext/opcache/ZendAccelerator.c b/ext/opcache/ZendAccelerator.c
index f756f37a4896de86f5e886a8414f24e3a1eaffd1..38a95413cd20daee2e96640c699bb69c87cba78d 100644 (file)
--- a/ext/opcache/ZendAccelerator.c
+++ b/ext/opcache/ZendAccelerator.c
@@ -619,27 +619,42 @@ static void accel_use_shm_interned_strings(void)
 #ifndef ZEND_WIN32
 static inline void kill_all_lockers(struct flock *mem_usage_check)
 {
-       int tries = 10;
-
+       int success, tries;
        /* so that other process won't try to force while we are busy cleaning up */
        ZCSG(force_restart_time) = 0;
        while (mem_usage_check->l_pid > 0) {
+               /* Clear previous errno, reset success and tries */
+               errno = 0;
+               success = 0;
+               tries = 10;
+
                while (tries--) {
-                       zend_accel_error(ACCEL_LOG_WARNING, "Killed locker %d", mem_usage_check->l_pid);
+                       zend_accel_error(ACCEL_LOG_WARNING, "Attempting to kill locker %d", mem_usage_check->l_pid);
                        if (kill(mem_usage_check->l_pid, SIGKILL)) {
+                               if (errno == ESRCH) {
+                                       /* Process died before the signal was sent */
+                                       success = 1;
+                                       zend_accel_error(ACCEL_LOG_WARNING, "Process %d died before SIGKILL was sent", mem_usage_check->l_pid);
+                               }
                                break;
                        }
                        /* give it a chance to die */
                        usleep(20000);
                        if (kill(mem_usage_check->l_pid, 0)) {
-                               /* can't kill it */
+                               if (errno == ESRCH) {
+                                       /* successfully killed locker, process no longer exists  */
+                                       success = 1;
+                                       zend_accel_error(ACCEL_LOG_WARNING, "Killed locker %d", mem_usage_check->l_pid);
+                               }
                                break;
                        }
                        usleep(10000);
                }
-               if (!tries) {
-                       zend_accel_error(ACCEL_LOG_WARNING, "Can't kill %d after 10 tries!", mem_usage_check->l_pid);
+               if (!success) {
+                       /* errno is not ESRCH or we ran out of tries to kill the locker */
                        ZCSG(force_restart_time) = time(NULL); /* restore forced restart request */
+                       /* cannot kill the locker, bail out with error */
+                       zend_accel_error(ACCEL_LOG_ERROR, "Cannot kill process %d: %s!", mem_usage_check->l_pid, strerror(errno));
                }
 
                mem_usage_check->l_type = F_WRLCK;

diff --git a/ext/opcache/tests/log_verbosity_bug.phpt b/ext/opcache/tests/log_verbosity_bug.phpt
new file mode 100644 (file)
index 0000000..725b888
--- /dev/null
+++ b/ext/opcache/tests/log_verbosity_bug.phpt
@@ -0,0 +1,19 @@
+--TEST--
+Test ACCEL_LOG_FATAL will cause the process to die even if not logged
+--DESCRIPTION--
+This test forces the opcache to error by setting memory_comsumption very large.
+The resulting ACCEL_LOG_FATAL should cause php to die.
+The process should die regardless of the log_verbosity_level.
+--INI--
+opcache.enable=1
+opcache.enable_cli=1
+opcache.memory_consumption=999999999
+opcache.log_verbosity_level=-1
+--SKIPIF--
+<?php require_once('skipif.inc'); ?>
+--FILE--
+<?php
+var_dump("Script should fail");
+?>
+--EXPECTF--
+

diff --git a/ext/opcache/zend_accelerator_debug.c b/ext/opcache/zend_accelerator_debug.c
index 8fcc01b4d4eb03bb1eaa96c7ddf5e4f5512448cc..12f80ab79b8182a1199068fe730d55e2c302ced2 100644 (file)
--- a/ext/opcache/zend_accelerator_debug.c
+++ b/ext/opcache/zend_accelerator_debug.c
@@ -30,22 +30,20 @@
 
 void zend_accel_error(int type, const char *format, ...)
 {
-    va_list args;
+       va_list args;
        time_t timestamp;
        char *time_string;
        FILE * fLog = NULL;
 
-       if (type > ZCG(accel_directives).log_verbosity_level) {
-               return;
-       }
+       if (type <= ZCG(accel_directives).log_verbosity_level) {
 
        timestamp = time(NULL);
        time_string = asctime(localtime(&timestamp));
        time_string[24] = 0;
 
        if (!ZCG(accel_directives).error_log ||
-           !*ZCG(accel_directives).error_log ||
-           strcmp(ZCG(accel_directives).error_log, "stderr") == 0) {
+               !*ZCG(accel_directives).error_log ||
+               strcmp(ZCG(accel_directives).error_log, "stderr") == 0) {
 
                fLog = stderr;
        } else {
@@ -56,33 +54,40 @@ void zend_accel_error(int type, const char *format, ...)
        }
 
 #ifdef ZTS
-    fprintf(fLog, "%s (" ZEND_ULONG_FMT "): ", time_string, (zend_ulong)tsrm_thread_id());
+               fprintf(fLog, "%s (" ZEND_ULONG_FMT "): ", time_string, (zend_ulong)tsrm_thread_id());
 #else
-    fprintf(fLog, "%s (%d): ", time_string, getpid());
+               fprintf(fLog, "%s (%d): ", time_string, getpid());
 #endif
 
-       switch (type) {
-               case ACCEL_LOG_FATAL:
-                       fprintf(fLog, "Fatal Error ");
-                       break;
-               case ACCEL_LOG_ERROR:
-                       fprintf(fLog, "Error ");
-                       break;
-               case ACCEL_LOG_WARNING:
-                       fprintf(fLog, "Warning ");
-                       break;
-               case ACCEL_LOG_INFO:
-                       fprintf(fLog, "Message ");
-                       break;
-               case ACCEL_LOG_DEBUG:
-                       fprintf(fLog, "Debug ");
-                       break;
-       }
+               switch (type) {
+                       case ACCEL_LOG_FATAL:
+                               fprintf(fLog, "Fatal Error ");
+                               break;
+                       case ACCEL_LOG_ERROR:
+                               fprintf(fLog, "Error ");
+                               break;
+                       case ACCEL_LOG_WARNING:
+                               fprintf(fLog, "Warning ");
+                               break;
+                       case ACCEL_LOG_INFO:
+                               fprintf(fLog, "Message ");
+                               break;
+                       case ACCEL_LOG_DEBUG:
+                               fprintf(fLog, "Debug ");
+                               break;
+               }
+
+               va_start(args, format);
+               vfprintf(fLog, format, args);
+               va_end(args);
+               fprintf(fLog, "\n");
 
-    va_start(args, format);
-    vfprintf(fLog, format, args);
-    va_end(args);
-       fprintf(fLog, "\n");
+               fflush(fLog);
+               if (fLog != stderr) {
+                       fclose(fLog);
+               }
+       }
+       /* perform error handling even without logging the error */
        switch (type) {
                case ACCEL_LOG_ERROR:
                        zend_bailout();
@@ -91,8 +96,5 @@ void zend_accel_error(int type, const char *format, ...)
                        exit(-2);
                        break;
        }
-       fflush(fLog);
-       if (fLog != stderr) {
-               fclose(fLog);
-       }
+
 }