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. |
danog
force-pushed
the
disable_deepbind_use_dlmopen
branch
from
3ebc822 to
8292577
Compare
Zend/zend_extensions.c
Outdated
| @@ -19,6 +19,7 @@ | |||
|
|
|||
| #include "zend_extensions.h" | |||
| #include "zend_system_id.h" | |||
| #include "SAPI.h" | |||
There was a problem hiding this comment.
You still have this include, but I don't understand why you still need this?
sapi/phpdbg/phpdbg_prompt.c
Outdated
| @@ -19,6 +19,7 @@ | |||
| #include <stdio.h> | |||
| #include <string.h> | |||
| #include "zend.h" | |||
| #include "main/SAPI.h" | |||
There was a problem hiding this comment.
Why did you add this include? I removed it and it still compiles. Same question for the other SAPI files.
There was a problem hiding this comment.
It still fails here and elsewhere if removed (though not locally on my glibc linux machine either): https://github.com/php/php-src/actions/runs/15554443234/job/43791709784?pr=18612
There was a problem hiding this comment.
I understand. That's unfortunate.
The portability include should not require the SAPI header.
Going via CG() globals (or alike) is better but I see this had issues on ZTS. Not sure how to fix this at this moment. I may be able to take a look later this week.
There was a problem hiding this comment.
Hmm, I don't quite see why is requiring the SAPI header such a dealbreaker; granted, it should probably be included by zend_portability.h directly (though that's a bit trickier to do), but even if included separately it shouldn't be an issue IMO
There was a problem hiding this comment.
If it depends on the SAPI header (and only that one really), maybe ... it should be defined in the SAPI header instead now?
There was a problem hiding this comment.
If it depends on the SAPI header (and only that one really), maybe ... it should be defined in the SAPI header instead now?
I think that's wrong too. The macro abstracts away loading the library on different plaforms, so clearly it belongs to zend_portability.h
I wonder if it isn't even better to have a true process-wide global variable instead? No more trouble with CG(...) in ZTS and no dependency on the SAPI level...
There was a problem hiding this comment.
Agree, a new static variable is better than making this part of sapi_module.
|
Not sure about the causes of the windows failure, the UNIX build seems to use the same inputs to the linker... |
|
@danog Not sure, but I believe these extern declarations also need ZEND_API. |
|
All done! |
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.