Skip to content

Commit f4e9d0e

Browse files
committed
Don't return temporary from SXE write_property handler
Return the original value. If we don't return the original value, we need to own the zval, which we don't. For clarity also switch things to work on a zend_string* value instead of a zval*.
1 parent afde6dc commit f4e9d0e

File tree

2 files changed

+31
-24
lines changed

2 files changed

+31
-24
lines changed

ext/simplexml/simplexml.c

Lines changed: 17 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -374,11 +374,9 @@ static zval *sxe_dimension_read(zend_object *object, zval *offset, int type, zva
374374
/* }}} */
375375

376376
/* {{{ change_node_zval() */
377-
static void change_node_zval(xmlNodePtr node, zval *value)
377+
static void change_node_zval(xmlNodePtr node, zend_string *value)
378378
{
379-
xmlChar *buffer;
380-
ZEND_ASSERT(Z_TYPE_P(value) == IS_STRING);
381-
buffer = xmlEncodeEntitiesReentrant(node->doc, (xmlChar *)Z_STRVAL_P(value));
379+
xmlChar *buffer = xmlEncodeEntitiesReentrant(node->doc, (xmlChar *)ZSTR_VAL(value));
382380
/* check for NULL buffer in case of memory error in xmlEncodeEntitiesReentrant */
383381
if (buffer) {
384382
xmlNodeSetContent(node, buffer);
@@ -400,10 +398,10 @@ static zval *sxe_prop_dim_write(zend_object *object, zval *member, zval *value,
400398
int is_attr = 0;
401399
int nodendx = 0;
402400
int test = 0;
403-
int new_value = 0;
404401
zend_long cnt = 0;
405-
zval tmp_zv, zval_copy;
402+
zval tmp_zv;
406403
zend_string *trim_str;
404+
zend_string *value_str = NULL;
407405

408406
sxe = php_sxe_fetch_object(object);
409407

@@ -484,23 +482,18 @@ static zval *sxe_prop_dim_write(zend_object *object, zval *member, zval *value,
484482
case IS_TRUE:
485483
case IS_DOUBLE:
486484
case IS_NULL:
487-
if (Z_TYPE_P(value) != IS_STRING) {
488-
ZVAL_STR(&zval_copy, zval_get_string_func(value));
489-
value = &zval_copy;
490-
new_value = 1;
491-
}
492-
break;
493485
case IS_STRING:
486+
value_str = zval_get_string(value);
494487
break;
495488
case IS_OBJECT:
496489
if (Z_OBJCE_P(value) == sxe_class_entry) {
490+
zval zval_copy;
497491
if (sxe_object_cast_ex(Z_OBJ_P(value), &zval_copy, IS_STRING) == FAILURE) {
498492
zend_error(E_ERROR, "Unable to cast node to string");
499493
/* FIXME: Should not be fatal */
500494
}
501495

502-
value = &zval_copy;
503-
new_value = 1;
496+
value_str = Z_STR(zval_copy);
504497
break;
505498
}
506499
/* break is missing intentionally */
@@ -544,8 +537,8 @@ static zval *sxe_prop_dim_write(zend_object *object, zval *member, zval *value,
544537
if (!member || Z_TYPE_P(member) == IS_LONG) {
545538
if (node->type == XML_ATTRIBUTE_NODE) {
546539
zend_throw_error(NULL, "Cannot create duplicate attribute");
547-
if (new_value) {
548-
zval_ptr_dtor(value);
540+
if (value_str) {
541+
zend_string_release(value_str);
549542
}
550543
return &EG(error_zval);
551544
}
@@ -583,34 +576,34 @@ static zval *sxe_prop_dim_write(zend_object *object, zval *member, zval *value,
583576
if (is_attr) {
584577
newnode = (xmlNodePtr) attr;
585578
}
586-
if (value) {
579+
if (value_str) {
587580
while ((tempnode = (xmlNodePtr) newnode->children)) {
588581
xmlUnlinkNode(tempnode);
589582
php_libxml_node_free_resource((xmlNodePtr) tempnode);
590583
}
591-
change_node_zval(newnode, value);
584+
change_node_zval(newnode, value_str);
592585
}
593586
} else if (counter > 1) {
594587
php_error_docref(NULL, E_WARNING, "Cannot assign to an array of nodes (duplicate subnodes or attr detected)");
595588
value = &EG(error_zval);
596589
} else if (elements) {
597590
if (!node) {
598591
if (!member || Z_TYPE_P(member) == IS_LONG) {
599-
newnode = xmlNewTextChild(mynode->parent, mynode->ns, mynode->name, value ? (xmlChar *)Z_STRVAL_P(value) : NULL);
592+
newnode = xmlNewTextChild(mynode->parent, mynode->ns, mynode->name, value_str ? (xmlChar *)ZSTR_VAL(value_str) : NULL);
600593
} else {
601-
newnode = xmlNewTextChild(mynode, mynode->ns, (xmlChar *)Z_STRVAL_P(member), value ? (xmlChar *)Z_STRVAL_P(value) : NULL);
594+
newnode = xmlNewTextChild(mynode, mynode->ns, (xmlChar *)Z_STRVAL_P(member), value_str ? (xmlChar *)ZSTR_VAL(value_str) : NULL);
602595
}
603596
} else if (!member || Z_TYPE_P(member) == IS_LONG) {
604597
if (member && cnt < Z_LVAL_P(member)) {
605598
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);
606599
}
607-
newnode = xmlNewTextChild(mynode->parent, mynode->ns, mynode->name, value ? (xmlChar *)Z_STRVAL_P(value) : NULL);
600+
newnode = xmlNewTextChild(mynode->parent, mynode->ns, mynode->name, value_str ? (xmlChar *)ZSTR_VAL(value_str) : NULL);
608601
}
609602
} else if (attribs) {
610603
if (Z_TYPE_P(member) == IS_LONG) {
611604
php_error_docref(NULL, E_WARNING, "Cannot change attribute number " ZEND_LONG_FMT " when only %d attributes exist", Z_LVAL_P(member), nodendx);
612605
} else {
613-
newnode = (xmlNodePtr)xmlNewProp(node, (xmlChar *)Z_STRVAL_P(member), value ? (xmlChar *)Z_STRVAL_P(value) : NULL);
606+
newnode = (xmlNodePtr)xmlNewProp(node, (xmlChar *)Z_STRVAL_P(member), value_str ? (xmlChar *)ZSTR_VAL(value_str) : NULL);
614607
}
615608
}
616609
}
@@ -621,8 +614,8 @@ static zval *sxe_prop_dim_write(zend_object *object, zval *member, zval *value,
621614
if (pnewnode) {
622615
*pnewnode = newnode;
623616
}
624-
if (new_value) {
625-
zval_ptr_dtor(value);
617+
if (value_str) {
618+
zend_string_release(value_str);
626619
}
627620
return value;
628621
}

ext/simplexml/tests/038.phpt

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
--TEST--
2+
SimpleXML: Property assignment return value
3+
--SKIPIF--
4+
<?php if (!extension_loaded("simplexml")) print "skip"; ?>
5+
--FILE--
6+
<?php
7+
$xml =<<<EOF
8+
<root></root>
9+
EOF;
10+
$root = simplexml_load_string($xml);
11+
var_dump($root->prop = 42);
12+
?>
13+
--EXPECT--
14+
int(42)

0 commit comments

Comments
 (0)