You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Ruediger Pluem <rp...@apache.org> on 2018/08/03 09:46:43 UTC
Re: svn commit: r1198930 - in /httpd/httpd/trunk: include/mod_core.h
server/core.c server/main.c
On 11/07/2011 09:57 PM, sf@apache.org wrote:
> Author: sf
> Date: Mon Nov 7 20:57:02 2011
> New Revision: 1198930
>
>
> Modified: httpd/httpd/trunk/server/main.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/main.c?rev=1198930&r1=1198929&r2=1198930&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/server/main.c (original)
> +++ httpd/httpd/trunk/server/main.c Mon Nov 7 20:57:02 2011
> @@ -34,6 +34,7 @@
> #include "http_log.h"
> #include "http_config.h"
> #include "http_core.h"
> +#include "mod_core.h"
> #include "http_request.h"
> #include "http_vhost.h"
> #include "apr_uri.h"
> @@ -459,6 +460,7 @@ int main(int argc, const char * const ar
> ap_pglobal = process->pool;
> pconf = process->pconf;
> ap_server_argv0 = process->short_name;
> + ap_init_rng(ap_pglobal);
With APR trunk used this now causes httpd to SEGFAULT in EVP_cleanup when it stops in case mod_ssl is loaded.
This is because mod_ssl stored data in Openssl data structures that points to it (likely static data in mod_ssl), but it
gets unloaded due to the pconf pool cleanup before the crypto_lib_cleanup runs EVP_cleanup as it is a cleanup on the
parent pool ap_pglobal.
One approach that made this go away was the following patch to APR:
Index: srclib/apr/crypto/apr_crypto.c
===================================================================
--- srclib/apr/crypto/apr_crypto.c (revision 1837332)
+++ srclib/apr/crypto/apr_crypto.c (working copy)
@@ -534,8 +534,7 @@
lib->pool = pool;
apr_hash_set(active_libs, lib->name, APR_HASH_KEY_STRING, lib);
if (apr_pool_parent_get(pool)) {
- apr_pool_cleanup_register(pool, lib, crypto_lib_cleanup,
- apr_pool_cleanup_null);
+ apr_pool_pre_cleanup_register(pool, lib, crypto_lib_cleanup);
}
}
else {
So using a pre_cleanup instead of a cleanup. But I guess this would only fix this specific use case. I guess we have
a larger problem lurking around that a shared object can put some data in Openssl that points to it (e.g. static data in
the shared object) and that it is gone by the time the pool cleanup runs. In this case we are lucky that a child pool
causes the shared object to be unloaded and hence the pre_cleanup works here. But IMHO we don't need to have this
connection always. But I guess that discussion is more for dev@apr. Feel free to cross post.
Regards
Rüdiger
Re: svn commit: r1198930 - in /httpd/httpd/trunk: include/mod_core.h
server/core.c server/main.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 08/03/2018 03:49 PM, Stefan Eissing wrote:
> Thanks, Rüdiger. Maybe this sheds some light on EVP_cleanup():
>
> https://github.com/google/keyczar/issues/135
>
> Seems libssl install algorithms into libcrypto and does not clean
> up on termination which means EVP_cleanup sees data which is already gone.
I guess if there would be a cleanup function in OpenSSL for the stuff libssl adds
we would be fine here as well as we could call this in a pconf cleanup.
Question is, if there is such a function in the OpenSSL API.
Regards
Rüdiger
Re: svn commit: r1198930 - in /httpd/httpd/trunk: include/mod_core.h
server/core.c server/main.c
Posted by Stefan Eissing <st...@greenbytes.de>.
Thanks, Rüdiger. Maybe this sheds some light on EVP_cleanup():
https://github.com/google/keyczar/issues/135
Seems libssl install algorithms into libcrypto and does not clean
up on termination which means EVP_cleanup sees data which is already gone.
-Stefan
> Am 03.08.2018 um 15:42 schrieb Ruediger Pluem <rp...@apache.org>:
>
>
>
> On 08/03/2018 03:28 PM, Stefan Eissing wrote:
>> (I am trying to understand this issue, since someone reported probs
>> with mod_md/mod_ssl/some db drive init order related to openssl init.)
>>
>> The the move from cleanup() to pre_cleanup() fixed this issue. It means (as I read it)
>> that something was happening in sub-pools cleanup()s that was out-of-order.
>>
>> Looking at apr_crypto_init(), it registers the cleanup at the "root" pool,
>> does this give a clue?
>>
>> So we might have:
>>
>> - mod_ssl loading libssl, loading libcrypto
>> cleanup at pool
>> - ...
>> - someone (indirectly) calling apr_crypto_init()
>> cleanup at root pool
>>
>> root pool destroy:
>> sub pool destroy
>> -> mod_ssl cleanup called
>> -> apr cleanup called
>
> We have
>
> 1. ap_init_rng in main.c causes apr_crypto_init called with ap_pglobal
> 2. This causes apr__crypto_openssl_term to be registered as a cleanup with ap_pglobal.
> 3. mod_ssl is loaded via the pconf pool which is a child of ap_pglobal
> 4. The loading of mod_ssl triggers the loading of libssl via the dynamic linker.
> 5. libssl is initialized in ssl_hook_pre_config via
> SSL_load_error_strings();
> SSL_library_init();
> 6. The above seems to cause references to static data in libssl being saved in data structures
> managed by libcrypto.
> 7. apr_pool_destroy(process->pool) in main.c:283 destroys the ap_pglobal which in turn
> a) first destroys pconf
> b) which causes mod_ssl to be unloaded
> c) which cause libssl to be unloaded
> 8. The cleanups of ap_pglobal are now called, especially apr__crypto_openssl_term which
> a) calls EVP_cleanup()
> b) which tries to access the data stored in 6., but is gone since 7c)
>
> Using a pre_cleanup moves 8. before 7. and thus removes the issue.
>
> Regards
>
> Rüdiger
Re: svn commit: r1198930 - in /httpd/httpd/trunk: include/mod_core.h
server/core.c server/main.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 08/03/2018 03:28 PM, Stefan Eissing wrote:
> (I am trying to understand this issue, since someone reported probs
> with mod_md/mod_ssl/some db drive init order related to openssl init.)
>
> The the move from cleanup() to pre_cleanup() fixed this issue. It means (as I read it)
> that something was happening in sub-pools cleanup()s that was out-of-order.
>
> Looking at apr_crypto_init(), it registers the cleanup at the "root" pool,
> does this give a clue?
>
> So we might have:
>
> - mod_ssl loading libssl, loading libcrypto
> cleanup at pool
> - ...
> - someone (indirectly) calling apr_crypto_init()
> cleanup at root pool
>
> root pool destroy:
> sub pool destroy
> -> mod_ssl cleanup called
> -> apr cleanup called
We have
1. ap_init_rng in main.c causes apr_crypto_init called with ap_pglobal
2. This causes apr__crypto_openssl_term to be registered as a cleanup with ap_pglobal.
3. mod_ssl is loaded via the pconf pool which is a child of ap_pglobal
4. The loading of mod_ssl triggers the loading of libssl via the dynamic linker.
5. libssl is initialized in ssl_hook_pre_config via
SSL_load_error_strings();
SSL_library_init();
6. The above seems to cause references to static data in libssl being saved in data structures
managed by libcrypto.
7. apr_pool_destroy(process->pool) in main.c:283 destroys the ap_pglobal which in turn
a) first destroys pconf
b) which causes mod_ssl to be unloaded
c) which cause libssl to be unloaded
8. The cleanups of ap_pglobal are now called, especially apr__crypto_openssl_term which
a) calls EVP_cleanup()
b) which tries to access the data stored in 6., but is gone since 7c)
Using a pre_cleanup moves 8. before 7. and thus removes the issue.
Regards
Rüdiger
Re: svn commit: r1198930 - in /httpd/httpd/trunk: include/mod_core.h
server/core.c server/main.c
Posted by Stefan Eissing <st...@greenbytes.de>.
(I am trying to understand this issue, since someone reported probs
with mod_md/mod_ssl/some db drive init order related to openssl init.)
The the move from cleanup() to pre_cleanup() fixed this issue. It means (as I read it)
that something was happening in sub-pools cleanup()s that was out-of-order.
Looking at apr_crypto_init(), it registers the cleanup at the "root" pool,
does this give a clue?
So we might have:
- mod_ssl loading libssl, loading libcrypto
cleanup at pool
- ...
- someone (indirectly) calling apr_crypto_init()
cleanup at root pool
root pool destroy:
sub pool destroy
-> mod_ssl cleanup called
-> apr cleanup called
with the fix reverting the cleanup order? Is that what's happening?
-Stefan
> Am 03.08.2018 um 14:50 schrieb Ruediger Pluem <rp...@apache.org>:
>
>
>
> On 08/03/2018 12:45 PM, Yann Ylavic wrote:
>> On Fri, Aug 3, 2018 at 11:46 AM, Ruediger Pluem <rp...@apache.org> wrote:
>>>> + ap_init_rng(ap_pglobal);
>>>
>>> With APR trunk used this now causes httpd to SEGFAULT in EVP_cleanup
>>> when it stops in case mod_ssl is loaded. This is because mod_ssl
>>> stored data in Openssl data structures that points to it (likely
>>> static data in mod_ssl), but it gets unloaded due to the pconf pool
>>> cleanup before the crypto_lib_cleanup runs EVP_cleanup as it is a
>>> cleanup on the parent pool ap_pglobal.
>>
>> Ouch, ISTM that mod_ssl should cleanup what it owns after itself.
>> Any idea which static data (or code/callbacks) in mod_ssl are still pointed to?
>
> The static data referenced here is in libssl. So libssl gets unloaded because mod_ssl gets unloaded,
> libcrypto stays as we have it linked to APR. So we get caught by some internal referencing
> between libssl and libcrypto. So probably the crypto part of APR needs to load libssl as well.
>
> Regards
>
> Rüdiger
Re: svn commit: r1198930 - in /httpd/httpd/trunk: include/mod_core.h
server/core.c server/main.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 08/03/2018 12:45 PM, Yann Ylavic wrote:
> On Fri, Aug 3, 2018 at 11:46 AM, Ruediger Pluem <rp...@apache.org> wrote:
>>> + ap_init_rng(ap_pglobal);
>>
>> With APR trunk used this now causes httpd to SEGFAULT in EVP_cleanup
>> when it stops in case mod_ssl is loaded. This is because mod_ssl
>> stored data in Openssl data structures that points to it (likely
>> static data in mod_ssl), but it gets unloaded due to the pconf pool
>> cleanup before the crypto_lib_cleanup runs EVP_cleanup as it is a
>> cleanup on the parent pool ap_pglobal.
>
> Ouch, ISTM that mod_ssl should cleanup what it owns after itself.
> Any idea which static data (or code/callbacks) in mod_ssl are still pointed to?
The static data referenced here is in libssl. So libssl gets unloaded because mod_ssl gets unloaded,
libcrypto stays as we have it linked to APR. So we get caught by some internal referencing
between libssl and libcrypto. So probably the crypto part of APR needs to load libssl as well.
Regards
Rüdiger
Re: svn commit: r1198930 - in /httpd/httpd/trunk: include/mod_core.h
server/core.c server/main.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Fri, Aug 3, 2018 at 2:33 PM, Stefan Eissing
<st...@greenbytes.de> wrote:
>>
>>> Is a reference counting de-allocation not better fitting?
>>
>> Not sure, at least for httpd, the first module loaded will be the last
>> cleaned up.
>> But refcounting and/or attaching the cleanup to the longest living
>> pool could be an option from a general usage POV.
>
> But that assumes that no module uses a function that uses a
> function...that calls crypto_init on a temp pool during conf test,
> for example, right?
This shouldn't be an issue from the APR's point of view (performance
is another matter).
But from the lib's POV this is another story though, for instance
openssl can't re-init so unless crypto_lib_init() was called at a
higher level you wouldn't be able to call that function multiple times
in the same program...
This is why the caller should be able to control the lifetime of
init/deinit (just like it controls the lifetime of DSOs), and here
call crypto_lib_init() outside the function (e.g. on a pool living at
least until that function is not called anymore).
Re: svn commit: r1198930 - in /httpd/httpd/trunk: include/mod_core.h
server/core.c server/main.c
Posted by Stefan Eissing <st...@greenbytes.de>.
> Am 03.08.2018 um 14:24 schrieb Yann Ylavic <yl...@gmail.com>:
>
> On Fri, Aug 3, 2018 at 1:41 PM, Stefan Eissing
> <st...@greenbytes.de> wrote:
>>
>> Never looked at it before. How is the abstraction in apr_crypto supposed to manage
>> the lifetime of the components? E.g. when calling apr_crypto_prng_init()
>> one ties the whole openssl crypto's lifetime to the pool given there?
>
> I'll talk about apr_crypto_lib_init() instead, which is where the
> lifetime of the libs is handled (e.g. openssl), while
> apr_crypto_prng_init() is only a consumer of apr_crypto_lib_init().
>
> But yes, almost, apr_crypto_lib_init() maintains the
> active/initialized libs in a hashtable (APR's root pool).
> If the lib to init is not already there it's inited and a cleanup is
> registered on the given pool to deinit (and remove from the global
> hashtable).
>
>>
>> Does everyone check for APR_EREINIT? And if it comes, what is one supposed to do?
>
> APR_EREINIT should at least no be handled as a real error, one can do
> whatever when it (if it matters to not be the first consumer), but
> usually nothing special has to be done: treat it like SUCCESS.
>
> In httpd, it means that the core and/or a previously loaded module
> already inited the lib, so be it, it will be deinited with that module
> or the core, not this module's business.
> But anyway, even if APR_SUCCESS is returned, there is nothing more to
> do for the module, pconf will do the deinit job.
>
>> Is a reference counting de-allocation not better fitting?
>
> Not sure, at least for httpd, the first module loaded will be the last
> cleaned up.
> But refcounting and/or attaching the cleanup to the longest living
> pool could be an option from a general usage POV.
But that assumes that no module uses a function that uses a function...that calls crypto_init on a temp pool during conf test, for example, right?
Re: svn commit: r1198930 - in /httpd/httpd/trunk: include/mod_core.h
server/core.c server/main.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Fri, Aug 3, 2018 at 1:41 PM, Stefan Eissing
<st...@greenbytes.de> wrote:
>
> Never looked at it before. How is the abstraction in apr_crypto supposed to manage
> the lifetime of the components? E.g. when calling apr_crypto_prng_init()
> one ties the whole openssl crypto's lifetime to the pool given there?
I'll talk about apr_crypto_lib_init() instead, which is where the
lifetime of the libs is handled (e.g. openssl), while
apr_crypto_prng_init() is only a consumer of apr_crypto_lib_init().
But yes, almost, apr_crypto_lib_init() maintains the
active/initialized libs in a hashtable (APR's root pool).
If the lib to init is not already there it's inited and a cleanup is
registered on the given pool to deinit (and remove from the global
hashtable).
>
> Does everyone check for APR_EREINIT? And if it comes, what is one supposed to do?
APR_EREINIT should at least no be handled as a real error, one can do
whatever when it (if it matters to not be the first consumer), but
usually nothing special has to be done: treat it like SUCCESS.
In httpd, it means that the core and/or a previously loaded module
already inited the lib, so be it, it will be deinited with that module
or the core, not this module's business.
But anyway, even if APR_SUCCESS is returned, there is nothing more to
do for the module, pconf will do the deinit job.
> Is a reference counting de-allocation not better fitting?
Not sure, at least for httpd, the first module loaded will be the last
cleaned up.
But refcounting and/or attaching the cleanup to the longest living
pool could be an option from a general usage POV.
Re: svn commit: r1198930 - in /httpd/httpd/trunk: include/mod_core.h
server/core.c server/main.c
Posted by Stefan Eissing <st...@greenbytes.de>.
> Am 03.08.2018 um 13:34 schrieb Yann Ylavic <yl...@gmail.com>:
>
> On Fri, Aug 3, 2018 at 12:45 PM, Yann Ylavic <yl...@gmail.com> wrote:
>> On Fri, Aug 3, 2018 at 11:46 AM, Ruediger Pluem <rp...@apache.org> wrote:
>>>> + ap_init_rng(ap_pglobal);
>>>
>>> With APR trunk used this now causes httpd to SEGFAULT in EVP_cleanup
>>> when it stops in case mod_ssl is loaded. This is because mod_ssl
>>> stored data in Openssl data structures that points to it (likely
>>> static data in mod_ssl), but it gets unloaded due to the pconf pool
>>> cleanup before the crypto_lib_cleanup runs EVP_cleanup as it is a
>>> cleanup on the parent pool ap_pglobal.
>>
>> Ouch, ISTM that mod_ssl should cleanup what it owns after itself.
>> Any idea which static data (or code/callbacks) in mod_ssl are still pointed to?
>
> Hmm, while all the libcrypto part is initialized in APR, the libssl
> one is (used in) mod_ssl only, so SSL_load_error_strings() and
> SSL_library_init() are scoped to mod_ssl but not de-inited with pconf.
> I'm not sure there is a way to de-init them (and only them)...
>
> Possibly apr_crypto_lib_init() should be able to init libssl too (optionally).
Never looked at it before. How is the abstraction in apr_crypto supposed to manage
the lifetime of the components? E.g. when calling apr_crypto_prng_init()
one ties the whole openssl crypto's lifetime to the pool given there?
Does everyone check for APR_EREINIT? And if it comes, what is one supposed to do?
Is a reference counting de-allocation not better fitting?
-Stefan
Re: svn commit: r1198930 - in /httpd/httpd/trunk: include/mod_core.h
server/core.c server/main.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Fri, Aug 3, 2018 at 12:45 PM, Yann Ylavic <yl...@gmail.com> wrote:
> On Fri, Aug 3, 2018 at 11:46 AM, Ruediger Pluem <rp...@apache.org> wrote:
>>> + ap_init_rng(ap_pglobal);
>>
>> With APR trunk used this now causes httpd to SEGFAULT in EVP_cleanup
>> when it stops in case mod_ssl is loaded. This is because mod_ssl
>> stored data in Openssl data structures that points to it (likely
>> static data in mod_ssl), but it gets unloaded due to the pconf pool
>> cleanup before the crypto_lib_cleanup runs EVP_cleanup as it is a
>> cleanup on the parent pool ap_pglobal.
>
> Ouch, ISTM that mod_ssl should cleanup what it owns after itself.
> Any idea which static data (or code/callbacks) in mod_ssl are still pointed to?
Hmm, while all the libcrypto part is initialized in APR, the libssl
one is (used in) mod_ssl only, so SSL_load_error_strings() and
SSL_library_init() are scoped to mod_ssl but not de-inited with pconf.
I'm not sure there is a way to de-init them (and only them)...
Possibly apr_crypto_lib_init() should be able to init libssl too (optionally).
Re: svn commit: r1198930 - in /httpd/httpd/trunk: include/mod_core.h
server/core.c server/main.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Fri, Aug 3, 2018 at 11:46 AM, Ruediger Pluem <rp...@apache.org> wrote:
>> + ap_init_rng(ap_pglobal);
>
> With APR trunk used this now causes httpd to SEGFAULT in EVP_cleanup
> when it stops in case mod_ssl is loaded. This is because mod_ssl
> stored data in Openssl data structures that points to it (likely
> static data in mod_ssl), but it gets unloaded due to the pconf pool
> cleanup before the crypto_lib_cleanup runs EVP_cleanup as it is a
> cleanup on the parent pool ap_pglobal.
Ouch, ISTM that mod_ssl should cleanup what it owns after itself.
Any idea which static data (or code/callbacks) in mod_ssl are still pointed to?
> One approach that made this go away was the following patch to APR:
>
> Index: srclib/apr/crypto/apr_crypto.c
> ===================================================================
> --- srclib/apr/crypto/apr_crypto.c (revision 1837332)
> +++ srclib/apr/crypto/apr_crypto.c (working copy)
> @@ -534,8 +534,7 @@
> lib->pool = pool;
> apr_hash_set(active_libs, lib->name, APR_HASH_KEY_STRING, lib);
> if (apr_pool_parent_get(pool)) {
> - apr_pool_cleanup_register(pool, lib, crypto_lib_cleanup,
> - apr_pool_cleanup_null);
> + apr_pool_pre_cleanup_register(pool, lib, crypto_lib_cleanup);
> }
> }
> else {
>
> So using a pre_cleanup instead of a cleanup. But I guess this would only fix this specific use case. I guess we have
> a larger problem lurking around that a shared object can put some data in Openssl that points to it (e.g. static data in
> the shared object) and that it is gone by the time the pool cleanup runs. In this case we are lucky that a child pool
> causes the shared object to be unloaded and hence the pre_cleanup works here. But IMHO we don't need to have this
> connection always.
It looks fragile yes :/
I'd prefer each module to cleanup after itself in openssl, if possible...
Regards,
Yann.