Skip to content

Commit ed60b9e

Browse files
iliaaliliaal
committed
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
1 parent e23dc68 commit ed60b9e

5 files changed

Lines changed: 108 additions & 21 deletions

File tree

‎NEWS‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,10 @@ PHP NEWS
3232
. Fixed bug GH-23897 (php:function() assertion failure after a failed
3333
registerPHPFunctions()). (David Carlier)
3434

35+
- FFI:
36+
. Fixed crashes with FFI callbacks created from __call() trampolines
37+
and array callables whose object is released. (Ilia Alshanetsky)
38+
3539
- FTP:
3640
. Fixed bug GH-23619 (cryptic error on servers that don't support TLS
3741
session resumption on data connection). (ndossche)

‎ext/ffi/ffi.c‎

Lines changed: 13 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -923,9 +923,7 @@ static void zend_ffi_callback_hash_dtor(zval *zv) /* {{{ */
923923
zend_ffi_callback_data *callback_data = Z_PTR_P(zv);
924924

925925
ffi_closure_free(callback_data->callback);
926-
if (callback_data->fcc.function_handler->common.fn_flags & ZEND_ACC_CLOSURE) {
927-
OBJ_RELEASE(ZEND_CLOSURE_OBJECT(callback_data->fcc.function_handler));
928-
}
926+
zend_fcc_dtor(&callback_data->fcc);
929927
for (int i = 0; i < callback_data->arg_count; ++i) {
930928
if (callback_data->arg_types[i]->type == FFI_TYPE_STRUCT) {
931929
efree(callback_data->arg_types[i]);
@@ -941,43 +939,34 @@ static void zend_ffi_callback_hash_dtor(zval *zv) /* {{{ */
941939
static void zend_ffi_callback_trampoline(ffi_cif* cif, void* ret, void** args, void* data) /* {{{ */
942940
{
943941
zend_ffi_callback_data *callback_data = (zend_ffi_callback_data*)data;
944-
zend_fcall_info fci;
942+
zval *params;
945943
zend_ffi_type *ret_type;
946944
zval retval;
947945
ALLOCA_FLAG(use_heap)
948946

949-
fci.size = sizeof(zend_fcall_info);
950-
ZVAL_UNDEF(&fci.function_name);
951-
fci.retval = &retval;
952-
fci.params = do_alloca(sizeof(zval) *callback_data->arg_count, use_heap);
953-
fci.object = NULL;
954-
fci.param_count = callback_data->arg_count;
955-
fci.named_params = NULL;
947+
params = do_alloca(sizeof(zval) *callback_data->arg_count, use_heap);
956948

957949
if (callback_data->type->func.args) {
958950
int n = 0;
959951
zend_ffi_type *arg_type;
960952

961953
ZEND_HASH_PACKED_FOREACH_PTR(callback_data->type->func.args, arg_type) {
962954
arg_type = ZEND_FFI_TYPE(arg_type);
963-
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);
955+
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);
964956
n++;
965957
} ZEND_HASH_FOREACH_END();
966958
}
967959

968-
ZVAL_UNDEF(&retval);
969-
if (zend_call_function(&fci, &callback_data->fcc) != SUCCESS) {
970-
zend_throw_error(zend_ffi_exception_ce, "Cannot call callback");
971-
}
960+
zend_call_known_fcc(&callback_data->fcc, &retval, callback_data->arg_count, params, NULL);
972961

973962
if (callback_data->arg_count) {
974963
int n = 0;
975964

976965
for (n = 0; n < callback_data->arg_count; n++) {
977-
zval_ptr_dtor(&fci.params[n]);
966+
zval_ptr_dtor(&params[n]);
978967
}
979968
}
980-
free_alloca(fci.params, use_heap);
969+
free_alloca(params, use_heap);
981970

982971
if (EG(exception)) {
983972
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) /* {{{ *
10351024
arg_count = type->func.args ? zend_hash_num_elements(type->func.args) : 0;
10361025
if (arg_count < fcc.function_handler->common.required_num_args) {
10371026
zend_throw_error(zend_ffi_exception_ce, "Attempt to assign an invalid callback, insufficient number of arguments");
1027+
zend_release_fcall_info_cache(&fcc);
10381028
return NULL;
10391029
}
10401030

10411031
callback = ffi_closure_alloc(sizeof(ffi_closure), &code);
10421032
if (!callback) {
10431033
zend_throw_error(zend_ffi_exception_ce, "Cannot allocate callback");
1034+
zend_release_fcall_info_cache(&fcc);
10441035
return NULL;
10451036
}
10461037

@@ -1067,6 +1058,7 @@ static void *zend_ffi_create_callback(zend_ffi_type *type, zval *value) /* {{{ *
10671058
}
10681059
efree(callback_data);
10691060
ffi_closure_free(callback);
1061+
zend_release_fcall_info_cache(&fcc);
10701062
return NULL;
10711063
}
10721064
n++;
@@ -1082,6 +1074,7 @@ static void *zend_ffi_create_callback(zend_ffi_type *type, zval *value) /* {{{ *
10821074
}
10831075
efree(callback_data);
10841076
ffi_closure_free(callback);
1077+
zend_release_fcall_info_cache(&fcc);
10851078
return NULL;
10861079
}
10871080

@@ -1103,6 +1096,7 @@ free_on_failure: ;
11031096
}
11041097
efree(callback_data);
11051098
ffi_closure_free(callback);
1099+
zend_release_fcall_info_cache(&fcc);
11061100
return NULL;
11071101
}
11081102

@@ -1112,9 +1106,7 @@ free_on_failure: ;
11121106
}
11131107
zend_hash_next_index_insert_ptr(FFI_G(callbacks), callback_data);
11141108

1115-
if (fcc.function_handler->common.fn_flags & ZEND_ACC_CLOSURE) {
1116-
GC_ADDREF(ZEND_CLOSURE_OBJECT(fcc.function_handler));
1117-
}
1109+
zend_fcc_addref(&callback_data->fcc);
11181110

11191111
return code;
11201112
}

‎ext/ffi/tests/callback_create_failure.phpt‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
--TEST--
2+
FFI callback creation failure releases a __call trampoline
3+
--EXTENSIONS--
4+
ffi
5+
--INI--
6+
ffi.enable=1
7+
--FILE--
8+
<?php
9+
$ffi = FFI::cdef("struct E {}; typedef int (*cb_t)(struct E); struct S { cb_t f; };");
10+
11+
class Callback {
12+
public function __call(string $name, array $arguments): int {
13+
return 0;
14+
}
15+
}
16+
17+
$s = $ffi->new("struct S");
18+
try {
19+
$s->f = [new Callback(), 'compare'];
20+
} catch (FFI\Exception $e) {
21+
echo $e::class, ": ", $e->getMessage(), PHP_EOL;
22+
}
23+
echo "Done\n";
24+
?>
25+
--EXPECT--
26+
FFI\Exception: Cannot prepare callback CIF
27+
Done

‎ext/ffi/tests/callback_dtor_call.phpt‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
--TEST--
2+
FFI callback bound to a __call() trampoline
3+
--EXTENSIONS--
4+
ffi
5+
--INI--
6+
ffi.enable=1
7+
--FILE--
8+
<?php
9+
$ffi = FFI::cdef("typedef int (*cb_t)(int); struct S { cb_t f; cb_t g; };");
10+
11+
class Callback {
12+
public function __call(string $name, array $arguments): int {
13+
echo $name, "(", $arguments[0], ")\n";
14+
15+
return $arguments[0] * 2;
16+
}
17+
}
18+
19+
$callback = new Callback();
20+
$s = $ffi->new("struct S");
21+
$s->g = [$callback, 'unused'];
22+
$s->f = [$callback, 'double'];
23+
var_dump(($s->f)(1));
24+
var_dump(($s->f)(2));
25+
var_dump(($s->f)(3));
26+
?>
27+
--EXPECT--
28+
double(1)
29+
int(2)
30+
double(2)
31+
int(4)
32+
double(3)
33+
int(6)

‎ext/ffi/tests/callback_object_lifetime.phpt‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
--TEST--
2+
FFI callback keeps the object of an array callable alive
3+
--EXTENSIONS--
4+
ffi
5+
--INI--
6+
ffi.enable=1
7+
--FILE--
8+
<?php
9+
$ffi = FFI::cdef("typedef int (*cb_t)(int); struct S { cb_t f; };");
10+
11+
class Callback {
12+
public int $factor = 21;
13+
14+
public function multiply(int $x): int {
15+
return $this->factor * $x;
16+
}
17+
18+
public function __destruct() {
19+
echo "Callback::__destruct\n";
20+
}
21+
}
22+
23+
$s = $ffi->new("struct S");
24+
$s->f = [new Callback(), 'multiply'];
25+
var_dump(($s->f)(2));
26+
echo "Done\n";
27+
?>
28+
--EXPECT--
29+
int(42)
30+
Done
31+
Callback::__destruct

0 commit comments

Comments
 (0)