From d0a56f707f9b043d9af512524e9c486044cbf510 Mon Sep 17 00:00:00 2001 From: Nikita Popov Date: Thu, 9 May 2019 14:19:53 +0200 Subject: [PATCH] Fixed bug #71030 Make sure to always fetch the RHS of a list assignment first, instead of special casing known self-assignments, which will not detect cases using references correctly. As a side-effect, it is no longer possible to do something like byRef(list($x) = $y). This worked by accident previously, but only if $y was a CV and the self-assignment case did not trigger. However it shouldn't work for the same reason that byRef($x = $y) doesn't. Conversely byRef(list(&$x) = $y) and byRef($x =& $y) continue to be legal. --- NEWS | 2 ++ UPGRADING | 3 +++ Zend/tests/bug71030.phpt | 28 +++++++++++++++++++++ Zend/tests/bug73663.phpt | 15 ++++++++--- Zend/zend_compile.c | 54 ++-------------------------------------- 5 files changed, 47 insertions(+), 55 deletions(-) create mode 100644 Zend/tests/bug71030.phpt diff --git a/NEWS b/NEWS index 01d879f3b0..9aa933846a 100644 --- a/NEWS +++ b/NEWS @@ -13,6 +13,8 @@ PHP NEWS (Ryan McCullagh, Nikita) . Fixed bug #75921 (Inconsistent: No warning in some cases when stdObj is created on the fly). (David Walker) + . Fixed bug #71030 (Self-assignment in list() may have inconsistent behavior). + (Nikita) - CLI: . The built-in CLI server now reports the request method in log files. diff --git a/UPGRADING b/UPGRADING index 1b54d10482..ec49bb1d61 100644 --- a/UPGRADING +++ b/UPGRADING @@ -37,6 +37,9 @@ PHP 7.4 UPGRADE NOTES $a ? $b : ($c ? $d : $e) RFC: https://wiki.php.net/rfc/ternary_associativity + . Passing the result of a (non-reference) list() assignment by reference is + consistently disallowed now. Previously this worked if the right hand side + was a simple (CV) variable and did not occur as part of the list(). - Curl: . Attempting to serialize a CURLFile class will now generate an exception. diff --git a/Zend/tests/bug71030.phpt b/Zend/tests/bug71030.phpt new file mode 100644 index 0000000000..86d204bf62 --- /dev/null +++ b/Zend/tests/bug71030.phpt @@ -0,0 +1,28 @@ +--TEST-- +Bug #71030: Self-assignment in list() may have inconsistent behavior +--FILE-- + +--EXPECT-- +int(1) +int(2) +int(1) +int(2) diff --git a/Zend/tests/bug73663.phpt b/Zend/tests/bug73663.phpt index 66b9a0565d..0c6373bc95 100644 --- a/Zend/tests/bug73663.phpt +++ b/Zend/tests/bug73663.phpt @@ -14,15 +14,18 @@ $func = function (&$ref) { $array = [1]; var_dump(list($val) = $array); // NG: Invalid opcode -change(list($val) = $array); +change(list(&$val) = $array); var_dump($array); $array = [1]; +$func(list(&$val) = $array); +var_dump($array); -$func(list($val) = $array); +$array = [1]; +change(list($val) = $array); var_dump($array); ?> ---EXPECT-- +--EXPECTF-- array(1) { [0]=> int(1) @@ -71,3 +74,9 @@ array(10) { [9]=> int(10) } + +Notice: Only variables should be passed by reference in %s on line %d +array(1) { + [0]=> + int(1) +} diff --git a/Zend/zend_compile.c b/Zend/zend_compile.c index 71d3bc769b..34ff16f870 100644 --- a/Zend/zend_compile.c +++ b/Zend/zend_compile.c @@ -2631,53 +2631,6 @@ zend_bool zend_is_assign_to_self(zend_ast *var_ast, zend_ast *expr_ast) /* {{{ * } /* }}} */ -/* Detects if list($a, $b, $c) contains variable with given name */ -zend_bool zend_list_has_assign_to(zend_ast *list_ast, zend_string *name) /* {{{ */ -{ - zend_ast_list *list = zend_ast_get_list(list_ast); - uint32_t i; - for (i = 0; i < list->children; i++) { - zend_ast *elem_ast = list->child[i]; - zend_ast *var_ast; - - if (!elem_ast) { - continue; - } - var_ast = elem_ast->child[0]; - - /* Recursively check nested list()s */ - if (var_ast->kind == ZEND_AST_ARRAY && zend_list_has_assign_to(var_ast, name)) { - return 1; - } - - if (var_ast->kind == ZEND_AST_VAR && var_ast->child[0]->kind == ZEND_AST_ZVAL) { - zend_string *var_name = zval_get_string(zend_ast_get_zval(var_ast->child[0])); - zend_bool result = zend_string_equals(var_name, name); - zend_string_release_ex(var_name, 0); - if (result) { - return 1; - } - } - } - - return 0; -} -/* }}} */ - -/* Detects patterns like list($a, $b, $c) = $a */ -zend_bool zend_list_has_assign_to_self(zend_ast *list_ast, zend_ast *expr_ast) /* {{{ */ -{ - /* Only check simple variables on the RHS, as only CVs cause issues with this. */ - if (expr_ast->kind == ZEND_AST_VAR && expr_ast->child[0]->kind == ZEND_AST_ZVAL) { - zend_string *name = zval_get_string(zend_ast_get_zval(expr_ast->child[0])); - zend_bool result = zend_list_has_assign_to(list_ast, name); - zend_string_release_ex(name, 0); - return result; - } - return 0; -} -/* }}} */ - void zend_compile_assign(znode *result, zend_ast *ast) /* {{{ */ { zend_ast *var_ast = ast->child[0]; @@ -2754,12 +2707,9 @@ void zend_compile_assign(znode *result, zend_ast *ast) /* {{{ */ zend_compile_var(&expr_node, expr_ast, BP_VAR_W, 1); /* MAKE_REF is usually not necessary for CVs. However, if there are * self-assignments, this forces the RHS to evaluate first. */ - if (expr_node.op_type != IS_CV - || zend_list_has_assign_to_self(var_ast, expr_ast)) { - zend_emit_op(&expr_node, ZEND_MAKE_REF, &expr_node, NULL); - } + zend_emit_op(&expr_node, ZEND_MAKE_REF, &expr_node, NULL); } else { - if (zend_list_has_assign_to_self(var_ast, expr_ast)) { + if (expr_ast->kind == ZEND_AST_VAR) { /* list($a, $b) = $a should evaluate the right $a first */ znode cv_node; -- 2.50.1