Skip to content

Commit 9e69641

Browse files
iliaaliliaal
committed
Fix SimpleXML integer offsets that cannot resolve aliasing a node
sxe_get_element_by_offset scanned with nodendx <= offset, so a negative offset skipped the loop and returned the node it started from, and the SXE_ITER_NONE branches of the read and write handlers aliased the node for every offset other than 0. Reads and isset() reported an existing element and writes overwrote it. An offset that resolves to no element now warns and leaves the document alone. Closes GH-23068
1 parent 067f438 commit 9e69641

3 files changed

Lines changed: 121 additions & 15 deletions

File tree

‎NEWS‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,10 @@ PHP NEWS
5555
. Fixed bug GH-23043 (broken session id code can cause zend_mm_heap
5656
corrupted). (ndossche)
5757

58+
- SimpleXML:
59+
. Fixed integer element offsets that cannot resolve aliasing an existing
60+
element. (iliaal)
61+
5862
- Sockets:
5963
. Fixed various memory related issues in ext/sockets. (David Carlier)
6064

‎ext/simplexml/simplexml.c‎

Lines changed: 23 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -134,7 +134,7 @@ static xmlNodePtr sxe_get_element_by_offset(php_sxe_object *sxe, zend_long offse
134134
return NULL;
135135
}
136136
}
137-
while (node && nodendx <= offset) {
137+
while (node && (offset < 0 || nodendx <= offset)) {
138138
if (node->type == XML_ELEMENT_NODE && match_ns(node, sxe->iter.nsprefix, sxe->iter.isprefix)) {
139139
if (sxe->iter.type == SXE_ITER_CHILD || (
140140
sxe->iter.type == SXE_ITER_ELEMENT && xmlStrEqual(node->name, BAD_CAST ZSTR_VAL(sxe->iter.name)))) {
@@ -302,14 +302,16 @@ static zval *sxe_prop_dim_read(zend_object *object, zval *member, bool elements,
302302
}
303303
if (!member || Z_TYPE_P(member) == IS_LONG) {
304304
zend_long cnt = 0;
305+
bool appendable = true;
305306
xmlNodePtr mynode = node;
306307

307308
if (sxe->iter.type == SXE_ITER_CHILD) {
308309
node = php_sxe_get_first_node_non_destructive(sxe, node);
309310
}
310311
if (sxe->iter.type == SXE_ITER_NONE) {
311-
if (member && Z_LVAL_P(member) > 0) {
312-
php_error_docref(NULL, E_WARNING, "Cannot add element %s number " ZEND_LONG_FMT " when only 0 such elements exist", mynode->name, Z_LVAL_P(member));
312+
if (member && Z_LVAL_P(member) != 0) {
313+
node = NULL;
314+
appendable = false;
313315
}
314316
} else if (member) {
315317
node = sxe_get_element_by_offset(sxe, Z_LVAL_P(member), node, &cnt);
@@ -319,11 +321,13 @@ static zval *sxe_prop_dim_read(zend_object *object, zval *member, bool elements,
319321
if (node) {
320322
node_as_zval(sxe, node, rv, SXE_ITER_NONE, NULL, sxe->iter.nsprefix, sxe->iter.isprefix);
321323
} else if (type == BP_VAR_W || type == BP_VAR_RW) {
322-
if (member && cnt < Z_LVAL_P(member)) {
324+
if (member && (Z_LVAL_P(member) < 0 || cnt < Z_LVAL_P(member))) {
323325
php_error_docref(NULL, E_WARNING, "Cannot add element %s number " ZEND_LONG_FMT " when only " ZEND_LONG_FMT " such elements exist", mynode->name, Z_LVAL_P(member), cnt);
324326
}
325-
node = xmlNewTextChild(mynode->parent, mynode->ns, mynode->name, NULL);
326-
node_as_zval(sxe, node, rv, SXE_ITER_NONE, NULL, sxe->iter.nsprefix, sxe->iter.isprefix);
327+
if (appendable && (!member || Z_LVAL_P(member) >= 0)) {
328+
node = xmlNewTextChild(mynode->parent, mynode->ns, mynode->name, NULL);
329+
node_as_zval(sxe, node, rv, SXE_ITER_NONE, NULL, sxe->iter.nsprefix, sxe->iter.isprefix);
330+
}
327331
}
328332
} else {
329333
/* In BP_VAR_IS mode only return a proper node if it actually exists. */
@@ -527,19 +531,18 @@ static zval *sxe_prop_dim_write(zend_object *object, zval *member, zval *value,
527531
if (!member || Z_TYPE_P(member) == IS_LONG) {
528532
if (node->type == XML_ATTRIBUTE_NODE) {
529533
zend_throw_error(NULL, "Cannot create duplicate attribute");
530-
if (value_str) {
531-
zend_string_release(value_str);
532-
}
533-
return &EG(error_zval);
534+
value = &EG(error_zval);
535+
goto out;
534536
}
535537

536538
if (sxe->iter.type == SXE_ITER_NONE) {
537-
newnode = node;
538-
++counter;
539-
if (member && Z_LVAL_P(member) > 0) {
539+
if (member && Z_LVAL_P(member) != 0) {
540540
php_error_docref(NULL, E_WARNING, "Cannot add element %s number " ZEND_LONG_FMT " when only 0 such elements exist", mynode->name, Z_LVAL_P(member));
541541
value = &EG(error_zval);
542+
goto out;
542543
}
544+
newnode = node;
545+
++counter;
543546
} else if (member) {
544547
newnode = sxe_get_element_by_offset(sxe, Z_LVAL_P(member), node, &cnt);
545548
if (newnode) {
@@ -586,10 +589,14 @@ static zval *sxe_prop_dim_write(zend_object *object, zval *member, zval *value,
586589
newnode = xmlNewTextChild(mynode, NULL, (xmlChar *)Z_STRVAL_P(member), value_str ? (xmlChar *)ZSTR_VAL(value_str) : NULL);
587590
}
588591
} else if (!member || Z_TYPE_P(member) == IS_LONG) {
589-
if (member && cnt < Z_LVAL_P(member)) {
592+
if (member && (Z_LVAL_P(member) < 0 || cnt < Z_LVAL_P(member))) {
590593
php_error_docref(NULL, E_WARNING, "Cannot add element %s number " ZEND_LONG_FMT " when only " ZEND_LONG_FMT " such elements exist", mynode->name, Z_LVAL_P(member), cnt);
591594
}
592-
newnode = xmlNewTextChild(mynode->parent, mynode->ns, mynode->name, value_str ? (xmlChar *)ZSTR_VAL(value_str) : NULL);
595+
if (member && Z_LVAL_P(member) < 0) {
596+
value = &EG(error_zval);
597+
} else {
598+
newnode = xmlNewTextChild(mynode->parent, mynode->ns, mynode->name, value_str ? (xmlChar *)ZSTR_VAL(value_str) : NULL);
599+
}
593600
}
594601
} else if (attribs) {
595602
if (Z_TYPE_P(member) == IS_LONG) {
@@ -600,6 +607,7 @@ static zval *sxe_prop_dim_write(zend_object *object, zval *member, zval *value,
600607
}
601608
}
602609

610+
out:
603611
if (member == &tmp_zv) {
604612
zval_ptr_dtor_str(&tmp_zv);
605613
}

‎ext/simplexml/tests/sxe_int_offset_no_alias.phpt‎

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,94 @@
1+
--TEST--
2+
Integer offsets that cannot resolve must never alias or mutate a node
3+
--EXTENSIONS--
4+
simplexml
5+
--FILE--
6+
<?php
7+
function fresh(): SimpleXMLElement {
8+
return simplexml_load_string('<r a="1" b="2"><item>a</item><item>b</item><item>c</item></r>');
9+
}
10+
11+
function state(SimpleXMLElement $x): string {
12+
return trim(strstr($x->asXML(), '<r'));
13+
}
14+
15+
echo "== element list ==\n";
16+
$x = fresh();
17+
var_dump(isset($x->item[-1]));
18+
var_dump($x->item[-1]);
19+
var_dump((string) $x->item[0]);
20+
$x->item[-1] = 'Z';
21+
echo state($x), "\n";
22+
unset($x->item[-1]);
23+
echo state($x), "\n";
24+
$x->item[5] = 'P';
25+
echo state($x), "\n";
26+
27+
echo "== single element ==\n";
28+
$x = fresh();
29+
$n = $x->item[0];
30+
var_dump(isset($n[-1]));
31+
var_dump($n[-1]);
32+
var_dump($n[5]);
33+
$n[-1] = 'Z';
34+
echo state($x), "\n";
35+
$n[5] = 'Y';
36+
echo state($x), "\n";
37+
unset($n[-1]);
38+
echo state($x), "\n";
39+
var_dump((string) $n[0]);
40+
41+
echo "== nested write ==\n";
42+
$x = fresh();
43+
try {
44+
$x->item[-1]->kid = 'K';
45+
} catch (Throwable $e) {
46+
echo $e::class, ': ', $e->getMessage(), "\n";
47+
}
48+
echo state($x), "\n";
49+
50+
echo "== attributes ==\n";
51+
$x = fresh();
52+
$at = $x->attributes();
53+
var_dump(isset($at[-1]));
54+
var_dump($at[-1]);
55+
$at[-1] = 'z';
56+
echo state($x), "\n";
57+
?>
58+
--EXPECTF--
59+
== element list ==
60+
bool(false)
61+
NULL
62+
string(1) "a"
63+
64+
Warning: main(): Cannot add element item number -1 when only 3 such elements exist in %s on line %d
65+
<r a="1" b="2"><item>a</item><item>b</item><item>c</item></r>
66+
<r a="1" b="2"><item>a</item><item>b</item><item>c</item></r>
67+
68+
Warning: main(): Cannot add element item number 5 when only 3 such elements exist in %s on line %d
69+
<r a="1" b="2"><item>a</item><item>b</item><item>c</item><item>P</item></r>
70+
== single element ==
71+
bool(false)
72+
NULL
73+
NULL
74+
75+
Warning: main(): Cannot add element item number -1 when only 0 such elements exist in %s on line %d
76+
<r a="1" b="2"><item>a</item><item>b</item><item>c</item></r>
77+
78+
Warning: main(): Cannot add element item number 5 when only 0 such elements exist in %s on line %d
79+
<r a="1" b="2"><item>a</item><item>b</item><item>c</item></r>
80+
<r a="1" b="2"><item>a</item><item>b</item><item>c</item></r>
81+
string(1) "a"
82+
== nested write ==
83+
84+
Warning: main(): Cannot add element item number -1 when only 3 such elements exist in %s on line %d
85+
86+
Notice: Indirect modification of overloaded element of SimpleXMLElement has no effect in %s on line %d
87+
Error: Attempt to assign property "kid" on null
88+
<r a="1" b="2"><item>a</item><item>b</item><item>c</item></r>
89+
== attributes ==
90+
bool(false)
91+
NULL
92+
93+
Warning: main(): Cannot change attribute number -1 when only 0 attributes exist in %s on line %d
94+
<r a="1" b="2"><item>a</item><item>b</item><item>c</item></r>

0 commit comments

Comments
 (0)