use apr_pool_cleanup_register to clean up oidc_cfg_provider_t

rather than calling oidc_cfg_provider_destroy everywhere explicitly

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

Author: zandbelt

ChangeLog

@@ -1,6 +1,6 @@
 04/15/2025
 - fix memory leaks when using provider specific client keys in a multi-provider setup
-- bump to 2.4.17rc0
+- bump to 2.4.17rc1
 
 04/08/2025
 - proto: pass the scope parameter as returned from the token endpoint in the OIDC_scope

src/cfg/provider.c

@@ -862,9 +862,16 @@ void oidc_cfg_provider_merge(apr_pool_t *pool, oidc_provider_t *dst, const oidc_
 	dst->profile = add->profile != OIDC_CONFIG_POS_INT_UNSET ? add->profile : base->profile;
 }
 
+static apr_status_t oidc_provider_config_cleanup(void *data) {
+	oidc_provider_t *provider = (oidc_provider_t *)data;
+	oidc_cfg_provider_destroy(provider);
+	return APR_SUCCESS;
+}
+
 oidc_provider_t *oidc_cfg_provider_create(apr_pool_t *pool) {
 	oidc_provider_t *provider = apr_pcalloc(pool, sizeof(oidc_provider_t));
 	oidc_cfg_provider_init(provider);
+	apr_pool_cleanup_register(pool, provider, oidc_provider_config_cleanup, oidc_provider_config_cleanup);
 	return provider;
 }
 

src/handle/discovery.c

@@ -367,7 +367,6 @@ int oidc_discovery_response(request_rec *r, oidc_cfg_t *c) {
 	if (oidc_cfg_metadata_dir_get(c) == NULL) {
 		if ((oidc_provider_static_config(r, c, &provider) == TRUE) && (issuer != NULL)) {
 			if (_oidc_strcmp(oidc_cfg_provider_issuer_get(provider), issuer) != 0) {
-				oidc_cfg_provider_destroy(provider);
 				return oidc_util_html_send_error(
 				    r, "Invalid Request",
 				    apr_psprintf(
@@ -377,7 +376,6 @@ int oidc_discovery_response(request_rec *r, oidc_cfg_t *c) {
 				    HTTP_INTERNAL_SERVER_ERROR);
 			}
 		}
-		oidc_cfg_provider_destroy(provider);
 		return oidc_request_authenticate_user(r, c, NULL, target_link_uri, login_hint, NULL, NULL,
 						      auth_request_params, path_scopes);
 	}
@@ -448,19 +446,14 @@ int oidc_discovery_response(request_rec *r, oidc_cfg_t *c) {
 					       oidc_cfg_provider_ssl_validate_server_get(provider), &j_jwks,
 					       &force_refresh);
 			json_decref(j_jwks);
-			oidc_cfg_provider_destroy(provider);
 			return OK;
 		} else {
 			/* now we've got a selected OP, send the user there to authenticate */
-			int rv = oidc_request_authenticate_user(r, c, provider, target_link_uri, login_hint, NULL, NULL,
-								auth_request_params, path_scopes);
-			oidc_cfg_provider_destroy(provider);
-			return rv;
+			return oidc_request_authenticate_user(r, c, provider, target_link_uri, login_hint, NULL, NULL,
+							      auth_request_params, path_scopes);
 		}
 	}
 
-	oidc_cfg_provider_destroy(provider);
-
 	/* something went wrong */
 	return oidc_util_html_send_error(r, "Invalid Request",
 					 "Could not find valid provider metadata for the selected OpenID Connect "

src/handle/info.c

@@ -106,19 +106,15 @@ int oidc_info_request(request_rec *r, oidc_cfg_t *c, oidc_session_t *session, ap
 
 				/* get the current provider info */
 				oidc_provider_t *provider = NULL;
-				if (oidc_get_provider_from_session(r, c, session, &provider) == FALSE) {
-					oidc_cfg_provider_destroy(provider);
+				if (oidc_get_provider_from_session(r, c, session, &provider) == FALSE)
 					return HTTP_INTERNAL_SERVER_ERROR;
-				}
 
 				/* execute the actual refresh grant */
 				if (oidc_refresh_token_grant(r, c, session, provider, NULL, NULL, NULL) == FALSE) {
 					oidc_warn(r, "access_token could not be refreshed");
-					oidc_cfg_provider_destroy(provider);
 					return HTTP_INTERNAL_SERVER_ERROR;
 				}
 				needs_save = TRUE;
-				oidc_cfg_provider_destroy(provider);
 			}
 		}
 	}

src/handle/logout.c

@@ -119,7 +119,6 @@ static void oidc_logout_revoke_tokens(request_rec *r, oidc_cfg_t *c, oidc_sessio
 
 out:
 
-	oidc_cfg_provider_destroy(provider);
 	oidc_debug(r, "leave");
 }
 
@@ -234,7 +233,6 @@ int oidc_logout_request(request_rec *r, oidc_cfg_t *c, oidc_session_t *session,
 				}
 				if (provider) {
 					oidc_logout_cleanup_by_sid(r, sid, c, provider, revoke_tokens);
-					oidc_cfg_provider_destroy(provider);
 				} else {
 					oidc_info(r, "No provider for front channel logout found");
 				}
@@ -434,10 +432,6 @@ static int oidc_logout_backchannel(request_rec *r, oidc_cfg_t *cfg) {
 		oidc_jwt_destroy(jwt);
 		jwt = NULL;
 	}
-	if (provider != NULL) {
-		oidc_cfg_provider_destroy(provider);
-		provider = NULL;
-	}
 
 	oidc_http_hdr_err_out_add(r, OIDC_HTTP_HDR_CACHE_CONTROL, "no-cache, no-store");
 	oidc_http_hdr_err_out_add(r, OIDC_HTTP_HDR_PRAGMA, "no-cache");
@@ -524,7 +518,5 @@ int oidc_logout(request_rec *r, oidc_cfg_t *c, oidc_session_t *session) {
 		url = s_logout_request;
 	}
 
-	oidc_cfg_provider_destroy(provider);
-
 	return oidc_logout_request(r, c, session, url, TRUE);
 }

src/handle/refresh.c

@@ -389,8 +389,6 @@ int oidc_refresh_token_request(request_rec *r, oidc_cfg_t *c, oidc_session_t *se
 	/* add the redirect location header */
 	oidc_http_hdr_out_location_set(r, return_to);
 
-	oidc_cfg_provider_destroy(provider);
-
 	return HTTP_MOVED_TEMPORARILY;
 }
 
@@ -427,21 +425,16 @@ apr_byte_t oidc_refresh_access_token_before_expiry(request_rec *r, oidc_cfg_t *c
 	if (t_expires > apr_time_now())
 		return TRUE;
 
-	if (oidc_get_provider_from_session(r, cfg, session, &provider) == FALSE) {
-		oidc_cfg_provider_destroy(provider);
+	if (oidc_get_provider_from_session(r, cfg, session, &provider) == FALSE)
 		return FALSE;
-	}
 
 	if (oidc_refresh_token_grant(r, cfg, session, provider, NULL, NULL, NULL) == FALSE) {
 		oidc_warn(r, "access_token could not be refreshed");
-		oidc_cfg_provider_destroy(provider);
 		*needs_save = FALSE;
 		return FALSE;
 	}
 
 	*needs_save = TRUE;
 
-	oidc_cfg_provider_destroy(provider);
-
 	return TRUE;
 }

src/handle/response.c

@@ -565,7 +565,6 @@ static int oidc_response_process(request_rec *r, oidc_cfg_t *c, oidc_session_t *
 			oidc_http_hdr_out_location_set(r,
 						       oidc_util_absolute_url(r, c, oidc_cfg_default_sso_url_get(c)));
 			OIDC_METRICS_COUNTER_INC(r, c, OM_AUTHN_RESPONSE_ERROR_STATE_MISMATCH);
-			oidc_cfg_provider_destroy(provider);
 			return HTTP_MOVED_TEMPORARILY;
 		}
 		oidc_error(r,
@@ -578,7 +577,7 @@ static int oidc_response_process(request_rec *r, oidc_cfg_t *c, oidc_session_t *
 		}
 
 		OIDC_METRICS_COUNTER_INC(r, c, OM_AUTHN_RESPONSE_ERROR_STATE_MISMATCH);
-		oidc_cfg_provider_destroy(provider);
+
 		return oidc_util_html_send_error(r, "Invalid Authorization Response",
 						 "Could not match the authorization response to an earlier request via "
 						 "the state parameter and corresponding state cookie",
@@ -588,21 +587,18 @@ static int oidc_response_process(request_rec *r, oidc_cfg_t *c, oidc_session_t *
 	/* see if the response is an error response */
 	if (apr_table_get(params, OIDC_PROTO_ERROR) != NULL) {
 		OIDC_METRICS_COUNTER_INC(r, c, OM_AUTHN_RESPONSE_ERROR_PROVIDER);
-		oidc_cfg_provider_destroy(provider);
 		return oidc_response_authorization_error(r, c, proto_state, apr_table_get(params, OIDC_PROTO_ERROR),
 							 apr_table_get(params, OIDC_PROTO_ERROR_DESCRIPTION));
 	}
 
 	/* handle the code, implicit or hybrid flow */
 	if (oidc_response_flows(r, c, proto_state, provider, params, response_mode, &jwt) == FALSE) {
 		OIDC_METRICS_COUNTER_INC(r, c, OM_AUTHN_RESPONSE_ERROR_PROTOCOL);
-		oidc_cfg_provider_destroy(provider);
 		return oidc_response_authorization_error(r, c, proto_state, "Error in handling response type.", NULL);
 	}
 
 	if (jwt == NULL) {
 		oidc_error(r, "no id_token was provided");
-		oidc_cfg_provider_destroy(provider);
 		return oidc_response_authorization_error(r, c, proto_state, "No id_token was provided.", NULL);
 	}
 
@@ -638,7 +634,6 @@ static int oidc_response_process(request_rec *r, oidc_cfg_t *c, oidc_session_t *
 			if (_oidc_strcmp(session->remote_user, r->user) != 0) {
 				oidc_warn(r, "user set from new id_token is different from current one");
 				oidc_jwt_destroy(jwt);
-				oidc_cfg_provider_destroy(provider);
 				return oidc_response_authorization_error(r, c, proto_state, "User changed!", NULL);
 			}
 		}
@@ -652,7 +647,6 @@ static int oidc_response_process(request_rec *r, oidc_cfg_t *c, oidc_session_t *
 			apr_table_get(params, OIDC_PROTO_STATE), original_url, userinfo_jwt) == FALSE) {
 			oidc_proto_state_destroy(proto_state);
 			oidc_jwt_destroy(jwt);
-			oidc_cfg_provider_destroy(provider);
 			return HTTP_INTERNAL_SERVER_ERROR;
 		}
 
@@ -662,15 +656,13 @@ static int oidc_response_process(request_rec *r, oidc_cfg_t *c, oidc_session_t *
 		oidc_error(r, "remote user could not be set");
 		oidc_jwt_destroy(jwt);
 		OIDC_METRICS_COUNTER_INC(r, c, OM_AUTHN_RESPONSE_ERROR_REMOTE_USER);
-		oidc_cfg_provider_destroy(provider);
 		return oidc_response_authorization_error(
 		    r, c, proto_state, "Remote user could not be set: contact the website administrator", NULL);
 	}
 
 	/* cleanup */
 	oidc_proto_state_destroy(proto_state);
 	oidc_jwt_destroy(jwt);
-	oidc_cfg_provider_destroy(provider);
 
 	/* check that we've actually authenticated a user; functions as error handling for oidc_get_remote_user */
 	if (r->user == NULL) {

src/handle/session_management.c

@@ -173,27 +173,23 @@ int oidc_session_management(request_rec *r, oidc_cfg_t *c, oidc_session_t *sessi
 	/* see if this is a request for the OP iframe */
 	if (_oidc_strcmp("iframe_op", cmd) == 0) {
 		if (oidc_cfg_provider_check_session_iframe_get(provider) != NULL) {
-			oidc_cfg_provider_destroy(provider);
 			return oidc_session_management_iframe_op(r, c, session,
 								 oidc_cfg_provider_check_session_iframe_get(provider));
 		}
-		oidc_cfg_provider_destroy(provider);
 		return HTTP_NOT_FOUND;
 	}
 
 	/* see if this is a request for the RP iframe */
 	if (_oidc_strcmp("iframe_rp", cmd) == 0) {
 		if ((oidc_cfg_provider_client_id_get(provider) != NULL) &&
 		    (oidc_cfg_provider_check_session_iframe_get(provider) != NULL)) {
-			oidc_cfg_provider_destroy(provider);
 			return oidc_session_management_iframe_rp(r, c, session,
 								 oidc_cfg_provider_client_id_get(provider),
 								 oidc_cfg_provider_check_session_iframe_get(provider));
 		}
 		oidc_debug(r, "iframe_rp command issued but no client (%s) and/or no check_session_iframe (%s) set",
 			   oidc_cfg_provider_client_id_get(provider),
 			   oidc_cfg_provider_check_session_iframe_get(provider));
-		oidc_cfg_provider_destroy(provider);
 		return HTTP_NOT_FOUND;
 	}
 
@@ -206,7 +202,6 @@ int oidc_session_management(request_rec *r, oidc_cfg_t *c, oidc_session_t *sessi
 		 *       those for the redirect_uri itself; do we need to store those as part of the
 		 *       session now?
 		 */
-		oidc_cfg_provider_destroy(provider);
 		return oidc_request_authenticate_user(
 		    r, c, provider, apr_psprintf(r->pool, "%s?session=iframe_rp", oidc_util_redirect_uri(r, c)), NULL,
 		    id_token_hint, "none", oidc_cfg_dir_path_auth_request_params_get(r),
@@ -216,7 +211,5 @@ int oidc_session_management(request_rec *r, oidc_cfg_t *c, oidc_session_t *sessi
 	/* handle failure in fallthrough */
 	oidc_error(r, "unknown command: %s", cmd);
 
-	oidc_cfg_provider_destroy(provider);
-
 	return HTTP_INTERNAL_SERVER_ERROR;
 }

src/handle/userinfo.c

@@ -197,7 +197,6 @@ apr_byte_t oidc_userinfo_refresh_claims(request_rec *r, oidc_cfg_t *cfg, oidc_se
 		/* get the current provider info */
 		if (oidc_get_provider_from_session(r, cfg, session, &provider) == FALSE) {
 			*needs_save = TRUE;
-			oidc_cfg_provider_destroy(provider);
 			return FALSE;
 		}
 
@@ -239,8 +238,6 @@ apr_byte_t oidc_userinfo_refresh_claims(request_rec *r, oidc_cfg_t *cfg, oidc_se
 
 	oidc_debug(r, "return: %d", rc);
 
-	oidc_cfg_provider_destroy(provider);
-
 	return rc;
 }