From 621c25a9a83d7e42339b12d780a7ba6f8619bfb1 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 1/2] 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. --- ext/simplexml/simplexml.c | 12 +++++++++++- ext/simplexml/tests/gh12192.phpt | 27 +++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-) create mode 100644 ext/simplexml/tests/gh12192.phpt diff --git a/ext/simplexml/simplexml.c b/ext/simplexml/simplexml.c index 57c0627d61a6a..68f5c09fe5e86 100644 --- a/ext/simplexml/simplexml.c +++ b/ext/simplexml/simplexml.c @@ -77,6 +77,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 +96,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(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 +1635,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); diff --git a/ext/simplexml/tests/gh12192.phpt b/ext/simplexml/tests/gh12192.phpt new file mode 100644 index 0000000000000..545bd176fb02b --- /dev/null +++ b/ext/simplexml/tests/gh12192.phpt @@ -0,0 +1,27 @@ +--TEST-- +GH-12192 (SimpleXML infinite loop when getName() is called within foreach) +--EXTENSIONS-- +simplexml +--FILE-- +foo"; +$xml = simplexml_load_string($xml); + +$a = $xml->a; + +foreach ($a as $test) { + var_dump($a->key()); + var_dump($a->getName()); +} + +var_dump($a); + +?> +--EXPECT-- +string(1) "a" +string(1) "a" +object(SimpleXMLElement)#2 (1) { + [0]=> + string(3) "foo" +} From f7edd07f7d2a535bed8a815748236c36c6f6e640 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Wed, 13 Sep 2023 22:50:49 +0200 Subject: [PATCH 2/2] Don't throw away iterator data --- ext/simplexml/simplexml.c | 24 ++++++++++++++++-------- ext/simplexml/tests/gh12192.phpt | 16 +++++++++++++--- 2 files changed, 29 insertions(+), 11 deletions(-) diff --git a/ext/simplexml/simplexml.c b/ext/simplexml/simplexml.c index 68f5c09fe5e86..d51089d88b099 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); @@ -99,7 +100,7 @@ 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(sxe, false); + return php_sxe_reset_iterator_no_clear_iter_data(sxe, false); } else { return node; } @@ -2460,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) { @@ -2481,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 index 545bd176fb02b..4d648495a1068 100644 --- a/ext/simplexml/tests/gh12192.phpt +++ b/ext/simplexml/tests/gh12192.phpt @@ -5,23 +5,33 @@ simplexml --FILE-- foo"; +$xml = "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" -object(SimpleXMLElement)#2 (1) { +string(1) "1" +Iteration +string(1) "a" +string(1) "a" +string(1) "2" +object(SimpleXMLElement)#2 (2) { [0]=> - string(3) "foo" + string(1) "1" + [1]=> + string(1) "2" }