Fix assertion failure when var_dump'ing void FFI result #10568
Conversation
| } catch (Throwable $_) { | ||
| die('skip libc.so.6 not available'); | ||
| } |
There was a problem hiding this comment.
| } catch (Throwable $_) { | |
| die('skip libc.so.6 not available'); | |
| } | |
| } catch (Throwable) { | |
| die('skip libc.so.6 not available'); | |
| } |
You can omit the variable as of PHP 8.0
There was a problem hiding this comment.
Thanks, I'll wait for other reviews to come in and then I'll update this
There was a problem hiding this comment.
I think it's better to return NULL for void in first pace.
diff --git a/ext/ffi/ffi.c b/ext/ffi/ffi.c
index 107d2862c1..3ee3c27638 100644
--- a/ext/ffi/ffi.c
+++ b/ext/ffi/ffi.c
@@ -2777,7 +2777,11 @@ static ZEND_FUNCTION(ffi_trampoline) /* {{{ */
free_alloca(arg_values, arg_values_use_heap);
}
- zend_ffi_cdata_to_zval(NULL, ret, ZEND_FFI_TYPE(type->func.ret_type), BP_VAR_R, return_value, 0, 1, 0);
+ if (ZEND_FFI_TYPE(type->func.ret_type)->kind != ZEND_FFI_TYPE_VOID) {
+ zend_ffi_cdata_to_zval(NULL, ret, ZEND_FFI_TYPE(type->func.ret_type), BP_VAR_R, return_value, 0, 1, 0);
+ } else {
+ ZVAL_NULL(return_value);
+ }
free_alloca(ret, ret_use_heap);
exit:I don't care about $_ in tests, but it's better to update it, if this makes questions.
Please , update the PR, or I can do this myself a bit later.
|
I'll check it tonight, thanks |
I think that would be a backward incompatibility break. and with your fix you get The root cause of this bug is in the If you insist on your fix, I will change this PR to your fix instead of mine. I just wanted to share my perspective about this :) |
|
OK. Even if this "void" was returned by mistake, keeping compatibility makes sense. Do you agree? |
|
Yes, I agree. Do you want me to open a second PR that targets master-only, or do you change it when merging up? |
|
No thanks. I'll do this myself tomorrow. |
* PHP-8.1: Fix assertion failure when var_dump'ing void FFI result (#10568)
* PHP-8.2: Fix assertion failure when var_dump'ing void FFI result (#10568)
No description provided.