You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by Eric Covener <co...@gmail.com> on 2019/08/23 12:34:56 UTC
Re: svn commit: r1833525 - in /apr/apr/trunk: crypto/apr_crypto.c
crypto/apr_crypto_internal.c test/testcrypto.c util-misc/apu_dso.c
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 <yl...@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
covener@gmail.com
Re: svn commit: r1833525 - in /apr/apr/trunk: crypto/apr_crypto.c
crypto/apr_crypto_internal.c test/testcrypto.c util-misc/apu_dso.c
Posted by Graham Leggett <mi...@sharp.fm>.
On 23 Aug 2019, at 14:34, Eric Covener <co...@gmail.com> wrote:
> 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?
Pool lifetime issues have been fixed in the apu_dso_init() function, most specifically you can now run the init, then clean up the pool, then run init a second time, then clean up the pool the second time, and this will work. Previously init could only ever be called once, but practically it was being called over and over again by every httpd module that touches DSO (sql, etc).
This fix isn’t enough - the same pool lifetime bug exists in apu_dso_load(), and each DSO’s init function. The fix that will work is implement the same strategy for pool cleanup implemented in apu_dso_init() everywhere else, but this means lots of repetition.
I have wanted to implement a generic solution to this, but have not had time to figure that out.
Regards,
Graham
—
Re: svn commit: r1833525 - in /apr/apr/trunk: crypto/apr_crypto.c
crypto/apr_crypto_internal.c test/testcrypto.c util-misc/apu_dso.c
Posted by Eric Covener <co...@gmail.com>.
my crypto adventure was short-lived and didn't ship (and now LRU'ed
from my brain).
I had this simpler change to our bundled APU 1.5.2 if it helps at all.
index 0f169b4f..958aa068 100644
--- srclib/apr-util/crypto/apr_crypto.c
+++ srclib/apr-util/crypto/apr_crypto.c
@@ -173,10 +173,11 @@ APU_DECLARE(apr_status_t) apr_crypto_get_driver(
return APR_SUCCESS;
}
-#if APU_DSO_BUILD
/* 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);
+#if APU_DSO_BUILD
#if defined(NETWARE)
apr_snprintf(modname, sizeof(modname), "crypto%s.nlm", name);
On Sun, Sep 12, 2021 at 12:56 PM Yann Ylavic <yl...@gmail.com> wrote:
>
> On Fri, Aug 23, 2019 at 2:35 PM Eric Covener <co...@gmail.com> wrote:
> >
> > 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?
>
> Was this resolved?
>
> It seems to me that in the !DSO case we should init the driver with
> the given pool and call init each time like in the DSO case.
> Something like the attached patch?
>
>
> Cheers;
> Yann.
--
Eric Covener
covener@gmail.com
Re: svn commit: r1833525 - in /apr/apr/trunk: crypto/apr_crypto.c
crypto/apr_crypto_internal.c test/testcrypto.c util-misc/apu_dso.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Fri, Aug 23, 2019 at 2:35 PM Eric Covener <co...@gmail.com> wrote:
>
> 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?
Was this resolved?
It seems to me that in the !DSO case we should init the driver with
the given pool and call init each time like in the DSO case.
Something like the attached patch?
Cheers;
Yann.