From 10f5a06d3cb4e954cf6446ff7a037ed3b95b4fa7 Mon Sep 17 00:00:00 2001 From: Max Semenik Date: Tue, 12 Sep 2023 14:36:20 +0300 Subject: [PATCH 1/3] Fix GH-12186: segfault copying/cloning a finalized HashContext Closes GH-12186. Closes GH-12187. --- NEWS | 4 ++++ ext/hash/hash.c | 11 ++++++++++- ext/hash/tests/gh12186_1.phpt | 17 +++++++++++++++++ ext/hash/tests/gh12186_2.phpt | 17 +++++++++++++++++ ext/hash/tests/reuse.phpt | 2 +- 5 files changed, 49 insertions(+), 2 deletions(-) create mode 100644 ext/hash/tests/gh12186_1.phpt create mode 100644 ext/hash/tests/gh12186_2.phpt diff --git a/NEWS b/NEWS index 15c85841eea..88cd54f5479 100644 --- a/NEWS +++ b/NEWS @@ -5,6 +5,10 @@ PHP NEWS - Filter: . Fix explicit FILTER_REQUIRE_SCALAR with FILTER_CALLBACK (ilutov) +- Hash: + . Fixed bug GH-12186 (segfault copying/cloning a finalized HashContext). + (MaxSem) + - SimpleXML: . Fixed bug GH-12170 (Can't use xpath with comments in SimpleXML). (nielsdos) diff --git a/ext/hash/hash.c b/ext/hash/hash.c index 2d29ff61fa0..c01a1f0f3ec 100644 --- a/ext/hash/hash.c +++ b/ext/hash/hash.c @@ -680,7 +680,7 @@ PHP_FUNCTION(hash_init) #define PHP_HASHCONTEXT_VERIFY(hash) { \ if (!hash->context) { \ - zend_argument_type_error(1, "must be a valid Hash Context resource"); \ + zend_argument_type_error(1, "must be a valid, non-finalized HashContext"); \ RETURN_THROWS(); \ } \ } @@ -837,11 +837,15 @@ PHP_FUNCTION(hash_final) PHP_FUNCTION(hash_copy) { zval *zhash; + php_hashcontext_object *context; if (zend_parse_parameters(ZEND_NUM_ARGS(), "O", &zhash, php_hashcontext_ce) == FAILURE) { RETURN_THROWS(); } + context = php_hashcontext_from_object(Z_OBJ_P(zhash)); + PHP_HASHCONTEXT_VERIFY(context); + RETVAL_OBJ(Z_OBJ_HANDLER_P(zhash, clone_obj)(Z_OBJ_P(zhash))); if (php_hashcontext_from_object(Z_OBJ_P(return_value))->context == NULL) { @@ -1405,6 +1409,11 @@ static zend_object *php_hashcontext_clone(zend_object *zobj) { zend_object *znew = php_hashcontext_create(zobj->ce); php_hashcontext_object *newobj = php_hashcontext_from_object(znew); + if (!oldobj->context) { + zend_throw_exception(zend_ce_value_error, "Cannot clone a finalized HashContext", 0); + return znew; + } + zend_objects_clone_members(znew, zobj); newobj->ops = oldobj->ops; diff --git a/ext/hash/tests/gh12186_1.phpt b/ext/hash/tests/gh12186_1.phpt new file mode 100644 index 00000000000..5e34b1dd78e --- /dev/null +++ b/ext/hash/tests/gh12186_1.phpt @@ -0,0 +1,17 @@ +--TEST-- +Hash: bug #12186 - segfault in hash_copy() on a finalized context +--FILE-- +getMessage() . "\n"; +} + +?> +--EXPECTF-- +hash_copy(): Argument #1 ($context) must be a valid, non-finalized HashContext diff --git a/ext/hash/tests/gh12186_2.phpt b/ext/hash/tests/gh12186_2.phpt new file mode 100644 index 00000000000..64a12b15c39 --- /dev/null +++ b/ext/hash/tests/gh12186_2.phpt @@ -0,0 +1,17 @@ +--TEST-- +Hash: bug #12186 - segfault when cloning a finalized context +--FILE-- +getMessage() . "\n"; +} + +?> +--EXPECTF-- +Cannot clone a finalized HashContext diff --git a/ext/hash/tests/reuse.phpt b/ext/hash/tests/reuse.phpt index 229236dd713..b5cfc79c162 100644 --- a/ext/hash/tests/reuse.phpt +++ b/ext/hash/tests/reuse.phpt @@ -14,4 +14,4 @@ catch (\Error $e) { ?> --EXPECT-- -hash_update(): Argument #1 ($context) must be a valid Hash Context resource +hash_update(): Argument #1 ($context) must be a valid, non-finalized HashContext From 4d888cf53f8e26396d4d2f4a8d15e1e8007c1c6d Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Tue, 12 Sep 2023 20:11:28 +0200 Subject: [PATCH 2/3] Fix GH-12192: SimpleXML infinite loop when getName() is called within foreach This happens because getName() resets the iterator to the start because it overwrites the iterator data. We add a version of get_first_node that does not overwrite the iterator data. Closes GH-12193. --- NEWS | 2 ++ ext/simplexml/simplexml.c | 34 ++++++++++++++++++++++------- ext/simplexml/tests/gh12192.phpt | 37 ++++++++++++++++++++++++++++++++ 3 files changed, 65 insertions(+), 8 deletions(-) create mode 100644 ext/simplexml/tests/gh12192.phpt diff --git a/NEWS b/NEWS index 88cd54f5479..aac57e97942 100644 --- a/NEWS +++ b/NEWS @@ -11,6 +11,8 @@ PHP NEWS - SimpleXML: . Fixed bug GH-12170 (Can't use xpath with comments in SimpleXML). (nielsdos) + . Fixed bug GH-12192 (SimpleXML infinite loop when getName() is called + within foreach). (nielsdos) 28 Sep 2023, PHP 8.1.24 diff --git a/ext/simplexml/simplexml.c b/ext/simplexml/simplexml.c index 044daac6dd0..6441b331dbf 100644 --- a/ext/simplexml/simplexml.c +++ b/ext/simplexml/simplexml.c @@ -45,6 +45,7 @@ PHP_SXE_API zend_class_entry *sxe_get_element_class_entry(void) /* {{{ */ static php_sxe_object* php_sxe_object_new(zend_class_entry *ce, zend_function *fptr_count); static xmlNodePtr php_sxe_reset_iterator(php_sxe_object *sxe, int use_data); +static xmlNodePtr php_sxe_reset_iterator_no_clear_iter_data(php_sxe_object *sxe, int use_data); static xmlNodePtr php_sxe_iterator_fetch(php_sxe_object *sxe, xmlNodePtr node, int use_data); static void php_sxe_iterator_dtor(zend_object_iterator *iter); static int php_sxe_iterator_valid(zend_object_iterator *iter); @@ -77,6 +78,7 @@ static void _node_as_zval(php_sxe_object *sxe, xmlNodePtr node, zval *value, SXE } /* }}} */ +/* Important: this overwrites the iterator data, if you wish to keep it use php_sxe_get_first_node_non_destructive() instead! */ static xmlNodePtr php_sxe_get_first_node(php_sxe_object *sxe, xmlNodePtr node) /* {{{ */ { php_sxe_object *intern; @@ -95,6 +97,15 @@ static xmlNodePtr php_sxe_get_first_node(php_sxe_object *sxe, xmlNodePtr node) / } /* }}} */ +static xmlNodePtr php_sxe_get_first_node_non_destructive(php_sxe_object *sxe, xmlNodePtr node) +{ + if (sxe && sxe->iter.type != SXE_ITER_NONE) { + return php_sxe_reset_iterator_no_clear_iter_data(sxe, false); + } else { + return node; + } +} + static inline int match_ns(php_sxe_object *sxe, xmlNodePtr node, xmlChar *name, int prefix) /* {{{ */ { if (name == NULL && (node->ns == NULL || node->ns->prefix == NULL)) { @@ -1625,7 +1636,7 @@ PHP_METHOD(SimpleXMLElement, getName) sxe = Z_SXEOBJ_P(ZEND_THIS); GET_NODE(sxe, node); - node = php_sxe_get_first_node(sxe, node); + node = php_sxe_get_first_node_non_destructive(sxe, node); if (node) { namelen = xmlStrlen(node->name); RETURN_STRINGL((char*)node->name, namelen); @@ -2450,15 +2461,9 @@ static xmlNodePtr php_sxe_iterator_fetch(php_sxe_object *sxe, xmlNodePtr node, i } /* }}} */ -static xmlNodePtr php_sxe_reset_iterator(php_sxe_object *sxe, int use_data) /* {{{ */ +static xmlNodePtr php_sxe_reset_iterator_no_clear_iter_data(php_sxe_object *sxe, int use_data) { xmlNodePtr node; - - if (!Z_ISUNDEF(sxe->iter.data)) { - zval_ptr_dtor(&sxe->iter.data); - ZVAL_UNDEF(&sxe->iter.data); - } - GET_NODE(sxe, node) if (node) { @@ -2471,10 +2476,23 @@ static xmlNodePtr php_sxe_reset_iterator(php_sxe_object *sxe, int use_data) /* { case SXE_ITER_ATTRLIST: node = (xmlNodePtr) node->properties; } + if (use_data) { + ZEND_ASSERT(Z_ISUNDEF(sxe->iter.data)); + } return php_sxe_iterator_fetch(sxe, node, use_data); } return NULL; } + +static xmlNodePtr php_sxe_reset_iterator(php_sxe_object *sxe, int use_data) /* {{{ */ +{ + if (!Z_ISUNDEF(sxe->iter.data)) { + zval_ptr_dtor(&sxe->iter.data); + ZVAL_UNDEF(&sxe->iter.data); + } + + return php_sxe_reset_iterator_no_clear_iter_data(sxe, use_data); +} /* }}} */ zend_object_iterator *php_sxe_get_iterator(zend_class_entry *ce, zval *object, int by_ref) /* {{{ */ diff --git a/ext/simplexml/tests/gh12192.phpt b/ext/simplexml/tests/gh12192.phpt new file mode 100644 index 00000000000..4d648495a10 --- /dev/null +++ b/ext/simplexml/tests/gh12192.phpt @@ -0,0 +1,37 @@ +--TEST-- +GH-12192 (SimpleXML infinite loop when getName() is called within foreach) +--EXTENSIONS-- +simplexml +--FILE-- +12"; +$xml = simplexml_load_string($xml); + +$a = $xml->a; + +foreach ($a as $test) { + echo "Iteration\n"; + var_dump($a->key()); + var_dump($a->getName()); + var_dump((string) $test); +} + +var_dump($a); + +?> +--EXPECT-- +Iteration +string(1) "a" +string(1) "a" +string(1) "1" +Iteration +string(1) "a" +string(1) "a" +string(1) "2" +object(SimpleXMLElement)#2 (2) { + [0]=> + string(1) "1" + [1]=> + string(1) "2" +} From 39a9e561f98e10d346a31b3285aeb2ebb535f39d Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Sat, 16 Sep 2023 14:18:33 +0200 Subject: [PATCH 3/3] Fix GH-12223: Entity reference produces infinite loop in var_dump/print_r Closes GH-12223. --- NEWS | 2 + ext/simplexml/simplexml.c | 6 +++ ext/simplexml/tests/gh12223.phpt | 67 ++++++++++++++++++++++++++++++++ 3 files changed, 75 insertions(+) create mode 100644 ext/simplexml/tests/gh12223.phpt diff --git a/NEWS b/NEWS index aac57e97942..2691caec352 100644 --- a/NEWS +++ b/NEWS @@ -13,6 +13,8 @@ PHP NEWS . Fixed bug GH-12170 (Can't use xpath with comments in SimpleXML). (nielsdos) . Fixed bug GH-12192 (SimpleXML infinite loop when getName() is called within foreach). (nielsdos) + . Fixed bug GH-12223 (Entity reference produces infinite loop in + var_dump/print_r). (nielsdos) 28 Sep 2023, PHP 8.1.24 diff --git a/ext/simplexml/simplexml.c b/ext/simplexml/simplexml.c index 6441b331dbf..2853c0272ec 100644 --- a/ext/simplexml/simplexml.c +++ b/ext/simplexml/simplexml.c @@ -1197,6 +1197,12 @@ static HashTable *sxe_get_prop_hash(zend_object *object, int is_debug) /* {{{ */ sxe_properties_add(rv, name, namelen, &value); } next_iter: + if (UNEXPECTED(node->type == XML_ENTITY_DECL)) { + /* Entity decls are linked together via the next pointer. + * The only way to get to an entity decl is via an entity reference in the document. + * If we then continue iterating, we'll end up in the DTD. Even worse, if the entities reference each other we'll infinite loop. */ + break; + } if (use_iter) { node = php_sxe_iterator_fetch(sxe, node->next, 0); } else { diff --git a/ext/simplexml/tests/gh12223.phpt b/ext/simplexml/tests/gh12223.phpt new file mode 100644 index 00000000000..0be61dec2ff --- /dev/null +++ b/ext/simplexml/tests/gh12223.phpt @@ -0,0 +1,67 @@ +--TEST-- +GH-12223: Entity reference produces infinite loop in var_dump/print_r +--EXTENSIONS-- +simplexml +--FILE-- + + + + +]> +&c; +XML; + +$sxe = simplexml_load_string($xml); + +var_dump($sxe); +print_r($sxe); + +?> +--EXPECT-- +object(SimpleXMLElement)#1 (1) { + ["c"]=> + object(SimpleXMLElement)#2 (1) { + ["c"]=> + object(SimpleXMLElement)#3 (1) { + ["b"]=> + object(SimpleXMLElement)#4 (1) { + ["b"]=> + object(SimpleXMLElement)#5 (1) { + ["a"]=> + object(SimpleXMLElement)#6 (1) { + ["a"]=> + string(9) "something" + } + } + } + } + } +} +SimpleXMLElement Object +( + [c] => SimpleXMLElement Object + ( + [c] => SimpleXMLElement Object + ( + [b] => SimpleXMLElement Object + ( + [b] => SimpleXMLElement Object + ( + [a] => SimpleXMLElement Object + ( + [a] => something + ) + + ) + + ) + + ) + + ) + +)