diff --git a/NEWS b/NEWS
index e3b60ecc14a4..74ce6669549c 100644
--- a/NEWS
+++ b/NEWS
@@ -67,6 +67,8 @@ PHP NEWS
NULL deref on first use after a failed open). (Girgias)
- DOM:
+ . Fixed a use-after-free when reflected attribute setters replace an
+ attribute whose child still has a live wrapper. (Jakub Zelenka)
. Fixed bug GH-23352 (UAF reading an attribute value node retained across
DOMDocument::adoptNode()). (David Carlier)
. Fixed bug GH-24260 (querySelectorAll() returns duplicate elements for
diff --git a/ext/dom/element.c b/ext/dom/element.c
index 8af5d54db338..f370b0e958db 100644
--- a/ext/dom/element.c
+++ b/ext/dom/element.c
@@ -156,7 +156,7 @@ static xmlAttrPtr dom_element_reflected_attribute_write(dom_object *obj, zval *n
/* Typed property, so it is a string already */
ZEND_ASSERT(Z_TYPE_P(newval) == IS_STRING);
- return xmlSetNsProp(nodep, NULL, (const xmlChar *) name, (const xmlChar *) Z_STRVAL_P(newval));
+ return dom_set_attribute_value(nodep, NULL, (const xmlChar *) name, (const xmlChar *) Z_STRVAL_P(newval));
}
/* {{{ className string
@@ -1030,11 +1030,7 @@ static void dom_set_attribute_ns_modern(dom_object *intern, xmlNodePtr elemp, ze
if (errorcode == 0) {
php_dom_libxml_ns_mapper *ns_mapper = php_dom_get_ns_mapper(intern);
xmlNsPtr ns = php_dom_libxml_ns_mapper_get_ns_raw_prefix_string(ns_mapper, prefix, xmlStrlen(prefix), uri);
- xmlNodePtr existing = (xmlNodePtr) xmlHasNsProp(elemp, localname, ns == NULL ? NULL : ns->href);
- if (existing != NULL && existing->type != XML_ATTRIBUTE_DECL) {
- node_list_unlink(existing->children);
- }
- xmlAttrPtr attr = xmlSetNsProp(elemp, ns, localname, BAD_CAST value);
+ xmlAttrPtr attr = dom_set_attribute_value(elemp, ns, localname, BAD_CAST value);
if (UNEXPECTED(attr == NULL)) {
php_dom_throw_error(INVALID_STATE_ERR, /* strict */ true);
} else {
diff --git a/ext/dom/php_dom.c b/ext/dom/php_dom.c
index 7bc99e68794c..445eacdfa198 100644
--- a/ext/dom/php_dom.c
+++ b/ext/dom/php_dom.c
@@ -2408,6 +2408,15 @@ void dom_remove_all_children(xmlNodePtr nodep)
}
}
+xmlAttrPtr dom_set_attribute_value(xmlNodePtr element, xmlNsPtr ns, const xmlChar *name, const xmlChar *value)
+{
+ xmlAttrPtr existing = xmlHasNsProp(element, name, ns == NULL ? NULL : ns->href);
+ if (existing != NULL && existing->type != XML_ATTRIBUTE_DECL) {
+ node_list_unlink(existing->children);
+ }
+ return xmlSetNsProp(element, ns, name, value);
+}
+
void php_dom_get_content_into_zval(const xmlNode *nodep, zval *return_value, bool null_on_failure)
{
ZEND_ASSERT(nodep != NULL);
diff --git a/ext/dom/php_dom.h b/ext/dom/php_dom.h
index d399d745084b..2e6f6ef7c452 100644
--- a/ext/dom/php_dom.h
+++ b/ext/dom/php_dom.h
@@ -140,6 +140,7 @@ zend_object *dom_xpath_objects_new(zend_class_entry *class_type);
#endif
bool dom_get_strict_error(php_libxml_ref_obj *document);
void node_list_unlink(xmlNodePtr node);
+xmlAttrPtr dom_set_attribute_value(xmlNodePtr element, xmlNsPtr ns, const xmlChar *name, const xmlChar *value);
int dom_check_qname(char *qname, char **localname, char **prefix, int uri_len, int name_len);
xmlNsPtr dom_get_ns(xmlNodePtr node, char *uri, int *errorcode, char *prefix);
xmlNsPtr dom_get_ns_unchecked(xmlNodePtr nodep, char *uri, char *prefix);
diff --git a/ext/dom/tests/reflected_attribute_write_live_child.phpt b/ext/dom/tests/reflected_attribute_write_live_child.phpt
new file mode 100644
index 000000000000..b387784a32ca
--- /dev/null
+++ b/ext/dom/tests/reflected_attribute_write_live_child.phpt
@@ -0,0 +1,63 @@
+--TEST--
+Reflected attribute writes keep attribute children with live wrappers
+--EXTENSIONS--
+dom
+--FILE--
+documentElement;
+ $text = $element->getAttributeNode('class')->firstChild;
+
+ $write($element);
+
+ echo $label, ": detached: ";
+ var_dump($text->parentNode === null);
+ echo $label, ": text: ";
+ var_dump($text->textContent);
+ echo $label, ": new value: ";
+ var_dump($element->getAttribute('class'));
+}
+
+$doc = new DOMDocument();
+$doc->loadXML('');
+check('legacy className', $doc, static fn ($element) => $element->className = 'new');
+
+$doc = Dom\XMLDocument::createFromString('');
+check('modern className', $doc, static fn ($element) => $element->className = 'new');
+
+$doc = Dom\XMLDocument::createFromString('');
+check('classList value', $doc, static fn ($element) => $element->classList->value = 'new');
+
+$doc = Dom\XMLDocument::createFromString('');
+check('classList update', $doc, static fn ($element) => $element->classList->add('new'));
+
+$doc = Dom\XMLDocument::createFromString('');
+$element = $doc->documentElement;
+$element->id = 'old';
+$text = $element->getAttributeNode('id')->firstChild;
+$element->id = 'new';
+echo "id: detached: ";
+var_dump($text->parentNode === null);
+echo "id: text: ";
+var_dump($text->textContent);
+echo "id: new value: ";
+var_dump($element->id);
+
+?>
+--EXPECT--
+legacy className: detached: bool(true)
+legacy className: text: string(3) "old"
+legacy className: new value: string(3) "new"
+modern className: detached: bool(true)
+modern className: text: string(3) "old"
+modern className: new value: string(3) "new"
+classList value: detached: bool(true)
+classList value: text: string(3) "old"
+classList value: new value: string(3) "new"
+classList update: detached: bool(true)
+classList update: text: string(3) "old"
+classList update: new value: string(7) "old new"
+id: detached: bool(true)
+id: text: string(3) "old"
+id: new value: string(3) "new"
diff --git a/ext/dom/token_list.c b/ext/dom/token_list.c
index 14d310cd5a70..b9f3b330b4f4 100644
--- a/ext/dom/token_list.c
+++ b/ext/dom/token_list.c
@@ -143,7 +143,7 @@ static void dom_token_list_update(dom_token_list_object *intern)
/* 2. Set an attribute value for the associated element using associated attribute’s local name and the result of
* running the ordered set serializer for token set. */
char *value = dom_ordered_set_serializer(token_set);
- xmlSetNsProp(dom_token_list_get_element(intern), NULL, BAD_CAST "class", BAD_CAST value);
+ dom_set_attribute_value(dom_token_list_get_element(intern), NULL, BAD_CAST "class", BAD_CAST value);
efree(intern->cached_string);
intern->cached_string = value;
}
@@ -382,7 +382,7 @@ zend_result dom_token_list_value_write(dom_object *obj, zval *newval)
zend_value_error("Value must not contain any null bytes");
return FAILURE;
}
- xmlSetNsProp(dom_token_list_get_element(intern), NULL, BAD_CAST "class", BAD_CAST Z_STRVAL_P(newval));
+ dom_set_attribute_value(dom_token_list_get_element(intern), NULL, BAD_CAST "class", BAD_CAST Z_STRVAL_P(newval));
/* Note: we don't update the set here, the set is always lazily updated for performance reasons. */
return SUCCESS;
}