Re: svn commit: r1836439 [2/3] - in /apr/apr/trunk: ./ crypto/ include/ include/private/ test/

[Yann Ylavic <yl...@gmail.com>]

On Sun, Jul 22, 2018 at 3:11 PM,  <mi...@apache.org> wrote:
> --- apr/apr/trunk/crypto/apr_crypto_openssl.c (original)
> +++ apr/apr/trunk/crypto/apr_crypto_openssl.c Sun Jul 22 13:11:32 2018
>
> @@ -106,16 +143,87 @@ static apr_status_t crypto_error(const a
>   */
>  static apr_status_t crypto_shutdown(void)
>  {
> -    return apr_crypto_lib_term("openssl");
> +    ERR_free_strings();
> +    EVP_cleanup();
> +    ENGINE_cleanup();
> +    return APR_SUCCESS;
> +}

So you reverted this...

> +
> +static apr_status_t crypto_shutdown_helper(void *data)
> +{
> +    return crypto_shutdown();
>  }
>
>  /**
>   * Initialise the crypto library and perform one time initialisation.
>   */
>  static apr_status_t crypto_init(apr_pool_t *pool, const char *params,
> -                                const apu_err_t **result)
> +        const apu_err_t **result)
> +{
> +#if APR_USE_OPENSSL_PRE_1_1_API
> +    (void)CRYPTO_malloc_init();
> +#else
> +    OPENSSL_malloc_init();
> +#endif
> +    ERR_load_crypto_strings();
> +    /* SSL_load_error_strings(); */
> +    OpenSSL_add_all_algorithms();
> +    ENGINE_load_builtin_engines();
> +    ENGINE_register_all_complete();
> +
> +    apr_pool_cleanup_register(pool, pool, crypto_shutdown_helper,
> +            apr_pool_cleanup_null);
> +
> +    return APR_SUCCESS;
> +}
> -    return apr_crypto_lib_init("openssl", params, result, pool);

... and this (including for the other libs), because?

The goal was to allow the initialization of the crypto libs
independently from DSOs (openssl, for instance, does not play very
well there).

In httpd there is (at least) APR, the httpd core, mod_ssl and mod_md
that can all use the crypto library, how is each one supposed to
initialize it? I thought it was nice for APR to provide a single
initializer, once for all, with the lifetime of the given pool (as
usual)...


Regards,
Yann.

Re: svn commit: r1836439 [2/3] - in /apr/apr/trunk: ./ crypto/ include/ include/private/ test/

[Graham Leggett <mi...@sharp.fm>]

On 23 Jul 2018, at 01:26, Yann Ylavic <yl...@gmail.com> wrote:

> On Sun, Jul 22, 2018 at 3:11 PM,  <mi...@apache.org> wrote:
>> --- apr/apr/trunk/crypto/apr_crypto_openssl.c (original)
>> +++ apr/apr/trunk/crypto/apr_crypto_openssl.c Sun Jul 22 13:11:32 2018
>> 
>> @@ -106,16 +143,87 @@ static apr_status_t crypto_error(const a
>>  */
>> static apr_status_t crypto_shutdown(void)
>> {
>> -    return apr_crypto_lib_term("openssl");
>> +    ERR_free_strings();
>> +    EVP_cleanup();
>> +    ENGINE_cleanup();
>> +    return APR_SUCCESS;
>> +}
> 
> So you reverted this...
> 
>> +
>> +static apr_status_t crypto_shutdown_helper(void *data)
>> +{
>> +    return crypto_shutdown();
>> }
>> 
>> /**
>>  * Initialise the crypto library and perform one time initialisation.
>>  */
>> static apr_status_t crypto_init(apr_pool_t *pool, const char *params,
>> -                                const apu_err_t **result)
>> +        const apu_err_t **result)
>> +{
>> +#if APR_USE_OPENSSL_PRE_1_1_API
>> +    (void)CRYPTO_malloc_init();
>> +#else
>> +    OPENSSL_malloc_init();
>> +#endif
>> +    ERR_load_crypto_strings();
>> +    /* SSL_load_error_strings(); */
>> +    OpenSSL_add_all_algorithms();
>> +    ENGINE_load_builtin_engines();
>> +    ENGINE_register_all_complete();
>> +
>> +    apr_pool_cleanup_register(pool, pool, crypto_shutdown_helper,
>> +            apr_pool_cleanup_null);
>> +
>> +    return APR_SUCCESS;
>> +}
>> -    return apr_crypto_lib_init("openssl", params, result, pool);
> 
> ... and this (including for the other libs), because?

Because it was an accident. I missed this one when merging.

Let me take a look at this tomorrow.

> The goal was to allow the initialization of the crypto libs
> independently from DSOs (openssl, for instance, does not play very
> well there).
> 
> In httpd there is (at least) APR, the httpd core, mod_ssl and mod_md
> that can all use the crypto library, how is each one supposed to
> initialize it? I thought it was nice for APR to provide a single
> initializer, once for all, with the lifetime of the given pool (as
> usual)…

I've raised this issue before, but there was little interest in solving it - glad to see this has got some traction.

I believe libraries like NSS are reference counted and initialise themselves sensibly. Not sure about the behaviour of openssl, however the openssl people should be able to explain what people should be doing.

Regards,
Graham
—