Author: brane
Date: Mon Jun 16 04:40:36 2025
New Revision: 1926457
URL: http://svn.apache.org/viewvc?rev=1926457&view=rev
Log:
On the user-defined-authn branch: Add pool cleanup for registered schemes.
Add more logging for scheme registration and initialisation.
* auth/auth.h
(serf__authn_scheme_t::user_pool): New member. Needed for pool cleanup.
* auth/auth.c
(AUTHN_SCHEMES_SIZE): Reduce this constant by one, as in most we were
subtracting one from the value.
(serf__authn_scheme_t): Adjust so that the size doesn't change.
(init_authn_schemes_guard): Add a serf_config_t arguments, for logging.
(lock_authn_schemes, unlock_authn_schemes): Pass that argument on.
(cleanup_user_scheme): Pool cleanup function for user-defined schemes.
(serf_authn_register_scheme): Add logging and register the pool cleanup.
(serf__authn__unregister_scheme): Add logging and kill the pool cleanup.
(init_authn_schemes_guard): Add logging.
* test/test_auth.c
(test_authn_registered_pool_cleanup): Test pool cleanup of registered
auhtentication schemes.
(test_user_authentication): Disable this test for now, builders complain
because it deliberately does not pass.
Modified:
serf/branches/user-defined-authn/auth/auth.c
serf/branches/user-defined-authn/auth/auth.h
serf/branches/user-defined-authn/test/test_auth.c
Modified: serf/branches/user-defined-authn/auth/auth.c
URL:
http://svn.apache.org/viewvc/serf/branches/user-defined-authn/auth/auth.c?rev=1926457&r1=1926456&r2=1926457&view=diff
==============================================================================
--- serf/branches/user-defined-authn/auth/auth.c (original)
+++ serf/branches/user-defined-authn/auth/auth.c Mon Jun 16 04:40:36 2025
@@ -47,8 +47,8 @@
The size of the array is the number of bits in serf__authn_scheme_t::type,
plus one slot for the NULL sentinel.
*/
-#define AUTHN_SCHEMES_SIZE (sizeof(unsigned int) * CHAR_BIT + 1)
-static const serf__authn_scheme_t *serf_authn_schemes[AUTHN_SCHEMES_SIZE] = {
+#define AUTHN_SCHEMES_SIZE (sizeof(unsigned int) * CHAR_BIT)
+static const serf__authn_scheme_t *serf_authn_schemes[AUTHN_SCHEMES_SIZE + 1]
= {
#ifdef SERF_HAVE_SPNEGO
&serf__spnego_authn_scheme,
#ifdef WIN32
@@ -69,13 +69,13 @@ static const serf__authn_scheme_t *serf_
/* Guard access to serf_authn_schemes and related global data. */
static apr_thread_mutex_t *authn_schemes_guard;
static apr_pool_t *authn_schemes_guard_pool;
-static apr_status_t init_authn_schemes_guard();
+static apr_status_t init_authn_schemes_guard(serf_config_t *config);
#endif
static apr_status_t lock_authn_schemes(serf_config_t *config)
{
#if APR_HAS_THREADS
- apr_status_t status = init_authn_schemes_guard();
+ apr_status_t status = init_authn_schemes_guard(config);
if (status == APR_SUCCESS) {
status = apr_thread_mutex_lock(authn_schemes_guard);
if (status) {
@@ -94,7 +94,7 @@ static apr_status_t lock_authn_schemes(s
static apr_status_t unlock_authn_schemes(serf_config_t *config)
{
#if APR_HAS_THREADS
- apr_status_t status = init_authn_schemes_guard();
+ apr_status_t status = init_authn_schemes_guard(config);
if (status == APR_SUCCESS) {
status = apr_thread_mutex_unlock(authn_schemes_guard);
if (status) {
@@ -623,6 +623,42 @@ static unsigned int find_next_user_schem
return avail & -avail;
}
+/* Pool cleanup handler for user-defined schemes. */
+static apr_status_t cleanup_user_scheme(void* data)
+{
+ const serf__authn_scheme_t *const authn_scheme = data;
+ const serf__authn_scheme_t *slot = NULL;
+ int index;
+
+ apr_status_t lock_status = init_authn_schemes_guard(NULL);
+ if (!lock_status)
+ lock_status = lock_authn_schemes(NULL);
+ if (lock_status)
+ return lock_status;
+
+ /* Find the scheme in the table. */
+ for (index = 0; index < AUTHN_SCHEMES_SIZE; ++index) {
+ slot = serf_authn_schemes[index];
+ if (slot == NULL || slot == authn_scheme)
+ break;
+ }
+
+ if (slot != NULL) {
+ /* Remove the scheme type from the registered mask. */
+ user_authn_registered &= ~slot->type;
+
+ /* Remove the scheme from the table, moving the other
+ schemes back over its position. */
+ for (; slot != NULL && index < AUTHN_SCHEMES_SIZE; ++index)
+ serf_authn_schemes[index] = slot = serf_authn_schemes[index + 1];
+ }
+
+ lock_status = unlock_authn_schemes(NULL);
+ if (lock_status)
+ return lock_status;
+ return APR_SUCCESS;
+}
+
apr_status_t serf_authn_register_scheme(
serf_context_t *ctx, const char *name, void *baton, int flags,
serf_authn_init_conn_func_t init_conn,
@@ -640,6 +676,9 @@ apr_status_t serf_authn_register_scheme(
char *cp;
int index;
+ serf__log(LOGLVL_INFO, LOGCOMP_AUTHN, __FILE__, ctx->config,
+ "Registering user-defined scheme: %s\n", name);
+
*type = SERF_AUTHN_NONE;
authn_scheme = apr_palloc(result_pool, sizeof(*authn_scheme));
@@ -659,8 +698,9 @@ apr_status_t serf_authn_register_scheme(
/* User-defined scheme data. */
authn_scheme->user_magic = serf__authn_user__magic;
- authn_scheme->user_baton = baton;
+ authn_scheme->user_pool = result_pool;
authn_scheme->user_flags = flags;
+ authn_scheme->user_baton = baton;
authn_scheme->user_init_conn_func = init_conn;
authn_scheme->user_handle_func = handle;
authn_scheme->user_setup_request_func = setup_request;
@@ -680,7 +720,7 @@ apr_status_t serf_authn_register_scheme(
/* Scan the array for a free slot and also check that this
scheme type hasn't been used yet. */
- for (index = 0; index < AUTHN_SCHEMES_SIZE - 1; ++index)
+ for (index = 0; index < AUTHN_SCHEMES_SIZE; ++index)
{
const serf__authn_scheme_t *const slot = serf_authn_schemes[index];
if (slot == NULL)
@@ -692,7 +732,7 @@ apr_status_t serf_authn_register_scheme(
goto cleanup;
}
}
- if (index >= AUTHN_SCHEMES_SIZE - 1) {
+ if (index >= AUTHN_SCHEMES_SIZE) {
/* No more space in the table. Not very likely. */
status = APR_ENOSPC;
goto cleanup;
@@ -702,6 +742,8 @@ apr_status_t serf_authn_register_scheme(
authn_scheme->type = scheme_type;
serf_authn_schemes[index] = authn_scheme;
serf_authn_schemes[index + 1] = NULL;
+ apr_pool_cleanup_register(authn_scheme->user_pool, authn_scheme,
+ cleanup_user_scheme, apr_pool_cleanup_null);
*type = scheme_type;
/* Add the scheme type to the registered mask. */
@@ -723,6 +765,7 @@ apr_status_t serf__authn__unregister_sch
const char *name,
apr_pool_t *scratch_pool)
{
+ const serf__authn_scheme_t *authn_scheme = NULL;
const unsigned int scheme_type = type;
apr_status_t lock_status;
apr_status_t status;
@@ -730,6 +773,9 @@ apr_status_t serf__authn__unregister_sch
char *cp;
int index;
+ serf__log(LOGLVL_INFO, LOGCOMP_AUTHN, __FILE__, ctx->config,
+ "Unregistering user-defined scheme: %s\n", name);
+
/* Generate a lower-case key for the scheme. */
key = cp = apr_pstrdup(scratch_pool, name);
while (*cp) {
@@ -744,18 +790,19 @@ apr_status_t serf__authn__unregister_sch
status = APR_SUCCESS;
/* Look for the scheme in the table. */
- for (index = 0; index < AUTHN_SCHEMES_SIZE - 1; ++index)
+ for (index = 0; index < AUTHN_SCHEMES_SIZE; ++index)
{
- const serf__authn_scheme_t *const slot = serf_authn_schemes[index];
- if (slot == NULL) {
+ authn_scheme = serf_authn_schemes[index];
+ if (authn_scheme == NULL) {
status = APR_ENOENT;
goto cleanup;
}
- if (slot->type == scheme_type && 0 == strcmp(slot->key, key))
+ if (authn_scheme->type == scheme_type
+ && 0 == strcmp(authn_scheme->key, key))
break;
}
- if (index >= AUTHN_SCHEMES_SIZE - 1) {
+ if (index >= AUTHN_SCHEMES_SIZE) {
/* The scheme wasn't registered */
status = APR_ENOENT;
goto cleanup;
@@ -764,13 +811,16 @@ apr_status_t serf__authn__unregister_sch
/* Move all the following schemes back. This is a memmove, but
it doesn't make much sense to use that since we don't knkow
how many schemes are left after this one. */
- for (; index < AUTHN_SCHEMES_SIZE - 1; ++index)
+ for (; index < AUTHN_SCHEMES_SIZE; ++index)
{
serf_authn_schemes[index] = serf_authn_schemes[index + 1];
if (serf_authn_schemes[index] == NULL)
break;
}
+ apr_pool_cleanup_kill(authn_scheme->user_pool, authn_scheme,
+ cleanup_user_scheme);
+
/* Remove the scheme type from the registered mask. */
user_authn_registered &= ~scheme_type;
@@ -789,7 +839,7 @@ apr_status_t serf__authn__unregister_sch
will be allocated ...
... yuck. */
-static apr_status_t init_authn_schemes_guard()
+static apr_status_t init_authn_schemes_guard(serf_config_t *config)
{
static volatile apr_uint32_t global_state = 0; /* uninitialized */
static const apr_uint32_t uninitialized = 0;
@@ -832,6 +882,9 @@ static apr_status_t init_authn_schemes_g
return APR_EGENERAL; /* FIXME: Just abort()? */
}
+ serf__log(LOGLVL_DEBUG, LOGCOMP_AUTHN, __FILE__, config,
+ "Initializing authn schemes mutex");
+
/* Create a self-contained root pool for the mutex. */
status = apr_allocator_create(&allocator);
if (status || !allocator)
Modified: serf/branches/user-defined-authn/auth/auth.h
URL:
http://svn.apache.org/viewvc/serf/branches/user-defined-authn/auth/auth.h?rev=1926457&r1=1926456&r2=1926457&view=diff
==============================================================================
--- serf/branches/user-defined-authn/auth/auth.h (original)
+++ serf/branches/user-defined-authn/auth/auth.h Mon Jun 16 04:40:36 2025
@@ -116,6 +116,10 @@ struct serf__authn_scheme_t {
/* The magic number that helps verify the user-defined scheme data. */
apr_uint64_t user_magic;
+ /* The pool that this scheme was allocated from; NULL for static objects.
+ This pull is used for pool cleanup handling. */
+ apr_pool_t *user_pool;
+
/* The flags for this authentication scheme */
int user_flags;
Modified: serf/branches/user-defined-authn/test/test_auth.c
URL:
http://svn.apache.org/viewvc/serf/branches/user-defined-authn/test/test_auth.c?rev=1926457&r1=1926456&r2=1926457&view=diff
==============================================================================
--- serf/branches/user-defined-authn/test/test_auth.c (original)
+++ serf/branches/user-defined-authn/test/test_auth.c Mon Jun 16 04:40:36 2025
@@ -625,6 +625,38 @@ static void test_authn_unregister_unknow
CuAssertIntEquals(tc, APR_ENOENT, status);
}
+static void test_authn_registered_pool_cleanup(CuTest *tc)
+{
+ test_baton_t *tb = tc->testBaton;
+ void *const baton = (void *)0xdeadbeef;
+ apr_pool_t *scheme_pool;
+ apr_status_t status;
+ int type;
+
+ status = setup_test_context(tb, tb->pool);
+ CuAssertIntEquals(tc, APR_SUCCESS, status);
+
+ /* Create a pool for the new scheme. */
+ apr_pool_create(&scheme_pool, tb->pool);
+ CuAssertTrue(tc, scheme_pool != NULL);
+
+ /* Register an authentication scheme */
+ status = serf_authn_register_scheme(tb->context, "Killed", baton,
+ SERF_AUTHN_FLAG_NONE,
+ NULL, NULL, NULL, NULL,
+ scheme_pool, &type);
+ CuAssertIntEquals(tc, APR_SUCCESS, status);
+ CuAssertTrue(tc, type != SERF_AUTHN_NONE);
+
+ /* Destroy the pool. Its cleanup function should unregister the scheme. */
+ apr_pool_destroy(scheme_pool);
+
+
+ /* Try to unregister the scheme; this should fail. */
+ status = serf_authn_unregister_scheme(tb->context,
+ type, "Killed", tb->pool);
+ CuAssertIntEquals(tc, APR_ENOENT, status);
+}
typedef struct user_authn_baton user_authn_t;
struct user_authn_baton {
@@ -852,7 +884,8 @@ CuSuite *test_auth(void)
SUITE_ADD_TEST(suite, test_authn_register_two);
SUITE_ADD_TEST(suite, test_authn_register_twice);
SUITE_ADD_TEST(suite, test_authn_unregister_unknown);
- SUITE_ADD_TEST(suite, test_user_authentication);
+ SUITE_ADD_TEST(suite, test_authn_registered_pool_cleanup);
+ /* SUITE_ADD_TEST(suite, test_user_authentication); */
/* SUITE_ADD_TEST(suite, test_user_authentication_keepalive_off); */
return suite;
