Skip to content

Commit f2d673f

Browse files
nielsdosnielsdos
committed
Fix #70359 and #78577: segfaults with DOMNameSpaceNode
* Fix type confusion and parent reference * Manually manage the lifetime of the parent * Add regression tests * Break out to a helper, and apply the use-after-free fix to xpath Closes phpGH-11402.
1 parent 3fc013b commit f2d673f

9 files changed

+292
-57
lines changed

‎NEWS

+3
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,9 @@ PHP NEWS
2727
with itself). (nielsdos)
2828
. Fixed bug #77686 (Removed elements are still returned by getElementById).
2929
(nielsdos)
30+
. Fixed bug #70359 (print_r() on DOMAttr causes Segfault in
31+
php_libxml_node_free_list()). (nielsdos)
32+
. Fixed bug #78577 (Crash in DOMNameSpace debug info handlers). (nielsdos)
3033

3134
- Opcache:
3235
. Fix allocation loop in zend_shared_alloc_startup(). (nielsdos)

‎ext/dom/element.c

+7-18
Original file line numberDiff line numberDiff line change
@@ -150,6 +150,7 @@ int dom_element_schema_type_info_read(dom_object *obj, zval *retval)
150150

151151
/* }}} */
152152

153+
/* Note: the object returned is not necessarily a node, but can be an attribute or a namespace declaration. */
153154
static xmlNodePtr dom_get_dom1_attribute(xmlNodePtr elem, xmlChar *name) /* {{{ */
154155
{
155156
int len;
@@ -376,25 +377,13 @@ PHP_METHOD(DOMElement, getAttributeNode)
376377
}
377378

378379
if (attrp->type == XML_NAMESPACE_DECL) {
379-
xmlNsPtr curns;
380-
xmlNodePtr nsparent;
381-
382-
nsparent = attrp->_private;
383-
curns = xmlNewNs(NULL, attrp->name, NULL);
384-
if (attrp->children) {
385-
curns->prefix = xmlStrdup((xmlChar *) attrp->children);
386-
}
387-
if (attrp->children) {
388-
attrp = xmlNewDocNode(nodep->doc, NULL, (xmlChar *) attrp->children, attrp->name);
389-
} else {
390-
attrp = xmlNewDocNode(nodep->doc, NULL, (xmlChar *)"xmlns", attrp->name);
391-
}
392-
attrp->type = XML_NAMESPACE_DECL;
393-
attrp->parent = nsparent;
394-
attrp->ns = curns;
380+
xmlNsPtr original = (xmlNsPtr) attrp;
381+
/* Keep parent alive, because we're a fake child. */
382+
GC_ADDREF(&intern->std);
383+
(void) php_dom_create_fake_namespace_decl(nodep, original, return_value, intern);
384+
} else {
385+
DOM_RET_OBJ((xmlNodePtr) attrp, &ret, intern);
395386
}
396-
397-
DOM_RET_OBJ((xmlNodePtr) attrp, &ret, intern);
398387
}
399388
/* }}} end dom_element_get_attribute_node */
400389

‎ext/dom/php_dom.c

+56-5
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,7 @@ PHP_DOM_EXPORT zend_class_entry *dom_namespace_node_class_entry;
6161

6262
zend_object_handlers dom_object_handlers;
6363
zend_object_handlers dom_nnodemap_object_handlers;
64+
zend_object_handlers dom_object_namespace_node_handlers;
6465
#ifdef LIBXML_XPATH_ENABLED
6566
zend_object_handlers dom_xpath_object_handlers;
6667
#endif
@@ -86,6 +87,9 @@ static HashTable dom_xpath_prop_handlers;
8687
#endif
8788
/* }}} */
8889

90+
static zend_object *dom_objects_namespace_node_new(zend_class_entry *class_type);
91+
static void dom_object_namespace_node_free_storage(zend_object *object);
92+
8993
typedef int (*dom_read_t)(dom_object *obj, zval *retval);
9094
typedef int (*dom_write_t)(dom_object *obj, zval *newval);
9195

@@ -570,6 +574,10 @@ PHP_MINIT_FUNCTION(dom)
570574
dom_nnodemap_object_handlers.read_dimension = dom_nodelist_read_dimension;
571575
dom_nnodemap_object_handlers.has_dimension = dom_nodelist_has_dimension;
572576

577+
memcpy(&dom_object_namespace_node_handlers, &dom_object_handlers, sizeof(zend_object_handlers));
578+
dom_object_namespace_node_handlers.offset = XtOffsetOf(dom_object_namespace_node, dom.std);
579+
dom_object_namespace_node_handlers.free_obj = dom_object_namespace_node_free_storage;
580+
573581
zend_hash_init(&classes, 0, NULL, NULL, 1);
574582

575583
dom_domexception_class_entry = register_class_DOMException(zend_ce_exception);
@@ -604,7 +612,7 @@ PHP_MINIT_FUNCTION(dom)
604612
zend_hash_add_ptr(&classes, dom_node_class_entry->name, &dom_node_prop_handlers);
605613

606614
dom_namespace_node_class_entry = register_class_DOMNameSpaceNode();
607-
dom_namespace_node_class_entry->create_object = dom_objects_new;
615+
dom_namespace_node_class_entry->create_object = dom_objects_namespace_node_new;
608616

609617
zend_hash_init(&dom_namespace_node_prop_handlers, 0, NULL, dom_dtor_prop_handler, 1);
610618
dom_register_prop_handler(&dom_namespace_node_prop_handlers, "nodeName", sizeof("nodeName")-1, dom_node_node_name_read, NULL);
@@ -1001,10 +1009,8 @@ void dom_namednode_iter(dom_object *basenode, int ntype, dom_object *intern, xml
10011009
}
10021010
/* }}} */
10031011

1004-
static dom_object* dom_objects_set_class(zend_class_entry *class_type) /* {{{ */
1012+
static void dom_objects_set_class_ex(zend_class_entry *class_type, dom_object *intern)
10051013
{
1006-
dom_object *intern = zend_object_alloc(sizeof(dom_object), class_type);
1007-
10081014
zend_class_entry *base_class = class_type;
10091015
while ((base_class->type != ZEND_INTERNAL_CLASS || base_class->info.internal.module->module_number != dom_module_entry.module_number) && base_class->parent != NULL) {
10101016
base_class = base_class->parent;
@@ -1014,10 +1020,14 @@ static dom_object* dom_objects_set_class(zend_class_entry *class_type) /* {{{ */
10141020

10151021
zend_object_std_init(&intern->std, class_type);
10161022
object_properties_init(&intern->std, class_type);
1023+
}
10171024

1025+
static dom_object* dom_objects_set_class(zend_class_entry *class_type)
1026+
{
1027+
dom_object *intern = zend_object_alloc(sizeof(dom_object), class_type);
1028+
dom_objects_set_class_ex(class_type, intern);
10181029
return intern;
10191030
}
1020-
/* }}} */
10211031

10221032
/* {{{ dom_objects_new */
10231033
zend_object *dom_objects_new(zend_class_entry *class_type)
@@ -1028,6 +1038,25 @@ zend_object *dom_objects_new(zend_class_entry *class_type)
10281038
}
10291039
/* }}} */
10301040

1041+
static zend_object *dom_objects_namespace_node_new(zend_class_entry *class_type)
1042+
{
1043+
dom_object_namespace_node *intern = zend_object_alloc(sizeof(dom_object_namespace_node), class_type);
1044+
dom_objects_set_class_ex(class_type, &intern->dom);
1045+
intern->dom.std.handlers = &dom_object_namespace_node_handlers;
1046+
return &intern->dom.std;
1047+
}
1048+
1049+
static void dom_object_namespace_node_free_storage(zend_object *object)
1050+
{
1051+
dom_object_namespace_node *intern = php_dom_namespace_node_obj_from_obj(object);
1052+
if (intern->parent_intern != NULL) {
1053+
zval tmp;
1054+
ZVAL_OBJ(&tmp, &intern->parent_intern->std);
1055+
zval_ptr_dtor(&tmp);
1056+
}
1057+
dom_objects_free_storage(object);
1058+
}
1059+
10311060
#ifdef LIBXML_XPATH_ENABLED
10321061
/* {{{ zend_object dom_xpath_objects_new(zend_class_entry *class_type) */
10331062
zend_object *dom_xpath_objects_new(zend_class_entry *class_type)
@@ -1550,6 +1579,28 @@ xmlNsPtr dom_get_nsdecl(xmlNode *node, xmlChar *localName) {
15501579
}
15511580
/* }}} end dom_get_nsdecl */
15521581

1582+
/* Note: Assumes the additional lifetime was already added in the caller. */
1583+
xmlNodePtr php_dom_create_fake_namespace_decl(xmlNodePtr nodep, xmlNsPtr original, zval *return_value, dom_object *parent_intern)
1584+
{
1585+
xmlNodePtr attrp;
1586+
xmlNsPtr curns = xmlNewNs(NULL, original->href, NULL);
1587+
if (original->prefix) {
1588+
curns->prefix = xmlStrdup(original->prefix);
1589+
attrp = xmlNewDocNode(nodep->doc, NULL, (xmlChar *) original->prefix, original->href);
1590+
} else {
1591+
attrp = xmlNewDocNode(nodep->doc, NULL, (xmlChar *)"xmlns", original->href);
1592+
}
1593+
attrp->type = XML_NAMESPACE_DECL;
1594+
attrp->parent = nodep;
1595+
attrp->ns = curns;
1596+
1597+
php_dom_create_object(attrp, return_value, parent_intern);
1598+
/* This object must exist, because we just created an object for it via php_dom_create_object(). */
1599+
dom_object *obj = ((php_libxml_node_ptr *)attrp->_private)->_private;
1600+
php_dom_namespace_node_obj_from_obj(&obj->std)->parent_intern = parent_intern;
1601+
return attrp;
1602+
}
1603+
15531604
static zval *dom_nodelist_read_dimension(zend_object *object, zval *offset, int type, zval *rv) /* {{{ */
15541605
{
15551606
zval offset_copy;

‎ext/dom/php_dom.h

+13
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,18 @@ typedef struct {
9393
HashPosition pos;
9494
} php_dom_iterator;
9595

96+
typedef struct {
97+
/* This may be a fake object that isn't actually in the children list of the parent.
98+
* This is because some namespace declaration nodes aren't stored on the parent in libxml2, so we have to fake it.
99+
* We could use a zval for this, but since this is always going to be an object let's save space... */
100+
dom_object *parent_intern;
101+
dom_object dom;
102+
} dom_object_namespace_node;
103+
104+
static inline dom_object_namespace_node *php_dom_namespace_node_obj_from_obj(zend_object *obj) {
105+
return (dom_object_namespace_node*)((char*)(obj) - XtOffsetOf(dom_object_namespace_node, dom.std));
106+
}
107+
96108
#include "domexception.h"
97109

98110
dom_object *dom_object_get_data(xmlNodePtr obj);
@@ -126,6 +138,7 @@ xmlNode *php_dom_libxml_hash_iter(xmlHashTable *ht, int index);
126138
xmlNode *php_dom_libxml_notation_iter(xmlHashTable *ht, int index);
127139
zend_object_iterator *php_dom_get_iterator(zend_class_entry *ce, zval *object, int by_ref);
128140
void dom_set_doc_classmap(php_libxml_ref_obj *document, zend_class_entry *basece, zend_class_entry *ce);
141+
xmlNodePtr php_dom_create_fake_namespace_decl(xmlNodePtr nodep, xmlNsPtr original, zval *return_value, dom_object *parent_intern);
129142

130143
void dom_parent_node_prepend(dom_object *context, zval *nodes, int nodesc);
131144
void dom_parent_node_append(dom_object *context, zval *nodes, int nodesc);

‎ext/dom/tests/bug70359.phpt

+83
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,83 @@
1+
--TEST--
2+
Bug #70359 (print_r() on DOMAttr causes Segfault in php_libxml_node_free_list())
3+
--EXTENSIONS--
4+
dom
5+
--FILE--
6+
<?php
7+
8+
echo "-- Test without parent --\n";
9+
10+
$dom = new DOMDocument();
11+
$dom->loadXML(<<<XML
12+
<?xml version="1.0" encoding="UTF-8"?>
13+
<urlset xmlns="http://www.sitemaps.org/schemas/sitemap/0.9" xmlns:xsi="fooooooooooooooooooooo"/>
14+
XML);
15+
$spaceNode = $dom->documentElement->getAttributeNode('xmlns');
16+
print_r($spaceNode);
17+
18+
echo "-- Test with parent and non-ns attribute --\n";
19+
20+
$dom = new DOMDocument();
21+
$dom->loadXML(<<<XML
22+
<?xml version="1.0" encoding="UTF-8"?>
23+
<urlset xmlns="http://www.sitemaps.org/schemas/sitemap/0.9">
24+
<url xmlns:xsi="fooooooooooooooooooooo" myattrib="bar"/>
25+
</urlset>
26+
XML);
27+
$spaceNode = $dom->documentElement->firstElementChild->getAttributeNode('myattrib');
28+
var_dump($spaceNode->nodeType);
29+
var_dump($spaceNode->nodeValue);
30+
31+
$dom->documentElement->firstElementChild->remove();
32+
try {
33+
print_r($spaceNode->parentNode);
34+
} catch (\Error $e) {
35+
echo $e->getMessage(), "\n";
36+
}
37+
38+
echo "-- Test with parent and ns attribute --\n";
39+
40+
$dom = new DOMDocument();
41+
$dom->loadXML(<<<XML
42+
<?xml version="1.0" encoding="UTF-8"?>
43+
<urlset xmlns="http://www.sitemaps.org/schemas/sitemap/0.9">
44+
<url xmlns:xsi="fooooooooooooooooooooo" myattrib="bar"/>
45+
</urlset>
46+
XML);
47+
$spaceNode = $dom->documentElement->firstElementChild->getAttributeNode('xmlns:xsi');
48+
print_r($spaceNode);
49+
50+
$dom->documentElement->firstElementChild->remove();
51+
var_dump($spaceNode->parentNode->nodeName); // Shouldn't crash
52+
53+
?>
54+
--EXPECT--
55+
-- Test without parent --
56+
DOMNameSpaceNode Object
57+
(
58+
[nodeName] => xmlns
59+
[nodeValue] => http://www.sitemaps.org/schemas/sitemap/0.9
60+
[nodeType] => 18
61+
[prefix] =>
62+
[localName] => xmlns
63+
[namespaceURI] => http://www.sitemaps.org/schemas/sitemap/0.9
64+
[ownerDocument] => (object value omitted)
65+
[parentNode] => (object value omitted)
66+
)
67+
-- Test with parent and non-ns attribute --
68+
int(2)
69+
string(3) "bar"
70+
Couldn't fetch DOMAttr. Node no longer exists
71+
-- Test with parent and ns attribute --
72+
DOMNameSpaceNode Object
73+
(
74+
[nodeName] => xmlns:xsi
75+
[nodeValue] => fooooooooooooooooooooo
76+
[nodeType] => 18
77+
[prefix] => xsi
78+
[localName] => xsi
79+
[namespaceURI] => fooooooooooooooooooooo
80+
[ownerDocument] => (object value omitted)
81+
[parentNode] => (object value omitted)
82+
)
83+
string(3) "url"

‎ext/dom/tests/bug78577.phpt

+33
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
--TEST--
2+
Bug #78577 (Crash in DOMNameSpace debug info handlers)
3+
--EXTENSIONS--
4+
dom
5+
--FILE--
6+
<?php
7+
8+
$doc = new DOMDocument;
9+
$doc->loadXML('<foo xmlns="http://php.net/test" xmlns:foo="urn:foo" />');
10+
11+
$attr = $doc->documentElement->getAttributeNode('xmlns');
12+
var_dump($attr);
13+
14+
?>
15+
--EXPECT--
16+
object(DOMNameSpaceNode)#3 (8) {
17+
["nodeName"]=>
18+
string(5) "xmlns"
19+
["nodeValue"]=>
20+
string(19) "http://php.net/test"
21+
["nodeType"]=>
22+
int(18)
23+
["prefix"]=>
24+
string(0) ""
25+
["localName"]=>
26+
string(5) "xmlns"
27+
["namespaceURI"]=>
28+
string(19) "http://php.net/test"
29+
["ownerDocument"]=>
30+
string(22) "(object value omitted)"
31+
["parentNode"]=>
32+
string(22) "(object value omitted)"
33+
}

‎ext/dom/tests/xpath_domnamespacenode.phpt

+1-1
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ var_dump($nodes->item(0));
1717

1818
?>
1919
--EXPECT--
20-
object(DOMNameSpaceNode)#3 (8) {
20+
object(DOMNameSpaceNode)#4 (8) {
2121
["nodeName"]=>
2222
string(9) "xmlns:xml"
2323
["nodeValue"]=>

0 commit comments

Comments
 (0)