Skip to content

Commit 145e8c2

Browse files
iliaaliliaal
committed
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 Closes GH-22830
1 parent 04f3d28 commit 145e8c2

3 files changed

Lines changed: 112 additions & 3 deletions

File tree

‎NEWS‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,10 @@ PHP NEWS
55
- Date:
66
. Fixed leak on double DatePeriod::__construct() call. (ilutov)
77

8+
- DOM:
9+
. Fixed bug GH-22825 (DOMElement::setAttribute() fails silently when the DTD
10+
declares a default value for the attribute). (iliaal)
11+
812
- Sockets:
913
. Fixed various memory related issues in ext/sockets. (David Carlier)
1014

‎ext/dom/element.c‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -468,6 +468,8 @@ PHP_METHOD(DOMElement, setAttribute)
468468
break;
469469
case XML_NAMESPACE_DECL:
470470
RETURN_FALSE;
471+
case XML_ATTRIBUTE_DECL:
472+
break;
471473
EMPTY_SWITCH_DEFAULT_CASE();
472474
}
473475
}
@@ -595,6 +597,8 @@ static bool dom_remove_attribute(xmlNodePtr thisp, xmlNodePtr attrp)
595597

596598
break;
597599
}
600+
case XML_ATTRIBUTE_DECL:
601+
return false;
598602
EMPTY_SWITCH_DEFAULT_CASE();
599603
}
600604
return true;
@@ -726,7 +730,11 @@ static void dom_element_set_attribute_node_common(INTERNAL_FUNCTION_PARAMETERS,
726730
existattrp = xmlHasProp(nodep, attrp->name);
727731
}
728732

729-
if (existattrp != NULL && existattrp->type != XML_ATTRIBUTE_DECL) {
733+
if (existattrp != NULL && existattrp->type == XML_ATTRIBUTE_DECL) {
734+
existattrp = NULL;
735+
}
736+
737+
if (existattrp != NULL) {
730738
if ((oldobj = php_dom_object_get_data((xmlNodePtr) existattrp)) != NULL &&
731739
((php_libxml_node_ptr *)oldobj->ptr)->node == (xmlNodePtr) attrp)
732740
{
@@ -1788,8 +1796,7 @@ PHP_METHOD(DOMElement, toggleAttribute)
17881796

17891797
/* Step 5 */
17901798
if (force_is_null || !force) {
1791-
dom_remove_attribute(thisp, attribute);
1792-
retval = false;
1799+
retval = !dom_remove_attribute(thisp, attribute);
17931800
goto out;
17941801
}
17951802

‎ext/dom/tests/gh22825.phpt‎

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,98 @@
1+
--TEST--
2+
GH-22825 (DOMElement::setAttribute() reaches EMPTY_SWITCH_DEFAULT_CASE() with DTD #FIXED default attributes)
3+
--EXTENSIONS--
4+
dom
5+
--FILE--
6+
<?php
7+
$cases = [
8+
['<!ATTLIST root A CDATA #FIXED "d">', '<root/>', 'A'],
9+
['<!ATTLIST root A CDATA "d">', '<root/>', 'A'],
10+
['<!ATTLIST root p:A CDATA #FIXED "d">', '<root xmlns:p="urn:p"/>', 'p:A'],
11+
];
12+
13+
function element(string $attlist, string $root): DOMElement {
14+
$doc = new DOMDocument();
15+
$doc->loadXML("<!DOCTYPE root [$attlist]>$root");
16+
return $doc->documentElement;
17+
}
18+
19+
foreach ($cases as [$attlist, $root, $name]) {
20+
echo "--- $attlist ---\n";
21+
22+
$el = element($attlist, $root);
23+
echo "hasAttribute: ";
24+
var_dump($el->hasAttribute($name));
25+
echo "getAttribute: ";
26+
var_dump($el->getAttribute($name));
27+
28+
$el = element($attlist, $root);
29+
$result = $el->setAttribute($name, 'v');
30+
echo "setAttribute: ", is_object($result) ? $result::class : var_export($result, true), "\n";
31+
echo "after setAttribute: ", trim($el->ownerDocument->saveXML($el)), "\n";
32+
33+
$el = element($attlist, $root);
34+
echo "removeAttribute: ";
35+
var_dump($el->removeAttribute($name));
36+
echo "still present after removeAttribute: ";
37+
var_dump($el->hasAttribute($name));
38+
39+
$el = element($attlist, $root);
40+
echo "toggleAttribute(false): ";
41+
var_dump($el->toggleAttribute($name, false));
42+
echo "still present after toggleAttribute(false): ";
43+
var_dump($el->hasAttribute($name));
44+
45+
$el = element($attlist, $root);
46+
echo "toggleAttribute(true): ";
47+
var_dump($el->toggleAttribute($name, true));
48+
echo "still present after toggleAttribute(true): ";
49+
var_dump($el->hasAttribute($name));
50+
51+
$el = element($attlist, $root);
52+
$attr = $el->ownerDocument->createAttribute($name);
53+
$attr->value = 'z';
54+
echo "setAttributeNode: ";
55+
var_dump($el->setAttributeNode($attr));
56+
echo "after setAttributeNode: ", trim($el->ownerDocument->saveXML($el)), "\n";
57+
}
58+
?>
59+
--EXPECT--
60+
--- <!ATTLIST root A CDATA #FIXED "d"> ---
61+
hasAttribute: bool(true)
62+
getAttribute: string(1) "d"
63+
setAttribute: DOMAttr
64+
after setAttribute: <root A="v"/>
65+
removeAttribute: bool(false)
66+
still present after removeAttribute: bool(true)
67+
toggleAttribute(false): bool(true)
68+
still present after toggleAttribute(false): bool(true)
69+
toggleAttribute(true): bool(true)
70+
still present after toggleAttribute(true): bool(true)
71+
setAttributeNode: NULL
72+
after setAttributeNode: <root A="z"/>
73+
--- <!ATTLIST root A CDATA "d"> ---
74+
hasAttribute: bool(true)
75+
getAttribute: string(1) "d"
76+
setAttribute: DOMAttr
77+
after setAttribute: <root A="v"/>
78+
removeAttribute: bool(false)
79+
still present after removeAttribute: bool(true)
80+
toggleAttribute(false): bool(true)
81+
still present after toggleAttribute(false): bool(true)
82+
toggleAttribute(true): bool(true)
83+
still present after toggleAttribute(true): bool(true)
84+
setAttributeNode: NULL
85+
after setAttributeNode: <root A="z"/>
86+
--- <!ATTLIST root p:A CDATA #FIXED "d"> ---
87+
hasAttribute: bool(true)
88+
getAttribute: string(1) "d"
89+
setAttribute: DOMAttr
90+
after setAttribute: <root xmlns:p="urn:p" p:A="v"/>
91+
removeAttribute: bool(false)
92+
still present after removeAttribute: bool(true)
93+
toggleAttribute(false): bool(true)
94+
still present after toggleAttribute(false): bool(true)
95+
toggleAttribute(true): bool(true)
96+
still present after toggleAttribute(true): bool(true)
97+
setAttributeNode: NULL
98+
after setAttributeNode: <root xmlns:p="urn:p" p:A="z"/>

0 commit comments

Comments
 (0)