From 1a4e401bf03855b7b61895baf14cc0b5389e2b1a Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Tue, 19 Sep 2023 21:31:26 +0200 Subject: [PATCH 1/2] Fix bug #55098: SimpleXML iteration produces infinite loop Closes GH-12247. --- NEWS | 1 + ext/simplexml/simplexml.c | 71 +++++++++--------------- ext/simplexml/tests/bug55098.phpt | 92 +++++++++++++++++++++++++++++++ ext/simplexml/tests/bug62639.phpt | 4 +- 4 files changed, 121 insertions(+), 47 deletions(-) create mode 100644 ext/simplexml/tests/bug55098.phpt diff --git a/NEWS b/NEWS index 8c728b07443..15ff5939b84 100644 --- a/NEWS +++ b/NEWS @@ -25,6 +25,7 @@ PHP NEWS var_dump/print_r). (nielsdos) . Fixed bug GH-12208 (SimpleXML infinite loop when a cast is used inside a foreach). (nielsdos) + . Fixed bug #55098 (SimpleXML iteration produces infinite loop). (nielsdos) 28 Sep 2023, PHP 8.1.24 diff --git a/ext/simplexml/simplexml.c b/ext/simplexml/simplexml.c index a90cbec2fd4..31461014d83 100644 --- a/ext/simplexml/simplexml.c +++ b/ext/simplexml/simplexml.c @@ -78,25 +78,6 @@ 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; - xmlNodePtr retnode = NULL; - - if (sxe && sxe->iter.type != SXE_ITER_NONE) { - php_sxe_reset_iterator(sxe, 1); - if (!Z_ISUNDEF(sxe->iter.data)) { - intern = Z_SXEOBJ_P(&sxe->iter.data); - GET_NODE(intern, retnode) - } - return retnode; - } else { - return node; - } -} -/* }}} */ - static xmlNodePtr php_sxe_get_first_node_non_destructive(php_sxe_object *sxe, xmlNodePtr node) { if (sxe && sxe->iter.type != SXE_ITER_NONE) { @@ -184,7 +165,7 @@ static xmlNodePtr sxe_get_element_by_name(php_sxe_object *sxe, xmlNodePtr node, if (sxe->iter.type == SXE_ITER_NONE) { sxe->iter.type = SXE_ITER_CHILD; } - node = php_sxe_get_first_node(sxe, node); + node = php_sxe_get_first_node_non_destructive(sxe, node); sxe->iter.type = orgtype; } @@ -270,11 +251,11 @@ long_dim: if (sxe->iter.type == SXE_ITER_ATTRLIST) { attribs = 1; elements = 0; - node = php_sxe_get_first_node(sxe, node); + node = php_sxe_get_first_node_non_destructive(sxe, node); attr = (xmlAttrPtr)node; test = sxe->iter.name != NULL; } else if (sxe->iter.type != SXE_ITER_CHILD) { - node = php_sxe_get_first_node(sxe, node); + node = php_sxe_get_first_node_non_destructive(sxe, node); attr = node ? node->properties : NULL; test = 0; if (!member && node && node->parent && @@ -322,7 +303,7 @@ long_dim: xmlNodePtr mynode = node; if (sxe->iter.type == SXE_ITER_CHILD) { - node = php_sxe_get_first_node(sxe, node); + node = php_sxe_get_first_node_non_destructive(sxe, node); } if (sxe->iter.type == SXE_ITER_NONE) { if (member && Z_LVAL_P(member) > 0) { @@ -456,12 +437,12 @@ long_dim: if (sxe->iter.type == SXE_ITER_ATTRLIST) { attribs = 1; elements = 0; - node = php_sxe_get_first_node(sxe, node); + node = php_sxe_get_first_node_non_destructive(sxe, node); attr = (xmlAttrPtr)node; test = sxe->iter.name != NULL; } else if (sxe->iter.type != SXE_ITER_CHILD) { mynode = node; - node = php_sxe_get_first_node(sxe, node); + node = php_sxe_get_first_node_non_destructive(sxe, node); attr = node ? node->properties : NULL; test = 0; if (!member && node && node->parent && @@ -707,7 +688,7 @@ static int sxe_prop_dim_exists(zend_object *object, zval *member, int check_empt attribs = 0; elements = 1; if (sxe->iter.type == SXE_ITER_CHILD) { - node = php_sxe_get_first_node(sxe, node); + node = php_sxe_get_first_node_non_destructive(sxe, node); } } } @@ -715,11 +696,11 @@ static int sxe_prop_dim_exists(zend_object *object, zval *member, int check_empt if (sxe->iter.type == SXE_ITER_ATTRLIST) { attribs = 1; elements = 0; - node = php_sxe_get_first_node(sxe, node); + node = php_sxe_get_first_node_non_destructive(sxe, node); attr = (xmlAttrPtr)node; test = sxe->iter.name != NULL; } else if (sxe->iter.type != SXE_ITER_CHILD) { - node = php_sxe_get_first_node(sxe, node); + node = php_sxe_get_first_node_non_destructive(sxe, node); attr = node ? node->properties : NULL; test = 0; } @@ -759,7 +740,7 @@ static int sxe_prop_dim_exists(zend_object *object, zval *member, int check_empt if (elements) { if (Z_TYPE_P(member) == IS_LONG) { if (sxe->iter.type == SXE_ITER_CHILD) { - node = php_sxe_get_first_node(sxe, node); + node = php_sxe_get_first_node_non_destructive(sxe, node); } node = sxe_get_element_by_offset(sxe, Z_LVAL_P(member), node, NULL); } else { @@ -829,7 +810,7 @@ static void sxe_prop_dim_delete(zend_object *object, zval *member, bool elements attribs = 0; elements = 1; if (sxe->iter.type == SXE_ITER_CHILD) { - node = php_sxe_get_first_node(sxe, node); + node = php_sxe_get_first_node_non_destructive(sxe, node); } } } @@ -837,11 +818,11 @@ static void sxe_prop_dim_delete(zend_object *object, zval *member, bool elements if (sxe->iter.type == SXE_ITER_ATTRLIST) { attribs = 1; elements = 0; - node = php_sxe_get_first_node(sxe, node); + node = php_sxe_get_first_node_non_destructive(sxe, node); attr = (xmlAttrPtr)node; test = sxe->iter.name != NULL; } else if (sxe->iter.type != SXE_ITER_CHILD) { - node = php_sxe_get_first_node(sxe, node); + node = php_sxe_get_first_node_non_destructive(sxe, node); attr = node ? node->properties : NULL; test = 0; } @@ -878,7 +859,7 @@ static void sxe_prop_dim_delete(zend_object *object, zval *member, bool elements if (elements) { if (Z_TYPE_P(member) == IS_LONG) { if (sxe->iter.type == SXE_ITER_CHILD) { - node = php_sxe_get_first_node(sxe, node); + node = php_sxe_get_first_node_non_destructive(sxe, node); } node = sxe_get_element_by_offset(sxe, Z_LVAL_P(member), node, NULL); if (node) { @@ -1011,7 +992,7 @@ static int sxe_prop_is_empty(zend_object *object) /* {{{ */ } if (sxe->iter.type == SXE_ITER_ELEMENT) { - node = php_sxe_get_first_node(sxe, node); + node = php_sxe_get_first_node_non_destructive(sxe, node); } if (!node || node->type != XML_ENTITY_DECL) { attr = node ? (xmlAttrPtr)node->properties : NULL; @@ -1025,7 +1006,7 @@ static int sxe_prop_is_empty(zend_object *object) /* {{{ */ } GET_NODE(sxe, node); - node = php_sxe_get_first_node(sxe, node); + node = php_sxe_get_first_node_non_destructive(sxe, node); is_empty = 1; ZVAL_UNDEF(&iter_data); if (node && sxe->iter.type != SXE_ITER_ATTRLIST) { @@ -1120,7 +1101,7 @@ static HashTable *sxe_get_prop_hash(zend_object *object, int is_debug) /* {{{ */ } if (is_debug || sxe->iter.type != SXE_ITER_CHILD) { if (sxe->iter.type == SXE_ITER_ELEMENT) { - node = php_sxe_get_first_node(sxe, node); + node = php_sxe_get_first_node_non_destructive(sxe, node); } if (!node || node->type != XML_ENTITY_DECL) { attr = node ? (xmlAttrPtr)node->properties : NULL; @@ -1142,7 +1123,7 @@ static HashTable *sxe_get_prop_hash(zend_object *object, int is_debug) /* {{{ */ } GET_NODE(sxe, node); - node = php_sxe_get_first_node(sxe, node); + node = php_sxe_get_first_node_non_destructive(sxe, node); if (node && sxe->iter.type != SXE_ITER_ATTRLIST) { if (node->type == XML_ATTRIBUTE_NODE) { @@ -1301,7 +1282,7 @@ PHP_METHOD(SimpleXMLElement, xpath) } GET_NODE(sxe, nodeptr); - nodeptr = php_sxe_get_first_node(sxe, nodeptr); + nodeptr = php_sxe_get_first_node_non_destructive(sxe, nodeptr); if (!nodeptr) { return; } @@ -1410,7 +1391,7 @@ PHP_METHOD(SimpleXMLElement, asXML) 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) { RETURN_FALSE; @@ -1533,7 +1514,7 @@ PHP_METHOD(SimpleXMLElement, getNamespaces) 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) { if (node->type == XML_ELEMENT_NODE) { @@ -1618,7 +1599,7 @@ PHP_METHOD(SimpleXMLElement, children) } GET_NODE(sxe, node); - node = php_sxe_get_first_node(sxe, node); + node = php_sxe_get_first_node_non_destructive(sxe, node); if (!node) { return; } @@ -1667,7 +1648,7 @@ PHP_METHOD(SimpleXMLElement, attributes) 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) { return; } @@ -1708,7 +1689,7 @@ PHP_METHOD(SimpleXMLElement, addChild) return; } - node = php_sxe_get_first_node(sxe, node); + node = php_sxe_get_first_node_non_destructive(sxe, node); if (node == NULL) { php_error_docref(NULL, E_WARNING, "Cannot add child. Parent is not a permanent member of the XML tree"); @@ -1768,7 +1749,7 @@ PHP_METHOD(SimpleXMLElement, addAttribute) 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 && node->type != XML_ELEMENT_NODE) { node = node->parent; @@ -2615,7 +2596,7 @@ void *simplexml_export_node(zval *object) /* {{{ */ sxe = Z_SXEOBJ_P(object); GET_NODE(sxe, node); - return php_sxe_get_first_node(sxe, node); + return php_sxe_get_first_node_non_destructive(sxe, node); } /* }}} */ diff --git a/ext/simplexml/tests/bug55098.phpt b/ext/simplexml/tests/bug55098.phpt new file mode 100644 index 00000000000..71c8b424ecd --- /dev/null +++ b/ext/simplexml/tests/bug55098.phpt @@ -0,0 +1,92 @@ +--TEST-- +Bug #55098 (SimpleXML iteration produces infinite loop) +--EXTENSIONS-- +simplexml +--FILE-- +123"; +$xml = simplexml_load_string($xmlString); + +$nodes = $xml->a->b; + +function test($nodes, $name, $callable) { + echo "--- $name ---\n"; + foreach ($nodes as $nodeData) { + echo "nodeData: " . $nodeData . "\n"; + $callable($nodes); + } +} + +test($nodes, "asXml", fn ($n) => $n->asXml()); +test($nodes, "attributes", fn ($n) => $n->attributes()); +test($nodes, "children", fn ($n) => $n->children()); +test($nodes, "getNamespaces", fn ($n) => $n->getNamespaces()); +test($nodes, "xpath", fn ($n) => $n->xpath("/root/a/b")); +test($nodes, "var_dump", fn ($n) => var_dump($n)); +test($nodes, "manipulation combined with querying", function ($n) { + $n->addAttribute("attr", "value"); + (bool) $n["attr"]; + $n->addChild("child", "value"); + $n->outer[]->inner = "foo"; + (bool) $n->outer; + (bool) $n; + isset($n->outer); + isset($n["attr"]); + unset($n->outer); + unset($n["attr"]); + unset($n->child); +}); +?> +--EXPECT-- +--- asXml --- +nodeData: 1 +nodeData: 2 +nodeData: 3 +--- attributes --- +nodeData: 1 +nodeData: 2 +nodeData: 3 +--- children --- +nodeData: 1 +nodeData: 2 +nodeData: 3 +--- getNamespaces --- +nodeData: 1 +nodeData: 2 +nodeData: 3 +--- xpath --- +nodeData: 1 +nodeData: 2 +nodeData: 3 +--- var_dump --- +nodeData: 1 +object(SimpleXMLElement)#3 (3) { + [0]=> + string(1) "1" + [1]=> + string(1) "2" + [2]=> + string(1) "3" +} +nodeData: 2 +object(SimpleXMLElement)#3 (3) { + [0]=> + string(1) "1" + [1]=> + string(1) "2" + [2]=> + string(1) "3" +} +nodeData: 3 +object(SimpleXMLElement)#3 (3) { + [0]=> + string(1) "1" + [1]=> + string(1) "2" + [2]=> + string(1) "3" +} +--- manipulation combined with querying --- +nodeData: 1 +nodeData: 2 +nodeData: 3 diff --git a/ext/simplexml/tests/bug62639.phpt b/ext/simplexml/tests/bug62639.phpt index 286e6c5d8fd..517162d84eb 100644 --- a/ext/simplexml/tests/bug62639.phpt +++ b/ext/simplexml/tests/bug62639.phpt @@ -41,7 +41,7 @@ foreach ($a2->b->c->children() as $key => $value) { var_dump($value); }?> --EXPECT-- -object(A)#2 (2) { +object(A)#4 (2) { ["@attributes"]=> array(1) { ["attr"]=> @@ -50,7 +50,7 @@ object(A)#2 (2) { [0]=> string(10) "Some Value" } -object(A)#3 (2) { +object(A)#6 (2) { ["@attributes"]=> array(1) { ["attr"]=> From da6097ffc83f3fb6bc51584d6403588eaefd3409 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Fri, 15 Sep 2023 17:15:11 +0200 Subject: [PATCH 2/2] Fix GH-12215: Module entry being overwritten causes type errors in ext/dom (<= PHP 8.3) When we try to load an extension multiple times, we still overwrite the type, module number, and handle. If the module number is used to indicate module boundaries (e.g. in reflection and in dom, see e.g. dom_objects_set_class_ex), then all sorts of error can happen. In the case of ext/dom, OP's error happens because the following happens: - The property handler is set up incorrectly in dom_objects_set_class_ex() because the wrong module number is specified. The class highest in the hierarchy is DOMNode, so the property handler is incorrectly set to that of DOMNode instead of DOMDocument. - The documentElement property doesn't exist on DOMNode, it only exists on DOMDocument, so it tries to read using zend_std_read_property(). As there is no user property called documentElement, that read operation returns an undef value. However, the type is still checked, resulting in the strange exception. Closes GH-12219. --- NEWS | 2 ++ ext/standard/dl.c | 17 ++++++++++++++++- 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/NEWS b/NEWS index 15ff5939b84..afdebe1d02f 100644 --- a/NEWS +++ b/NEWS @@ -5,6 +5,8 @@ PHP NEWS - Core: . Fixed bug GH-12207 (memory leak when class using trait with doc block). (rioderelfte) + . Fixed bug GH-12215 (Module entry being overwritten causes type errors in + ext/dom). (nielsdos) - Filter: . Fix explicit FILTER_REQUIRE_SCALAR with FILTER_CALLBACK (ilutov) diff --git a/ext/standard/dl.c b/ext/standard/dl.c index 90e538f19d4..0df3848852f 100644 --- a/ext/standard/dl.c +++ b/ext/standard/dl.c @@ -225,15 +225,30 @@ PHPAPI int php_load_extension(const char *filename, int type, int start_now) DL_UNLOAD(handle); return FAILURE; } + + int old_type = module_entry->type; + int old_module_number = module_entry->module_number; + void *old_handle = module_entry->handle; + module_entry->type = type; module_entry->module_number = zend_next_free_module(); module_entry->handle = handle; - if ((module_entry = zend_register_module_ex(module_entry)) == NULL) { + zend_module_entry *added_module_entry; + if ((added_module_entry = zend_register_module_ex(module_entry)) == NULL) { + /* Module loading failed, potentially because the module was already loaded. + * It is especially important in that case to restore the old type, module_number, and handle. + * Overwriting the values for an already-loaded module causes problem when these fields are used + * to uniquely identify module boundaries (e.g. in dom and reflection). */ + module_entry->type = old_type; + module_entry->module_number = old_module_number; + module_entry->handle = old_handle; DL_UNLOAD(handle); return FAILURE; } + module_entry = added_module_entry; + if ((type == MODULE_TEMPORARY || start_now) && zend_startup_module_ex(module_entry) == FAILURE) { DL_UNLOAD(handle); return FAILURE;