Fix #79177 Check for undefined result in zend_ffi_callback_trampoline.

[lisachenko]

This situation can happen when userland exception is thrown within callback body.

[lisachenko]

It would be nice to add a test case, but it isn't easy to reproduce this bug with existing callbacks. So, test is not provided right now. Only possible to acheive this case with PHP object handlers right now as those callbacks return complex pointers and not simple values.

[dstogov]

We can't ignore C result value. So, in case of exception in callback, we will have to die (fatal error).

[lisachenko]

@dstogov Thank you for reply! I'm not sure that we need to die (fatal error) in this case. I think that better option will be to check if there is a global exception, if it present, then just terminate trampoline call (it will be done automatically after checking EG(exception) later). But I'm agree that we should to check if there is a return value and no exception, in that case we should throw exception and terminate this call.

Maybe do you know better way of handling exceptions thrown within FFI callback? With my patch it works perfectly, but I want to double check with you if we can handle this situation better.

[dstogov]

I'm going to commit this patch https://gist.github.com/dstogov/1195bede1f22d169acb6a9d03d22fbfa
Note, that we return to some C code, and it may use return value, or arguments passed by reference.
We just cannot guarantee anything, in case of exception.

Exceptions should be caught by the PHP callback itself and it should care about recovery and reporting error to C.

[lisachenko]

Please check this test case:

use FFI\CData;

$php = FFI::cdef("
typedef unsigned int fake_struct;
typedef fake_struct* (*zend_write_func_t)(const char *str, size_t str_length);
extern zend_write_func_t zend_write;
");

echo "Before", PHP_EOL;

$originalHandler = clone $php->zend_write;
$php->zend_write = function($str, $len): CData {
    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;
}
echo get_class($exception), ': ', $exception->getMessage(), PHP_EOL;

What it will output for your patch?

[lisachenko]

I would also vote not to generate warnings, as it is old-style IMHO ) Better just to pack existing exception as previous exception and throw a new error with previous exception set.

[nikic]

@dstogov Your patch looks fine to me. I don't see any other way to handle exceptions here.

[lisachenko]

@dstogov With your patch try..catch (\Throwable $e) for my test case doesn't work.

[dstogov]

It's clear, we can't catch, because we die early.

[dstogov]

I agree, we can remove warning and add note about FFI into uncaught exception error itself

[lisachenko]

It's clear, we can't catch, because we die early.

Is it possible to handle this somehow for Z-engine library? Because this situation is quite unique: throwing a PHP exception from PHP itself. PHP knows about exception and doesn't try to use return value, so it behaves valid. It would be awesome to generate Error level, but catchable, because I know how to handle it. And someone could use the same approach to clean allocated resources via FFI::free() for example and preserve subsequent calls. Please leave this option for developers to handle it ourselves (I know about possible segfaults, missing return values, etc)

[dstogov]

Let you override zend_fopen().
In case of exception, result value is going to be used uninitialized (not necessary NULL) and then you'll get a crash in some completely unrelated place.
I can't imagine a good fix for this problem.

[lisachenko]

@dstogov @nikic I have one idea about this issue. This issue is only happen for callback that should return pointers to structures and never for scalar types. Thus I want to propose to handle this situation bu returning null values (not undef), so external code should deal with empty pointers (this is typically what we should do)

[nikic]

@lisachenko In that case it is your job to catch the exception and explicitly return the value you want, and that the library providing the hook expects in an error condition.

[lisachenko]

Let you override zend_fopen().

See my previous message, in case of internal PHP exceptions in callbacks that return pointers, result could be filled as null pointer, so all checks of if (handle.fp) will work correctly as I can see. And for PHP itself, it will throw an exception as soon as possible after returning from trampoline callback.

And for all other return types we will throw an error immediately (better catchable).

[lisachenko]

@lisachenko In that case it is your job to catch the exception and explicitly return the value you want, and that the library providing the hook expects in an error condition.

This is good idea, but I'm looking for a way to use exceptions inside PHP callbacks. Otherwise we should prevent throwing of exceptions within PHP callbacks or wrap this information into specific FFI\CallbackException that will have special setReturnValue(CData $result) method to set internal result for this call and handle it.

As of current patch PHP exceptions cant be used inside C callbacks that have return values.

[dstogov]

Different C functions report errors differently (even with scalars). Some returns 0, some negative number, some NULL, some specially formed addresses (e.g. MAP_FAILED)

[lisachenko]

Ok, I have checked several language bindings for FFI and handling of exceptions in that languages. And recipe is only one: do not unwind this error back to C code.

So, let's proceed with throwing an Error.

There are only small chances to implement a wrapper in form of coroutine or special exception class with method setReturnValue to send a return value from outside of this callback and handle this exception.
Best example is implementation of SoapServer class and SoapFault - this is close as I can imagine.

UPD1: If C callback doesn't have a return value, then we can use our exception from callback and handle it.

[cmb69]

I'm closing this in favor of PR #6366.