Disallow use of positional args after unpacking

This commit disallows the use of trailing positional arguments
after argument unpacking was used. The following calls are no
longer valid:

    fn(...$array, $var);
    fn(...$array1, $var, ...$array2);

However, all of the following continue to be valid:

    fn($var, ...$array);
    fn(...$array1, ...$array2);
    fn($var, ...$array1, ...$array2);

The reason behind this change is a stack allocation issue pointed
out by Dmitry: As of PHP 5.5 the stack necessary for pushing
arguments is precomputed and preallocated, as such the individual
SEND opcodes no longer verify that there is enough stack space.
The unpacked arguments will occupy some of that preallocated
space and as such following positional arguments could write past
a stack page boundary.

An alternative resolution for this issue is to ensure that there
is enough space for the remaining arguments in the UNPACK opcode.
However making this allocation precise (rather than using a
conversative over-estimate) would require some effort. Given that
this particular aspect of the feature wasn't very popular in the
first place, it doesn't seem worth the effort.

diff --git a/Zend/tests/arg_unpack/basic.phpt b/Zend/tests/arg_unpack/basic.phpt
index 9c0365586aec7bc583e5b1c1c2b791b62e8a0346..f8bd01937828fa621010a3fe0db0a257519c303d 100644 (file)
--- a/Zend/tests/arg_unpack/basic.phpt
+++ b/Zend/tests/arg_unpack/basic.phpt
@@ -30,12 +30,12 @@ test(...getArray([1, 2, 3]));
 test(...arrayGen([]));
 test(...arrayGen([1, 2, 3]));
 
-test(1, ...[2, 3], ...[4, 5], 6);
-test(1, ...getArray([2, 3]), ...arrayGen([4, 5]), 6);
+test(1, ...[2, 3], ...[4, 5]);
+test(1, ...getArray([2, 3]), ...arrayGen([4, 5]));
 
 test2(...[1, 2]);
 test2(...[1, 2, 3]);
-test2(...[1], ...[], ...[], ...[2, 3], 4, ...[5, 6]);
+test2(...[1], ...[], ...[], ...[2, 3], ...[4, 5]);
 
 ?>
 --EXPECT--
@@ -75,7 +75,7 @@ array(3) {
   [2]=>
   int(3)
 }
-array(6) {
+array(5) {
   [0]=>
   int(1)
   [1]=>
@@ -86,10 +86,8 @@ array(6) {
   int(4)
   [4]=>
   int(5)
-  [5]=>
-  int(6)
 }
-array(6) {
+array(5) {
   [0]=>
   int(1)
   [1]=>
@@ -100,8 +98,6 @@ array(6) {
   int(4)
   [4]=>
   int(5)
-  [5]=>
-  int(6)
 }
 int(1)
 int(2)

diff --git a/Zend/tests/arg_unpack/by_ref.phpt b/Zend/tests/arg_unpack/by_ref.phpt
index 0619a3bab8d66f21d3d550f92fd2b9dbbe62e8cb..7c8a86be48c25a976df08135dd9ad69a1277ec3a 100644 (file)
--- a/Zend/tests/arg_unpack/by_ref.phpt
+++ b/Zend/tests/arg_unpack/by_ref.phpt
@@ -17,46 +17,37 @@ $array = [1, 2, 3];
 test1(...$array);
 var_dump($array);
 
-$array1 = [1, 2]; $val2 = 3; $array2 = [4, 5];
-test1(...$array1, $val2, ...$array2);
-var_dump($array1, $val2, $array2);
+$array1 = [1, 2]; $array2 = [3, 4];
+test1(...$array1, ...$array2);
+var_dump($array1, $array2);
 
 function test2($val1, &$ref1, $val2, &$ref2) {
     $ref1++;
     $ref2++;
 }
 
-$array = [1, 2, 3, 4];
+$array = [0, 0, 0, 0];
 test2(...$array);
 var_dump($array);
 
-$a = $b = $c = $d = 0;
+$array1 = [1, 2]; $array2 = [4, 5];
+test1(...$array1, ...$array2);
+var_dump($array1, $array2);
 
-$array = [];
-test2(...$array, $a, $b, $c, $d);
-var_dump($array, $a, $b, $c, $d);
+$a = $b = $c = $d = 0;
+$array = [0, 0, 0, 0];
 
-$array = [1];
-test2(...$array, $a, $b, $c, $d);
-var_dump($array, $a, $b, $c, $d);
+test2($a, ...$array);
+var_dump($a, $array);
 
-$array = [1, 2];
-test2(...$array, $a, $b, $c, $d);
-var_dump($array, $a, $b, $c, $d);
+test2($a, $b, ...$array);
+var_dump($a, $b, $array);
 
-$array = [1, 2, 3];
-test2(...$array, $a, $b, $c, $d);
-var_dump($array, $a, $b, $c, $d);
+test2($a, $b, $c, ...$array);
+var_dump($a, $b, $c, $array);
 
-$vars = [];
-$array = [];
-test2(...$array, $vars['a'], $vars['b'], $vars['c'], $vars['d']);
-var_dump($vars);
-
-$vars = [];
-$array = [1];
-test2(...$array, $vars['a'], $vars['b'], $vars['c'], $vars['d']);
-var_dump($vars);
+test2($a, $b, $c, $d, ...$array);
+var_dump($a, $b, $c, $d, $array);
 
 ?>
 --EXPECTF--
@@ -74,76 +65,81 @@ array(2) {
   [1]=>
   int(3)
 }
-int(4)
 array(2) {
   [0]=>
-  int(5)
+  int(4)
   [1]=>
-  int(6)
+  int(5)
 }
 array(4) {
   [0]=>
-  int(1)
+  int(0)
   [1]=>
-  int(3)
+  int(1)
   [2]=>
-  int(3)
+  int(0)
   [3]=>
-  int(5)
-}
-array(0) {
+  int(1)
 }
-int(0)
-int(1)
-int(0)
-int(1)
-array(1) {
+array(2) {
   [0]=>
-  int(1)
+  int(2)
+  [1]=>
+  int(3)
 }
-int(1)
-int(1)
-int(1)
-int(1)
 array(2) {
+  [0]=>
+  int(5)
+  [1]=>
+  int(6)
+}
+int(0)
+array(4) {
   [0]=>
   int(1)
   [1]=>
-  int(3)
+  int(0)
+  [2]=>
+  int(1)
+  [3]=>
+  int(0)
 }
+int(0)
 int(1)
-int(2)
-int(1)
-int(1)
-array(3) {
+array(4) {
   [0]=>
   int(1)
   [1]=>
-  int(3)
+  int(1)
   [2]=>
-  int(3)
+  int(1)
+  [3]=>
+  int(0)
 }
+int(0)
 int(2)
-int(2)
-int(1)
-int(1)
-
-Notice: Undefined index: a in %s on line %d
-
-Notice: Undefined index: c in %s on line %d
-array(2) {
-  ["b"]=>
+int(0)
+array(4) {
+  [0]=>
+  int(2)
+  [1]=>
   int(1)
-  ["d"]=>
+  [2]=>
   int(1)
+  [3]=>
+  int(0)
 }
-
-Notice: Undefined index: b in %s on line %d
-
-Notice: Undefined index: d in %s on line %d
-array(2) {
-  ["a"]=>
+int(0)
+int(3)
+int(0)
+int(1)
+array(4) {
+  [0]=>
+  int(2)
+  [1]=>
   int(1)
-  ["c"]=>
+  [2]=>
   int(1)
+  [3]=>
+  int(0)
 }

diff --git a/Zend/tests/arg_unpack/dynamic.phpt b/Zend/tests/arg_unpack/dynamic.phpt
index efed84da78152a453fd0c5d2e33548ad3faf9900..8f129f85a635695c24c4d4f69536d577b20a1705 100644 (file)
--- a/Zend/tests/arg_unpack/dynamic.phpt
+++ b/Zend/tests/arg_unpack/dynamic.phpt
@@ -9,7 +9,7 @@ $fn = function(...$args) {
 
 $fn(...[]);
 $fn(...[1, 2, 3]);
-$fn(1, ...[2, 3], ...[], 4, 5);
+$fn(1, ...[2, 3], ...[], ...[4, 5]);
 
 ?>
 --EXPECT--

diff --git a/Zend/tests/arg_unpack/invalid_type.phpt b/Zend/tests/arg_unpack/invalid_type.phpt
index 3efffebc765280701a2b9fdf69d4d89d14b1e6de..1ef545558c7da54b7d868bd3598397d774315ca0 100644 (file)
--- a/Zend/tests/arg_unpack/invalid_type.phpt
+++ b/Zend/tests/arg_unpack/invalid_type.phpt
@@ -12,7 +12,7 @@ test(...42);
 test(...new stdClass);
 
 test(1, 2, 3, ..."foo", ...[4, 5]);
-test(1, 2, ...new StdClass, 3, ...3.14, ...[4, 5]);
+test(1, 2, 3, ...new StdClass, ...3.14, ...[4, 5]);
 
 ?>
 --EXPECTF--

diff --git a/Zend/tests/arg_unpack/method.phpt b/Zend/tests/arg_unpack/method.phpt
index d6a6e4712b8bd16fbcb998a2d8ea3bfed6d9e2ff..fb9ace83782627a4088febc7a2e9df2d550857f2 100644 (file)
--- a/Zend/tests/arg_unpack/method.phpt
+++ b/Zend/tests/arg_unpack/method.phpt
@@ -14,8 +14,7 @@ class Foo {
 }
 
 $foo = new Foo;
-$foo->test(...[1, 2], 3, 4, ...[], 5);
-Foo::test2(1, 2, ...[3, 4], ...[], 5);
+Foo::test2(1, 2, ...[3, 4], ...[], ...[5]);
 
 ?>
 --EXPECT--
@@ -31,15 +30,3 @@ array(5) {
   [4]=>
   int(5)
 }
-array(5) {
-  [0]=>
-  int(1)
-  [1]=>
-  int(2)
-  [2]=>
-  int(3)
-  [3]=>
-  int(4)
-  [4]=>
-  int(5)
-}

diff --git a/Zend/tests/arg_unpack/new.phpt b/Zend/tests/arg_unpack/new.phpt
index 3cf224f288f9cecc625dd4a21c56c60e6e0e8ca6..7a0968e2c9564758fc1308ddfbed4b3d4e179368 100644 (file)
--- a/Zend/tests/arg_unpack/new.phpt
+++ b/Zend/tests/arg_unpack/new.phpt
@@ -11,7 +11,7 @@ class Foo {
 
 new Foo(...[]);
 new Foo(...[1, 2, 3]);
-new Foo(...[1], 2, ...[], ...[3, 4], 5);
+new Foo(...[1], ...[], ...[2, 3]);
 
 ?>
 --EXPECT--
@@ -25,15 +25,11 @@ array(3) {
   [2]=>
   int(3)
 }
-array(5) {
+array(3) {
   [0]=>
   int(1)
   [1]=>
   int(2)
   [2]=>
   int(3)
-  [3]=>
-  int(4)
-  [4]=>
-  int(5)
 }

diff --git a/Zend/tests/arg_unpack/positional_arg_after_unpack_error.phpt b/Zend/tests/arg_unpack/positional_arg_after_unpack_error.phpt
new file mode 100644 (file)
index 0000000..30e13e3
--- /dev/null
+++ b/Zend/tests/arg_unpack/positional_arg_after_unpack_error.phpt
@@ -0,0 +1,10 @@
+--TEST--
+Positional arguments cannot be used after argument unpacking
+--FILE--
+<?php
+
+var_dump(...[1, 2, 3], 4);
+
+?>
+--EXPECTF--
+Fatal error: Cannot use positional argument after argument unpacking in %s on line %d

diff --git a/Zend/tests/arg_unpack/traversable_throwing_exception.phpt b/Zend/tests/arg_unpack/traversable_throwing_exception.phpt
index 8ddc24dc743049a2ca3ae120cda1640b3a4aed35..abbf5375876b9eef8944360718af52cde40b1c54 100644 (file)
--- a/Zend/tests/arg_unpack/traversable_throwing_exception.phpt
+++ b/Zend/tests/arg_unpack/traversable_throwing_exception.phpt
@@ -20,11 +20,11 @@ function gen() {
 }
 
 try {
-    test(1, 2, ...new Foo, 3, 4);
+    test(1, 2, ...new Foo, ...[3, 4]);
 } catch (Exception $e) { var_dump($e->getMessage()); }
 
 try {
-    test(1, 2, ...gen(), 3, 4);
+    test(1, 2, ...gen(), ...[3, 4]);
 } catch (Exception $e) { var_dump($e->getMessage()); }
 
 ?>

diff --git a/Zend/tests/arg_unpack/traversable_with_by_ref_parameters.phpt b/Zend/tests/arg_unpack/traversable_with_by_ref_parameters.phpt
index e862341652f71d542dfb0ad3af4a3edebf7c02dc..711287ededeee2b79edc2000fc38aae9e0948529 100644 (file)
--- a/Zend/tests/arg_unpack/traversable_with_by_ref_parameters.phpt
+++ b/Zend/tests/arg_unpack/traversable_with_by_ref_parameters.phpt
@@ -13,8 +13,6 @@ function gen($array) {
     }
 }
 
-test(...gen([1, 2, 3]), $a);
-var_dump($a);
 test(1, 2, 3, $b, ...gen([4, 5, 6]));
 var_dump($b);
 
@@ -25,7 +23,6 @@ test(...gen([1, 2]), ...gen([3, 4]));
 ?>
 --EXPECTF--
 int(42)
-int(42)
 
 Warning: Cannot pass by-reference argument 4 of test() by unpacking a Traversable, passing by-value instead in %s on line %d
 

diff --git a/Zend/zend_compile.c b/Zend/zend_compile.c
index d53cb96cd00b9c86d39f8389f0821d6c0dee996c..1bcd430840b8a0416fbe1039d655c4d06b3479eb 100644 (file)
--- a/Zend/zend_compile.c
+++ b/Zend/zend_compile.c
@@ -2614,6 +2614,11 @@ void zend_do_pass_param(znode *param, zend_uchar op TSRMLS_DC) /* {{{ */
        function_ptr = fcall->fbc;
        fcall->arg_num++;
 
+       if (fcall->uses_argument_unpacking) {
+               zend_error_noreturn(E_COMPILE_ERROR,
+                       "Cannot use positional argument after argument unpacking");
+       }
+
        if (original_op == ZEND_SEND_REF) {
                if (function_ptr &&
                    function_ptr->common.function_name &&
@@ -2717,6 +2722,7 @@ void zend_do_unpack_params(znode *params TSRMLS_DC) /* {{{ */
        zend_function_call_entry *fcall;
 
        zend_stack_top(&CG(function_call_stack), (void **) &fcall);
+       fcall->uses_argument_unpacking = 1;
 
        if (fcall->fbc) {
                /* If argument unpacking is used argument numbers and sending modes can no longer be

diff --git a/Zend/zend_compile.h b/Zend/zend_compile.h
index 270c085d0378d36bf2255b9b254a760f0c874c1d..5362058fa266f7c429ef2cef1a8863291cc466cd 100644 (file)
--- a/Zend/zend_compile.h
+++ b/Zend/zend_compile.h
@@ -366,6 +366,7 @@ typedef struct _zend_function_state {
 typedef struct _zend_function_call_entry {
        zend_function *fbc;
        zend_uint arg_num;
+       zend_bool uses_argument_unpacking;
 } zend_function_call_entry;
 
 typedef struct _zend_switch_entry {