From: Arran Cudbard-Bell Date: Sun, 27 Nov 2016 21:01:25 +0000 (-0500) Subject: Fix thread local destructors X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=4e39d22bb3dd69a65b131da383dad0818ab776c7;p=thirdparty%2Ffreeradius-server.git Fix thread local destructors _Thread_local void *foo; set_pthread_destructor(&foo); Doesn't work. Pointers to _Thread_local storage seem to become invalidated when threads exit. Instead we need to store the value of the local storage... Which involves moving some macros around, but isn't a huge issue. --- diff --git a/src/include/threads.h b/src/include/threads.h index 8076fe12da4..a5c4c5146e9 100644 --- a/src/include/threads.h +++ b/src/include/threads.h @@ -48,7 +48,7 @@ static inline int __fr_thread_local_destructor_##_n(pthread_destructor_t *ctx)\ func(_n);\ return 0;\ }\ -static inline _t __fr_thread_local_init_##_n(pthread_destructor_t func)\ +static inline _t __fr_thread_local_set_destructor_##_n(pthread_destructor_t func, void *value)\ {\ static pthread_destructor_t *ctx;\ if (!ctx) {\ @@ -56,15 +56,15 @@ static inline _t __fr_thread_local_init_##_n(pthread_destructor_t func)\ talloc_set_destructor(ctx, __fr_thread_local_destructor_##_n);\ *ctx = func;\ }\ + _n = value\ return _n;\ } - -# define fr_thread_local_init(_n, _f) __fr_thread_local_init_##_n(_f) -# define fr_thread_local_set(_n, _v) ((int)!((_n = _v) || 1)) -# define fr_thread_local_get(_n) _n +# define fr_thread_local_set_destructor(_n, _f, _v) __fr_thread_local_set_destructor_##_n(_f, _v) #else # include -/** Create a thread local variable with initializers/destructors +/** Pre-initialise resources required for a thread local destructor + * + * @note If destructors are not required, just use __Thread_local. * * @param _t Type of variable e.g. 'char *'. Must be a pointer type. * @param _n Name of variable e.g. 'my_tls'. @@ -73,43 +73,34 @@ static inline _t __fr_thread_local_init_##_n(pthread_destructor_t func)\ static pthread_key_t __fr_thread_local_key_##_n;\ static pthread_once_t __fr_thread_local_once_##_n = PTHREAD_ONCE_INIT;\ static pthread_destructor_t __fr_thread_local_destructor_##_n = NULL;\ -static void __fr_thread_local_destroy_##_n(UNUSED void *unused)\ +static void __fr_thread_local_destroy_##_n(void *value)\ {\ - __fr_thread_local_destructor_##_n(_n);\ + __fr_thread_local_destructor_##_n(value);\ }\ static void __fr_thread_local_key_init_##_n(void)\ {\ (void) pthread_key_create(&__fr_thread_local_key_##_n, __fr_thread_local_destroy_##_n);\ }\ -static _t __fr_thread_local_init_##_n(pthread_destructor_t func)\ +static _t __fr_thread_local_set_destructor_##_n(pthread_destructor_t func, void *value)\ {\ __fr_thread_local_destructor_##_n = func;\ if (_n) return _n; \ + if (!value) return _n; \ (void) pthread_once(&__fr_thread_local_once_##_n, __fr_thread_local_key_init_##_n);\ - (void) pthread_setspecific(__fr_thread_local_key_##_n, &(_n));\ + (void) pthread_setspecific(__fr_thread_local_key_##_n, value);\ + _n = value;\ return _n;\ } -/** If variable is NULL, call initialization function, else return the variable value - * - * @param _n Name of variable e.g. 'my_tls'. - * @param _f Destructor, called when the thread exits to clean up any data. - */ -# define fr_thread_local_init(_n, _f) __fr_thread_local_init_##_n(_f) - -/** Set a new variable value - * - * @param _n Name of variable e.g. 'my_tls'. - * @param _f Function to call for initialization. - */ -# define fr_thread_local_set(_n, _v) ((int)!((_n = _v) || 1)) - -/** Get an existing variable value +/** Set a destructor for thread local storage to free the memory on thread exit * - * @note In practice this is rarely used, and a call to fr_thread_local_init - * is used to retrieve the value of the variable. + * @note Pointers to thread local storage seem to become unusable as threads are + * destroyed. So we need to store the address of the memory to free, not + * the address of the thread local variable. * * @param _n Name of variable e.g. 'my_tls'. + * @param _f Destructor, called when the thread exits to clean up any data. + * @param _v Memory to free. */ -# define fr_thread_local_get(_n) _n +# define fr_thread_local_set_destructor(_n, _f, _v) __fr_thread_local_set_destructor_##_n(_f, _v) #endif #endif /* _FR_THREADS_H */ diff --git a/src/lib/log.c b/src/lib/log.c index 18a8313001c..dc38535ad0c 100644 --- a/src/lib/log.c +++ b/src/lib/log.c @@ -153,7 +153,8 @@ static char const *fr_errno_macro_names[] = { */ static void _fr_logging_free(void *arg) { - talloc_free(arg); + char *buff = talloc_get_type_abort(arg, char); + talloc_free(buff); } /** Log to thread local error buffer @@ -167,22 +168,14 @@ void fr_strerror_printf(char const *fmt, ...) char *buffer; - buffer = fr_thread_local_init(fr_strerror_buffer, _fr_logging_free); + buffer = fr_strerror_buffer; if (!buffer) { - int ret; - buffer = talloc_zero_array(NULL, char, (FR_STRERROR_BUFSIZE * 2) + 1); /* One byte extra for status */ if (!buffer) { fr_perror("Failed allocating memory for libradius error buffer"); return; } - - ret = fr_thread_local_set(fr_strerror_buffer, buffer); - if (ret != 0) { - fr_perror("Failed setting up TLS for libradius error buffer: %s", fr_syserror(ret)); - talloc_free(buffer); - return; - } + fr_thread_local_set_destructor(fr_strerror_buffer, _fr_logging_free, buffer); } /* @@ -222,7 +215,7 @@ char const *fr_strerror(void) { char *buffer; - buffer = fr_thread_local_get(fr_strerror_buffer); + buffer = fr_strerror_buffer; if (!buffer) return ""; switch (buffer[FR_STRERROR_BUFSIZE * 2]) { @@ -249,20 +242,14 @@ char const *fr_syserror(int num) char *buffer, *p, *end; int ret; - buffer = fr_thread_local_init(fr_syserror_buffer, _fr_logging_free); + buffer = fr_syserror_buffer; if (!buffer) { buffer = talloc_array(NULL, char, FR_STRERROR_BUFSIZE); if (!buffer) { fr_perror("Failed allocating memory for system error buffer"); return NULL; } - - ret = fr_thread_local_set(fr_syserror_buffer, buffer); - if (ret != 0) { - fr_perror("Failed setting up TLS for system error buffer: %s", fr_syserror(ret)); - talloc_free(buffer); - return NULL; - } + fr_thread_local_set_destructor(fr_syserror_buffer, _fr_logging_free, buffer); } if (!num) return "No error"; diff --git a/src/main/modules.c b/src/main/modules.c index 6ba142c617d..8978a172b8e 100644 --- a/src/main/modules.c +++ b/src/main/modules.c @@ -470,7 +470,7 @@ module_instance_t *module_find_with_method(rlm_components_t *method, CONF_SECTIO void *module_thread_instance_find(void *instance) { module_instance_t *inst = instance; - rbtree_t *tree = fr_thread_local_get(module_thread_inst_tree); + rbtree_t *tree = module_thread_inst_tree; module_thread_instance_t find, *found; if (!inst->module->thread_instantiate || !inst->module->thread_inst_size) return NULL; @@ -486,16 +486,19 @@ void *module_thread_instance_find(void *instance) /** Destructor for module_thread_instance_t * + * @note This cannot be converted to a talloc destructor, + * as we need to call thread_detach *before* any of the children + * of the talloc ctx are freed. */ -static int _module_thread_instance_free(void *thread_inst_data) +static void _module_thread_instance_free(void *to_free) { - module_thread_instance_t *thread_inst = talloc_parent(thread_inst_data); + module_thread_instance_t *thread_inst = talloc_get_type_abort(to_free, module_thread_instance_t); if (thread_inst->inst->module->thread_detach) { (void) thread_inst->inst->module->thread_detach(thread_inst->data); } - return 0; + talloc_free(thread_inst); } /** Frees the thread local instance free and any thread local instance data @@ -504,7 +507,8 @@ static int _module_thread_instance_free(void *thread_inst_data) */ static void _module_thread_inst_tree_free(void *to_free) { - rbtree_t *thread_inst_tree = talloc_get_type_abort(to_free, rbtree_t); + rbtree_t *thread_inst_tree = talloc_get_type_abort(to_free , rbtree_t); + rbtree_free(thread_inst_tree); } @@ -565,7 +569,6 @@ static int _module_thread_instantiate(void *instance, void *ctx) talloc_set_name(thread_inst->data, "%s", type_name); talloc_free(type_name); - talloc_set_destructor(thread_inst->data, _module_thread_instance_free); rbtree_insert(thread_inst_ctx->tree, thread_inst); } @@ -597,10 +600,12 @@ int modules_thread_instantiate(CONF_SECTION *root, fr_event_list_t *el) modules = cf_section_sub_find(root, "modules"); if (!modules) return 0; - thread_inst_tree = fr_thread_local_init(module_thread_inst_tree, _module_thread_inst_tree_free); + thread_inst_tree = module_thread_inst_tree; if (!thread_inst_tree) { - MEM(thread_inst_tree = module_thread_inst_tree = rbtree_create(NULL, _module_thread_inst_tree_cmp, - rbtree_node_talloc_free, 0)); + MEM(thread_inst_tree = rbtree_create(NULL, _module_thread_inst_tree_cmp, + _module_thread_instance_free, 0)); + fr_thread_local_set_destructor(module_thread_inst_tree, + _module_thread_inst_tree_free, thread_inst_tree); } ctx.el = el; diff --git a/src/modules/rlm_krb5/krb5.c b/src/modules/rlm_krb5/krb5.c index 5c362386dbd..a5d6f08a6e2 100644 --- a/src/modules/rlm_krb5/krb5.c +++ b/src/modules/rlm_krb5/krb5.c @@ -51,22 +51,15 @@ char const *rlm_krb5_error(rlm_krb5_t const *inst, krb5_context context, krb5_er if (!rad_cond_assert(inst)) return NULL; - buffer = fr_thread_local_init(krb5_error_buffer, _krb5_logging_free); + buffer = krb5_error_buffer; if (!buffer) { - int ret; - buffer = talloc_array(NULL, char, KRB5_STRERROR_BUFSIZE); if (!buffer) { ERROR("Failed allocating memory for krb5 error buffer"); return NULL; } - ret = fr_thread_local_set(krb5_error_buffer, buffer); - if (ret != 0) { - ERROR("Failed setting up TLS for krb5 error buffer: %s", fr_syserror(ret)); - talloc_free(buffer); - return NULL; - } + fr_thread_local_set_destructor(krb5_error_buffer, _krb5_logging_free, buffer); } msg = krb5_get_error_message(context, code); diff --git a/src/modules/rlm_python/rlm_python.c b/src/modules/rlm_python/rlm_python.c index a8d5a595854..1610ce54397 100644 --- a/src/modules/rlm_python/rlm_python.c +++ b/src/modules/rlm_python/rlm_python.c @@ -637,19 +637,14 @@ static rlm_rcode_t do_python(rlm_python_t const *inst, REQUEST *request, PyObjec * Check to see if we've got a thread state tree * If not, create one. */ - thread_tree = fr_thread_local_init(local_thread_state, _python_thread_tree_free); + thread_tree = local_thread_state; if (!thread_tree) { thread_tree = rbtree_create(NULL, _python_inst_cmp, _python_thread_entry_free, 0); if (!thread_tree) { RERROR("Failed allocating thread state tree"); return RLM_MODULE_FAIL; } - - ret = fr_thread_local_set(local_thread_state, thread_tree); - if (ret != 0) { - talloc_free(thread_tree); - return RLM_MODULE_FAIL; - } + fr_thread_local_set_destructor(local_thread_state, _python_thread_tree_free, thread_tree); } find.inst = inst; diff --git a/src/modules/rlm_sigtran/rlm_sigtran.c b/src/modules/rlm_sigtran/rlm_sigtran.c index 24d23ccd1f9..90bce2f5cf2 100644 --- a/src/modules/rlm_sigtran/rlm_sigtran.c +++ b/src/modules/rlm_sigtran/rlm_sigtran.c @@ -175,7 +175,7 @@ static rlm_rcode_t CC_HINT(nonnull) mod_authorize(void *instance, UNUSED void *t * Retrieve the thread specific pipe we use * to communicate with the multiplexer. */ - fd_ptr = fr_thread_local_init(req_pipe, _req_pipe_unregister); + fd_ptr = req_pipe; if (!fd_ptr) { fd_ptr = talloc(NULL, int); fd = sigtran_client_thread_register(); @@ -185,7 +185,7 @@ static rlm_rcode_t CC_HINT(nonnull) mod_authorize(void *instance, UNUSED void *t return RLM_MODULE_FAIL; } *fd_ptr = fd; - fr_thread_local_set(req_pipe, fd_ptr); + fr_thread_local_set_destructor(req_pipe, _req_pipe_unregister, fd_ptr); } else { fd = *fd_ptr; }