(I can't quote easily with the html indentation and must edit your parts, so please use plain text)
On Tue, Jul 24, 2018 at 4:12 PM, Graham Leggett <minf...@sharp.fm> wrote: > On 24 Jul 2018, at 15:26, Yann Ylavic <ylavic....@gmail.com> wrote: > >>> Having looked at this in more detail, I am -1 on this change on the >>> following basis: >>> >>> - We create a hard link between our crypto libraries and APR itself, >>> which both breaks and renders pointless the existing DSO mechanism, >>> and makes APR significantly more expensive and less attractive to >>> use. > >> This is being discussed in the other, so I won't my arguments here. > > Just to clarify, I’m -1 (veto). So I have to convince you (let's try again :p ) or revert, this is it? I'd like to ear what others think though... >> I agree that APR shouldn't de-init *but* when openssl is unloaded, >> which APR can't control, even with DSO. > > APR controls the loading and unloading of APR DSOs, there is nothing > stopping us making the DSO unload a noop when broken versions of openssl are > present. But de-init may be required/desired when APR loads (e.g.) openssl for its own use no? What if the user wants openssl (< 1.1) cleanup when the program stops (possibly that'd clear some memory or alike)? >> What matters more is that APR shouldn't init either unless it is the >> one loading openssl (dlopen() is refcounted!), and that's user's >> knowledge still, not APR’s. > > The apr_crypto_nss driver supports a “noinit” option that does exactly this, > nothing stops us adding the same to apr_crypto_openssl. Please note that apr_crypto_lib_init() takes parameters as argument too. >> The latest openssl is certainly a noop when init is called multiple >> times, but earlier versions leak all over the place when this happens. >> The openssl team tried hard to de-init/re-init(), but finally failed >> to find a portable way to do it and reintroduced some static variables >> which prevent it, including in the latest/master code. >> Unless we want to take (and fail) the same way, we have no other >> choice than let the user decide when to init and de-init, ie. with our >> usual given pool's lifetime. > > What our choices are must be confirmed by the openssl team. I;ve asked on > the openssl-users list and am waiting for confirmation. > > If openssl v1.1.0 is still broken, we just noop the DSO unload - however see > below. We must take multiple openssl versions into account, not only the latest one. If you want an example of what needs to be done for openssl loading/unloading to work and/or not leak accross versions, the history of httpd's ssl_hook_pre_config() and ssl_cleanup_pre_config() are worth a look too. >> I see apr_crypto_lib_init() as the initial call to control openssl >> (de-)init for users of APR and openssl (or another crypto lib handled >> by APR) in the same program. > > This function is functionally identical to the apr_crypto_make() function. You probably mean crypto_init() but yes, same arguments and code moved from there to apr_crypto_lib_init() is on purpose. > > What seems to be a more sensible approach is a general mechanism that allows > the caller to tell APR what external libraries have been loaded. > > In other words, don’t try and initialise openssl, just make a note that the > caller has told us that openssl has been used externally FYI. > > Later on, should we load our DSO, we are in a position to bypass the init > step and to to noop the unload step and avoid any problems. I fail to see how it's better than not calling apr_crypto_lib_init(), and that doesn't seem contradictory with my proposal either. Pass that option to apr_crypto_get_driver() if it shouldn't init the lib because your code already does it, or call apr_crypto_get_driver() without the option because you want APR to handle its own openssl stuff, or call apr_crypto_lib_init() explicitely because you also use openssl but don't want to handle all of its init oddnesses. All these uses cases look interesting to me. Regards, Yann.
