On Tue, Jul 24, 2018 at 2:10 PM, Graham Leggett <minf...@sharp.fm> wrote: > On 12 Jun 2018, at 22:11, yla...@apache.org wrote: > >> Author: ylavic >> Date: Tue Jun 12 20:11:09 2018 >> New Revision: 1833421 >> >> URL: http://svn.apache.org/viewvc?rev=1833421&view=rev >> Log: >> apr_crypto: follow up to r1833359. >> >> Link underlying crypto libraries (openssl, nss, and commoncrypto) with libapr >> when the corresponding --with is configured, and allow to initialize, >> terminate >> or check whether initialized with respectively them with >> apr_crypto_lib_init(), >> apr_crypto_lib_term() or apr_crypto_lib_is_initialized(). >> >> This allows for users to control the (un)initialization of those libraries, >> notably when they are also used independently in the user code and that doing >> this multiple times can cause leaks or unexpected behaviour. >> >> The initialization code is moved from >> "apr_crypto_{openssl,nss,commoncrypto}.c" >> where previously loaded dynamically (DSO) to "apr_crypto_internal.c" which is >> linked with libapr. >> >> Now apr_crypto_prng_init() can make sure the underlying crypto lib is ready. > > 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. > > - This doesn’t fix the problem for OpenSSL. The problem is related to > legacy OpenSSL’s inability to de-init and re-init, which google shows > me is a universal problem suffered by other libraries, not just us. > The fix appears to be to not de-init the library in the DSO, but I;ve > asked the openssl user’s list to be sure. In other words, this > attempt to init is unnecessary, we should actually not de-init. I agree that APR shouldn't de-init *but* when openssl is unloaded, which APR can't control, even with DSO. 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 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. 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 change affects NSS, which doesn’t suffer from this problem as > NSS supports proper reference counting and multiple inits. Touching > the NSS code is unnecessary. So it shouldn't hurt, we aim to provide portable code, same user code regardless of the underlying system/lib right? > > - This problem appears to have been fixed in openssl v1.1.0, where no > initialisation is required at all. I have asked on openssl-users for > clarification on this. > https://www.openssl.org/docs/man1.1.0/crypto/OPENSSL_init_crypto.html This isn't the only version supported by OpenSSL, distros nor APR (you even added compatibility code for 0.9.8b in one of your latests commits...). I think we should provide a mechanism that works for as much (maintained) versions as possible, which seems to be the case here. > > To be clear, I am only referring to this change here, this is > unrelated to the PRNG patch which is a separate discussion. Agreed, thanks. Regards, Yann.
