Introduce new zend_module_entry hook: child_startup_func #13551
Conversation
Starting threads in `request_startup_func` is unsafe because SAPIs may fork after this hook. Yet, some modules may start threads as part of their initialisation and have no easy way to safely do so. For example, the `curl` module calls `curl_global_init()` in `request_startup_func` to initialize `libcurl`, but this function may start threads at least on MacOS. This change introduces a new hook: `child_startup_func`. This hook is executed once per request-handling process, which are assumed to be non-forking processes. Modules should use this hook for their thread-creating initialization.
|
Would it make sense to instead have a zend_fork hook? This would probably make pcntl_fork a tiny bit safer, as then extensions can just do some pre-fork cleanup as well, possibly synchronizing on some state to make it safer (like calling curl_global_cleanup too). |
|
Unfortunately it's not possible to transition the process in a fork-safe state after using libdispatch, for instance. I suspect this will be the case for most libraries using threads. |
| @@ -1304,7 +1312,8 @@ int main(int argc, char *argv[]) | |||
| sapi_module->ini_entries = php_ini_builder_finish(&ini_builder); | |||
|
|
|||
| /* startup after we get the above ini override se we get things right */ | |||
| if (sapi_module->startup(sapi_module) == FAILURE) { | |||
| if (sapi_module->startup(sapi_module) == FAILURE | |||
| || sapi_module->child_startup(sapi_module) == FAILURE) { | |||
There was a problem hiding this comment.
I think there's not much point to force SAPI's to call child startup if they don't create any children.
There was a problem hiding this comment.
It would be better to let SAPI code to call it during startup if no child_startup defined.
There was a problem hiding this comment.
Agreed, but the separation of startup/child_startup happens to be useful even for these SAPIs in the context of opcache.preload
| { | ||
| return php_module_child_startup(sapi_module); | ||
| } | ||
| /* }}} */ |
There was a problem hiding this comment.
NIT: we no longer add /* }}} */ and /* {{{ */ to the new functions
Starting threads in
module_startup_func(MINIT) is unsafe because SAPIs may fork after this hook. Yet, some modules may start threads as part of their initialisation and have no easy way to safely do so.For example, the
curlmodule callscurl_global_init()inrequest_startup_functo initializelibcurl, but this function starts threads at least on MacOS.This change introduces a new hook:
child_startup_func(CHINIT). This hook is executed once per request-handling process, which are assumed to be non-forking processes. Modules should use this hook for their thread-creating initialization.This fixes a crash observed in #13468, and follows an idea from @bukka.
NB: Forking a process with threads may leave child processes in a corrupted state and lead to crashes or deadlocks. In the case of #13468, curl was indirectly using
libdispatchduringMINIT, which created threads, and the library defensively crashed child processes trying to use it again, after that. This problem is well described here and here.NB2: opcache preloading may execute php code during module startup, in the main process. This requires calling the child startup hook, but this breaks the assumption that processes calling the child startup hook do not fork. It supports executing the preload script in a sub-process when preload_user is specified. In this case it's safe to call the child startup hook (in the sub-process). I've changed preloading so that the preload script is always executed in a sub-process. As a side-effect,
FFI::load()can never be used during preloading, andffi.preloadshould be used instead. Previously,FFI::load()could be used during preloading when fork wasn't needed.NB3: We can not guarantee that
pcntl_fork()will ever be safe