From f20e11cbe12057bddade6ff9a2ff9ea1bb6084c9 Mon Sep 17 00:00:00 2001 From: Arnaud Le Blanc Date: Sun, 3 Apr 2022 13:16:33 +0200 Subject: [PATCH 1/2] Clear recorded errors before executing shutdown functions Recorded errors may be attached to the wrong cached script when a fatal error occurs during recording. This happens because the fatal error will cause a bailout, which may prevent the recorded errors from being freed. If an other script is compiled after bailout, or if a class is linked after bailout, the recorded errors will be attached to it. This change fixes this by freeing recorded errors before executing shutdown functions. Fixes GH-8063 --- Zend/zend_inheritance.c | 207 ++++++++++++++----------- ext/opcache/tests/gh8063-001.phpt | 31 ++++ ext/opcache/tests/gh8063-002.phpt | 31 ++++ ext/opcache/tests/gh8063-003.phpt | 30 ++++ ext/opcache/tests/gh8063/BadClass.inc | 8 + ext/opcache/tests/gh8063/BadClass2.inc | 15 ++ ext/opcache/tests/gh8063/Bar.inc | 3 + ext/opcache/tests/gh8063/Baz.inc | 3 + ext/opcache/tests/gh8063/Foo.inc | 8 + 9 files changed, 242 insertions(+), 94 deletions(-) create mode 100644 ext/opcache/tests/gh8063-001.phpt create mode 100644 ext/opcache/tests/gh8063-002.phpt create mode 100644 ext/opcache/tests/gh8063-003.phpt create mode 100644 ext/opcache/tests/gh8063/BadClass.inc create mode 100644 ext/opcache/tests/gh8063/BadClass2.inc create mode 100644 ext/opcache/tests/gh8063/Bar.inc create mode 100644 ext/opcache/tests/gh8063/Baz.inc create mode 100644 ext/opcache/tests/gh8063/Foo.inc diff --git a/Zend/zend_inheritance.c b/Zend/zend_inheritance.c index 020c5e6553920..45b284d0237b8 100644 --- a/Zend/zend_inheritance.c +++ b/Zend/zend_inheritance.c @@ -2775,101 +2775,113 @@ ZEND_API zend_class_entry *zend_do_link_class(zend_class_entry *ce, zend_string #endif bool orig_record_errors = EG(record_errors); - if (ce->ce_flags & ZEND_ACC_IMMUTABLE) { - if (is_cacheable) { - if (zend_inheritance_cache_get && zend_inheritance_cache_add) { - zend_class_entry *ret = zend_inheritance_cache_get(ce, parent, traits_and_interfaces); - if (ret) { - if (traits_and_interfaces) { - free_alloca(traits_and_interfaces, use_heap); - } - zv = zend_hash_find_known_hash(CG(class_table), key); - Z_CE_P(zv) = ret; - return ret; - } - /* Make sure warnings (such as deprecations) thrown during inheritance - * will be recoreded in the inheritance cache. */ - zend_begin_record_errors(); - } else { - is_cacheable = 0; + if (ce->ce_flags & ZEND_ACC_IMMUTABLE && is_cacheable) { + if (zend_inheritance_cache_get && zend_inheritance_cache_add) { + zend_class_entry *ret = zend_inheritance_cache_get(ce, parent, traits_and_interfaces); + if (ret) { + if (traits_and_interfaces) { + free_alloca(traits_and_interfaces, use_heap); + } + zv = zend_hash_find_known_hash(CG(class_table), key); + Z_CE_P(zv) = ret; + return ret; } - proto = ce; + + /* Make sure warnings (such as deprecations) thrown during inheritance + * will be recorded in the inheritance cache. */ + zend_begin_record_errors(); + } else { + is_cacheable = 0; } - /* Lazy class loading */ - ce = zend_lazy_class_load(ce); - zv = zend_hash_find_known_hash(CG(class_table), key); - Z_CE_P(zv) = ce; - } else if (ce->ce_flags & ZEND_ACC_FILE_CACHED) { - /* Lazy class loading */ - ce = zend_lazy_class_load(ce); - ce->ce_flags &= ~ZEND_ACC_FILE_CACHED; - zv = zend_hash_find_known_hash(CG(class_table), key); - Z_CE_P(zv) = ce; + proto = ce; } - if (CG(unlinked_uses)) { - zend_hash_index_del(CG(unlinked_uses), (zend_long)(zend_uintptr_t) ce); - } + zend_try { + if (ce->ce_flags & ZEND_ACC_IMMUTABLE) { + /* Lazy class loading */ + ce = zend_lazy_class_load(ce); + zv = zend_hash_find_known_hash(CG(class_table), key); + Z_CE_P(zv) = ce; + } else if (ce->ce_flags & ZEND_ACC_FILE_CACHED) { + /* Lazy class loading */ + ce = zend_lazy_class_load(ce); + ce->ce_flags &= ~ZEND_ACC_FILE_CACHED; + zv = zend_hash_find_known_hash(CG(class_table), key); + Z_CE_P(zv) = ce; + } - orig_linking_class = CG(current_linking_class); - CG(current_linking_class) = is_cacheable ? ce : NULL; + if (CG(unlinked_uses)) { + zend_hash_index_del(CG(unlinked_uses), (zend_long)(zend_uintptr_t) ce); + } - if (ce->ce_flags & ZEND_ACC_ENUM) { - /* Only register builtin enum methods during inheritance to avoid persisting them in - * opcache. */ - zend_enum_register_funcs(ce); - } + orig_linking_class = CG(current_linking_class); + CG(current_linking_class) = is_cacheable ? ce : NULL; - if (parent) { - if (!(parent->ce_flags & ZEND_ACC_LINKED)) { - add_dependency_obligation(ce, parent); + if (ce->ce_flags & ZEND_ACC_ENUM) { + /* Only register builtin enum methods during inheritance to avoid persisting them in + * opcache. */ + zend_enum_register_funcs(ce); } - zend_do_inheritance(ce, parent); - } - if (ce->num_traits) { - zend_do_bind_traits(ce, traits_and_interfaces); - } - if (ce->num_interfaces) { - /* Also copy the parent interfaces here, so we don't need to reallocate later. */ - uint32_t num_parent_interfaces = parent ? parent->num_interfaces : 0; - zend_class_entry **interfaces = emalloc( - sizeof(zend_class_entry *) * (ce->num_interfaces + num_parent_interfaces)); - if (num_parent_interfaces) { - memcpy(interfaces, parent->interfaces, - sizeof(zend_class_entry *) * num_parent_interfaces); + if (parent) { + if (!(parent->ce_flags & ZEND_ACC_LINKED)) { + add_dependency_obligation(ce, parent); + } + zend_do_inheritance(ce, parent); + } + if (ce->num_traits) { + zend_do_bind_traits(ce, traits_and_interfaces); } - memcpy(interfaces + num_parent_interfaces, traits_and_interfaces + ce->num_traits, - sizeof(zend_class_entry *) * ce->num_interfaces); + if (ce->num_interfaces) { + /* Also copy the parent interfaces here, so we don't need to reallocate later. */ + uint32_t num_parent_interfaces = parent ? parent->num_interfaces : 0; + zend_class_entry **interfaces = emalloc( + sizeof(zend_class_entry *) * (ce->num_interfaces + num_parent_interfaces)); - zend_do_implement_interfaces(ce, interfaces); - } else if (parent && parent->num_interfaces) { - zend_do_inherit_interfaces(ce, parent); - } - if (!(ce->ce_flags & (ZEND_ACC_INTERFACE|ZEND_ACC_TRAIT)) - && (ce->ce_flags & (ZEND_ACC_IMPLICIT_ABSTRACT_CLASS|ZEND_ACC_EXPLICIT_ABSTRACT_CLASS)) - ) { - zend_verify_abstract_class(ce); - } - if (ce->ce_flags & ZEND_ACC_ENUM) { - zend_verify_enum(ce); - } + if (num_parent_interfaces) { + memcpy(interfaces, parent->interfaces, + sizeof(zend_class_entry *) * num_parent_interfaces); + } + memcpy(interfaces + num_parent_interfaces, traits_and_interfaces + ce->num_traits, + sizeof(zend_class_entry *) * ce->num_interfaces); - /* Normally Stringable is added during compilation. However, if it is imported from a trait, - * we need to explicilty add the interface here. */ - if (ce->__tostring && !(ce->ce_flags & ZEND_ACC_TRAIT) + zend_do_implement_interfaces(ce, interfaces); + } else if (parent && parent->num_interfaces) { + zend_do_inherit_interfaces(ce, parent); + } + if (!(ce->ce_flags & (ZEND_ACC_INTERFACE|ZEND_ACC_TRAIT)) + && (ce->ce_flags & (ZEND_ACC_IMPLICIT_ABSTRACT_CLASS|ZEND_ACC_EXPLICIT_ABSTRACT_CLASS)) + ) { + zend_verify_abstract_class(ce); + } + if (ce->ce_flags & ZEND_ACC_ENUM) { + zend_verify_enum(ce); + } + + /* Normally Stringable is added during compilation. However, if it is imported from a trait, + * we need to explicilty add the interface here. */ + if (ce->__tostring && !(ce->ce_flags & ZEND_ACC_TRAIT) && !zend_class_implements_interface(ce, zend_ce_stringable)) { - ZEND_ASSERT(ce->__tostring->common.fn_flags & ZEND_ACC_TRAIT_CLONE); - ce->ce_flags |= ZEND_ACC_RESOLVED_INTERFACES; - ce->num_interfaces++; - ce->interfaces = perealloc(ce->interfaces, - sizeof(zend_class_entry *) * ce->num_interfaces, ce->type == ZEND_INTERNAL_CLASS); - ce->interfaces[ce->num_interfaces - 1] = zend_ce_stringable; - do_interface_implementation(ce, zend_ce_stringable); - } + ZEND_ASSERT(ce->__tostring->common.fn_flags & ZEND_ACC_TRAIT_CLONE); + ce->ce_flags |= ZEND_ACC_RESOLVED_INTERFACES; + ce->num_interfaces++; + ce->interfaces = perealloc(ce->interfaces, + sizeof(zend_class_entry *) * ce->num_interfaces, ce->type == ZEND_INTERNAL_CLASS); + ce->interfaces[ce->num_interfaces - 1] = zend_ce_stringable; + do_interface_implementation(ce, zend_ce_stringable); + } + + zend_build_properties_info_table(ce); + } zend_catch { + /* Do not leak recorded errors to the next linked class. */ + if (!orig_record_errors) { + EG(record_errors) = false; + zend_free_recorded_errors(); + } + zend_bailout(); + } zend_end_try(); - zend_build_properties_info_table(ce); EG(record_errors) = orig_record_errors; if (!(ce->ce_flags & ZEND_ACC_UNRESOLVED_VARIANCE)) { @@ -3038,22 +3050,29 @@ zend_class_entry *zend_try_early_bind(zend_class_entry *ce, zend_class_entry *pa orig_linking_class = CG(current_linking_class); CG(current_linking_class) = is_cacheable ? ce : NULL; - if (is_cacheable) { - zend_begin_record_errors(); - } + zend_try{ + if (is_cacheable) { + zend_begin_record_errors(); + } - zend_do_inheritance_ex(ce, parent_ce, status == INHERITANCE_SUCCESS); - if (parent_ce && parent_ce->num_interfaces) { - zend_do_inherit_interfaces(ce, parent_ce); - } - zend_build_properties_info_table(ce); - if ((ce->ce_flags & (ZEND_ACC_IMPLICIT_ABSTRACT_CLASS|ZEND_ACC_INTERFACE|ZEND_ACC_TRAIT|ZEND_ACC_EXPLICIT_ABSTRACT_CLASS)) == ZEND_ACC_IMPLICIT_ABSTRACT_CLASS) { - zend_verify_abstract_class(ce); - } - ZEND_ASSERT(!(ce->ce_flags & ZEND_ACC_UNRESOLVED_VARIANCE)); - ce->ce_flags |= ZEND_ACC_LINKED; + zend_do_inheritance_ex(ce, parent_ce, status == INHERITANCE_SUCCESS); + if (parent_ce && parent_ce->num_interfaces) { + zend_do_inherit_interfaces(ce, parent_ce); + } + zend_build_properties_info_table(ce); + if ((ce->ce_flags & (ZEND_ACC_IMPLICIT_ABSTRACT_CLASS|ZEND_ACC_INTERFACE|ZEND_ACC_TRAIT|ZEND_ACC_EXPLICIT_ABSTRACT_CLASS)) == ZEND_ACC_IMPLICIT_ABSTRACT_CLASS) { + zend_verify_abstract_class(ce); + } + ZEND_ASSERT(!(ce->ce_flags & ZEND_ACC_UNRESOLVED_VARIANCE)); + ce->ce_flags |= ZEND_ACC_LINKED; + + CG(current_linking_class) = orig_linking_class; + } zend_catch { + EG(record_errors) = false; + zend_free_recorded_errors(); + zend_bailout(); + } zend_end_try(); - CG(current_linking_class) = orig_linking_class; EG(record_errors) = false; if (is_cacheable) { diff --git a/ext/opcache/tests/gh8063-001.phpt b/ext/opcache/tests/gh8063-001.phpt new file mode 100644 index 0000000000000..320d40aa3a8c0 --- /dev/null +++ b/ext/opcache/tests/gh8063-001.phpt @@ -0,0 +1,31 @@ +--TEST-- +Bug GH-8063 (Opcache breaks autoloading after E_COMPILE_ERROR) 001 +--INI-- +opcache.enable=1 +opcache.enable_cli=1 +opcache.record_warnings=0 +--EXTENSIONS-- +opcache +--FILE-- + Date: Sat, 16 Apr 2022 22:49:39 +0100 Subject: [PATCH 2/2] Fix bug #77023: FPM cannot shutdown processes This change introduces subsequent kill of the process when idle process quit (SIGQUIT) does not succeed. It can happen in some situations and means that FPM is not able to scale down in dynamic pm. Using SIGKILL fixes the issue. --- NEWS | 1 + sapi/fpm/fpm/fpm_process_ctl.c | 20 ++++-- sapi/fpm/fpm/fpm_process_ctl.h | 3 +- .../bug77023-pm-dynamic-blocking-sigquit.phpt | 65 +++++++++++++++++++ 4 files changed, 84 insertions(+), 5 deletions(-) create mode 100644 sapi/fpm/tests/bug77023-pm-dynamic-blocking-sigquit.phpt diff --git a/NEWS b/NEWS index a04955990868f..0b44185be8fcb 100644 --- a/NEWS +++ b/NEWS @@ -13,6 +13,7 @@ PHP NEWS - FPM: . Fixed bug #76003 (FPM /status reports wrong number of active processe). (Jakub Zelenka) + . Fixed bug #77023 (FPM cannot shutdown processes). (Jakub Zelenka) - MySQLi: . Fixed bug GH-8267 (MySQLi uses unsupported format specifier on Windows). diff --git a/sapi/fpm/fpm/fpm_process_ctl.c b/sapi/fpm/fpm/fpm_process_ctl.c index 4b8dca4690ccb..6f439651f5138 100644 --- a/sapi/fpm/fpm/fpm_process_ctl.c +++ b/sapi/fpm/fpm/fpm_process_ctl.c @@ -138,6 +138,9 @@ int fpm_pctl_kill(pid_t pid, int how) /* {{{ */ case FPM_PCTL_QUIT : s = SIGQUIT; break; + case FPM_PCTL_KILL: + s = SIGKILL; + break; default : break; } @@ -310,6 +313,17 @@ static void fpm_pctl_check_request_timeout(struct timeval *now) /* {{{ */ } /* }}} */ +static void fpm_pctl_kill_idle_child(struct fpm_child_s *child) /* {{{ */ +{ + if (child->idle_kill) { + fpm_pctl_kill(child->pid, FPM_PCTL_KILL); + } else { + child->idle_kill = 1; + fpm_pctl_kill(child->pid, FPM_PCTL_QUIT); + } +} +/* }}} */ + static void fpm_pctl_perform_idle_server_maintenance(struct timeval *now) /* {{{ */ { struct fpm_worker_pool_s *wp; @@ -372,8 +386,7 @@ static void fpm_pctl_perform_idle_server_maintenance(struct timeval *now) /* {{{ fpm_request_last_activity(last_idle_child, &last); fpm_clock_get(&now); if (last.tv_sec < now.tv_sec - wp->config->pm_process_idle_timeout) { - last_idle_child->idle_kill = 1; - fpm_pctl_kill(last_idle_child->pid, FPM_PCTL_QUIT); + fpm_pctl_kill_idle_child(last_idle_child); } continue; @@ -385,8 +398,7 @@ static void fpm_pctl_perform_idle_server_maintenance(struct timeval *now) /* {{{ zlog(ZLOG_DEBUG, "[pool %s] currently %d active children, %d spare children, %d running children. Spawning rate %d", wp->config->name, active, idle, wp->running_children, wp->idle_spawn_rate); if (idle > wp->config->pm_max_spare_servers && last_idle_child) { - last_idle_child->idle_kill = 1; - fpm_pctl_kill(last_idle_child->pid, FPM_PCTL_QUIT); + fpm_pctl_kill_idle_child(last_idle_child); wp->idle_spawn_rate = 1; continue; } diff --git a/sapi/fpm/fpm/fpm_process_ctl.h b/sapi/fpm/fpm/fpm_process_ctl.h index f39a489f6177a..bae1036e26f07 100644 --- a/sapi/fpm/fpm/fpm_process_ctl.h +++ b/sapi/fpm/fpm/fpm_process_ctl.h @@ -44,7 +44,8 @@ enum { FPM_PCTL_TERM, FPM_PCTL_STOP, FPM_PCTL_CONT, - FPM_PCTL_QUIT + FPM_PCTL_QUIT, + FPM_PCTL_KILL }; #endif diff --git a/sapi/fpm/tests/bug77023-pm-dynamic-blocking-sigquit.phpt b/sapi/fpm/tests/bug77023-pm-dynamic-blocking-sigquit.phpt new file mode 100644 index 0000000000000..6fc0849343571 --- /dev/null +++ b/sapi/fpm/tests/bug77023-pm-dynamic-blocking-sigquit.phpt @@ -0,0 +1,65 @@ +--TEST-- +FPM: Blocked SIGQUIT prevents idle process to be killed +--EXTENSIONS-- +pcntl +--SKIPIF-- + +--FILE-- +start(); +$tester->expectLogStartNotices(); +$tester->multiRequest(2); +$tester->status([ + 'total processes' => 2, +]); +// wait for process to be killed +sleep(7); +$tester->expectLogWarning('child \\d+ exited on signal 9 \\(SIGKILL\\) after \\d+.\\d+ seconds from start', 'unconfined'); +$tester->expectLogNotice('child \\d+ started', 'unconfined'); +$tester->expectLogWarning('child \\d+ exited on signal 9 \\(SIGKILL\\) after \\d+.\\d+ seconds from start', 'unconfined'); +$tester->expectLogNotice('child \\d+ started', 'unconfined'); +$tester->status([ + 'total processes' => 1, +]); +$tester->terminate(); +$tester->expectLogTerminatingNotices(); +$tester->close(); + +?> +Done +--EXPECT-- +Done +--CLEAN-- +