From 2a7f23e9b913faff07ade8865cfb6d98de149d5c Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Thu, 28 Sep 2023 23:49:42 +0200 Subject: [PATCH 1/2] Fix type error on XSLTProcessor::transformToDoc return value with SimpleXML The return type is wrong. You can also use this method with SimpleXML. In fact, PHP provides a way that even third party libraries can hook into its XML handling. Therefore, we cannot even use the SimpleXML|DOMDocument|false union type as third party extensions may extend the possibilities. Broke in 8.1 in 1b35056a33. Closes GH-12315. --- NEWS | 4 ++ ext/xsl/php_xsl.stub.php | 2 +- ext/xsl/php_xsl_arginfo.h | 4 +- .../tests/transformToDoc_sxe_type_error.phpt | 53 +++++++++++++++++++ 4 files changed, 60 insertions(+), 3 deletions(-) create mode 100644 ext/xsl/tests/transformToDoc_sxe_type_error.phpt diff --git a/NEWS b/NEWS index 36633a64524..a6107b1a566 100644 --- a/NEWS +++ b/NEWS @@ -59,6 +59,10 @@ PHP NEWS . Fix return type of stub of xml_parse_into_struct(). (nielsdos) . Fix memory leak when calling xml_parse_into_struct() twice. (nielsdos) +- XSL: + . Fix type error on XSLTProcessor::transformToDoc return value with + SimpleXML. (nielsdos) + - Sockets: . Fix socket_export_stream() with wrong protocol (twosee) diff --git a/ext/xsl/php_xsl.stub.php b/ext/xsl/php_xsl.stub.php index de350acfce8..3d5f63d1b38 100644 --- a/ext/xsl/php_xsl.stub.php +++ b/ext/xsl/php_xsl.stub.php @@ -14,7 +14,7 @@ class XSLTProcessor * @param DOMDocument|SimpleXMLElement $document * @tentative-return-type */ - public function transformToDoc(object $document, ?string $returnClass = null): DOMDocument|false {} + public function transformToDoc(object $document, ?string $returnClass = null): object|false {} /** * @param DOMDocument|SimpleXMLElement $document diff --git a/ext/xsl/php_xsl_arginfo.h b/ext/xsl/php_xsl_arginfo.h index 11c96cdf062..484d5468bcc 100644 --- a/ext/xsl/php_xsl_arginfo.h +++ b/ext/xsl/php_xsl_arginfo.h @@ -1,11 +1,11 @@ /* This is a generated file, edit the .stub.php file instead. - * Stub hash: 7c920913c15c9cd663f19f7ec5ad81648d6eddbc */ + * Stub hash: 234923f47c0d9e83ba87d765fa7c1c2ea8d9f9b1 */ ZEND_BEGIN_ARG_WITH_TENTATIVE_RETURN_TYPE_INFO_EX(arginfo_class_XSLTProcessor_importStylesheet, 0, 1, _IS_BOOL, 0) ZEND_ARG_TYPE_INFO(0, stylesheet, IS_OBJECT, 0) ZEND_END_ARG_INFO() -ZEND_BEGIN_ARG_WITH_TENTATIVE_RETURN_OBJ_TYPE_MASK_EX(arginfo_class_XSLTProcessor_transformToDoc, 0, 1, DOMDocument, MAY_BE_FALSE) +ZEND_BEGIN_ARG_WITH_TENTATIVE_RETURN_TYPE_MASK_EX(arginfo_class_XSLTProcessor_transformToDoc, 0, 1, MAY_BE_OBJECT|MAY_BE_FALSE) ZEND_ARG_TYPE_INFO(0, document, IS_OBJECT, 0) ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, returnClass, IS_STRING, 1, "null") ZEND_END_ARG_INFO() diff --git a/ext/xsl/tests/transformToDoc_sxe_type_error.phpt b/ext/xsl/tests/transformToDoc_sxe_type_error.phpt new file mode 100644 index 00000000000..536f66bc0ea --- /dev/null +++ b/ext/xsl/tests/transformToDoc_sxe_type_error.phpt @@ -0,0 +1,53 @@ +--TEST-- +XSLTProcessor::transformToDoc return value type error with SimpleXML +--EXTENSIONS-- +xsl +simplexml +--FILE-- +load(__DIR__ . '/53965/collection.xsl'); +$processor->importStylesheet($dom); +$result = $processor->transformToDoc($sxe, AdvancedXMLElement::class); + +var_dump($result); +var_dump($result->h1->foo()); + +?> +--EXPECT-- +object(AdvancedXMLElement)#4 (3) { + ["h1"]=> + array(2) { + [0]=> + string(19) "Fight for your mind" + [1]=> + string(17) "Electric Ladyland" + } + ["h2"]=> + array(2) { + [0]=> + string(20) "by Ben Harper - 1995" + [1]=> + string(22) "by Jimi Hendrix - 1997" + } + ["hr"]=> + array(2) { + [0]=> + object(AdvancedXMLElement)#5 (0) { + } + [1]=> + object(AdvancedXMLElement)#6 (0) { + } + } +} +string(24) "foo: Fight for your mind" From e72fc12058dc0ee7bfe534dfa3daf46f3b357190 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Tue, 26 Sep 2023 23:02:31 +0200 Subject: [PATCH 2/2] Fix GH-10008: Narrowing occurred during type inference of ZEND_ADD_ARRAY_ELEMENT This test triggers narrowing for two ops: first ZEND_ADD_ARRAY_ELEMENT, and then ZEND_ASSIGN. The type inference happens in the following order: 1) The ZEND_ADD_ARRAY_ELEMENT infers type 0x40e04080 (packed flag is set), arr_type=0 at this point because it hasn't been set by ZEND_INIT_ARRAY yet. 2) The ZEND_INIT_ARRAY infers type 0x40804080 3) The ZEND_ADD_ARRAY_ELEMENT infers type 0x40e04080, arr_type=0x40804080, which does not have the packed flag set while the existing result of ZEND_ADD_ARRAY_ELEMENT has the packed flag set. This seems to occur because of the phi node introduced by the while loop. If I remove the loop the problem goes away. As Arnaud noted, this seems to be caused by a too wide type inference for arr_type==0. We should keep the invariant that if x>=y then key_type(x) >= key_type(y). If we write the possible results down in a table we get: ``` arr_type resulting key type --------------- -------------------------------------------------------------------------- HASH_ONLY -> MAY_BE_ARRAY_NUMERIC_HASH PACKED_ONLY -> MAY_BE_ARRAY_NUMERIC_HASH | MAY_BE_ARRAY_PACKED (== MAY_BE_ARRAY_KEY_LONG) HASH || PACKED -> MAY_BE_ARRAY_NUMERIC_HASH | MAY_BE_ARRAY_PACKED (== MAY_BE_ARRAY_KEY_LONG) 0 -> MAY_BE_ARRAY_NUMERIC_HASH | MAY_BE_ARRAY_PACKED (== MAY_BE_ARRAY_KEY_LONG) ``` As we can see, `HASH_ONLY > 0` but `MAY_BE_ARRAY_NUMERIC_HASH < MAY_BE_ARRAY_NUMERIC_HASH | MAY_BE_ARRAY_PACKED`, which violates the invariant. Instead if we modify the zero case to have MAY_BE_ARRAY_NUMERIC_HASH instead, we get the following table which satisfies the invariant. ``` arr_type resulting key type --------------- -------------------------------------------------------------------------- HASH_ONLY -> MAY_BE_ARRAY_NUMERIC_HASH PACKED_ONLY -> MAY_BE_ARRAY_NUMERIC_HASH | MAY_BE_ARRAY_PACKED (== MAY_BE_ARRAY_KEY_LONG) HASH || PACKED -> MAY_BE_ARRAY_NUMERIC_HASH | MAY_BE_ARRAY_PACKED (== MAY_BE_ARRAY_KEY_LONG) 0 -> MAY_BE_ARRAY_NUMERIC_HASH ``` Broke in 1ffbb73. Closes GH-10294. --- NEWS | 2 ++ Zend/Optimizer/zend_inference.c | 30 +++++++++++++++++++++--------- ext/opcache/tests/opt/gh10008.phpt | 30 ++++++++++++++++++++++++++++++ 3 files changed, 53 insertions(+), 9 deletions(-) create mode 100644 ext/opcache/tests/opt/gh10008.phpt diff --git a/NEWS b/NEWS index a6107b1a566..4158f8ad704 100644 --- a/NEWS +++ b/NEWS @@ -8,6 +8,8 @@ PHP NEWS . Fixed bug GH-12215 (Module entry being overwritten causes type errors in ext/dom). (nielsdos) . Fixed bug GH-12273 (__builtin_cpu_init check). (Freaky) + . Fixed bug GH-10008 (Narrowing occurred during type inference of + ZEND_ADD_ARRAY_ELEMENT). (nielsdos, arnaud-lb) - CType: . Fixed bug GH-11997 (ctype_alnum 5 times slower in PHP 8.1 or greater). diff --git a/Zend/Optimizer/zend_inference.c b/Zend/Optimizer/zend_inference.c index 91a142a9f86..b01c8b219ae 100644 --- a/Zend/Optimizer/zend_inference.c +++ b/Zend/Optimizer/zend_inference.c @@ -1926,6 +1926,21 @@ ZEND_API uint32_t zend_array_element_type(uint32_t t1, zend_uchar op_type, int w return tmp; } +static zend_always_inline uint32_t assign_long_dim_array_result_type(uint32_t arr_type) +{ + /* Rules: + * HASH_ONLY -> MAY_BE_ARRAY_NUMERIC_HASH + * PACKED_ONLY -> MAY_BE_ARRAY_NUMERIC_HASH | MAY_BE_ARRAY_PACKED (== MAY_BE_ARRAY_KEY_LONG) + * HASH || PACKED -> MAY_BE_ARRAY_NUMERIC_HASH | MAY_BE_ARRAY_PACKED (== MAY_BE_ARRAY_KEY_LONG) + * 0 -> MAY_BE_ARRAY_NUMERIC_HASH + */ + if (MAY_BE_PACKED(arr_type)) { + return MAY_BE_ARRAY_KEY_LONG; + } else { + return MAY_BE_ARRAY_NUMERIC_HASH; + } +} + static uint32_t assign_dim_array_result_type( uint32_t arr_type, uint32_t dim_type, uint32_t value_type, zend_uchar dim_op_type) { uint32_t tmp = 0; @@ -1939,13 +1954,13 @@ static uint32_t assign_dim_array_result_type( if (arr_type & (MAY_BE_UNDEF|MAY_BE_NULL|MAY_BE_FALSE)) { tmp |= MAY_BE_ARRAY_PACKED; } - tmp |= MAY_BE_HASH_ONLY(arr_type) ? MAY_BE_ARRAY_NUMERIC_HASH : MAY_BE_ARRAY_KEY_LONG; + tmp |= assign_long_dim_array_result_type(arr_type); } else { if (dim_type & (MAY_BE_LONG|MAY_BE_FALSE|MAY_BE_TRUE|MAY_BE_RESOURCE|MAY_BE_DOUBLE)) { if (arr_type & (MAY_BE_UNDEF|MAY_BE_NULL|MAY_BE_FALSE)) { tmp |= MAY_BE_ARRAY_PACKED; } - tmp |= MAY_BE_HASH_ONLY(arr_type) ? MAY_BE_ARRAY_NUMERIC_HASH : MAY_BE_ARRAY_KEY_LONG; + tmp |= assign_long_dim_array_result_type(arr_type); } if (dim_type & MAY_BE_STRING) { tmp |= MAY_BE_ARRAY_KEY_STRING; @@ -1954,7 +1969,7 @@ static uint32_t assign_dim_array_result_type( if (arr_type & (MAY_BE_UNDEF|MAY_BE_NULL|MAY_BE_FALSE)) { tmp |= MAY_BE_ARRAY_PACKED; } - tmp |= MAY_BE_HASH_ONLY(arr_type) ? MAY_BE_ARRAY_NUMERIC_HASH : MAY_BE_ARRAY_KEY_LONG; + tmp |= assign_long_dim_array_result_type(arr_type); } } if (dim_type & (MAY_BE_UNDEF|MAY_BE_NULL)) { @@ -3254,8 +3269,7 @@ static zend_always_inline int _zend_update_type_info( key_type |= MAY_BE_ARRAY_PACKED; } if (t1 & MAY_BE_ARRAY) { - key_type |= MAY_BE_HASH_ONLY(t1) ? - MAY_BE_ARRAY_NUMERIC_HASH : MAY_BE_ARRAY_KEY_LONG; + key_type |= assign_long_dim_array_result_type(t1); } } else { if (t2 & (MAY_BE_LONG|MAY_BE_FALSE|MAY_BE_TRUE|MAY_BE_RESOURCE|MAY_BE_DOUBLE)) { @@ -3263,8 +3277,7 @@ static zend_always_inline int _zend_update_type_info( key_type |= MAY_BE_ARRAY_PACKED; } if (t1 & MAY_BE_ARRAY) { - key_type |= MAY_BE_HASH_ONLY(t1) ? - MAY_BE_ARRAY_NUMERIC_HASH : MAY_BE_ARRAY_KEY_LONG; + key_type |= assign_long_dim_array_result_type(t1); } } if (t2 & MAY_BE_STRING) { @@ -3275,8 +3288,7 @@ static zend_always_inline int _zend_update_type_info( key_type |= MAY_BE_ARRAY_PACKED; } if (t1 & MAY_BE_ARRAY) { - key_type |= MAY_BE_HASH_ONLY(t1) ? - MAY_BE_ARRAY_NUMERIC_HASH : MAY_BE_ARRAY_KEY_LONG; + key_type |= assign_long_dim_array_result_type(t1); } } } diff --git a/ext/opcache/tests/opt/gh10008.phpt b/ext/opcache/tests/opt/gh10008.phpt new file mode 100644 index 00000000000..e92b7137bdb --- /dev/null +++ b/ext/opcache/tests/opt/gh10008.phpt @@ -0,0 +1,30 @@ +--TEST-- +GH-10008 (Narrowing occurred during type inference of ZEND_ADD_ARRAY_ELEMENT) +--INI-- +opcache.enable=1 +opcache.enable_cli=1 +opcache.optimization_level=0x20 +--EXTENSIONS-- +opcache +--FILE-- + $bool, $string_key => 123]; + } + + $bool = false; + } +} + +echo "Done\n"; + +?> +--EXPECT-- +Done