Skip to content

Fix #47531: No way of removing redundant xmlns: declarations #12542

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Fix #47531: No way of removing redundant xmlns: declarations #12542

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
133 changes: 98 additions & 35 deletions ext/dom/element.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -418,9 +418,68 @@ PHP_METHOD(DOMElement, setAttribute)
}
/* }}} end dom_element_set_attribute */

static bool dom_remove_attribute(xmlNodePtr attrp)
typedef struct {
xmlNodePtr current_node;
xmlNsPtr defined_ns;
} dom_deep_ns_redef_item;

/* Reconciliation for a *single* namespace, but reconciles *closest* to the subtree needing it. */
static void dom_deep_ns_redef(xmlNodePtr node, xmlNsPtr ns_to_redefine)
{
size_t worklist_capacity = 128;
dom_deep_ns_redef_item *worklist = emalloc(sizeof(dom_deep_ns_redef_item) * worklist_capacity);
worklist[0].current_node = node;
worklist[0].defined_ns = NULL;
size_t worklist_size = 1;

while (worklist_size > 0) {
worklist_size--;
dom_deep_ns_redef_item *current_worklist_item = &worklist[worklist_size];
ZEND_ASSERT(current_worklist_item->current_node->type == XML_ELEMENT_NODE);
xmlNsPtr defined_ns = current_worklist_item->defined_ns;

if (current_worklist_item->current_node->ns == ns_to_redefine) {
if (defined_ns == NULL) {
defined_ns = xmlNewNs(current_worklist_item->current_node, ns_to_redefine->href, ns_to_redefine->prefix);
}
current_worklist_item->current_node->ns = defined_ns;
}

for (xmlAttrPtr attr = current_worklist_item->current_node->properties; attr; attr = attr->next) {
if (attr->ns == ns_to_redefine) {
if (defined_ns == NULL) {
defined_ns = xmlNewNs(current_worklist_item->current_node, ns_to_redefine->href, ns_to_redefine->prefix);
}
attr->ns = defined_ns;
}
}

for (xmlNodePtr child = current_worklist_item->current_node->children; child; child = child->next) {
if (child->type != XML_ELEMENT_NODE) {
continue;
}
if (worklist_size == worklist_capacity) {
if (UNEXPECTED(worklist_capacity >= SIZE_MAX / 3 * 2 / sizeof(dom_deep_ns_redef_item))) {
/* Shouldn't be possible to hit, but checked for safety anyway */
return;
}
worklist_capacity = worklist_capacity * 3 / 2;
worklist = erealloc(worklist, sizeof(dom_deep_ns_redef_item) * worklist_capacity);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe use safe_erealloc() instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks sensible to me, maybe this should land in 8.3?

Probably. Could be landed in even lower versions as this is a proper bugfix, but I don't dare to do that because of reasons you know...

Maybe use safe_erealloc() instead?

I have to check for overflow of doing *3/2 already, so I folded the overflow check for sizeof(dom_deep_ns_redef_item) * worklist_capacity into that one already. I therefore don't think it's necessary to use safe_erealloc.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK for the realloc.

8.3 should be safe as it hasn't been "properly" released yet IMHO.

}
worklist[worklist_size].current_node = child;
worklist[worklist_size].defined_ns = defined_ns;
worklist_size++;
}
}

efree(worklist);
}

static bool dom_remove_attribute(xmlNodePtr thisp, xmlNodePtr attrp)
{
ZEND_ASSERT(thisp != NULL);
ZEND_ASSERT(attrp != NULL);

switch (attrp->type) {
case XML_ATTRIBUTE_NODE:
if (php_dom_object_get_data(attrp) == NULL) {
Expand All @@ -431,8 +490,42 @@ static bool dom_remove_attribute(xmlNodePtr attrp)
xmlUnlinkNode(attrp);
}
break;
case XML_NAMESPACE_DECL:
return false;
case XML_NAMESPACE_DECL: {
/* They will always be removed, but can be re-added.
*
* If any reference was left to the namespace, the only effect is that
* the definition is potentially moved closer to the element using it.
* If no reference was left, it is actually removed. */
xmlNsPtr ns = (xmlNsPtr) attrp;
if (thisp->nsDef == ns) {
thisp->nsDef = ns->next;
} else if (thisp->nsDef != NULL) {
xmlNsPtr prev = thisp->nsDef;
xmlNsPtr cur = prev->next;
while (cur) {
if (cur == ns) {
prev->next = cur->next;
break;
}
prev = cur;
cur = cur->next;
}
} else {
/* defensive: attrp not defined in thisp ??? */
#if ZEND_DEBUG
ZEND_UNREACHABLE();
#endif
break; /* defensive */
}

ns->next = NULL;
php_libxml_set_old_ns(thisp->doc, ns); /* note: can't deallocate as it might be referenced by a "fake namespace node" */
/* xmlReconciliateNs() redefines at the top of the tree instead of closest to the child, own reconciliation here.
* Similarly, the DOM version has other issues too (see dom_libxml_reconcile_ensure_namespaces_are_declared). */
dom_deep_ns_redef(thisp, ns);

break;
}
EMPTY_SWITCH_DEFAULT_CASE();
}
return true;
Expand Down Expand Up @@ -461,7 +554,7 @@ PHP_METHOD(DOMElement, removeAttribute)
RETURN_FALSE;
}

RETURN_BOOL(dom_remove_attribute(attrp));
RETURN_BOOL(dom_remove_attribute(nodep, attrp));
}
/* }}} end dom_element_remove_attribute */

Expand Down Expand Up @@ -1425,37 +1518,7 @@ PHP_METHOD(DOMElement, toggleAttribute)

/* Step 5 */
if (force_is_null || !force) {
if (attribute->type == XML_NAMESPACE_DECL) {
/* The behaviour isn't defined by spec, but by observing browsers I found
* that you can remove the nodes, but they'll get reconciled.
* So if any reference was left to the namespace, the only effect is that
* the definition is potentially moved closer to the element using it.
* If no reference was left, it is actually removed. */
xmlNsPtr ns = (xmlNsPtr) attribute;
if (thisp->nsDef == ns) {
thisp->nsDef = ns->next;
} else if (thisp->nsDef != NULL) {
xmlNsPtr prev = thisp->nsDef;
xmlNsPtr cur = prev->next;
while (cur) {
if (cur == ns) {
prev->next = cur->next;
break;
}
prev = cur;
cur = cur->next;
}
}

ns->next = NULL;
php_libxml_set_old_ns(thisp->doc, ns);
dom_reconcile_ns(thisp->doc, thisp);
} else {
/* TODO: in the future when namespace bugs are fixed,
* the above if-branch should be merged into this called function
* such that the removal will work properly with all APIs. */
dom_remove_attribute(attribute);
}
dom_remove_attribute(thisp, attribute);
retval = false;
goto out;
}
Expand Down
14 changes: 7 additions & 7 deletions ext/dom/tests/DOMElement_toggleAttribute.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,26 +123,26 @@ bool(true)
Toggling namespaces:
bool(false)
<?xml version="1.0"?>
<default:container xmlns:foo="some:ns2" xmlns:anotherone="some:ns3" xmlns:default="some:ns"><foo:bar/><default:baz/></default:container>
<container xmlns:foo="some:ns2" xmlns:anotherone="some:ns3" xmlns="some:ns"><foo:bar/><baz/></container>
bool(false)
<?xml version="1.0"?>
<default:container xmlns:foo="some:ns2" xmlns:default="some:ns"><foo:bar/><default:baz/></default:container>
<container xmlns:foo="some:ns2" xmlns="some:ns"><foo:bar/><baz/></container>
bool(true)
<?xml version="1.0"?>
<default:container xmlns:foo="some:ns2" xmlns:default="some:ns" xmlns:anotherone=""><foo:bar/><default:baz/></default:container>
<container xmlns:foo="some:ns2" xmlns="some:ns" xmlns:anotherone=""><foo:bar/><baz/></container>
bool(false)
<?xml version="1.0"?>
<default:container xmlns:default="some:ns" xmlns:anotherone="" xmlns:foo="some:ns2"><foo:bar/><default:baz/></default:container>
<container xmlns="some:ns" xmlns:anotherone=""><foo:bar xmlns:foo="some:ns2"/><baz/></container>
bool(false)
<?xml version="1.0"?>
<default:container xmlns:default="some:ns" xmlns:anotherone="" xmlns:foo="some:ns2"><foo:bar/><default:baz/></default:container>
<container xmlns="some:ns" xmlns:anotherone=""><foo:bar xmlns:foo="some:ns2"/><baz/></container>
Toggling namespaced attributes:
bool(true)
bool(true)
bool(true)
bool(false)
<?xml version="1.0"?>
<default:container xmlns:default="some:ns" xmlns:anotherone="" xmlns:foo="some:ns2" test:test=""><foo:bar foo:test="" doesnotexist:test=""/><default:baz/></default:container>
<container xmlns="some:ns" xmlns:anotherone="" test:test=""><foo:bar xmlns:foo="some:ns2" foo:test="" doesnotexist:test=""/><baz/></container>
namespace of test:test = NULL
namespace of foo:test = string(8) "some:ns2"
namespace of doesnotexist:test = NULL
Expand All @@ -153,6 +153,6 @@ bool(false)
bool(true)
bool(false)
<?xml version="1.0"?>
<default:container xmlns:default="some:ns" xmlns:anotherone="" xmlns:foo="some:ns2"><foo:bar doesnotexist:test2=""/><default:baz/></default:container>
<container xmlns="some:ns" xmlns:anotherone=""><foo:bar xmlns:foo="some:ns2" doesnotexist:test2=""/><baz/></container>
Checking toggled namespace:
string(0) ""
66 changes: 66 additions & 0 deletions ext/dom/tests/bug47531_a.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
--TEST--
Bug #47531 (No way of removing redundant xmlns: declarations)
--EXTENSIONS--
dom
--FILE--
<?php

$doc = new DOMDocument;
$doc->loadXML(<<<XML
<container xmlns:foo="some:ns">
<foo:first/>
<second>
<foo:child1/>
<foo:child2/>
<!-- comment -->
<child3>
<foo:child4/>
<foo:child5>
<p xmlns:foo="other:ns">
<span foo:foo="bar"/>
<span foo:foo="bar"/>
</p>
<foo:child6 foo:foo="bar">
<span foo:foo="bar"/>
<span foo:foo="bar"/>
</foo:child6>
</foo:child5>
</child3>
<child7 foo:foo="bar">
<foo:child8/>
</child7>
</second>
</container>
XML);
$root = $doc->documentElement;
var_dump($root->removeAttribute("xmlns:foo"));
echo $doc->saveXML();

?>
--EXPECT--
bool(true)
<?xml version="1.0"?>
<container>
<foo:first xmlns:foo="some:ns"/>
<second>
<foo:child1 xmlns:foo="some:ns"/>
<foo:child2 xmlns:foo="some:ns"/>
<!-- comment -->
<child3>
<foo:child4 xmlns:foo="some:ns"/>
<foo:child5 xmlns:foo="some:ns">
<p xmlns:foo="other:ns">
<span foo:foo="bar"/>
<span foo:foo="bar"/>
</p>
<foo:child6 foo:foo="bar">
<span foo:foo="bar"/>
<span foo:foo="bar"/>
</foo:child6>
</foo:child5>
</child3>
<child7 xmlns:foo="some:ns" foo:foo="bar">
<foo:child8/>
</child7>
</second>
</container>
67 changes: 67 additions & 0 deletions ext/dom/tests/bug47531_b.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
--TEST--
Bug #47531 (No way of removing redundant xmlns: declarations)
--EXTENSIONS--
dom
--FILE--
<?php

$doc = new DOMDocument;
$doc->loadXML(<<<XML
<container xmlns:foo="some:ns">
<foo:first/>
<foo:second xmlns:foo="some:ns2">
<foo:child1/>
<foo:child2/>
<!-- comment -->
<child3>
<foo:child4/>
<foo:child5 xmlns:foo="some:ns3">
<p xmlns:foo="other:ns">
<span foo:foo="bar"/>
<span foo:foo="bar"/>
</p>
<foo:child6 foo:foo="bar">
<span foo:foo="bar"/>
<span foo:foo="bar"/>
</foo:child6>
</foo:child5>
</child3>
<child7 xmlns:foo="some:ns" foo:foo="bar">
<foo:child8/>
</child7>
</foo:second>
</container>
XML);
$root = $doc->documentElement;
$elem = $root->firstElementChild->nextElementSibling;
var_dump($elem->removeAttribute("xmlns:foo"));
echo $doc->saveXML();

?>
--EXPECT--
bool(true)
<?xml version="1.0"?>
<container xmlns:foo="some:ns">
<foo:first/>
<foo:second xmlns:foo="some:ns2">
<foo:child1/>
<foo:child2/>
<!-- comment -->
<child3>
<foo:child4/>
<foo:child5 xmlns:foo="some:ns3">
<p xmlns:foo="other:ns">
<span foo:foo="bar"/>
<span foo:foo="bar"/>
</p>
<foo:child6 foo:foo="bar">
<span foo:foo="bar"/>
<span foo:foo="bar"/>
</foo:child6>
</foo:child5>
</child3>
<child7 xmlns:foo="some:ns" foo:foo="bar">
<foo:child8/>
</child7>
</foo:second>
</container>