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.

diff --git a/NEWS b/NEWS
index 01d879f3b09de567b99f8998a5f7b0c8a0f1b23b..9aa933846ad0fb69302fd0f933b4fdf238c690ba 100644 (file)
--- 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 1b54d10482c426df985dc4870129ff07952cd459..ec49bb1d613cdd19230f082c7560b4f227499248 100644 (file)
--- 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 (file)
index 0000000..86d204b
--- /dev/null
+++ b/Zend/tests/bug71030.phpt
@@ -0,0 +1,28 @@
+--TEST--
+Bug #71030: Self-assignment in list() may have inconsistent behavior
+--FILE--
+<?php
+
+function test1() {
+    $a = [1, 2];
+    $c =& $a;
+    list($c, $b) = $a;
+    var_dump($a, $b);
+}
+
+function test2() {
+    $a = [1, 2];
+    $_a = "a";
+    list($$_a, $b) = $a;
+    var_dump($a, $b);
+}
+
+test1();
+test2();
+
+?>
+--EXPECT--
+int(1)
+int(2)
+int(1)
+int(2)

diff --git a/Zend/tests/bug73663.phpt b/Zend/tests/bug73663.phpt
index 66b9a0565d83ab6b5be74d32b25f91b0281ca387..0c6373bc950936d1965c19d8691e106b211eaa98 100644 (file)
--- 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 71d3bc769b85fc156ded4dcc0eb1485f644b8118..34ff16f870469d15f37aab77c8f4359f6ba0ffea 100644 (file)
--- 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;