Fix GH-19653: Closure named argument unpacking between temporary closures can cause a crash

Due to user closures, the `fbc` address isn't unique if the memory address is reused.
We need to distinguish using a unique key, and we choose arg_info such
that it can be reused across different functions.

Closes GH-19654.

Author: ndossche

NEWS

@@ -8,6 +8,8 @@ PHP                                                                        NEWS
   . Fixed hard_timeout with --enable-zend-max-execution-timers. (Appla)
   . Fixed bug GH-19792 (SCCP causes UAF for return value if both warning and
     exception are triggered). (nielsdos)
+  . Fixed bug GH-19653 (Closure named argument unpacking between temporary
+    closures can cause a crash). (nielsdos, Arnaud, Bob)
 
 - Curl:
   . Fix cloning of CURLOPT_POSTFIELDS when using the clone operator instead

Zend/tests/closures/gh19653_1.phpt

@@ -0,0 +1,27 @@
+--TEST--
+GH-19653 (Closure named argument unpacking between temporary closures can cause a crash)
+--CREDITS--
+ivan-u7n
+--FILE--
+<?php
+
+function func1(string $a1 = 'a1', string $a2 = 'a2', string $a3 = 'a3') {
+	echo __FUNCTION__ . "() a1=$a1 a2=$a2 a3=$a3\n";
+}
+
+function usage1(?Closure $func1 = null) {
+	echo __FUNCTION__ . "() ";
+	($func1 ?? func1(...))(a3: 'm3+');
+}
+usage1();
+
+$func1 = function (string ...$args) {
+	echo "[function] ";
+	func1(...$args, a2: 'm2+');
+};
+usage1(func1: $func1);
+
+?>
+--EXPECT--
+usage1() func1() a1=a1 a2=a2 a3=m3+
+usage1() [function] func1() a1=a1 a2=m2+ a3=m3+

Zend/tests/closures/gh19653_2.phpt

@@ -0,0 +1,23 @@
+--TEST--
+GH-19653 (Closure named argument unpacking between temporary closures can cause a crash) - eval variation
+--CREDITS--
+arnaud-lb
+--FILE--
+<?php
+
+function usage1(Closure $c) {
+    $c(a: 1);
+}
+
+usage1(eval('return function($a) { var_dump($a); };'));
+usage1(eval('return function($b) { var_dump($b); };'));
+
+?>
+--EXPECTF--
+int(1)
+
+Fatal error: Uncaught Error: Unknown named parameter $a in %s:%d
+Stack trace:
+#0 %s(%d): usage1(Object(Closure))
+#1 {main}
+  thrown in %s on line %d

Zend/zend_execute.c

@@ -5058,7 +5058,12 @@ static zend_never_inline zend_result ZEND_FASTCALL zend_quick_check_constant(
 
 static zend_always_inline uint32_t zend_get_arg_offset_by_name(
 		zend_function *fbc, zend_string *arg_name, void **cache_slot) {
-	if (EXPECTED(*cache_slot == fbc)) {
+	/* Due to closures, the `fbc` address isn't unique if the memory address is reused.
+	 * The argument info will be however and uniquely positions the arguments.
+	 * We do support NULL arg_info, so we have to distinguish that from an uninitialized cache slot. */
+	void *unique_id = (void *) ((uintptr_t) fbc->common.arg_info | 1);
+
+	if (EXPECTED(*cache_slot == unique_id)) {
 		return *(uintptr_t *)(cache_slot + 1);
 	}
 
@@ -5069,8 +5074,10 @@ static zend_always_inline uint32_t zend_get_arg_offset_by_name(
 		for (uint32_t i = 0; i < num_args; i++) {
 			zend_arg_info *arg_info = &fbc->op_array.arg_info[i];
 			if (zend_string_equals(arg_name, arg_info->name)) {
-				*cache_slot = fbc;
-				*(uintptr_t *)(cache_slot + 1) = i;
+				if (!fbc->op_array.refcount || !(fbc->op_array.fn_flags & ZEND_ACC_CLOSURE)) {
+					*cache_slot = unique_id;
+					*(uintptr_t *)(cache_slot + 1) = i;
+				}
 				return i;
 			}
 		}
@@ -5079,16 +5086,18 @@ static zend_always_inline uint32_t zend_get_arg_offset_by_name(
 			zend_internal_arg_info *arg_info = &fbc->internal_function.arg_info[i];
 			size_t len = strlen(arg_info->name);
 			if (zend_string_equals_cstr(arg_name, arg_info->name, len)) {
-				*cache_slot = fbc;
+				*cache_slot = unique_id;
 				*(uintptr_t *)(cache_slot + 1) = i;
 				return i;
 			}
 		}
 	}
 
 	if (fbc->common.fn_flags & ZEND_ACC_VARIADIC) {
-		*cache_slot = fbc;
-		*(uintptr_t *)(cache_slot + 1) = fbc->common.num_args;
+		if (fbc->type == ZEND_INTERNAL_FUNCTION || !fbc->op_array.refcount || !(fbc->op_array.fn_flags & ZEND_ACC_CLOSURE)) {
+			*cache_slot = unique_id;
+			*(uintptr_t *)(cache_slot + 1) = fbc->common.num_args;
+		}
 		return fbc->common.num_args;
 	}