diff --git a/ChangeLog b/ChangeLog index 35510c39..8aef2eee 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,6 +1,7 @@ 09/20/2023 - performance: skip re-validating cached provider metadata - performance: use process based locking instead of global locking for Redis caching +- avoid potential process lifetime memory leak when mutex lock/unlock fails 09/19/2023 - fix performance issue with latin1 encoding when using OIDCPassClaimsAs latin1 diff --git a/src/cache/cache.h b/src/cache/cache.h index 9c34f676..349d7950 100644 --- a/src/cache/cache.h +++ b/src/cache/cache.h @@ -83,8 +83,8 @@ apr_byte_t oidc_cache_mutex_post_config(server_rec *s, oidc_cache_mutex_t *m, const char *type); apr_status_t oidc_cache_mutex_child_init(apr_pool_t *p, server_rec *s, oidc_cache_mutex_t *m); -apr_byte_t oidc_cache_mutex_lock(server_rec *s, oidc_cache_mutex_t *m); -apr_byte_t oidc_cache_mutex_unlock(server_rec *s, oidc_cache_mutex_t *m); +apr_byte_t oidc_cache_mutex_lock(apr_pool_t *pool, server_rec *s, oidc_cache_mutex_t *m); +apr_byte_t oidc_cache_mutex_unlock(apr_pool_t *pool, server_rec *s, oidc_cache_mutex_t *m); apr_byte_t oidc_cache_mutex_destroy(server_rec *s, oidc_cache_mutex_t *m); apr_byte_t oidc_cache_get(request_rec *r, const char *section, const char *key, diff --git a/src/cache/common.c b/src/cache/common.c index 453c96b7..5276b8b7 100644 --- a/src/cache/common.c +++ b/src/cache/common.c @@ -175,7 +175,8 @@ apr_status_t oidc_cache_mutex_child_init(apr_pool_t *p, server_rec *s, /* * mutex lock */ -apr_byte_t oidc_cache_mutex_lock(server_rec *s, oidc_cache_mutex_t *m) { +apr_byte_t oidc_cache_mutex_lock(apr_pool_t *pool, server_rec *s, + oidc_cache_mutex_t *m) { apr_status_t rv; @@ -187,7 +188,7 @@ apr_byte_t oidc_cache_mutex_lock(server_rec *s, oidc_cache_mutex_t *m) { if (rv != APR_SUCCESS) oidc_serror(s, "apr_global_mutex_lock/apr_proc_mutex_lock failed: %s (%d)", - oidc_cache_status2str(s->process->pool, rv), rv); + oidc_cache_status2str(pool, rv), rv); return TRUE; } @@ -195,7 +196,8 @@ apr_byte_t oidc_cache_mutex_lock(server_rec *s, oidc_cache_mutex_t *m) { /* * mutex unlock */ -apr_byte_t oidc_cache_mutex_unlock(server_rec *s, oidc_cache_mutex_t *m) { +apr_byte_t oidc_cache_mutex_unlock(apr_pool_t *pool, server_rec *s, + oidc_cache_mutex_t *m) { apr_status_t rv; @@ -207,7 +209,7 @@ apr_byte_t oidc_cache_mutex_unlock(server_rec *s, oidc_cache_mutex_t *m) { if (rv != APR_SUCCESS) oidc_serror(s, "apr_global_mutex_unlock/apr_proc_mutex_unlock failed: %s (%d)", - oidc_cache_status2str(s->process->pool, rv), rv); + oidc_cache_status2str(pool, rv), rv); return TRUE; } diff --git a/src/cache/redis.c b/src/cache/redis.c index 611f5859..d9bacb62 100644 --- a/src/cache/redis.c +++ b/src/cache/redis.c @@ -324,8 +324,8 @@ apr_byte_t oidc_cache_redis_get(request_rec *r, const char *section, redisReply *reply = NULL; apr_byte_t rv = FALSE; - /* grab the global lock */ - if (oidc_cache_mutex_lock(r->server, context->mutex) == FALSE) + /* grab the processlock */ + if (oidc_cache_mutex_lock(r->pool, r->server, context->mutex) == FALSE) return FALSE; /* get */ @@ -366,8 +366,8 @@ apr_byte_t oidc_cache_redis_get(request_rec *r, const char *section, /* free the reply object resources */ oidc_cache_redis_reply_free(&reply); - /* unlock the global mutex */ - oidc_cache_mutex_unlock(r->server, context->mutex); + /* unlock the process mutex */ + oidc_cache_mutex_unlock(r->pool, r->server, context->mutex); /* return the status */ return rv; @@ -385,8 +385,8 @@ apr_byte_t oidc_cache_redis_set(request_rec *r, const char *section, const char apr_byte_t rv = FALSE; apr_uint32_t timeout; - /* grab the global lock */ - if (oidc_cache_mutex_lock(r->server, context->mutex) == FALSE) + /* grab the process lock */ + if (oidc_cache_mutex_lock(r->pool, r->server, context->mutex) == FALSE) return FALSE; /* see if we should be clearing this entry */ @@ -412,8 +412,8 @@ apr_byte_t oidc_cache_redis_set(request_rec *r, const char *section, const char /* free the reply object resources */ oidc_cache_redis_reply_free(&reply); - /* unlock the global mutex */ - oidc_cache_mutex_unlock(r->server, context->mutex); + /* unlock the process mutex */ + oidc_cache_mutex_unlock(r->pool, r->server, context->mutex); /* return the status */ return rv; @@ -424,9 +424,9 @@ static int oidc_cache_redis_destroy_impl(server_rec *s) { oidc_cache_cfg_redis_t *context = (oidc_cache_cfg_redis_t*) cfg->cache_cfg; if (context != NULL) { - oidc_cache_mutex_lock(s, context->mutex); + oidc_cache_mutex_lock(s->process->pool, s, context->mutex); context->disconnect(context); - oidc_cache_mutex_unlock(s, context->mutex); + oidc_cache_mutex_unlock(s->process->pool, s, context->mutex); oidc_cache_mutex_destroy(s, context->mutex); cfg->cache_cfg = NULL; } diff --git a/src/cache/shm.c b/src/cache/shm.c index be5c12bf..2f9b1cc1 100644 --- a/src/cache/shm.c +++ b/src/cache/shm.c @@ -179,7 +179,7 @@ static apr_byte_t oidc_cache_shm_get(request_rec *r, const char *section, *value = NULL; /* grab the global lock */ - if (oidc_cache_mutex_lock(r->server, context->mutex) == FALSE) + if (oidc_cache_mutex_lock(r->pool, r->server, context->mutex) == FALSE) return FALSE; /* get the pointer to the start of the shared memory block */ @@ -213,7 +213,7 @@ static apr_byte_t oidc_cache_shm_get(request_rec *r, const char *section, } /* release the global lock */ - oidc_cache_mutex_unlock(r->server, context->mutex); + oidc_cache_mutex_unlock(r->pool, r->server, context->mutex); return TRUE; } @@ -252,7 +252,7 @@ static apr_byte_t oidc_cache_shm_set(request_rec *r, const char *section, } /* grab the global lock */ - if (oidc_cache_mutex_lock(r->server, context->mutex) == FALSE) + if (oidc_cache_mutex_lock(r->pool, r->server, context->mutex) == FALSE) return FALSE; /* get a pointer to the shared memory block */ @@ -325,7 +325,7 @@ static apr_byte_t oidc_cache_shm_set(request_rec *r, const char *section, } /* release the global lock */ - oidc_cache_mutex_unlock(r->server, context->mutex); + oidc_cache_mutex_unlock(r->pool, r->server, context->mutex); return TRUE; } @@ -339,11 +339,11 @@ static int oidc_cache_shm_destroy(server_rec *s) { oidc_slog(s, APLOG_TRACE1, "destroy: %pp (shm=%pp,s=%pp, p=%d)", context, context ? context->shm : 0, s, context ? context->is_parent : -1); if ((context) && (context->is_parent == TRUE) && (context->shm) && (context->mutex)) { - oidc_cache_mutex_lock(s, context->mutex); + oidc_cache_mutex_lock(s->process->pool, s, context->mutex); rv = apr_shm_destroy(context->shm); oidc_sdebug(s, "apr_shm_destroy returned: %d", rv); context->shm = NULL; - oidc_cache_mutex_unlock(s, context->mutex); + oidc_cache_mutex_unlock(s->process->pool, s, context->mutex); } if ((context) && (context->mutex)) {