From: Nikita Popov <nikic@php.net>
Date: Wed, 26 Feb 2014 15:08:58 +0000 (+0100)
Subject: Disallow use of positional args after unpacking
X-Git-Tag: php-5.6.0alpha3~1^2~11
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=d3b484df8268f7ab31c4ac39734d4b68ce2e6159;p=php

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 9c0365586a..f8bd019378 100644
--- 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 0619a3bab8..7c8a86be48 100644
--- 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 efed84da78..8f129f85a6 100644
--- 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 3efffebc76..1ef545558c 100644
--- 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 d6a6e4712b..fb9ace8378 100644
--- 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 3cf224f288..7a0968e2c9 100644
--- 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
index 0000000000..30e13e3d10
--- /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 8ddc24dc74..abbf537587 100644
--- 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 e862341652..711287eded 100644
--- 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 d53cb96cd0..1bcd430840 100644
--- 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 270c085d03..5362058fa2 100644
--- 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 {