ext/pdo: Make execute array bindings atomic

NEWS

@@ -32,6 +32,10 @@ PHP                                                                        NEWS
   . Fixed bug GH-23897 (php:function() assertion failure after a failed
     registerPHPFunctions()). (David Carlier)
 
+- FFI:
+  . Fixed crashes with FFI callbacks created from __call() trampolines
+    and array callables whose object is released. (Ilia Alshanetsky)
+
 - FTP:
   . Fixed bug GH-23619 (cryptic error on servers that don't support TLS
     session resumption on data connection). (ndossche)
@@ -80,6 +84,8 @@ PHP                                                                        NEWS
     column index. (Ilia Alshanetsky)
   . Fixed PDOStatement::bindColumn() registering a binding for a column name
     that is not in the result set. (Ilia Alshanetsky)
+  . Fixed PDOStatement::execute() leaving a partial set of bindings in place
+    when binding its array argument fails. (Ilia Alshanetsky)
 
 - Readline:
   . Fixed a heap over-read in the interactive shell prompt when cli.prompt is

ext/ffi/ffi.c

@@ -923,9 +923,7 @@ static void zend_ffi_callback_hash_dtor(zval *zv) /* {{{ */
 	zend_ffi_callback_data *callback_data = Z_PTR_P(zv);
 
 	ffi_closure_free(callback_data->callback);
-	if (callback_data->fcc.function_handler->common.fn_flags & ZEND_ACC_CLOSURE) {
-		OBJ_RELEASE(ZEND_CLOSURE_OBJECT(callback_data->fcc.function_handler));
-	}
+	zend_fcc_dtor(&callback_data->fcc);
 	for (int i = 0; i < callback_data->arg_count; ++i) {
 		if (callback_data->arg_types[i]->type == FFI_TYPE_STRUCT) {
 			efree(callback_data->arg_types[i]);
@@ -941,43 +939,34 @@ static void zend_ffi_callback_hash_dtor(zval *zv) /* {{{ */
 static void zend_ffi_callback_trampoline(ffi_cif* cif, void* ret, void** args, void* data) /* {{{ */
 {
 	zend_ffi_callback_data *callback_data = (zend_ffi_callback_data*)data;
-	zend_fcall_info fci;
+	zval *params;
 	zend_ffi_type *ret_type;
 	zval retval;
 	ALLOCA_FLAG(use_heap)
 
-	fci.size = sizeof(zend_fcall_info);
-	ZVAL_UNDEF(&fci.function_name);
-	fci.retval = &retval;
-	fci.params = do_alloca(sizeof(zval) *callback_data->arg_count, use_heap);
-	fci.object = NULL;
-	fci.param_count = callback_data->arg_count;
-	fci.named_params = NULL;
+	params = do_alloca(sizeof(zval) *callback_data->arg_count, use_heap);
 
 	if (callback_data->type->func.args) {
 		int n = 0;
 		zend_ffi_type *arg_type;
 
 		ZEND_HASH_PACKED_FOREACH_PTR(callback_data->type->func.args, arg_type) {
 			arg_type = ZEND_FFI_TYPE(arg_type);
-			zend_ffi_cdata_to_zval(NULL, args[n], arg_type, BP_VAR_R, &fci.params[n], (zend_ffi_flags)(arg_type->attr & ZEND_FFI_ATTR_CONST), 0, 0);
+			zend_ffi_cdata_to_zval(NULL, args[n], arg_type, BP_VAR_R, &params[n], (zend_ffi_flags)(arg_type->attr & ZEND_FFI_ATTR_CONST), 0, 0);
 			n++;
 		} ZEND_HASH_FOREACH_END();
 	}
 
-	ZVAL_UNDEF(&retval);
-	if (zend_call_function(&fci, &callback_data->fcc) != SUCCESS) {
-		zend_throw_error(zend_ffi_exception_ce, "Cannot call callback");
-	}
+	zend_call_known_fcc(&callback_data->fcc, &retval, callback_data->arg_count, params, NULL);
 
 	if (callback_data->arg_count) {
 		int n = 0;
 
 		for (n = 0; n < callback_data->arg_count; n++) {
-			zval_ptr_dtor(&fci.params[n]);
+			zval_ptr_dtor(&params[n]);
 		}
 	}
-	free_alloca(fci.params, use_heap);
+	free_alloca(params, use_heap);
 
 	if (EG(exception)) {
 		zend_error_noreturn(E_ERROR, "Throwing from FFI callbacks is not allowed");
@@ -1035,12 +1024,14 @@ static void *zend_ffi_create_callback(zend_ffi_type *type, zval *value) /* {{{ *
 	arg_count = type->func.args ? zend_hash_num_elements(type->func.args) : 0;
 	if (arg_count < fcc.function_handler->common.required_num_args) {
 		zend_throw_error(zend_ffi_exception_ce, "Attempt to assign an invalid callback, insufficient number of arguments");
+		zend_release_fcall_info_cache(&fcc);
 		return NULL;
 	}
 
 	callback = ffi_closure_alloc(sizeof(ffi_closure), &code);
 	if (!callback) {
 		zend_throw_error(zend_ffi_exception_ce, "Cannot allocate callback");
+		zend_release_fcall_info_cache(&fcc);
 		return NULL;
 	}
 
@@ -1067,6 +1058,7 @@ static void *zend_ffi_create_callback(zend_ffi_type *type, zval *value) /* {{{ *
 				}
 				efree(callback_data);
 				ffi_closure_free(callback);
+				zend_release_fcall_info_cache(&fcc);
 				return NULL;
 			}
 			n++;
@@ -1082,6 +1074,7 @@ static void *zend_ffi_create_callback(zend_ffi_type *type, zval *value) /* {{{ *
 		}
 		efree(callback_data);
 		ffi_closure_free(callback);
+		zend_release_fcall_info_cache(&fcc);
 		return NULL;
 	}
 
@@ -1103,6 +1096,7 @@ free_on_failure: ;
 		}
 		efree(callback_data);
 		ffi_closure_free(callback);
+		zend_release_fcall_info_cache(&fcc);
 		return NULL;
 	}
 
@@ -1112,9 +1106,7 @@ free_on_failure: ;
 	}
 	zend_hash_next_index_insert_ptr(FFI_G(callbacks), callback_data);
 
-	if (fcc.function_handler->common.fn_flags & ZEND_ACC_CLOSURE) {
-		GC_ADDREF(ZEND_CLOSURE_OBJECT(fcc.function_handler));
-	}
+	zend_fcc_addref(&callback_data->fcc);
 
 	return code;
 }

ext/ffi/tests/callback_create_failure.phpt

@@ -0,0 +1,27 @@
+--TEST--
+FFI callback creation failure releases a __call trampoline
+--EXTENSIONS--
+ffi
+--INI--
+ffi.enable=1
+--FILE--
+<?php
+$ffi = FFI::cdef("struct E {}; typedef int (*cb_t)(struct E); struct S { cb_t f; };");
+
+class Callback {
+    public function __call(string $name, array $arguments): int {
+        return 0;
+    }
+}
+
+$s = $ffi->new("struct S");
+try {
+    $s->f = [new Callback(), 'compare'];
+} catch (FFI\Exception $e) {
+    echo $e::class, ": ", $e->getMessage(), PHP_EOL;
+}
+echo "Done\n";
+?>
+--EXPECT--
+FFI\Exception: Cannot prepare callback CIF
+Done

ext/ffi/tests/callback_dtor_call.phpt

@@ -0,0 +1,33 @@
+--TEST--
+FFI callback bound to a __call() trampoline
+--EXTENSIONS--
+ffi
+--INI--
+ffi.enable=1
+--FILE--
+<?php
+$ffi = FFI::cdef("typedef int (*cb_t)(int); struct S { cb_t f; cb_t g; };");
+
+class Callback {
+    public function __call(string $name, array $arguments): int {
+        echo $name, "(", $arguments[0], ")\n";
+
+        return $arguments[0] * 2;
+    }
+}
+
+$callback = new Callback();
+$s = $ffi->new("struct S");
+$s->g = [$callback, 'unused'];
+$s->f = [$callback, 'double'];
+var_dump(($s->f)(1));
+var_dump(($s->f)(2));
+var_dump(($s->f)(3));
+?>
+--EXPECT--
+double(1)
+int(2)
+double(2)
+int(4)
+double(3)
+int(6)

ext/ffi/tests/callback_object_lifetime.phpt

@@ -0,0 +1,31 @@
+--TEST--
+FFI callback keeps the object of an array callable alive
+--EXTENSIONS--
+ffi
+--INI--
+ffi.enable=1
+--FILE--
+<?php
+$ffi = FFI::cdef("typedef int (*cb_t)(int); struct S { cb_t f; };");
+
+class Callback {
+    public int $factor = 21;
+
+    public function multiply(int $x): int {
+        return $this->factor * $x;
+    }
+
+    public function __destruct() {
+        echo "Callback::__destruct\n";
+    }
+}
+
+$s = $ffi->new("struct S");
+$s->f = [new Callback(), 'multiply'];
+var_dump(($s->f)(2));
+echo "Done\n";
+?>
+--EXPECT--
+int(42)
+Done
+Callback::__destruct

ext/pdo/pdo_stmt.c

@@ -249,22 +249,22 @@ static void param_dtor(zval *el) /* {{{ */
 }
 /* }}} */
 
-static bool really_register_bound_param(struct pdo_bound_param_data *param, pdo_stmt_t *stmt, bool is_param) /* {{{ */
+static bool really_register_bound_param(struct pdo_bound_param_data *param, pdo_stmt_t *stmt, bool is_param, HashTable *hash)
 {
-	HashTable *hash;
 	zval *parameter;
 	struct pdo_bound_param_data *pparam = NULL;
 
-	hash = is_param ? stmt->bound_params : stmt->bound_columns;
-
 	if (!hash) {
-		ALLOC_HASHTABLE(hash);
-		zend_hash_init(hash, 13, NULL, param_dtor, 0);
+		hash = is_param ? stmt->bound_params : stmt->bound_columns;
+		if (!hash) {
+			ALLOC_HASHTABLE(hash);
+			zend_hash_init(hash, 13, NULL, param_dtor, 0);
 
-		if (is_param) {
-			stmt->bound_params = hash;
-		} else {
-			stmt->bound_columns = hash;
+			if (is_param) {
+				stmt->bound_params = hash;
+			} else {
+				stmt->bound_columns = hash;
+			}
 		}
 	}
 
@@ -381,7 +381,6 @@ static bool really_register_bound_param(struct pdo_bound_param_data *param, pdo_
 	}
 	return 1;
 }
-/* }}} */
 
 /* {{{ Execute a prepared statement, optionally binding parameters */
 PHP_METHOD(PDOStatement, execute)
@@ -402,13 +401,10 @@ PHP_METHOD(PDOStatement, execute)
 		zval *tmp;
 		zend_string *key = NULL;
 		zend_ulong num_index;
+		HashTable *bound_params = NULL;
 
-		if (stmt->bound_params) {
-			zend_hash_destroy(stmt->bound_params);
-			FREE_HASHTABLE(stmt->bound_params);
-			stmt->bound_params = NULL;
-		}
-
+		ALLOC_HASHTABLE(bound_params);
+		zend_hash_init(bound_params, 13, NULL, param_dtor, 0);
 		ZEND_HASH_FOREACH_KEY_VAL(Z_ARRVAL_P(input_params), num_index, key, tmp) {
 			memset(&param, 0, sizeof(param));
 
@@ -425,13 +421,21 @@ PHP_METHOD(PDOStatement, execute)
 			param.param_type = PDO_PARAM_STR;
 			ZVAL_COPY(&param.parameter, tmp);
 
-			if (!really_register_bound_param(&param, stmt, 1)) {
+			if (!really_register_bound_param(&param, stmt, 1, bound_params)) {
 				if (!Z_ISUNDEF(param.parameter)) {
 					zval_ptr_dtor(&param.parameter);
 				}
+				zend_hash_destroy(bound_params);
+				FREE_HASHTABLE(bound_params);
 				RETURN_FALSE;
 			}
 		} ZEND_HASH_FOREACH_END();
+
+		if (stmt->bound_params) {
+			zend_hash_destroy(stmt->bound_params);
+			FREE_HASHTABLE(stmt->bound_params);
+		}
+		stmt->bound_params = bound_params;
 	}
 
 	if (PDO_PLACEHOLDER_NONE == stmt->supports_placeholders) {
@@ -1458,7 +1462,7 @@ static void register_bound_param(INTERNAL_FUNCTION_PARAMETERS, int is_param) /*
 	}
 
 	ZVAL_COPY(&param.parameter, parameter);
-	if (!really_register_bound_param(&param, stmt, is_param)) {
+	if (!really_register_bound_param(&param, stmt, is_param, NULL)) {
 		if (!Z_ISUNDEF(param.parameter)) {
 			zval_ptr_dtor(&(param.parameter));
 		}
@@ -1502,7 +1506,7 @@ PHP_METHOD(PDOStatement, bindValue)
 	}
 
 	ZVAL_COPY(&param.parameter, parameter);
-	if (!really_register_bound_param(&param, stmt, TRUE)) {
+	if (!really_register_bound_param(&param, stmt, TRUE, NULL)) {
 		if (!Z_ISUNDEF(param.parameter)) {
 			zval_ptr_dtor(&(param.parameter));
 			ZVAL_UNDEF(&param.parameter);

ext/pdo/tests/pdo_execute_array_binding_failure.phpt

@@ -0,0 +1,65 @@
+--TEST--
+PDO: execute() keeps the existing bindings when binding its array argument fails
+--EXTENSIONS--
+pdo
+--SKIPIF--
+<?php
+$dir = getenv('REDIR_TEST_DIR');
+if (false == $dir) die('skip no driver');
+require_once $dir . 'pdo_test.inc';
+PDOTest::skip();
+?>
+--FILE--
+<?php
+if (getenv('REDIR_TEST_DIR') === false) putenv('REDIR_TEST_DIR='.__DIR__ . '/../../pdo/tests/');
+require_once getenv('REDIR_TEST_DIR') . 'pdo_test.inc';
+
+class ThrowingString
+{
+    public function __toString(): string
+    {
+        throw new RuntimeException('conversion failed');
+    }
+}
+
+$db = PDOTest::factory();
+$db->exec('CREATE TABLE test_execute_bind_fail (id int, name varchar(10))');
+$db->exec("INSERT INTO test_execute_bind_fail (id, name) VALUES (1, 'a')");
+$db->exec("INSERT INTO test_execute_bind_fail (id, name) VALUES (2, 'b')");
+
+$stmt = $db->prepare('SELECT name FROM test_execute_bind_fail WHERE id = :id AND name = :name');
+$id = 1;
+$name = 'a';
+$stmt->bindParam(':id', $id);
+$stmt->bindParam(':name', $name);
+
+try {
+    $stmt->execute([':id' => 2, ':name' => new ThrowingString()]);
+} catch (RuntimeException $e) {
+    echo $e::class, ": ", $e->getMessage(), PHP_EOL;
+}
+
+var_dump($stmt->execute());
+var_dump($stmt->fetchAll(PDO::FETCH_COLUMN));
+
+var_dump($stmt->execute([':id' => 2, ':name' => 'b']));
+var_dump($stmt->fetchAll(PDO::FETCH_COLUMN));
+?>
+--CLEAN--
+<?php
+require_once getenv('REDIR_TEST_DIR') . 'pdo_test.inc';
+$db = PDOTest::factory();
+PDOTest::dropTableIfExists($db, 'test_execute_bind_fail');
+?>
+--EXPECT--
+RuntimeException: conversion failed
+bool(true)
+array(1) {
+  [0]=>
+  string(1) "a"
+}
+bool(true)
+array(1) {
+  [0]=>
+  string(1) "b"
+}