avoid potential processlifetime memory leak upon cache lock/unlock error

Signed-off-by: Hans Zandbelt <[email protected]>

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 <any> latin1

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,

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,15 +188,16 @@ 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;
 }
 
 /*
  * 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;
 }

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;
 	}

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)) {