From 05aef8968d59454e3a733a9ca0862b87276ca9f9 Mon Sep 17 00:00:00 2001 From: "Christoph M. Becker" Date: Wed, 21 Oct 2020 19:08:36 +0200 Subject: [PATCH 1/4] Fix #79177: FFI doesn't handle well PHP exceptions within callback 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. --- ext/ffi/ffi.c | 4 ++++ ext/ffi/tests/bug79177.phpt | 40 +++++++++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+) create mode 100644 ext/ffi/tests/bug79177.phpt diff --git a/ext/ffi/ffi.c b/ext/ffi/ffi.c index f350f91d8a3c2..a99b2ed8873f6 100644 --- a/ext/ffi/ffi.c +++ b/ext/ffi/ffi.c @@ -886,6 +886,10 @@ static void zend_ffi_callback_trampoline(ffi_cif* cif, void* ret, void** args, v } free_alloca(fci.params, use_heap); + if (EG(exception)) { + return; + } + ret_type = ZEND_FFI_TYPE(callback_data->type->func.ret_type); if (ret_type->kind != ZEND_FFI_TYPE_VOID) { zend_ffi_zval_to_cdata(ret, ret_type, &retval); diff --git a/ext/ffi/tests/bug79177.phpt b/ext/ffi/tests/bug79177.phpt new file mode 100644 index 0000000000000..ae2f315b82f64 --- /dev/null +++ b/ext/ffi/tests/bug79177.phpt @@ -0,0 +1,40 @@ +--TEST-- +Bug #79177 (FFI doesn't handle well PHP exceptions within callback) +--SKIPIF-- + +--FILE-- +zend_write; +$php->zend_write = function($str, $len): string { + throw new \RuntimeException('Not allowed'); +}; +try { + echo "After", PHP_EOL; +} catch (\Throwable $exception) { + // Do not output anything here, as handler is overridden +} finally { + $php->zend_write = $originalHandler; +} +if (isset($exception)) { + echo $exception->getMessage(), PHP_EOL; +} +?> +--EXPECT-- +Before +Not allowed From 327e6c9d37dbb36c4a0097fcaabc9d58ecfe9323 Mon Sep 17 00:00:00 2001 From: "Christoph M. Becker" Date: Mon, 26 Oct 2020 15:30:31 +0100 Subject: [PATCH 2/4] Error on unhandled exceptions in FFI callbacks This has been discussed and agreed upon in a previous PR[1]. [1] --- ext/ffi/ffi.c | 3 ++- ext/ffi/tests/bug79177.phpt | 17 +++++++++++++---- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/ext/ffi/ffi.c b/ext/ffi/ffi.c index a99b2ed8873f6..51f62b9656c16 100644 --- a/ext/ffi/ffi.c +++ b/ext/ffi/ffi.c @@ -887,7 +887,8 @@ static void zend_ffi_callback_trampoline(ffi_cif* cif, void* ret, void** args, v free_alloca(fci.params, use_heap); if (EG(exception)) { - return; + zend_throw_error(zend_ffi_exception_ce, "Uncaught %s in PHP FFI callback", ZSTR_VAL(EG(exception)->ce->name)); + zend_exception_error(EG(exception), E_ERROR); } ret_type = ZEND_FFI_TYPE(callback_data->type->func.ret_type); diff --git a/ext/ffi/tests/bug79177.phpt b/ext/ffi/tests/bug79177.phpt index ae2f315b82f64..ed219d792eec6 100644 --- a/ext/ffi/tests/bug79177.phpt +++ b/ext/ffi/tests/bug79177.phpt @@ -18,14 +18,14 @@ typedef char (*zend_write_func_t)(const char *str, size_t str_length); extern zend_write_func_t zend_write; ", ffi_get_php_dll_name()); -echo "Before", PHP_EOL; +echo "Before\n"; $originalHandler = clone $php->zend_write; $php->zend_write = function($str, $len): string { throw new \RuntimeException('Not allowed'); }; try { - echo "After", PHP_EOL; + echo "After\n"; } catch (\Throwable $exception) { // Do not output anything here, as handler is overridden } finally { @@ -35,6 +35,15 @@ if (isset($exception)) { echo $exception->getMessage(), PHP_EOL; } ?> ---EXPECT-- +--EXPECTF-- Before -Not allowed + +Fatal error: Uncaught RuntimeException: Not allowed in %s:%d +Stack trace: +#0 %s(15): {closure}('After\n', 6) +#1 {main} + +Next FFI\Exception: Uncaught RuntimeException in PHP FFI callback in %s:%d +Stack trace: +#0 {main} + thrown in %s on line %d From 6503228b0a1dd95e0ebc64b2d814456a3dc5752b Mon Sep 17 00:00:00 2001 From: "Christoph M. Becker" Date: Wed, 28 Oct 2020 09:58:26 +0100 Subject: [PATCH 3/4] Improve exception message --- ext/ffi/ffi.c | 2 +- ext/ffi/tests/bug79177.phpt | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/ext/ffi/ffi.c b/ext/ffi/ffi.c index 51f62b9656c16..d251f12bbe3c0 100644 --- a/ext/ffi/ffi.c +++ b/ext/ffi/ffi.c @@ -887,7 +887,7 @@ static void zend_ffi_callback_trampoline(ffi_cif* cif, void* ret, void** args, v free_alloca(fci.params, use_heap); if (EG(exception)) { - zend_throw_error(zend_ffi_exception_ce, "Uncaught %s in PHP FFI callback", ZSTR_VAL(EG(exception)->ce->name)); + zend_throw_error(zend_ffi_exception_ce, "Throwing from FFI callbacks is not allowed"); zend_exception_error(EG(exception), E_ERROR); } diff --git a/ext/ffi/tests/bug79177.phpt b/ext/ffi/tests/bug79177.phpt index ed219d792eec6..92d856c8fab7e 100644 --- a/ext/ffi/tests/bug79177.phpt +++ b/ext/ffi/tests/bug79177.phpt @@ -43,7 +43,7 @@ Stack trace: #0 %s(15): {closure}('After\n', 6) #1 {main} -Next FFI\Exception: Uncaught RuntimeException in PHP FFI callback in %s:%d +Next FFI\Exception: Throwing from FFI callbacks is not allowed in %s:%d Stack trace: #0 {main} thrown in %s on line %d From d2959ffa58e5a52395ae305d9a7b8f238382ee58 Mon Sep 17 00:00:00 2001 From: "Christoph M. Becker" Date: Wed, 28 Oct 2020 12:01:28 +0100 Subject: [PATCH 4/4] Trigger E_ERROR instead of exception --- ext/ffi/ffi.c | 3 +-- ext/ffi/tests/bug79177.phpt | 10 ++++------ 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/ext/ffi/ffi.c b/ext/ffi/ffi.c index d251f12bbe3c0..d119ea050cbf5 100644 --- a/ext/ffi/ffi.c +++ b/ext/ffi/ffi.c @@ -887,8 +887,7 @@ static void zend_ffi_callback_trampoline(ffi_cif* cif, void* ret, void** args, v free_alloca(fci.params, use_heap); if (EG(exception)) { - zend_throw_error(zend_ffi_exception_ce, "Throwing from FFI callbacks is not allowed"); - zend_exception_error(EG(exception), E_ERROR); + zend_error(E_ERROR, "Throwing from FFI callbacks is not allowed"); } ret_type = ZEND_FFI_TYPE(callback_data->type->func.ret_type); diff --git a/ext/ffi/tests/bug79177.phpt b/ext/ffi/tests/bug79177.phpt index 92d856c8fab7e..d764437b2d50c 100644 --- a/ext/ffi/tests/bug79177.phpt +++ b/ext/ffi/tests/bug79177.phpt @@ -38,12 +38,10 @@ if (isset($exception)) { --EXPECTF-- Before -Fatal error: Uncaught RuntimeException: Not allowed in %s:%d +Warning: Uncaught RuntimeException: Not allowed in %s:%d Stack trace: -#0 %s(15): {closure}('After\n', 6) +#0 %s(%d): {closure}('After\n', 6) #1 {main} - -Next FFI\Exception: Throwing from FFI callbacks is not allowed in %s:%d -Stack trace: -#0 {main} thrown in %s on line %d + +Fatal error: Throwing from FFI callbacks is not allowed in %s on line %d