I have a question in this area, not necessarily a result of this commit.
I have been playing with a new crypto provider, in a non-DSO build,
intended to be used with httpd.
The provider does have both an initialization and termination API.
My issue is a result of the "rootp" being used for the driver hashmap
but "pconf" (in the httpd/mod_session_crypto case) being passed to the
drivers init() function the first time it's loaded. This means at
graceful restart, cleanups registered by the drivers init() function
will run, but apr_crypto_term will not, and the next call to get the
driver will not re-run the init().
Making the lifetimes match either way fixes my test -- either moving
the "rootp" stuff to DSO-only for apr_crypto_init or by passing
"rootp" to the DRIVER_LOAD macro for non-DSO init.
Any opinions?
On Thu, Jun 14, 2018 at 12:34 PM wrote:
>
> Author: ylavic
> Date: Thu Jun 14 16:34:49 2018
> New Revision: 1833525
>
> URL: http://svn.apache.org/viewvc?rev=1833525=rev
> Log:
> apr_crypto: follow up to r1833359: fix some root pool scopes (possible leaks).
>
> Keep the root pool scope for things that need it only (global lists of drivers
> or libs), but otherwise use the passed in pool (default/global PRNG, errors).
>
> This allows the caller to control the scope of initialization functions, and
> for instance be able to re-initialize when apr_crypto is unloaded/reloaded
> from
> a DSO attached to the passed-in pool (e.g. mod_ssl in httpd).
>
>
> Modified:
> apr/apr/trunk/crypto/apr_crypto.c
> apr/apr/trunk/crypto/apr_crypto_internal.c
> apr/apr/trunk/test/testcrypto.c
> apr/apr/trunk/util-misc/apu_dso.c
>
> Modified: apr/apr/trunk/crypto/apr_crypto.c
> URL:
> http://svn.apache.org/viewvc/apr/apr/trunk/crypto/apr_crypto.c?rev=1833525=1833524=1833525=diff
> ==
> --- apr/apr/trunk/crypto/apr_crypto.c (original)
> +++ apr/apr/trunk/crypto/apr_crypto.c Thu Jun 14 16:34:49 2018
> @@ -37,8 +37,6 @@ static apr_hash_t *drivers = NULL;
>
> #define ERROR_SIZE 1024
>
> -#define CLEANUP_CAST (apr_status_t (*)(void*))
> -
> APR_TYPEDEF_STRUCT(apr_crypto_t,
> apr_pool_t *pool;
> apr_crypto_driver_t *provider;
> @@ -87,26 +85,29 @@ static apr_status_t apr_crypto_term(void
> APR_DECLARE(apr_status_t) apr_crypto_init(apr_pool_t *pool)
> {
> apr_status_t rv;
> -apr_pool_t *parent;
> +apr_pool_t *rootp;
> int flags = 0;
>
> if (drivers != NULL) {
> return APR_SUCCESS;
> }
>
> -/* Top level pool scope, need process-scope lifetime */
> -for (parent = apr_pool_parent_get(pool);
> - parent && parent != pool;
> - parent = apr_pool_parent_get(pool))
> -pool = parent;
> +/* Top level pool scope, for drivers' process-scope lifetime */
> +rootp = pool;
> +for (;;) {
> +apr_pool_t *p = apr_pool_parent_get(rootp);
> +if (!p || p == rootp) {
> +break;
> +}
> +rootp = p;
> +}
> #if APR_HAVE_MODULAR_DSO
> /* deprecate in 2.0 - permit implicit initialization */
> -apu_dso_init(pool);
> +apu_dso_init(rootp);
> #endif
> -drivers = apr_hash_make(pool);
> -
> -apr_pool_cleanup_register(pool, NULL, apr_crypto_term,
> -apr_pool_cleanup_null);
> +drivers = apr_hash_make(rootp);
> +apr_pool_cleanup_register(rootp, NULL, apr_crypto_term,
> + apr_pool_cleanup_null);
>
> /* apr_crypto_prng_init() may already have been called with
> * non-default parameters, so ignore APR_EREINIT.
> @@ -203,6 +204,7 @@ APR_DECLARE(apr_status_t) apr_crypto_get
> char symname[34];
> apr_dso_handle_t *dso;
> apr_dso_handle_sym_t symbol;
> +apr_pool_t *rootp;
> #endif
> apr_status_t rv;
>
> @@ -227,7 +229,7 @@ APR_DECLARE(apr_status_t) apr_crypto_get
> #if APR_HAVE_MODULAR_DSO
> /* The driver DSO must have exactly the same lifetime as the
> * drivers hash table; ignore the passed-in pool */
> -pool = apr_hash_pool_get(drivers);
> +rootp = apr_hash_pool_get(drivers);
>
> #if defined(NETWARE)
> apr_snprintf(modname, sizeof(modname), "crypto%s.nlm", name);
> @@ -239,21 +241,19 @@ APR_DECLARE(apr_status_t) apr_crypto_get
> "apr_crypto_%s-" APR_STRINGIFY(APR_MAJOR_VERSION) ".so", name);
> #endif
> apr_snprintf(symname, sizeof(symname), "apr_crypto_%s_driver", name);
> -rv = apu_dso_load(, , modname, symname, pool);
> +rv = apu_dso_load(, , modname, symname, rootp);
> if (rv == APR_SUCCESS || rv == APR_EINIT) { /* previously loaded?!? */
> apr_crypto_driver_t *d = symbol;
> rv = APR_SUCCESS;
> if (d->init) {
> -rv = d->init(pool, params, result);
> +rv = d->init(rootp, params, result);
> if (rv == APR_EREINIT) {
> rv = APR_SUCCESS;
> }
> }
> if (rv ==
