From f30caced21f0ee472a1fb48b400d819567bedf3c Mon Sep 17 00:00:00 2001 From: Ilia Alshanetsky Date: Mon, 20 Jul 2026 08:29:57 -0400 Subject: [PATCH] Fix GH-22825: DOM attribute methods on a DTD default attribute xmlHasNsProp() falls back to the internal subset, so the legacy ext/dom attribute methods can receive a DTD attribute declaration instead of an attribute node. setAttribute() and dom_remove_attribute() enumerated only XML_ATTRIBUTE_NODE and XML_NAMESPACE_DECL, so a defaulted attribute asserted on debug builds and ran into __builtin_unreachable() on release builds, and setAttributeNode() skipped the declaration when unlinking the old attribute but still returned it, throwing after the tree was already modified. setAttribute() now creates the attribute as it did before 8.4, matching setAttributeNS(); removeAttribute() reports that nothing was removed, since the declaration lives in the DTD and not on the element; setAttributeNode() returns null; and toggleAttribute() derives its result from the removal instead of assuming it succeeded, so the returned value keeps agreeing with hasAttribute(). getAttribute() and hasAttribute() already handled the declaration, and the Dom\Element methods are unaffected because the spec-compliant lookup never consults the DTD. Fixes GH-22825 --- NEWS | 4 ++ ext/dom/element.c | 13 +++-- ext/dom/tests/gh22825.phpt | 98 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 112 insertions(+), 3 deletions(-) create mode 100644 ext/dom/tests/gh22825.phpt diff --git a/NEWS b/NEWS index ccae29155dbd..cfb82c4f0ca2 100644 --- a/NEWS +++ b/NEWS @@ -5,6 +5,10 @@ PHP NEWS - Date: . Fixed leak on double DatePeriod::__construct() call. (ilutov) +- DOM: + . Fixed bug GH-22825 (DOMElement::setAttribute() fails silently when the DTD + declares a default value for the attribute). (iliaal) + - Sockets: . Fixed various memory related issues in ext/sockets. (David Carlier) diff --git a/ext/dom/element.c b/ext/dom/element.c index e25805df53eb..b2564428db9c 100644 --- a/ext/dom/element.c +++ b/ext/dom/element.c @@ -468,6 +468,8 @@ PHP_METHOD(DOMElement, setAttribute) break; case XML_NAMESPACE_DECL: RETURN_FALSE; + case XML_ATTRIBUTE_DECL: + break; EMPTY_SWITCH_DEFAULT_CASE(); } } @@ -595,6 +597,8 @@ static bool dom_remove_attribute(xmlNodePtr thisp, xmlNodePtr attrp) break; } + case XML_ATTRIBUTE_DECL: + return false; EMPTY_SWITCH_DEFAULT_CASE(); } return true; @@ -726,7 +730,11 @@ static void dom_element_set_attribute_node_common(INTERNAL_FUNCTION_PARAMETERS, existattrp = xmlHasProp(nodep, attrp->name); } - if (existattrp != NULL && existattrp->type != XML_ATTRIBUTE_DECL) { + if (existattrp != NULL && existattrp->type == XML_ATTRIBUTE_DECL) { + existattrp = NULL; + } + + if (existattrp != NULL) { if ((oldobj = php_dom_object_get_data((xmlNodePtr) existattrp)) != NULL && ((php_libxml_node_ptr *)oldobj->ptr)->node == (xmlNodePtr) attrp) { @@ -1788,8 +1796,7 @@ PHP_METHOD(DOMElement, toggleAttribute) /* Step 5 */ if (force_is_null || !force) { - dom_remove_attribute(thisp, attribute); - retval = false; + retval = !dom_remove_attribute(thisp, attribute); goto out; } diff --git a/ext/dom/tests/gh22825.phpt b/ext/dom/tests/gh22825.phpt new file mode 100644 index 000000000000..987ed57a1454 --- /dev/null +++ b/ext/dom/tests/gh22825.phpt @@ -0,0 +1,98 @@ +--TEST-- +GH-22825 (DOMElement::setAttribute() reaches EMPTY_SWITCH_DEFAULT_CASE() with DTD #FIXED default attributes) +--EXTENSIONS-- +dom +--FILE-- +', '', 'A'], + ['', '', 'A'], + ['', '', 'p:A'], +]; + +function element(string $attlist, string $root): DOMElement { + $doc = new DOMDocument(); + $doc->loadXML("$root"); + return $doc->documentElement; +} + +foreach ($cases as [$attlist, $root, $name]) { + echo "--- $attlist ---\n"; + + $el = element($attlist, $root); + echo "hasAttribute: "; + var_dump($el->hasAttribute($name)); + echo "getAttribute: "; + var_dump($el->getAttribute($name)); + + $el = element($attlist, $root); + $result = $el->setAttribute($name, 'v'); + echo "setAttribute: ", is_object($result) ? $result::class : var_export($result, true), "\n"; + echo "after setAttribute: ", trim($el->ownerDocument->saveXML($el)), "\n"; + + $el = element($attlist, $root); + echo "removeAttribute: "; + var_dump($el->removeAttribute($name)); + echo "still present after removeAttribute: "; + var_dump($el->hasAttribute($name)); + + $el = element($attlist, $root); + echo "toggleAttribute(false): "; + var_dump($el->toggleAttribute($name, false)); + echo "still present after toggleAttribute(false): "; + var_dump($el->hasAttribute($name)); + + $el = element($attlist, $root); + echo "toggleAttribute(true): "; + var_dump($el->toggleAttribute($name, true)); + echo "still present after toggleAttribute(true): "; + var_dump($el->hasAttribute($name)); + + $el = element($attlist, $root); + $attr = $el->ownerDocument->createAttribute($name); + $attr->value = 'z'; + echo "setAttributeNode: "; + var_dump($el->setAttributeNode($attr)); + echo "after setAttributeNode: ", trim($el->ownerDocument->saveXML($el)), "\n"; +} +?> +--EXPECT-- +--- --- +hasAttribute: bool(true) +getAttribute: string(1) "d" +setAttribute: DOMAttr +after setAttribute: +removeAttribute: bool(false) +still present after removeAttribute: bool(true) +toggleAttribute(false): bool(true) +still present after toggleAttribute(false): bool(true) +toggleAttribute(true): bool(true) +still present after toggleAttribute(true): bool(true) +setAttributeNode: NULL +after setAttributeNode: +--- --- +hasAttribute: bool(true) +getAttribute: string(1) "d" +setAttribute: DOMAttr +after setAttribute: +removeAttribute: bool(false) +still present after removeAttribute: bool(true) +toggleAttribute(false): bool(true) +still present after toggleAttribute(false): bool(true) +toggleAttribute(true): bool(true) +still present after toggleAttribute(true): bool(true) +setAttributeNode: NULL +after setAttributeNode: +--- --- +hasAttribute: bool(true) +getAttribute: string(1) "d" +setAttribute: DOMAttr +after setAttribute: +removeAttribute: bool(false) +still present after removeAttribute: bool(true) +toggleAttribute(false): bool(true) +still present after toggleAttribute(false): bool(true) +toggleAttribute(true): bool(true) +still present after toggleAttribute(true): bool(true) +setAttributeNode: NULL +after setAttributeNode: