From ed60b9edebd884b4cb82e1135b81fc50ce43aea6 Mon Sep 17 00:00:00 2001 From: Ilia Alshanetsky Date: Sat, 26 Sep 2026 18:15:05 -0400 Subject: [PATCH 1/2] ext/ffi: Hold a reference to the callable of an FFI callback zend_ffi_create_callback() stored the callable's fcall info cache without owning it and passed it to zend_call_function(), which clears function_handler before running a __call() trampoline. Calling such a callback twice failed, destroying it read a NULL handler, an array callable's object could be freed while the callback still used it, and a failed creation left the trampoline in EG(trampoline). Take ownership with zend_fcc_addref(), call through zend_call_known_fcc(), release with zend_fcc_dtor(), and release the cache when creation fails. Closes GH-23933 --- NEWS | 4 +++ ext/ffi/ffi.c | 34 ++++++++------------- ext/ffi/tests/callback_create_failure.phpt | 27 ++++++++++++++++ ext/ffi/tests/callback_dtor_call.phpt | 33 ++++++++++++++++++++ ext/ffi/tests/callback_object_lifetime.phpt | 31 +++++++++++++++++++ 5 files changed, 108 insertions(+), 21 deletions(-) create mode 100644 ext/ffi/tests/callback_create_failure.phpt create mode 100644 ext/ffi/tests/callback_dtor_call.phpt create mode 100644 ext/ffi/tests/callback_object_lifetime.phpt diff --git a/NEWS b/NEWS index fb25e3a0b809..bde051dee660 100644 --- a/NEWS +++ b/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) diff --git a/ext/ffi/ffi.c b/ext/ffi/ffi.c index 6fb00a330d86..79783441c1db 100644 --- a/ext/ffi/ffi.c +++ b/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,18 +939,12 @@ 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; @@ -960,24 +952,21 @@ static void zend_ffi_callback_trampoline(ffi_cif* cif, void* ret, void** args, v 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, ¶ms[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(¶ms[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; } diff --git a/ext/ffi/tests/callback_create_failure.phpt b/ext/ffi/tests/callback_create_failure.phpt new file mode 100644 index 000000000000..2809d9404c2a --- /dev/null +++ b/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-- +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 diff --git a/ext/ffi/tests/callback_dtor_call.phpt b/ext/ffi/tests/callback_dtor_call.phpt new file mode 100644 index 000000000000..40d72227d657 --- /dev/null +++ b/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-- +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) diff --git a/ext/ffi/tests/callback_object_lifetime.phpt b/ext/ffi/tests/callback_object_lifetime.phpt new file mode 100644 index 000000000000..741dd5a1d74c --- /dev/null +++ b/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-- +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 From 3847ee15b110009965d2e6e62dfdbbcf30b63383 Mon Sep 17 00:00:00 2001 From: Ilia Alshanetsky Date: Fri, 25 Sep 2026 03:08:52 -0400 Subject: [PATCH 2/2] ext/pdo: Keep prior bindings when execute() fails to bind its array PDOStatement::execute() destroyed bound_params before registering the entries of its array argument, so a failure part-way through (a value whose string conversion throws, a name the driver rejects) left the entries registered so far in place, and a following execute() without arguments ran with them. Build the new table separately and install it only once every entry is registered. This also stops a value's __toString() that re-executes the statement from freeing the table being filled. --- NEWS | 2 + ext/pdo/pdo_stmt.c | 44 +++++++------ .../pdo_execute_array_binding_failure.phpt | 65 +++++++++++++++++++ 3 files changed, 91 insertions(+), 20 deletions(-) create mode 100644 ext/pdo/tests/pdo_execute_array_binding_failure.phpt diff --git a/NEWS b/NEWS index bde051dee660..a8fe415a2a40 100644 --- a/NEWS +++ b/NEWS @@ -84,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 diff --git a/ext/pdo/pdo_stmt.c b/ext/pdo/pdo_stmt.c index 97d1a058fd52..cecc0491a7ef 100644 --- a/ext/pdo/pdo_stmt.c +++ b/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(¶m, 0, sizeof(param)); @@ -425,13 +421,21 @@ PHP_METHOD(PDOStatement, execute) param.param_type = PDO_PARAM_STR; ZVAL_COPY(¶m.parameter, tmp); - if (!really_register_bound_param(¶m, stmt, 1)) { + if (!really_register_bound_param(¶m, stmt, 1, bound_params)) { if (!Z_ISUNDEF(param.parameter)) { zval_ptr_dtor(¶m.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(¶m.parameter, parameter); - if (!really_register_bound_param(¶m, stmt, is_param)) { + if (!really_register_bound_param(¶m, stmt, is_param, NULL)) { if (!Z_ISUNDEF(param.parameter)) { zval_ptr_dtor(&(param.parameter)); } @@ -1502,7 +1506,7 @@ PHP_METHOD(PDOStatement, bindValue) } ZVAL_COPY(¶m.parameter, parameter); - if (!really_register_bound_param(¶m, stmt, TRUE)) { + if (!really_register_bound_param(¶m, stmt, TRUE, NULL)) { if (!Z_ISUNDEF(param.parameter)) { zval_ptr_dtor(&(param.parameter)); ZVAL_UNDEF(¶m.parameter); diff --git a/ext/pdo/tests/pdo_execute_array_binding_failure.phpt b/ext/pdo/tests/pdo_execute_array_binding_failure.phpt new file mode 100644 index 000000000000..7ddecb4ee435 --- /dev/null +++ b/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-- + +--FILE-- +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-- + +--EXPECT-- +RuntimeException: conversion failed +bool(true) +array(1) { + [0]=> + string(1) "a" +} +bool(true) +array(1) { + [0]=> + string(1) "b" +}