From 64bd28dea56bd2482be1343e1e64e0494e54b3c2 Mon Sep 17 00:00:00 2001 From: Arran Cudbard-Bell Date: Sun, 12 May 2024 10:49:43 -0600 Subject: [PATCH] Add a manual unregister function to the map proc code - Constify instance data - Take a separate ctx for map proc allocation - Link map proc memory using a linking ctx instead of allocating it directly --- src/bin/unit_test_module.c | 10 ++-- src/lib/server/map_proc.c | 79 +++++++++++++++++++---------- src/lib/server/map_proc.h | 9 ++-- src/lib/server/map_proc_priv.h | 2 +- src/modules/rlm_client/rlm_client.c | 5 +- src/modules/rlm_csv/rlm_csv.c | 10 ++-- 6 files changed, 71 insertions(+), 44 deletions(-) diff --git a/src/bin/unit_test_module.c b/src/bin/unit_test_module.c index 83c15f9be0..4a5e1e85d3 100644 --- a/src/bin/unit_test_module.c +++ b/src/bin/unit_test_module.c @@ -47,8 +47,6 @@ RCSID("$Id$") # include #endif -#include - #define EXIT_WITH_FAILURE \ do { \ ret = EXIT_FAILURE; \ @@ -586,7 +584,7 @@ static bool do_xlats(fr_event_list_t *el, request_t *request, char const *filena /* * Verify the result of the map. */ -static int map_proc_verify(CONF_SECTION *cs, UNUSED void *mod_inst, UNUSED void *proc_inst, +static int map_proc_verify(CONF_SECTION *cs, UNUSED void const *mod_inst, UNUSED void *proc_inst, tmpl_t const *src, UNUSED map_list_t const *maps) { if (!src) { @@ -598,7 +596,7 @@ static int map_proc_verify(CONF_SECTION *cs, UNUSED void *mod_inst, UNUSED void return 0; } -static unlang_action_t mod_map_proc(rlm_rcode_t *p_result, UNUSED void *mod_inst, UNUSED void *proc_inst, +static unlang_action_t mod_map_proc(rlm_rcode_t *p_result, UNUSED void const *mod_inst, UNUSED void *proc_inst, UNUSED request_t *request, UNUSED fr_value_box_list_t *src, UNUSED map_list_t const *maps) { @@ -882,7 +880,7 @@ int main(int argc, char *argv[]) EXIT_WITH_FAILURE; } - if (map_proc_register(NULL, "test-fail", mod_map_proc, map_proc_verify, 0, 0) < 0) { + if (map_proc_register(NULL, NULL, "test-fail", mod_map_proc, map_proc_verify, 0, 0) < 0) { EXIT_WITH_FAILURE; } @@ -1172,6 +1170,8 @@ cleanup: } #endif + map_proc_unregister("test-fail"); + /* * Free thread data */ diff --git a/src/lib/server/map_proc.c b/src/lib/server/map_proc.c index 3452d679c0..e58c8c7545 100644 --- a/src/lib/server/map_proc.c +++ b/src/lib/server/map_proc.c @@ -24,13 +24,15 @@ * @copyright 2015 Arran Cudbard-bell (a.cudbardb@freeradius.org) */ - RCSID("$Id$") #include +#include #include +#include #include #include +#include static fr_rb_tree_t *map_proc_root = NULL; @@ -70,11 +72,6 @@ static int _map_proc_talloc_free(map_proc_t *proc) return 0; } -static void _map_proc_tree_free(void *proc) -{ - talloc_free(proc); -} - /** Find a map processor by name * * @param[in] name of map processor. @@ -94,10 +91,28 @@ map_proc_t *map_proc_find(char const *name) return fr_rb_find(map_proc_root, &find); } +static int _map_proc_tree_init(UNUSED void *uctx) +{ + MEM(map_proc_root = fr_rb_inline_talloc_alloc(NULL, map_proc_t, node, map_proc_cmp, NULL)); + return 0; +} + +static int _map_proc_tree_free(UNUSED void *uctx) +{ + fr_rb_tree_t *mpr = map_proc_root; + + fr_assert_msg(fr_rb_num_elements(mpr) == 0, "map_proc_t still registered"); + + map_proc_root = NULL; + talloc_free(mpr); + return 0; +} + /** Register a map processor * * This should be called by every module that provides a map processing function. * + * @param[in] ctx if non-null, the ctx to bind this map processor to. * @param[in] mod_inst of module registering the map_proc. * @param[in] name of map processor. If processor already exists, it is replaced. * @param[in] evaluate Module's map processor function. @@ -108,7 +123,7 @@ map_proc_t *map_proc_find(char const *name) * - 0 on success. * - -1 on failure. */ -int map_proc_register(void *mod_inst, char const *name, +int map_proc_register(TALLOC_CTX *ctx, void const *mod_inst, char const *name, map_proc_func_t evaluate, map_proc_instantiate_t instantiate, size_t inst_size, fr_value_box_safe_for_t literals_safe_for) { @@ -116,21 +131,21 @@ int map_proc_register(void *mod_inst, char const *name, fr_assert(name && name[0]); - if (!map_proc_root) { - map_proc_root = fr_rb_inline_talloc_alloc(NULL, map_proc_t, node, - map_proc_cmp, _map_proc_tree_free); - if (!map_proc_root) { - DEBUG("map_proc: Failed to create tree"); - return -1; - } - } + fr_atexit_global_once(_map_proc_tree_init, _map_proc_tree_free, NULL); /* * If it already exists, replace it. */ proc = map_proc_find(name); if (!proc) { - proc = talloc_zero(mod_inst, map_proc_t); + /* + * Don't allocate directly in the parent ctx, it might be mprotected + * later, and that'll cause segfaults if any of the map_proc_t are still + * protected when we start shuffling the contents of the rbtree. + */ + proc = talloc_zero(NULL, map_proc_t); + if (ctx) talloc_link_ctx(ctx, proc); + strlcpy(proc->name, name, sizeof(proc->name)); proc->length = strlen(proc->name); @@ -153,6 +168,27 @@ int map_proc_register(void *mod_inst, char const *name, return 0; } +/** Unregister a map processor by name + * + * @param[in] name of map processor to unregister. + * @return + * - 0 if map processor was found and unregistered. + * - -1 if map processor was not found. + */ +int map_proc_unregister(char const *name) +{ + map_proc_t *proc; + + proc = map_proc_find(name); + if (proc) { + talloc_free(proc); + return 0; + } + + return -1; +} + + /** Create a new map proc instance * * This should be called for every map {} section in the configuration. @@ -207,14 +243,3 @@ unlang_action_t map_proc(rlm_rcode_t *p_result, request_t *request, map_proc_ins { return inst->proc->evaluate(p_result, inst->proc->mod_inst, inst->data, request, result, inst->maps); } - -/** Free all map_processors unregistering them - * - */ -void map_proc_free(void) -{ - fr_rb_tree_t *mpr = map_proc_root; - - map_proc_root = NULL; - talloc_free(mpr); -} diff --git a/src/lib/server/map_proc.h b/src/lib/server/map_proc.h index c75c3d7203..ea9731fee1 100644 --- a/src/lib/server/map_proc.h +++ b/src/lib/server/map_proc.h @@ -60,7 +60,7 @@ extern "C" { * @param[in] maps Head of the list of maps to process. * @return one of UNLANG_ACTION_* */ -typedef unlang_action_t (*map_proc_func_t)(rlm_rcode_t *p_result, void *mod_inst, void *proc_inst, request_t *request, +typedef unlang_action_t (*map_proc_func_t)(rlm_rcode_t *p_result, void const *mod_inst, void *proc_inst, request_t *request, fr_value_box_list_t *result, map_list_t const *maps); /** Allocate new instance data for a map processor @@ -74,16 +74,17 @@ typedef unlang_action_t (*map_proc_func_t)(rlm_rcode_t *p_result, void *mod_inst * - 0 on success. * - -1 on failure. */ -typedef int (*map_proc_instantiate_t)(CONF_SECTION *cs, void *mod_inst, void *proc_inst, +typedef int (*map_proc_instantiate_t)(CONF_SECTION *cs, void const *mod_inst, void *proc_inst, tmpl_t const *src, map_list_t const *maps); map_proc_t *map_proc_find(char const *name); -void map_proc_free(void); -int map_proc_register(void *mod_inst, char const *name, +int map_proc_register(TALLOC_CTX *ctx, void const *mod_inst, char const *name, map_proc_func_t evaluate, map_proc_instantiate_t instantiate, size_t inst_size, fr_value_box_safe_for_t safe_for); +int map_proc_unregister(char const *name); + map_proc_inst_t *map_proc_instantiate(TALLOC_CTX *ctx, map_proc_t const *proc, CONF_SECTION *cs, tmpl_t const *src, map_list_t const *maps); diff --git a/src/lib/server/map_proc_priv.h b/src/lib/server/map_proc_priv.h index 4b333bfdc0..584571ca50 100644 --- a/src/lib/server/map_proc_priv.h +++ b/src/lib/server/map_proc_priv.h @@ -38,7 +38,7 @@ extern "C" { */ struct map_proc { fr_rb_node_t node; //!< Entry in the map processor tree. - void *mod_inst; //!< Module instance. + void const *mod_inst; //!< Module instance. char name[FR_MAX_STRING_LEN]; //!< Name of the map function. int length; //!< Length of name. diff --git a/src/modules/rlm_client/rlm_client.c b/src/modules/rlm_client/rlm_client.c index 58d92d13cd..6f91236ca1 100644 --- a/src/modules/rlm_client/rlm_client.c +++ b/src/modules/rlm_client/rlm_client.c @@ -116,7 +116,7 @@ static int _map_proc_client_get_vp(TALLOC_CTX *ctx, fr_pair_list_t *out, request * @param[in] maps Head of the map list. * @return UNLANG_ACTION_CALCULATE_RESULT */ -static unlang_action_t map_proc_client(rlm_rcode_t *p_result, UNUSED void *mod_inst, UNUSED void *proc_inst, +static unlang_action_t map_proc_client(rlm_rcode_t *p_result, UNUSED void const *mod_inst, UNUSED void *proc_inst, request_t *request, fr_value_box_list_t *client_override, map_list_t const *maps) { rlm_rcode_t rcode = RLM_MODULE_OK; @@ -357,7 +357,7 @@ static int mod_load(void) if (unlikely((xlat = xlat_func_register(NULL, "client", xlat_client, FR_TYPE_STRING)) == NULL)) return -1; xlat_func_args_set(xlat, xlat_client_args); - map_proc_register(NULL, "client", map_proc_client, NULL, 0, 0); + map_proc_register(NULL, NULL, "client", map_proc_client, NULL, 0, 0); return 0; } @@ -365,6 +365,7 @@ static int mod_load(void) static void mod_unload(void) { xlat_func_unregister("client"); + map_proc_unregister("client"); } /* diff --git a/src/modules/rlm_csv/rlm_csv.c b/src/modules/rlm_csv/rlm_csv.c index b3221eba1e..d849ad3d04 100644 --- a/src/modules/rlm_csv/rlm_csv.c +++ b/src/modules/rlm_csv/rlm_csv.c @@ -31,7 +31,7 @@ RCSID("$Id$") #include -static unlang_action_t mod_map_proc(rlm_rcode_t *p_result, void *mod_inst, UNUSED void *proc_inst, request_t *request, +static unlang_action_t mod_map_proc(rlm_rcode_t *p_result, void const *mod_inst, UNUSED void *proc_inst, request_t *request, fr_value_box_list_t *key, map_list_t const *maps); /* @@ -481,7 +481,7 @@ static int csv_map_verify(map_t *map, void *instance) /* * Verify the result of the map. */ -static int csv_maps_verify(CONF_SECTION *cs, void *mod_inst, UNUSED void *proc_inst, +static int csv_maps_verify(CONF_SECTION *cs, void const *mod_inst, UNUSED void *proc_inst, tmpl_t const *src, map_list_t const *maps) { map_t const *map = NULL; @@ -496,7 +496,7 @@ static int csv_maps_verify(CONF_SECTION *cs, void *mod_inst, UNUSED void *proc_i /* * This function doesn't change the map, so it's OK. */ - if (csv_map_verify(UNCONST(map_t *, map), mod_inst) < 0) return -1; + if (csv_map_verify(UNCONST(map_t *, map), UNCONST(void *, mod_inst)) < 0) return -1; } return 0; @@ -737,7 +737,7 @@ static int mod_bootstrap(module_inst_ctx_t const *mctx) /* * And register the `map csv { ... }` function. */ - map_proc_register(inst, mctx->mi->name, mod_map_proc, csv_maps_verify, 0, 0); + map_proc_register(inst, inst, mctx->mi->name, mod_map_proc, csv_maps_verify, 0, 0); return 0; } @@ -975,7 +975,7 @@ finish: * @param[in] maps Head of the map list. * @return UNLANG_ACTION_CALCULATE_RESULT */ -static unlang_action_t mod_map_proc(rlm_rcode_t *p_result, void *mod_inst, UNUSED void *proc_inst, request_t *request, +static unlang_action_t mod_map_proc(rlm_rcode_t *p_result, void const *mod_inst, UNUSED void *proc_inst, request_t *request, fr_value_box_list_t *key, map_list_t const *maps) { rlm_csv_t *inst = talloc_get_type_abort(mod_inst, rlm_csv_t); -- 2.47.3