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 <yla...@apache.org> wrote: > > Author: ylavic > Date: Thu Jun 14 16:34:49 2018 > New Revision: 1833525 > > URL: http://svn.apache.org/viewvc?rev=1833525&view=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&r1=1833524&r2=1833525&view=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(&dso, &symbol, modname, symname, pool); > + rv = apu_dso_load(&dso, &symbol, 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 == APR_SUCCESS) { > - *driver = symbol; > - name = apr_pstrdup(pool, name); > - apr_hash_set(drivers, name, APR_HASH_KEY_STRING, *driver); > - rv = APR_SUCCESS; > + apr_hash_set(drivers, d->name, APR_HASH_KEY_STRING, d); > + *driver = d; > } > } > apu_dso_mutex_unlock(); > @@ -274,32 +274,38 @@ APR_DECLARE(apr_status_t) apr_crypto_get > > /* Load statically-linked drivers: */ > #if APU_HAVE_OPENSSL > - if (name[0] == 'o' && !strcmp(name, "openssl")) { > + if (!strcmp(name, "openssl")) { > DRIVER_LOAD("openssl", apr_crypto_openssl_driver, pool, params, rv, > result); > } > + else > #endif > #if APU_HAVE_NSS > - if (name[0] == 'n' && !strcmp(name, "nss")) { > + if (!strcmp(name, "nss")) { > DRIVER_LOAD("nss", apr_crypto_nss_driver, pool, params, rv, result); > } > + else > #endif > #if APU_HAVE_COMMONCRYPTO > - if (name[0] == 'c' && !strcmp(name, "commoncrypto")) { > + if (!strcmp(name, "commoncrypto")) { > DRIVER_LOAD("commoncrypto", apr_crypto_commoncrypto_driver, pool, > params, rv, result); > } > + else > #endif > #if APU_HAVE_MSCAPI > - if (name[0] == 'm' && !strcmp(name, "mscapi")) { > + if (!strcmp(name, "mscapi")) { > DRIVER_LOAD("mscapi", apr_crypto_mscapi_driver, pool, params, rv, > result); > } > + else > #endif > #if APU_HAVE_MSCNG > - if (name[0] == 'm' && !strcmp(name, "mscng")) { > + if (!strcmp(name, "mscng")) { > DRIVER_LOAD("mscng", apr_crypto_mscng_driver, pool, params, rv, > result); > } > + else > #endif > + ; > > -#endif > +#endif /* !APR_HAVE_MODULAR_DSO */ > > return rv; > } > @@ -407,7 +413,7 @@ APR_DECLARE(apr_status_t) apr_crypto_lib > apr_pool_t *pool) > { > apr_status_t rv; > - apr_pool_t *rootp, *p; > + apr_pool_t *rootp; > struct crypto_lib *lib; > > if (!name) { > @@ -419,7 +425,11 @@ APR_DECLARE(apr_status_t) apr_crypto_lib > } > > rootp = pool; > - while ((p = apr_pool_parent_get(rootp))) { > + for (;;) { > + apr_pool_t *p = apr_pool_parent_get(rootp); > + if (!p || p == rootp) { > + break; > + } > rootp = p; > } > > @@ -495,8 +505,10 @@ APR_DECLARE(apr_status_t) apr_crypto_lib > if (rv == APR_SUCCESS) { > lib->pool = pool; > apr_hash_set(active_libs, lib->name, APR_HASH_KEY_STRING, lib); > - apr_pool_cleanup_register(lib->pool, lib, crypto_lib_cleanup, > - apr_pool_cleanup_null); > + if (apr_pool_parent_get(pool)) { > + apr_pool_cleanup_register(pool, lib, crypto_lib_cleanup, > + apr_pool_cleanup_null); > + } > } > else { > spare_lib_push(lib); > @@ -513,6 +525,9 @@ static apr_status_t crypto_lib_term(cons > if (!lib) { > return APR_EINIT; > } > + if (!apr_pool_parent_get(lib->pool)) { > + return APR_EBUSY; > + } > > rv = APR_ENOTIMPL; > #if APU_HAVE_OPENSSL > @@ -560,12 +575,15 @@ APR_DECLARE(apr_status_t) apr_crypto_lib > } > > if (!name) { > + apr_status_t rv = APR_SUCCESS; > apr_hash_index_t *hi = apr_hash_first(NULL, active_libs); > for (; hi; hi = apr_hash_next(hi)) { > - name = apr_hash_this_key(hi); > - crypto_lib_term(name); > + apr_status_t rt = crypto_lib_term(apr_hash_this_key(hi)); > + if (rt != APR_SUCCESS && (rv == APR_SUCCESS || rv == APR_EBUSY)) > { > + rv = rt; > + } > } > - return APR_SUCCESS; > + return rv; > } > > return crypto_lib_term(name); > > Modified: apr/apr/trunk/crypto/apr_crypto_internal.c > URL: > http://svn.apache.org/viewvc/apr/apr/trunk/crypto/apr_crypto_internal.c?rev=1833525&r1=1833524&r2=1833525&view=diff > ============================================================================== > --- apr/apr/trunk/crypto/apr_crypto_internal.c (original) > +++ apr/apr/trunk/crypto/apr_crypto_internal.c Thu Jun 14 16:34:49 2018 > @@ -284,8 +284,8 @@ const char *apr__crypto_mscng_version(vo > } > > apr_status_t apr__crypto_mscng_init(const char *params, > - const apu_err_t **result, > - apr_pool_t *pool) > + const apu_err_t **result, > + apr_pool_t *pool) > { > return APR_ENOTIMPL; > } > > Modified: apr/apr/trunk/test/testcrypto.c > URL: > http://svn.apache.org/viewvc/apr/apr/trunk/test/testcrypto.c?rev=1833525&r1=1833524&r2=1833525&view=diff > ============================================================================== > --- apr/apr/trunk/test/testcrypto.c (original) > +++ apr/apr/trunk/test/testcrypto.c Thu Jun 14 16:34:49 2018 > @@ -559,7 +559,10 @@ static void test_crypto_init(abts_case * > > apr_pool_create(&pool, NULL); > > - rv = apr_crypto_init(pool); > + /* Use root pool (top level init) so that the crypto lib is > + * not cleanup on pool destroy below (e.g. openssl can't re-init). > + */ > + rv = apr_crypto_init(apr_pool_parent_get(pool)); > ABTS_ASSERT(tc, "failed to init apr_crypto", rv == APR_SUCCESS); > > apr_pool_destroy(pool); > @@ -1680,7 +1683,7 @@ abts_suite *testcrypto(abts_suite *suite > { > suite = ADD_SUITE(suite); > > - /* test simple init and shutdown */ > + /* test simple init and shutdown (keep first) */ > abts_run_test(suite, test_crypto_init, NULL); > > /* test key parsing - openssl */ > > Modified: apr/apr/trunk/util-misc/apu_dso.c > URL: > http://svn.apache.org/viewvc/apr/apr/trunk/util-misc/apu_dso.c?rev=1833525&r1=1833524&r2=1833525&view=diff > ============================================================================== > --- apr/apr/trunk/util-misc/apu_dso.c (original) > +++ apr/apr/trunk/util-misc/apu_dso.c Thu Jun 14 16:34:49 2018 > @@ -106,6 +106,11 @@ apr_status_t apu_dso_init(apr_pool_t *po > return ret; > } > > +struct dso_entry { > + apr_dso_handle_t *handle; > + apr_dso_handle_sym_t *sym; > +}; > + > apr_status_t apu_dso_load(apr_dso_handle_t **dlhandleptr, > apr_dso_handle_sym_t *dsoptr, > const char *module, > @@ -118,11 +123,14 @@ apr_status_t apu_dso_load(apr_dso_handle > apr_array_header_t *paths; > apr_pool_t *global; > apr_status_t rv = APR_EDSOOPEN; > + struct dso_entry *entry; > char *eos = NULL; > int i; > > - *dsoptr = apr_hash_get(dsos, module, APR_HASH_KEY_STRING); > - if (*dsoptr) { > + entry = apr_hash_get(dsos, module, APR_HASH_KEY_STRING); > + if (entry) { > + *dlhandleptr = entry->handle; > + *dsoptr = entry->sym; > return APR_EINIT; > } > > @@ -199,7 +207,10 @@ apr_status_t apu_dso_load(apr_dso_handle > } > else { > module = apr_pstrdup(global, module); > - apr_hash_set(dsos, module, APR_HASH_KEY_STRING, *dsoptr); > + entry = apr_palloc(global, sizeof(*entry)); > + entry->handle = dlhandle; > + entry->sym = *dsoptr; > + apr_hash_set(dsos, module, APR_HASH_KEY_STRING, entry); > } > return rv; > } > > -- Eric Covener cove...@gmail.com
