[FFI] Segmentation fault when calling callback function

[fwflunky]

Description

The following code:

<?php
class Test {
   public $lib;
   public function __construct() {
      $this->lib = \FFI::cdef("extern void setCloseSessionCallback(void(*fun)(unsigned long cid));
      extern void testCloseSessionCallback();
      ", "./libsomelib.so");
   }
}

$tclass = new Test();
$tclass->lib->setCloseSessionCallback(function($cid){
   echo "ok\n";
});

$tclass->lib->testCloseSessionCallback(); //this function creates new std::thread from c++ stl, sleeps 2 seconds and calls function passed to setCloseSessionCallback
while(true); //prevent php script from completion because testCloseSessionCallback running in other thread

Resulted in this output:

segmentation fault (core dumped)

But I expected this output instead:

ok

gdb backtrace:

PHP Version

PHP 8.0.21 (zts) with Zend OPcache v8.0.21

Operating System

Manjaro Linux

[fwflunky]

if testCloseSessionCallback() function blocks (doesn't create new thread) program instead of while(true); this will work

[cmb69]

Yeah, I don't think that FFI callbacks are thread-safe (and they may have issue with fibers as well). I'm not sure if that's worth fixing, though.

[bwoebi]

@cmb69 What we could do is offering a possibilty to synchronize on EG(vm_interrupt). I.e. offer a way to wrap a function pointer in a function which stalls the thread until the next time the interrupt handler is invoked, then jumps to the original function.

I would not go down the rabbit hole of attempting to break the shared-nothing model (that's the responsibility of extensions like pthreads or parallel).
But just synchronizing in a way that no two threads access the same resources at the same time should be enough.

[smx-smx]

I am experiencing the same issue.

In my (rather esotheric) case, i'm providing PHP scripting as an optional add-on for my ezinject project. This add-on makes it possible to inject the PHP interpreter into other processes by using the embed SAPI, and replace native functions with PHP code: https://github.com/smx-smx/ezinject/tree/master/samples/php

A real world use of this system can be seen here: https://github.com/Simon34545/lginputhook/blob/main/service/inputhook/lginput-hook.php
This project uses ezinject and PHP to intercept keypresses. FFI callbacks are used as hooks for native functions.

The problem is that, without a locking mechanism, the process can crash in the same way described by the OP, especially if calls can come from arbitrary threads in the target process. (it's kind of a hit or miss)

[smx-smx]

@cmb69 What we could do is offering a possibilty to synchronize on EG(vm_interrupt). I.e. offer a way to wrap a function pointer in a function which stalls the thread until the next time the interrupt handler is invoked, then jumps to the original function.

I would not go down the rabbit hole of attempting to break the shared-nothing model (that's the responsibility of extensions like pthreads or parallel). But just synchronizing in a way that no two threads access the same resources at the same time should be enough.

I made a preliminary implementation of this approach. It can be found here: https://gist.github.com/smx-smx/608d2f339d1a822df4b1e125c3a33ca7
It's not cleaned up yet, and it's the first time i deal with the codebase (so bear with me 🙂)

it seems to work for the test case posted though (vs a crash on vanilla PHP)
any opinion?

Thanks

[bwoebi]

I think for ZTS support this will need passing _trsmls_cache around to the trampoline.

+	orig_interrupt_function = zend_interrupt_function;
+	zend_interrupt_function = zend_ffi_interrupt_function;

This part of the patch probably should be just called once, globally. Needs to happen in MINIT.

But yes, as a first approach, this seems sensible.

[smx-smx]

I didn't want to keep the interrupt handler function around after servicing the call, since the handler is specific for the active callback.
An incoming spurious interrupt (from outside FFI) would otherwise crash the process.
I guess an alternative would be to keep a flag that signals if there's a pending callback?

Either way, i'll convert it to a PR this evening so we can discuss about it

[bwoebi]

You cannot do that, after all, theoretically, another code might install another interrupt handler at the same time and it'd conflict. Hence interrupt handlers must be installed once for all.

Yes, you'll have to add a separate flag to check on.

[smx-smx]

I posted a PR with the changes so far.
I'm not entirely sure how to go about the ZTS handling, though.