From 757781a142abb82f3fcec22ec34b61983846b224 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Fri, 25 Oct 2024 20:34:20 +0200 Subject: [PATCH 1/3] Fix GH-16577: EG(strtod_state).freelist leaks with opcache.preload This happens because on ZTS we execute `executor_globals_ctor` which reset the `freelist` and `p5s` pointers, while on NTS we don't. On NTS we can reuse the caches but on ZTS we can't, the easiest fix is to call `zend_shutdown_strtod` when preloading is shut down. This regressed in GH-13974 and therefore only exists in PHP 8.4 and higher. Closes GH-16602. --- NEWS | 2 ++ ext/opcache/ZendAccelerator.c | 6 ++++++ ext/opcache/tests/gh16577.inc | 2 ++ ext/opcache/tests/gh16577.phpt | 20 ++++++++++++++++++++ 4 files changed, 30 insertions(+) create mode 100644 ext/opcache/tests/gh16577.inc create mode 100644 ext/opcache/tests/gh16577.phpt diff --git a/NEWS b/NEWS index 3ed4cc4cf7b..e793b532f5e 100644 --- a/NEWS +++ b/NEWS @@ -5,6 +5,8 @@ PHP NEWS - Core: . Fixed bug GH-16574 (Incorrect error "undefined method" messages). (nielsdos) + . Fixed bug GH-16577 (EG(strtod_state).freelist leaks with opcache.preload). + (nielsdos) - GD: . Fixed bug GH-16559 (UBSan abort in ext/gd/libgd/gd_interpolation.c:1007). diff --git a/ext/opcache/ZendAccelerator.c b/ext/opcache/ZendAccelerator.c index 3e8bdea9c7a..e0f8cda2298 100644 --- a/ext/opcache/ZendAccelerator.c +++ b/ext/opcache/ZendAccelerator.c @@ -4462,6 +4462,12 @@ static zend_result accel_preload(const char *config, bool in_child) /* Release stored values to avoid dangling pointers */ zend_shutdown_executor_values(/* fast_shutdown */ false); + /* On ZTS we execute `executor_globals_ctor` which reset the freelist and p5s pointers, while on NTS we don't. + * We have to clean up the memory before the actual request takes place to avoid a memory leak. */ +#ifdef ZTS + zend_shutdown_strtod(); +#endif + /* We don't want to preload constants. * Check that zend_shutdown_executor_values() also destroys constants. */ ZEND_ASSERT(zend_hash_num_elements(EG(zend_constants)) == EG(persistent_constants_count)); diff --git a/ext/opcache/tests/gh16577.inc b/ext/opcache/tests/gh16577.inc new file mode 100644 index 00000000000..f016d612817 --- /dev/null +++ b/ext/opcache/tests/gh16577.inc @@ -0,0 +1,2 @@ + +--FILE-- + +--EXPECT-- +float(1.5) +Done From 38e1b0ac8cd0bae34f0ebe77caf35e3da6d6d4d0 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Thu, 24 Oct 2024 21:47:55 +0200 Subject: [PATCH 2/3] Fix GH-16572: Incorrect result with reflection in low-trigger JIT When a recursive call happens with invalid arguments, the maximum valid arguments are computed and stored in `num_args`, but the RECV entry block we jump to is `call_num_args` instead. This can skip argument validation checks. Fix this by using `num_args` instead. Closes GH-16575. --- NEWS | 4 ++++ ext/opcache/jit/zend_jit_ir.c | 5 ++--- ext/opcache/tests/jit/gh16572.phpt | 24 ++++++++++++++++++++++++ 3 files changed, 30 insertions(+), 3 deletions(-) create mode 100644 ext/opcache/tests/jit/gh16572.phpt diff --git a/NEWS b/NEWS index e793b532f5e..d57f2303dd4 100644 --- a/NEWS +++ b/NEWS @@ -12,6 +12,10 @@ PHP NEWS . Fixed bug GH-16559 (UBSan abort in ext/gd/libgd/gd_interpolation.c:1007). (nielsdos) +- Opcache: + . Fixed bug GH-16572 (Incorrect result with reflection in low-trigger JIT). + (nielsdos) + - PDO: . Fixed bug GH-16167 (Prevent mixing PDO sub-classes with different DSN). (kocsismate) diff --git a/ext/opcache/jit/zend_jit_ir.c b/ext/opcache/jit/zend_jit_ir.c index c12298aaf3f..5cb118825cc 100644 --- a/ext/opcache/jit/zend_jit_ir.c +++ b/ext/opcache/jit/zend_jit_ir.c @@ -9887,6 +9887,7 @@ static int zend_jit_do_fcall(zend_jit_ctx *jit, const zend_op *opline, const zen if ((!func || func->type == ZEND_USER_FUNCTION) && opline->opcode != ZEND_DO_ICALL) { bool recursive_call_through_jmp = 0; + uint32_t num_args = 0; // JIT: EX(call) = NULL; ir_STORE(jit_CALL(rx, call), IR_NULL); @@ -9951,8 +9952,6 @@ static int zend_jit_do_fcall(zend_jit_ctx *jit, const zend_op *opline, const zen if (call_num_args <= func->op_array.num_args) { if (!trace || (trace->op == ZEND_JIT_TRACE_END && trace->stop == ZEND_JIT_TRACE_STOP_INTERPRETER)) { - uint32_t num_args; - if ((func->op_array.fn_flags & ZEND_ACC_HAS_TYPE_HINTS) != 0) { if (trace) { num_args = 0; @@ -10148,7 +10147,7 @@ static int zend_jit_do_fcall(zend_jit_ctx *jit, const zend_op *opline, const zen ir_insn *insn; /* attempt to convert direct recursive call into loop */ - begin = jit->bb_start_ref[call_num_args]; + begin = jit->bb_start_ref[num_args]; ZEND_ASSERT(begin != IR_UNUSED); insn = &jit->ctx.ir_base[begin]; if (insn->op == IR_BEGIN) { diff --git a/ext/opcache/tests/jit/gh16572.phpt b/ext/opcache/tests/jit/gh16572.phpt new file mode 100644 index 00000000000..b3a6850561c --- /dev/null +++ b/ext/opcache/tests/jit/gh16572.phpt @@ -0,0 +1,24 @@ +--TEST-- +GH-16572 (Incorrect result with reflection in low-trigger JIT) +--EXTENSIONS-- +opcache +--INI-- +opcache.jit=1215 +--FILE-- +getReturnType()); +?> +--EXPECTF-- +string(19) "ReflectionNamedType" + +Fatal error: Uncaught TypeError: dumpType(): Argument #1 ($rt) must be of type ReflectionType, null given, called in %s on line %d and defined in %s:%d +Stack trace: +#0 %s(%d): dumpType(NULL) +#1 %s(%d): dumpType(Object(ReflectionNamedType)) +#2 {main} + thrown in %s on line %d From 947e319b76dca6cf685777856cd511bedc55a2ec Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Fri, 25 Oct 2024 19:11:59 +0200 Subject: [PATCH 3/3] Fix GH-16594: Assertion failure in DOM -> before The invalid parent condition can actually happen because PHP's DOM is allows to get children of e.g. attributes; something normally not possible. Closes GH-16597. --- NEWS | 3 +++ ext/dom/parentnode/tree.c | 7 +++++-- ext/dom/tests/gh16594.phpt | 25 +++++++++++++++++++++++++ 3 files changed, 33 insertions(+), 2 deletions(-) create mode 100644 ext/dom/tests/gh16594.phpt diff --git a/NEWS b/NEWS index d57f2303dd4..4926783cc43 100644 --- a/NEWS +++ b/NEWS @@ -8,6 +8,9 @@ PHP NEWS . Fixed bug GH-16577 (EG(strtod_state).freelist leaks with opcache.preload). (nielsdos) +- DOM: + . Fixed bug GH-16594 (Assertion failure in DOM -> before). (nielsdos) + - GD: . Fixed bug GH-16559 (UBSan abort in ext/gd/libgd/gd_interpolation.c:1007). (nielsdos) diff --git a/ext/dom/parentnode/tree.c b/ext/dom/parentnode/tree.c index ef1ad42b68c..51cdd7cff43 100644 --- a/ext/dom/parentnode/tree.c +++ b/ext/dom/parentnode/tree.c @@ -239,8 +239,11 @@ static bool dom_is_pre_insert_valid_without_step_1(php_libxml_ref_obj *document, ZEND_ASSERT(parentNode != NULL); /* 1. If parent is not a Document, DocumentFragment, or Element node, then throw a "HierarchyRequestError" DOMException. - * => Impossible */ - ZEND_ASSERT(!php_dom_pre_insert_is_parent_invalid(parentNode)); + * => This is possible because we can grab children of attributes etc... (see e.g. GH-16594) */ + if (php_dom_pre_insert_is_parent_invalid(parentNode)) { + php_dom_throw_error(HIERARCHY_REQUEST_ERR, dom_get_strict_error(document)); + return false; + } if (node->doc != documentNode) { php_dom_throw_error(WRONG_DOCUMENT_ERR, dom_get_strict_error(document)); diff --git a/ext/dom/tests/gh16594.phpt b/ext/dom/tests/gh16594.phpt new file mode 100644 index 00000000000..adfde5948ee --- /dev/null +++ b/ext/dom/tests/gh16594.phpt @@ -0,0 +1,25 @@ +--TEST-- +GH-16594 (Assertion failure in DOM -> before) +--EXTENSIONS-- +dom +--FILE-- +createElement("test"); +$v9->setAttributeNodeNS($v7); +$v7->appendChild($v1); + +try { + $v1->before($v6); +} catch (DOMException $e) { + echo $e->getMessage(), "\n"; +} + +?> +--EXPECT-- +Hierarchy Request Error