Do not use RTLD_DEEPBIND if dlmopen is available #18612
Conversation
|
Ping? :) |
|
After some more testing, noticed that since full isolation via dlmopen of libphp.so with LM_ID_NEWLM can completely replace DEEPBIND in the sense that it avoids conflicts even better than DEEPBIND does, it has the side effect that isolation can't be enabled on the user side with the apache sapi, as it explicitly requires apache symbols from the parent scope of the executable (I initially assumed this wasn't the case). Fixed this by re-enabling deepbind only when the apache SAPI is enabled, checking via a SAPI module property instead of at compile-time, to avoid issues when building multiple SAPIs at the same time (this is not the case in most distros, but is still supported by the build system, so leaving it like this). Ping @iluuu1994 for a review :) |
|
Re: embed, no, deepbind does not need to be enabled for it to be usabile via dlmopen, as the embed SAPI alone only exports symbols, and only when the apache SAPI is enabled some symbols are imported. Re: litespeed, its SAPI is built as a standalone executable so it shouldn't need deepbind. |
Zend/zend_extensions.c
Outdated
| @@ -18,6 +18,7 @@ | |||
| */ | |||
|
|
|||
| #include "zend_extensions.h" | |||
| #include "zend_globals.h" | |||
There was a problem hiding this comment.
Are you sure we need all these additional #include?
main/SAPI.c
Outdated
| @@ -69,6 +69,7 @@ SAPI_API void sapi_startup(sapi_module_struct *sf) | |||
| { | |||
| sf->ini_entries = NULL; | |||
| sapi_module = *sf; | |||
| CG(isolate_symbols) = (sf->flags & SAPI_MODULE_FLAG_ISOLATE_SYMBOLS) != 0; | |||
There was a problem hiding this comment.
I meant assignment to CG(isolate_symbols) in php_apache_server_startup().
This way we don't need STANDARD_SAPI_MODULE_PROPERTIES_WITH_FLAGS and other modifications in SAPI layer.
There was a problem hiding this comment.
@dstogov Temporarily reverted back to using a new SAPI module property, as it turns out changing the value of compiler globals inside is impossible from SAPI activation/startup functions, as when ZTS is in use they are always called before initialization of the thread-safe storage for said compiler globals.
Zend/zend_globals.h
Outdated
| @@ -161,6 +161,8 @@ struct _zend_compiler_globals { | |||
| #ifdef ZTS | |||
| uint32_t copied_functions_count; | |||
| #endif | |||
|
|
|||
| bool isolate_symbols; | |||
There was a problem hiding this comment.
The bool is fine. The name is not self explainable. Please think about a better name (dl_isolate_symbols, isolate_dl_symbols, I'm not sure) and/or add a one line comment.
3ebc822 to
8292577
Compare
This pull request disables usage of RTLD_DEEPBIND if dlmopen is available.
Context:
After careful consideration, I believe this is the best approach, after considering the following alternatives:
php_embed_init; this is still SAPI-dependent behavior, which will still cause the same segfaults if jemalloc is used with the embed SAPI.I opted for the much cleaner approach of completely disabling RTLD_DEEPBIND if dlmopen with LM_ID_NEWLM is available, leaving to users the resposibility of isolating libphp.so when including it by using
dlmopen(LM_ID_NEWLM, "libphp.so", RTLD_LAZY);instead ofdlopen("libphp.so", RTLD_LAZY|RTLD_DEEPBIND);.dlmopenprovides full recursive isolation for all symbols both in the opened library, and in libraries opened by that library, avoiding symbol conflict issues even more effectively thanRTLD_DEEPBIND, which is not recursive.On platforms where GNU extensions aren't available (and dlmopen thus isn't available),
RTLD_DEEPBINDis left enabled; if equivalent namespace isolation methods are available on other platforms, they can be added with later pull requests if needed.