On 13.07.2015 16:03, Joe Orton wrote: > On Sat, Jul 11, 2015 at 04:40:20PM +0200, Kaspar Brand wrote: >> @@ -1902,5 +1907,7 @@ apr_status_t ssl_init_ModuleKill(void *data) >> >> free_dh_params(); >> >> + OBJ_cleanup(); >> + >> return APR_SUCCESS; > > From being burnt previously three or four times, I get scared by OpenSSL > process global stuff. > > Have you worked out that it's safe to do that call there? It looks odd > to do that there rather than alongside other global cleanups in > ssl_cleanup_pre_config, so it's at least worth a comment if you really > want this here.
Thanks for the heads up. After further digging, I believe it's not really comparable to the cases you're probably referring to (only found these two): 1) ERR_load_crypto_strings() / ERR_free_strings() Originally addressed in https://svn.apache.org/r101624, it was subsequently fixed in OpenSSL (0.9.8e and later, see [1]), and in late 2014 also adjusted for more recent OpenSSL versions (PR 53435 / https://bz.apache.org/bugzilla/show_bug.cgi?id=53435). 2) CRYPTO_new_ex_data() / CRYPTO_cleanup_all_ex_data() Addressed in https://svn.apache.org/r654119 (by dropping the CRYPTO_cleanup_all_ex_data call), see also PR 44975 / https://bz.apache.org/bugzilla/show_bug.cgi?id=44975). OBJ_create() / OBJ_cleanup() doesn't seem to suffer from these problems, but on the other hand, I'm also fine with dropping the OBJ_cleanup() call, since we only call OBJ_create() if the table entry doesn't exist yet (i.e. even repeated calls in ssl_init_Module / reloads do not cause any "leak"). > There is some complicated interaction between EVP_cleanup() and > OBJ_cleanup() which I haven't tried to decipher, but it looks like > EVP_cleanup() will actually do the cleanup call? This was added with 1.0.0, apparently [2]. The code would apply if OBJ_cleanup were called *before* EVP_cleanup (in all other cases, EVP_cleanup does not call OBJ_cleanup). But, to be on the safe side, I think we could a) move the OBJ_create() call to ssl_hook_pre_config and b) omit OBJ_cleanup(). Do you concur? Kaspar [1] https://git.openssl.org/?p=openssl.git;a=commitdiff;h=35e59297fca389b68bcad29876927666300ce971 [2] https://git.openssl.org/?p=openssl.git;a=commitdiff;h=246e09319c1d2a8140ffe1e5aeb1be26015696f0
