From 3b1680a23a760bb0da504aead20eedaeb858bd1d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=A1t=C3=A9=20Kocsis?= Date: Tue, 18 Aug 2020 19:20:56 +0200 Subject: [PATCH 1/5] Promote warnings to exceptions in ext/simplexml --- ext/simplexml/simplexml.c | 30 +++++++++---------- ext/simplexml/tests/012.phpt | 22 ++++++++------ ..._addAttribute_required_attribute_name.phpt | 12 ++++++-- .../tests/SimpleXMLElement_xpath_4.phpt | 13 ++++---- ext/simplexml/tests/bug37076_1.phpt | 15 ++++++---- ext/simplexml/tests/bug38406.phpt | 10 +++++-- 6 files changed, 62 insertions(+), 40 deletions(-) diff --git a/ext/simplexml/simplexml.c b/ext/simplexml/simplexml.c index f788502703b08..c468998ae0daf 100644 --- a/ext/simplexml/simplexml.c +++ b/ext/simplexml/simplexml.c @@ -436,7 +436,7 @@ static zval *sxe_prop_dim_write(zend_object *object, zval *member, zval *value, } if (!Z_STRLEN_P(member)) { - php_error_docref(NULL, E_WARNING, "Cannot write or create unnamed %s", attribs ? "attribute" : "element"); + zend_value_error("Cannot create or write %s with an empty name", attribs ? "attributes" : "elements"); if (member == &tmp_zv) { zval_ptr_dtor_str(&tmp_zv); } @@ -464,7 +464,7 @@ static zval *sxe_prop_dim_write(zend_object *object, zval *member, zval *value, * and could also be E_PARSE, but we use this only during parsing * and this is during runtime. */ - zend_throw_error(NULL, "Cannot create unnamed attribute"); + zend_value_error("Cannot create or write attributes with an empty name"); return &EG(error_zval); } if (attribs && !node && sxe->iter.type == SXE_ITER_ELEMENT) { @@ -501,7 +501,7 @@ static zval *sxe_prop_dim_write(zend_object *object, zval *member, zval *value, if (member == &tmp_zv) { zval_ptr_dtor_str(&tmp_zv); } - zend_error(E_WARNING, "It is not yet possible to assign complex types to %s", attribs ? "attributes" : "properties"); + zend_type_error("It's not possible to assign a complex type to %s, %s given", attribs ? "attributes" : "properties", zend_zval_type_name(value)); return &EG(error_zval); } } @@ -1684,8 +1684,8 @@ SXE_METHOD(addChild) } if (qname_len == 0) { - php_error_docref(NULL, E_WARNING, "Element name is required"); - return; + zend_argument_value_error(1, "cannot be empty"); + RETURN_THROWS(); } sxe = Z_SXEOBJ_P(ZEND_THIS); @@ -1749,8 +1749,8 @@ SXE_METHOD(addAttribute) } if (qname_len == 0) { - php_error_docref(NULL, E_WARNING, "Attribute name is required"); - return; + zend_argument_value_error(1, "cannot be empty"); + RETURN_THROWS(); } sxe = Z_SXEOBJ_P(ZEND_THIS); @@ -2274,8 +2274,8 @@ PHP_FUNCTION(simplexml_load_file) } if (ZEND_LONG_EXCEEDS_INT(options)) { - php_error_docref(NULL, E_WARNING, "Invalid options"); - RETURN_FALSE; + zend_argument_value_error(3, "is too large"); + RETURN_THROWS(); } docp = xmlReadFile(filename, NULL, (int)options); @@ -2319,16 +2319,16 @@ PHP_FUNCTION(simplexml_load_string) } if (ZEND_SIZE_T_INT_OVFL(data_len)) { - php_error_docref(NULL, E_WARNING, "Data is too long"); - RETURN_FALSE; + zend_argument_value_error(1, "is too long"); + RETURN_THROWS(); } if (ZEND_SIZE_T_INT_OVFL(ns_len)) { - php_error_docref(NULL, E_WARNING, "Namespace is too long"); - RETURN_FALSE; + zend_argument_value_error(4, "is too long"); + RETURN_THROWS(); } if (ZEND_LONG_EXCEEDS_INT(options)) { - php_error_docref(NULL, E_WARNING, "Invalid options"); - RETURN_FALSE; + zend_argument_value_error(3, "is too large"); + RETURN_THROWS(); } docp = xmlReadMemory(data, (int)data_len, NULL, NULL, (int)options); diff --git a/ext/simplexml/tests/012.phpt b/ext/simplexml/tests/012.phpt index b687b71988198..ee63d9aaa3800 100644 --- a/ext/simplexml/tests/012.phpt +++ b/ext/simplexml/tests/012.phpt @@ -14,8 +14,12 @@ EOF; $sxe = simplexml_load_string($xml); +try { + $sxe[""] = "warning"; +} catch (ValueError $exception) { + echo $exception->getMessage() . "\n"; +} -$sxe[""] = "warning"; $sxe["attr"] = "value"; echo $sxe->asXML(); @@ -24,19 +28,19 @@ $sxe["attr"] = "new value"; echo $sxe->asXML(); -$sxe[] = "error"; +try { + $sxe[] = "error"; +} catch (ValueError $exception) { + echo $exception->getMessage() . "\n"; +} __HALT_COMPILER(); ?> ===DONE=== ---EXPECTF-- -Warning: main(): Cannot write or create unnamed attribute in %s012.php on line %d +--EXPECT-- +Cannot create or write attributes with an empty name - -Fatal error: Uncaught Error: Cannot create unnamed attribute in %s012.php:%d -Stack trace: -#0 {main} - thrown in %s012.php on line %d +Cannot create or write attributes with an empty name diff --git a/ext/simplexml/tests/SimpleXMLElement_addAttribute_required_attribute_name.phpt b/ext/simplexml/tests/SimpleXMLElement_addAttribute_required_attribute_name.phpt index 34e2e683195b0..50928ff55e553 100644 --- a/ext/simplexml/tests/SimpleXMLElement_addAttribute_required_attribute_name.phpt +++ b/ext/simplexml/tests/SimpleXMLElement_addAttribute_required_attribute_name.phpt @@ -8,10 +8,16 @@ Havard Eide --FILE-- testfest"); -$a->addAttribute( "", "" ); + +try { + $a->addAttribute( "", "" ); +} catch (ValueError $exception) { + echo $exception->getMessage() . "\n"; +} + echo $a->asXML(); ?> ---EXPECTF-- -Warning: SimpleXMLElement::addAttribute(): Attribute name is required in %s on line %d +--EXPECT-- +SimpleXMLElement::addAttribute(): Argument #1 ($qualifiedName) cannot be empty testfest diff --git a/ext/simplexml/tests/SimpleXMLElement_xpath_4.phpt b/ext/simplexml/tests/SimpleXMLElement_xpath_4.phpt index b389bcfc582f0..bcff7db2db06a 100644 --- a/ext/simplexml/tests/SimpleXMLElement_xpath_4.phpt +++ b/ext/simplexml/tests/SimpleXMLElement_xpath_4.phpt @@ -7,11 +7,14 @@ if (PHP_INT_SIZE != 8) die("skip this test is for 64bit platforms only"); ?> --FILE-- getMessage() . "\n"; +} + ?> --EXPECTF-- Warning: Undefined variable $x in %s on line %d - -Warning: simplexml_load_string(): Invalid options in %s on line %d -bool(false) +simplexml_load_string(): Argument #3 ($options) is too large diff --git a/ext/simplexml/tests/bug37076_1.phpt b/ext/simplexml/tests/bug37076_1.phpt index 019915a1b4ee5..777de63f10fb9 100644 --- a/ext/simplexml/tests/bug37076_1.phpt +++ b/ext/simplexml/tests/bug37076_1.phpt @@ -4,13 +4,18 @@ Bug #37076 (SimpleXML ignores .=) (appending to unnamed attribute) --FILE-- "); -$xml->{""} .= "bar"; + +try { + $xml->{""} .= "bar"; +} catch (ValueError $exception) { + echo $exception->getMessage() . "\n"; +} + print $xml->asXML(); ?> ---EXPECTF-- -Warning: main(): Cannot write or create unnamed element in %s on line %d - -Warning: main(): Cannot write or create unnamed element in %s on line %d +--EXPECT-- +Cannot create or write elements with an empty name diff --git a/ext/simplexml/tests/bug38406.phpt b/ext/simplexml/tests/bug38406.phpt index 51a716cbd352e..7e20c7e70447f 100644 --- a/ext/simplexml/tests/bug38406.phpt +++ b/ext/simplexml/tests/bug38406.phpt @@ -13,7 +13,12 @@ $item->otherAttribute = $item->attribute; var_dump($item->otherAttribute); $a = array(); -$item->$a = new stdclass; + +try { + $item->$a = new stdclass; +} catch (TypeError $exception) { + echo $exception->getMessage() . "\n"; +} echo "Done\n"; ?> @@ -28,6 +33,5 @@ object(SimpleXMLElement)#%d (1) { } Warning: Array to string conversion in %s on line %d - -Warning: It is not yet possible to assign complex types to properties in %s on line %d +It's not possible to assign a complex type to properties, stdClass given Done From 6037ea76c89100a94cb38d0cfee5aa01469adcdf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=A1t=C3=A9=20Kocsis?= Date: Mon, 24 Aug 2020 18:58:12 +0200 Subject: [PATCH 2/5] Address code review --- ext/simplexml/simplexml.c | 9 ++++----- ext/simplexml/simplexml.stub.php | 4 ++-- ext/simplexml/simplexml_arginfo.h | 2 +- ext/simplexml/tests/012.phpt | 6 +++--- ext/simplexml/tests/bug37076_1.phpt | 2 +- ext/simplexml/tests/complex_node.phpt | 23 +++++++++++++++++++++++ 6 files changed, 34 insertions(+), 12 deletions(-) create mode 100644 ext/simplexml/tests/complex_node.phpt diff --git a/ext/simplexml/simplexml.c b/ext/simplexml/simplexml.c index c468998ae0daf..a42fbeb632fc6 100644 --- a/ext/simplexml/simplexml.c +++ b/ext/simplexml/simplexml.c @@ -411,7 +411,7 @@ static zval *sxe_prop_dim_write(zend_object *object, zval *member, zval *value, * and could also be E_PARSE, but we use this only during parsing * and this is during runtime. */ - zend_throw_error(NULL, "Cannot create unnamed attribute"); + zend_throw_error(NULL, "Cannot append to an attribute list"); return &EG(error_zval); } goto long_dim; @@ -436,7 +436,7 @@ static zval *sxe_prop_dim_write(zend_object *object, zval *member, zval *value, } if (!Z_STRLEN_P(member)) { - zend_value_error("Cannot create or write %s with an empty name", attribs ? "attributes" : "elements"); + zend_value_error("Cannot create %s with an empty name", attribs ? "attributes" : "elements"); if (member == &tmp_zv) { zval_ptr_dtor_str(&tmp_zv); } @@ -464,7 +464,7 @@ static zval *sxe_prop_dim_write(zend_object *object, zval *member, zval *value, * and could also be E_PARSE, but we use this only during parsing * and this is during runtime. */ - zend_value_error("Cannot create or write attributes with an empty name"); + zend_value_error("Cannot append to an attribute list"); return &EG(error_zval); } if (attribs && !node && sxe->iter.type == SXE_ITER_ELEMENT) { @@ -489,8 +489,7 @@ static zval *sxe_prop_dim_write(zend_object *object, zval *member, zval *value, if (Z_OBJCE_P(value) == sxe_class_entry) { zval zval_copy; if (sxe_object_cast_ex(Z_OBJ_P(value), &zval_copy, IS_STRING) == FAILURE) { - zend_error(E_ERROR, "Unable to cast node to string"); - /* FIXME: Should not be fatal */ + zend_throw_error(NULL, "Unable to cast node to string"); } value_str = Z_STR(zval_copy); diff --git a/ext/simplexml/simplexml.stub.php b/ext/simplexml/simplexml.stub.php index d3e48b0e6aaf7..18cddf2886162 100644 --- a/ext/simplexml/simplexml.stub.php +++ b/ext/simplexml/simplexml.stub.php @@ -59,7 +59,7 @@ public function rewind() {} /** @return bool */ public function valid() {} - /** @return ?SimpleXMLElement */ + /** @return SimpleXMLElement|null */ public function current() {} /** @return string|false */ @@ -71,7 +71,7 @@ public function next() {} /** @return bool */ public function hasChildren() {} - /** @return ?SimpleXMLElement */ + /** @return SimpleXMLElement|null */ public function getChildren() {} } diff --git a/ext/simplexml/simplexml_arginfo.h b/ext/simplexml/simplexml_arginfo.h index dc369e03212d8..5aa6d50ba1d03 100644 --- a/ext/simplexml/simplexml_arginfo.h +++ b/ext/simplexml/simplexml_arginfo.h @@ -1,5 +1,5 @@ /* This is a generated file, edit the .stub.php file instead. - * Stub hash: fbe25d8a7a0a1de0cbd5dc9118e77a2e8d5dbd67 */ + * Stub hash: 953089f230247acf18b9ac48c0a4c516d144a987 */ ZEND_BEGIN_ARG_WITH_RETURN_OBJ_TYPE_MASK_EX(arginfo_simplexml_load_file, 0, 1, SimpleXMLElement, MAY_BE_FALSE) ZEND_ARG_TYPE_INFO(0, filename, IS_STRING, 0) diff --git a/ext/simplexml/tests/012.phpt b/ext/simplexml/tests/012.phpt index ee63d9aaa3800..6c389250fa234 100644 --- a/ext/simplexml/tests/012.phpt +++ b/ext/simplexml/tests/012.phpt @@ -15,7 +15,7 @@ EOF; $sxe = simplexml_load_string($xml); try { - $sxe[""] = "warning"; + $sxe[""] = "value"; } catch (ValueError $exception) { echo $exception->getMessage() . "\n"; } @@ -38,9 +38,9 @@ __HALT_COMPILER(); ?> ===DONE=== --EXPECT-- -Cannot create or write attributes with an empty name +Cannot create attributes with an empty name -Cannot create or write attributes with an empty name +Cannot append to an attribute list diff --git a/ext/simplexml/tests/bug37076_1.phpt b/ext/simplexml/tests/bug37076_1.phpt index 777de63f10fb9..873e9f954aeb0 100644 --- a/ext/simplexml/tests/bug37076_1.phpt +++ b/ext/simplexml/tests/bug37076_1.phpt @@ -16,6 +16,6 @@ try { print $xml->asXML(); ?> --EXPECT-- -Cannot create or write elements with an empty name +Cannot create elements with an empty name diff --git a/ext/simplexml/tests/complex_node.phpt b/ext/simplexml/tests/complex_node.phpt new file mode 100644 index 0000000000000..d50fc132b9100 --- /dev/null +++ b/ext/simplexml/tests/complex_node.phpt @@ -0,0 +1,23 @@ +--TEST-- +Test that it's not possible to assign a complex type to nodes +--SKIPIF-- + +--FILE-- + + + Hello + +EOF); + +try { + $xml->child = new stdClass(); +} catch (Error $exception) { + echo $exception->getMessage() . "\n"; +} + +?> +--EXPECT-- +It's not possible to assign a complex type to nodes, array given From 0f7c6c61e510ae7526a914cbc9287d31dc63ffe4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=A1t=C3=A9=20Kocsis?= Date: Tue, 25 Aug 2020 11:31:48 +0200 Subject: [PATCH 3/5] Remove useless test --- ext/simplexml/tests/complex_node.phpt | 23 ----------------------- 1 file changed, 23 deletions(-) delete mode 100644 ext/simplexml/tests/complex_node.phpt diff --git a/ext/simplexml/tests/complex_node.phpt b/ext/simplexml/tests/complex_node.phpt deleted file mode 100644 index d50fc132b9100..0000000000000 --- a/ext/simplexml/tests/complex_node.phpt +++ /dev/null @@ -1,23 +0,0 @@ ---TEST-- -Test that it's not possible to assign a complex type to nodes ---SKIPIF-- - ---FILE-- - - - Hello - -EOF); - -try { - $xml->child = new stdClass(); -} catch (Error $exception) { - echo $exception->getMessage() . "\n"; -} - -?> ---EXPECT-- -It's not possible to assign a complex type to nodes, array given From 644dba86027aa2428e1d810ad9f492c2cc7256db Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=A1t=C3=A9=20Kocsis?= Date: Tue, 25 Aug 2020 11:44:48 +0200 Subject: [PATCH 4/5] Fix spelling and omitted return --- ext/simplexml/simplexml.c | 3 ++- ext/simplexml/tests/012.phpt | 2 +- ext/simplexml/tests/bug37076_1.phpt | 2 +- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/ext/simplexml/simplexml.c b/ext/simplexml/simplexml.c index a42fbeb632fc6..b1decc086e1d8 100644 --- a/ext/simplexml/simplexml.c +++ b/ext/simplexml/simplexml.c @@ -436,7 +436,7 @@ static zval *sxe_prop_dim_write(zend_object *object, zval *member, zval *value, } if (!Z_STRLEN_P(member)) { - zend_value_error("Cannot create %s with an empty name", attribs ? "attributes" : "elements"); + zend_value_error("Cannot create %s with an empty name", attribs ? "attribute" : "element"); if (member == &tmp_zv) { zval_ptr_dtor_str(&tmp_zv); } @@ -490,6 +490,7 @@ static zval *sxe_prop_dim_write(zend_object *object, zval *member, zval *value, zval zval_copy; if (sxe_object_cast_ex(Z_OBJ_P(value), &zval_copy, IS_STRING) == FAILURE) { zend_throw_error(NULL, "Unable to cast node to string"); + return &EG(error_zval); } value_str = Z_STR(zval_copy); diff --git a/ext/simplexml/tests/012.phpt b/ext/simplexml/tests/012.phpt index 6c389250fa234..307f8219cd70e 100644 --- a/ext/simplexml/tests/012.phpt +++ b/ext/simplexml/tests/012.phpt @@ -38,7 +38,7 @@ __HALT_COMPILER(); ?> ===DONE=== --EXPECT-- -Cannot create attributes with an empty name +Cannot create attribute with an empty name diff --git a/ext/simplexml/tests/bug37076_1.phpt b/ext/simplexml/tests/bug37076_1.phpt index 873e9f954aeb0..d002c93f08562 100644 --- a/ext/simplexml/tests/bug37076_1.phpt +++ b/ext/simplexml/tests/bug37076_1.phpt @@ -16,6 +16,6 @@ try { print $xml->asXML(); ?> --EXPECT-- -Cannot create elements with an empty name +Cannot create element with an empty name From 70b7ca6721c97f83adc671c6af1b7d42a5844919 Mon Sep 17 00:00:00 2001 From: Nikita Popov Date: Tue, 25 Aug 2020 13:29:11 +0200 Subject: [PATCH 5/5] Correctly report exception in SXE get_property_ptr_ptr --- Zend/zend_object_handlers.h | 8 +++++++- ext/simplexml/simplexml.c | 2 +- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/Zend/zend_object_handlers.h b/Zend/zend_object_handlers.h index b401d72ca294a..fedc70cc80997 100644 --- a/Zend/zend_object_handlers.h +++ b/Zend/zend_object_handlers.h @@ -60,7 +60,13 @@ typedef zval *(*zend_object_write_property_t)(zend_object *object, zend_string * typedef void (*zend_object_write_dimension_t)(zend_object *object, zval *offset, zval *value); -/* Used to create pointer to the property of the object, for future direct r/w access */ +/* Used to create pointer to the property of the object, for future direct r/w access. + * May return one of: + * * A zval pointer, without incrementing the reference count. + * * &EG(error_zval), if an exception has been thrown. + * * NULL, if acquiring a direct pointer is not possible. + * In this case, the VM will fall back to using read_property and write_property. + */ typedef zval *(*zend_object_get_property_ptr_ptr_t)(zend_object *object, zend_string *member, int type, void **cache_slot); /* Used to check if a property of the object exists */ diff --git a/ext/simplexml/simplexml.c b/ext/simplexml/simplexml.c index b1decc086e1d8..1944ecf797819 100644 --- a/ext/simplexml/simplexml.c +++ b/ext/simplexml/simplexml.c @@ -656,7 +656,7 @@ static zval *sxe_property_get_adr(zend_object *object, zend_string *zname, int f } ZVAL_STR(&member, zname); if (sxe_prop_dim_write(object, &member, NULL, 1, 0, &node) == &EG(error_zval)) { - return NULL; + return &EG(error_zval); } type = SXE_ITER_NONE; name = NULL;