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 8076fe12da423816e1490dd75b04c0dc716d7901..a5c4c5146e9f6cfca2d927fc59134312ddde5b6d 100644 (file)
--- 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 <pthread.h>
-/** 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 18a8313001c0c33e1d3bc33e3d43850011833dcb..dc38535ad0c86316804d51d7ff167865b7e0c1d1 100644 (file)
--- 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 6ba142c617dffbde6fe92cd618a4f964f5005ba3..8978a172b8e53e64d2c679cbfede595a13605155 100644 (file)
--- 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 5c362386dbdd3771a04d213ae7d2ba87aa01d72e..a5d6f08a6e29e6257058176954571c3748c37c51 100644 (file)
--- 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 a8d5a5958546da3af8bc2863c6c34ce95b8f621c..1610ce54397833a2d741f3a919ee9ae0469718bb 100644 (file)
--- 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 24d23ccd1f9336ba03860794e23f64b51b5dfb3e..90bce2f5cf2af79255ffd9f282bb8a81780179b7 100644 (file)
--- 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;
        }