Fix creating a first-class callable from FFI #12916
Conversation
This causes a NULL pointer access because the scope field is being accessed in `zend_closure_call_magic`. We get to that function because of the ZEND_ACC_CALL_VIA_TRAMPOLINE fn_flag that is checked in `zend_closure_from_frame`. The `zend_closure_from_frame` function does not take into account the reserved data or the internal handler function, causing us to end up in `zend_closure_call_magic` instead of the FFI trampoline. Get rid of the flag, and store the temporary internal function in another reserved slot so we can clean that up at the end of the FFI trampoline's execution.
There was a problem hiding this comment.
I'm not sure if this is a right approach.
By design CALL_VIA_TRAMPOLINE wasn't limited to usage by __call, __callstatic and __invoke. The limitation was introduced in zend_closure_from_frame(), that also misses the relevant checks, that were in zend_create_closure_from_callable().
I think we should fix zend_closure_from_frame() itself:
diff --git a/Zend/zend_closures.c b/Zend/zend_closures.c
index f1b29af7967..c7c306013bd 100644
--- a/Zend/zend_closures.c
+++ b/Zend/zend_closures.c
@@ -841,6 +841,15 @@ ZEND_API void zend_create_fake_closure(zval *res, zend_function *func, zend_clas
/* __call and __callStatic name the arguments "$arguments" in the docs. */
static zend_internal_arg_info trampoline_arg_info[] = {ZEND_ARG_VARIADIC_TYPE_INFO(false, arguments, IS_MIXED, false)};
+static ZEND_NAMED_FUNCTION(zend_closure_call_trampoline_handler) /* {{{ */ {
+ zend_function *mptr = EX(func)->internal_function.reserved[0];
+
+ EX(func) = mptr;
+ mptr->internal_function.handler(execute_data, return_value);
+ zend_free_trampoline(mptr);
+}
+/* }}} */
+
void zend_closure_from_frame(zval *return_value, zend_execute_data *call) { /* {{{ */
zval instance;
zend_internal_function trampoline;
@@ -861,14 +870,19 @@ void zend_closure_from_frame(zval *return_value, zend_execute_data *call) { /* {
memset(&trampoline, 0, sizeof(zend_internal_function));
trampoline.type = ZEND_INTERNAL_FUNCTION;
trampoline.fn_flags = mptr->common.fn_flags & (ZEND_ACC_STATIC | ZEND_ACC_VARIADIC);
- trampoline.handler = zend_closure_call_magic;
trampoline.function_name = mptr->common.function_name;
trampoline.scope = mptr->common.scope;
if (trampoline.fn_flags & ZEND_ACC_VARIADIC) {
trampoline.arg_info = trampoline_arg_info;
}
+ if (mptr->type == ZEND_INTERNAL_FUNCTION) {
+ trampoline.handler = zend_closure_call_trampoline_handler;
+ trampoline.reserved[0] = mptr;
+ } else {
+ trampoline.handler = zend_closure_call_magic;
+ zend_free_trampoline(mptr);
+ }
- zend_free_trampoline(mptr);
mptr = (zend_function *) &trampoline;
}
I'm not copletely sre if I'm right, and if my patch covers all the problems.
@nielsdos could you please check my idea and take care about this.
|
@dstogov Your patch looks fine and doesn't seem to cause problems. However, I found an additional problem with both your patch and my patch: $libc = FFI::cdef("int printf(const char *format, ...);", "libc.so.6");
$closure = $libc->printf(...);
$closure("1\n");
$closure("2\n");This will crash. This is because the following code in ffi.c: Lines 2858 to 2863 in ab5edb6 This will release the string, and the trampoline. Freeing the trampoline sets Furthermore, because the name becomes We would need to delay freeing the trampoline until the |
|
@nielsdos How do you reproduce the problem with |
For the following code: <?php
$test = new _ZendTestClass;
$closure = $test->test(...);
$closure(); // (1)
$closure(); // (2)This gives the following error on 8.2: After applying your patch it becomes possible to call it through a closure. This reproduces for FFI too. |
I think this is wrong. I don't see why this closure shouldn't be called. Probably my patch should be extended with some thunk that must take care about keeping the trampoline (or its re-creation). |
This causes a NULL pointer access because the scope field is being accessed in
zend_closure_call_magic. We get to that function because of the ZEND_ACC_CALL_VIA_TRAMPOLINE fn_flag that is checked inzend_closure_from_frame. Thezend_closure_from_framefunction does not take into account the reserved data or the internal handler function, causing us to end up inzend_closure_call_magicinstead of the FFI trampoline.Get rid of the flag, and store the temporary internal function in another reserved slot so we can clean that up at the end of the FFI trampoline's execution.