Fix #79177: FFI doesn't handle well PHP exceptions within callback

[cmb69]

When an FII callback function throws an unhandled exception, it makes
not much sense to return value to the C code. Instead, we declare that
the return value is undefined in this case, and let userland deal with
that.

---

Alternative fixes would be to clearly define the return value in this case, or to disallow unhandled exceptions in FFI callback functions altogether.

[nikic]

I'm not sure this makes sense. Do I understand right that this just leaves the return value uninitialized?

[cmb69]

Do I understand right that this just leaves the return value uninitialized?

Yes, this patch would.

Alternative fixes would be to clearly define the return value in this case, or to disallow unhandled exceptions in FFI callback functions altogether.

[nikic]

I don't like the idea of leaving the return value uninitialized, that's almost certainly going to lead to UB. I don't think there is a really good solution to this problem, but I think the pragmatic solution would be to do something like if (Z_ISUNDEF(retval)) ZVAL_NULL(&retval); here.

[cmb69]

would be to do something like if (Z_ISUNDEF(retval)) ZVAL_NULL(&retval); here.

That wouldn't solve the issue for char returns (and maybe some others; have to check), though. The NULL would be cast to empty string, and then the conversion would fail with an FFI\Exception:

php-src/ext/ffi/ffi.c

Lines 715 to 722 in e1daa19

	case ZEND_FFI_TYPE_CHAR:
	str = zval_get_tmp_string(value, &tmp_str);
	if (ZSTR_LEN(str) == 1) {
	*(char*)ptr = ZSTR_VAL(str)[0];
	} else {
	zend_ffi_assign_incompatible(value, type);
	return FAILURE;
	}

Maybe initialize the zval to a string with a single NUL byte in that case? However, that would somehow muddy this function.

[cmb69]

Welp,

php-src/ext/ffi/ffi.c

Lines 770 to 782 in e1daa19

	case ZEND_FFI_TYPE_STRUCT:
	case ZEND_FFI_TYPE_ARRAY:
	default:
	if (Z_TYPE_P(value) == IS_OBJECT && Z_OBJCE_P(value) == zend_ffi_cdata_ce) {
	zend_ffi_cdata *cdata = (zend_ffi_cdata*)Z_OBJ_P(value);
	if (zend_ffi_is_compatible_type(type, ZEND_FFI_TYPE(cdata->type)) &&
	type->size == ZEND_FFI_TYPE(cdata->type)->size) {
	memcpy(ptr, cdata->ptr, type->size);
	return SUCCESS;
	}
	}
	zend_ffi_assign_incompatible(value, type);
	return FAILURE;

[nikic]

I just realized that we're repeating #5120 here :) The discussion there came to the conclusion that there's really no way to handle this and we should convert the exception into a fatal error.

[cmb69]

Ah! Makes sense.

[cmb69]

@nikic, I was just trying this on 8.0, but zend_exception_error() does return there, although the header comment still states otherwise. That's caused by the E_DONT_BAIL:

php-src/Zend/zend_exceptions.c

Line 976 in 793bf12

zend_error_va(severity | E_DONT_BAIL,

Is it correct to call zend_bailout(); immediately after zend_exception_error()?

[nikic]

@cmb69 Actually, would it be sufficient to just cause an E_ERROR here? I think we already have code that automatically dumps pending exceptions when a fatal error occurs.

[cmb69]

Yes. That appears to be a good solution.