From cc7c0d438699cb29d7726b4a308257f6686a8c28 Mon Sep 17 00:00:00 2001 From: Marius Gerbershagen Date: Fri, 14 Feb 2020 22:31:46 +0100 Subject: [PATCH 1/2] multithreading: fix race condition on thread creation Due to the fact that the thread local environment is allocated with mmap, the garbage collector is only aware of it after the thread is listed in cl_core.processes. Therefore, we have to list the thread before we allocate any memory in its environment. We were doing this previously, however a bit earlier than needed which had the unfortunate side effect that not all threads listed in cl_core.processes had valid environment associated to them. Now, we delay the listing until immediately before allocating the contents of environment, ensuring that all listed threads have valid environments. --- src/c/main.d | 17 +++++++++++------ src/c/threads/process.d | 16 ++++++++++++++-- 2 files changed, 25 insertions(+), 8 deletions(-) diff --git a/src/c/main.d b/src/c/main.d index 8da2c0e41..a868328cd 100755 --- a/src/c/main.d +++ b/src/c/main.d @@ -179,7 +179,9 @@ ecl_init_env(cl_env_ptr env) env->method_cache = ecl_make_cache(64, 4096); env->slot_cache = ecl_make_cache(3, 4096); + env->interrupt_struct = ecl_alloc(sizeof(*env->interrupt_struct)); env->interrupt_struct->pending_interrupt = ECL_NIL; + env->interrupt_struct->signal_queue_spinlock = ECL_NIL; { int size = ecl_option_values[ECL_OPT_SIGNAL_QUEUE_SIZE]; env->interrupt_struct->signal_queue = cl_make_list(1, ecl_make_fixnum(size)); @@ -224,6 +226,13 @@ _ecl_alloc_env(cl_env_ptr parent) * Allocates the lisp environment for a thread. Depending on which * mechanism we use for detecting delayed signals, we may allocate * the environment using mmap or the garbage collector. + * + * Note that at this point we are not allocating any other memory + * which is stored via a pointer in the environment. If we would do + * that, an unlucky interrupt by the gc before the allocated + * environment is registered in cl_core.processes could lead to + * memory being freed because the gc is not aware of the pointer to + * the allocated memory in the environment. */ cl_env_ptr output; #if defined(ECL_USE_MPROTECT) @@ -249,10 +258,6 @@ _ecl_alloc_env(cl_env_ptr parent) } # endif #endif - if (!ecl_option_values[ECL_OPT_BOOTED]) - output->interrupt_struct = ecl_alloc_unprotected(sizeof(*output->interrupt_struct)); - else - output->interrupt_struct = ecl_alloc(sizeof(*output->interrupt_struct)); { size_t bytes = cl_core.default_sigmask_bytes; if (bytes == 0) { @@ -266,13 +271,13 @@ _ecl_alloc_env(cl_env_ptr parent) output->default_sigmask = cl_core.default_sigmask; } } + output->method_cache = output->slot_cache = NULL; + output->interrupt_struct = NULL; /* * An uninitialized environment _always_ disables interrupts. They * are activated later on by the thread entry point or init_unixint(). */ output->disable_interrupts = 1; - output->interrupt_struct->pending_interrupt = ECL_NIL; - output->interrupt_struct->signal_queue_spinlock = ECL_NIL; return output; } diff --git a/src/c/threads/process.d b/src/c/threads/process.d index 0c9440a96..375327a21 100755 --- a/src/c/threads/process.d +++ b/src/c/threads/process.d @@ -403,14 +403,20 @@ ecl_import_current_thread(cl_object name, cl_object bindings) process->process.phase = ECL_PROCESS_BOOTING; process->process.thread = current; + /* Immediately list the process such that its environment is + * marked by the gc when its contents are allocated */ + ecl_list_process(process); + + /* Now we can safely allocate memory for the environment contents + * and store pointers to it in the environment */ ecl_init_env(env); + env->cleanup = registered; env->bindings_array = process->process.initial_bindings; env->thread_local_bindings_size = env->bindings_array->vector.dim; env->thread_local_bindings = env->bindings_array->vector.self.t; ecl_enable_interrupts_env(env); - ecl_list_process(process); /* Activate the barrier so that processes can immediately start waiting. */ mp_barrier_unblock(1, process->process.exit_barrier); process->process.phase = ECL_PROCESS_ACTIVE; @@ -544,14 +550,20 @@ mp_process_enable(cl_object process) process->process.parent = mp_current_process(); process->process.trap_fpe_bits = process->process.parent->process.env->trap_fpe_bits; - ecl_list_process(process); /* Link environment and process together */ process_env = _ecl_alloc_env(the_env); process_env->own_process = process; process->process.env = process_env; + /* Immediately list the process such that its environment is + * marked by the gc when its contents are allocated */ + ecl_list_process(process); + + /* Now we can safely allocate memory for the environment contents + * and store pointers to it in the environment */ ecl_init_env(process_env); + process_env->trap_fpe_bits = process->process.trap_fpe_bits; process_env->bindings_array = process->process.initial_bindings; process_env->thread_local_bindings_size = From 711d17e3732525461a8e34c1f15b855d6102ef75 Mon Sep 17 00:00:00 2001 From: Marius Gerbershagen Date: Fri, 14 Feb 2020 22:40:56 +0100 Subject: [PATCH 2/2] si_clear_gfun_hash: only enqueue operations for threads with valid environments Fixes #560. --- src/c/clos/gfun.d | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/c/clos/gfun.d b/src/c/clos/gfun.d index 38759c0ea..64119961f 100644 --- a/src/c/clos/gfun.d +++ b/src/c/clos/gfun.d @@ -287,9 +287,11 @@ si_clear_gfun_hash(cl_object what) for (list = mp_all_processes(); !Null(list); list = ECL_CONS_CDR(list)) { cl_object process = ECL_CONS_CAR(list); struct cl_env_struct *env = process->process.env; - if (the_env != env) { - ecl_cache_remove_one(env->method_cache, what); - ecl_cache_remove_one(env->slot_cache, what); + if (the_env != env && env) { + if (env->method_cache) + ecl_cache_remove_one(env->method_cache, what); + if (env->slot_cache) + ecl_cache_remove_one(env->slot_cache, what); } } #endif