From 4ba8f0dc993953d3ddda6328e3ef17a2fc2cbde5 Mon Sep 17 00:00:00 2001 From: KJ Tsanaktsidis Date: Sun, 12 Nov 2023 13:24:55 +1100 Subject: [PATCH] Pass down "stack start" variables from closer to the top of the stack The implementation of `native_thread_init_stack` for the various threading models can use the address of a local variable as part of the calculation of the machine stack extents: * pthreads uses it as a lower-bound on the start of the stack, because glibc (and maybe other libcs) can store its own data on the stack before calling into user code on thread creation. * win32 uses it as an argument to VirtualQuery, which gets the extent of the memory mapping which contains the variable However, the local being used for this is actually allocated _inside_ the `native_thread_init_stack` frame; that means the caller might allocate a VALUE on the stack that actually lies outside the bounds stored in machine.stack_{start,end}. A local variable from one level above the topmost frame that stores VALUEs on the stack must be drilled down into the call to `native_thread_init_stack` to be used in the calculation. This probably doesn't _really_ matter for the win32 case (they'll be in the same memory mapping so VirtualQuery should return the same thing), but definitely could matter for the pthreads case. [Bug #20001] --- eval.c | 4 ++-- include/ruby/internal/interpreter.h | 2 +- internal/inits.h | 2 +- thread.c | 4 ++-- thread_none.c | 4 ++-- thread_pthread.c | 21 ++++++++++++++++----- thread_win32.c | 12 ++++++------ vm.c | 4 ++-- vm_core.h | 2 +- 9 files changed, 33 insertions(+), 22 deletions(-) diff --git a/eval.c b/eval.c index aa0eae0872..6424aed1dc 100644 --- a/eval.c +++ b/eval.c @@ -70,7 +70,7 @@ ruby_setup(void) if (GET_VM()) return 0; - ruby_init_stack((void *)&state); + ruby_init_stack(&state); /* * Disable THP early before mallocs happen because we want this to @@ -79,7 +79,7 @@ ruby_setup(void) #if defined(__linux__) && defined(PR_SET_THP_DISABLE) prctl(PR_SET_THP_DISABLE, 1, 0, 0, 0); #endif - Init_BareVM(); + Init_BareVM(&state); Init_heap(); rb_vm_encoded_insn_data_table_init(); Init_vm_objects(); diff --git a/include/ruby/internal/interpreter.h b/include/ruby/internal/interpreter.h index 662d39c0ec..d36d61ba54 100644 --- a/include/ruby/internal/interpreter.h +++ b/include/ruby/internal/interpreter.h @@ -141,7 +141,7 @@ void ruby_show_copyright(void); * * @param[in] addr A pointer somewhere on the stack, near its bottom. */ -void ruby_init_stack(volatile VALUE *addr); +void ruby_init_stack(volatile void *addr); /** * Initializes the VM and builtin libraries. diff --git a/internal/inits.h b/internal/inits.h index 03e180f77b..a2b2eea96f 100644 --- a/internal/inits.h +++ b/internal/inits.h @@ -29,7 +29,7 @@ int Init_enc_set_filesystem_encoding(void); void Init_newline(void); /* vm.c */ -void Init_BareVM(void); +void Init_BareVM(void *local_in_parent_frame); void Init_vm_objects(void); /* vm_backtrace.c */ diff --git a/thread.c b/thread.c index 070d1bbdfa..e7a9d7cd9b 100644 --- a/thread.c +++ b/thread.c @@ -522,9 +522,9 @@ static VALUE rb_threadptr_raise(rb_thread_t *, int, VALUE *); static VALUE rb_thread_to_s(VALUE thread); void -ruby_thread_init_stack(rb_thread_t *th) +ruby_thread_init_stack(rb_thread_t *th, void *local_in_parent_frame) { - native_thread_init_stack(th); + native_thread_init_stack(th, local_in_parent_frame); } const VALUE * diff --git a/thread_none.c b/thread_none.c index 4d53d3bf4d..767b929cf9 100644 --- a/thread_none.c +++ b/thread_none.c @@ -140,12 +140,12 @@ ruby_mn_threads_params(void) } void -ruby_init_stack(volatile VALUE *addr) +ruby_init_stack(volatile void *addr) { } static int -native_thread_init_stack(rb_thread_t *th) +native_thread_init_stack(rb_thread_t *th, void *local_in_parent_frame) { #if defined(__wasm__) && !defined(__EMSCRIPTEN__) th->ec->machine.stack_start = (VALUE *)rb_wasm_stack_get_base(); diff --git a/thread_pthread.c b/thread_pthread.c index 6d2f55a957..a6a6c9d127 100644 --- a/thread_pthread.c +++ b/thread_pthread.c @@ -1964,7 +1964,7 @@ reserve_stack(volatile char *limit, size_t size) #undef ruby_init_stack void -ruby_init_stack(volatile VALUE *addr) +ruby_init_stack(volatile void *addr) { native_main_thread.id = pthread_self(); @@ -2049,7 +2049,7 @@ ruby_init_stack(volatile VALUE *addr) {int err = (expr); if (err) {rb_bug_errno(#expr, err);}} static int -native_thread_init_stack(rb_thread_t *th) +native_thread_init_stack(rb_thread_t *th, void *local_in_parent_frame) { rb_nativethread_id_t curr = pthread_self(); @@ -2064,8 +2064,8 @@ native_thread_init_stack(rb_thread_t *th) size_t size; if (get_stack(&start, &size) == 0) { - uintptr_t diff = (uintptr_t)start - (uintptr_t)&curr; - th->ec->machine.stack_start = (VALUE *)&curr; + uintptr_t diff = (uintptr_t)start - (uintptr_t)local_in_parent_frame; + th->ec->machine.stack_start = (uintptr_t)local_in_parent_frame; th->ec->machine.stack_maxsize = size - diff; } } @@ -2185,8 +2185,19 @@ native_thread_create_dedicated(rb_thread_t *th) static void call_thread_start_func_2(rb_thread_t *th) { - native_thread_init_stack(th); + /* Capture the address of a local in this stack frame to mark the beginning of the + machine stack for this thread. This is required even if we can tell the real + stack beginning from the pthread API in native_thread_init_stack, because + glibc stores some of its own data on the stack before calling into user code + on a new thread, and replacing that data on fiber-switch would break it (see + bug #13887) */ + VALUE stack_start = 0; + VALUE *stack_start_addr = &stack_start; + native_thread_init_stack(th, stack_start_addr); thread_start_func_2(th, th->ec->machine.stack_start); + + /* Ensure that stack_start really was spilled to the stack */ + RB_GC_GUARD(stack_start) } static void * diff --git a/thread_win32.c b/thread_win32.c index bd983e0bd9..e92472cea9 100644 --- a/thread_win32.c +++ b/thread_win32.c @@ -582,7 +582,7 @@ rb_native_cond_destroy(rb_nativethread_cond_t *cond) } void -ruby_init_stack(volatile VALUE *addr) +ruby_init_stack(volatile void *addr) { } @@ -594,20 +594,20 @@ COMPILER_WARNING_PUSH COMPILER_WARNING_IGNORED(-Wmaybe-uninitialized) #endif static inline SIZE_T -query_memory_basic_info(PMEMORY_BASIC_INFORMATION mi) +query_memory_basic_info(PMEMORY_BASIC_INFORMATION mi, void *local_in_parent_frame) { - return VirtualQuery(mi, mi, sizeof(*mi)); + return VirtualQuery(local_in_parent_frame, mi, sizeof(*mi)); } COMPILER_WARNING_POP static void -native_thread_init_stack(rb_thread_t *th) +native_thread_init_stack(rb_thread_t *th, void *local_in_parent_frame) { MEMORY_BASIC_INFORMATION mi; char *base, *end; DWORD size, space; - CHECK_ERR(query_memory_basic_info(&mi)); + CHECK_ERR(query_memory_basic_info(&mi, local_in_parent_frame)); base = mi.AllocationBase; end = mi.BaseAddress; end += mi.RegionSize; @@ -638,7 +638,7 @@ thread_start_func_1(void *th_ptr) rb_thread_t *th = th_ptr; volatile HANDLE thread_id = th->nt->thread_id; - native_thread_init_stack(th); + native_thread_init_stack(th, &th); th->nt->interrupt_event = CreateEvent(0, TRUE, FALSE, 0); /* run */ diff --git a/vm.c b/vm.c index 37d631116a..945e7b91f7 100644 --- a/vm.c +++ b/vm.c @@ -4174,7 +4174,7 @@ rb_vm_set_progname(VALUE filename) extern const struct st_hash_type rb_fstring_hash_type; void -Init_BareVM(void) +Init_BareVM(void *local_in_parent_frame) { /* VM bootstrap: phase 1 */ rb_vm_t * vm = ruby_mimmalloc(sizeof(*vm)); @@ -4204,7 +4204,7 @@ Init_BareVM(void) th_init(th, 0, vm); rb_ractor_set_current_ec(th->ractor, th->ec); - ruby_thread_init_stack(th); + ruby_thread_init_stack(th, local_in_parent_frame); // setup ractor system rb_native_mutex_initialize(&vm->ractor.sync.lock); diff --git a/vm_core.h b/vm_core.h index 354603514e..91cfae77e9 100644 --- a/vm_core.h +++ b/vm_core.h @@ -1842,7 +1842,7 @@ rb_control_frame_t *rb_vm_get_binding_creatable_next_cfp(const rb_execution_cont VALUE *rb_vm_svar_lep(const rb_execution_context_t *ec, const rb_control_frame_t *cfp); int rb_vm_get_sourceline(const rb_control_frame_t *); void rb_vm_stack_to_heap(rb_execution_context_t *ec); -void ruby_thread_init_stack(rb_thread_t *th); +void ruby_thread_init_stack(rb_thread_t *th, void *local_in_parent_frame); rb_thread_t * ruby_thread_from_native(void); int ruby_thread_set_native(rb_thread_t *th); int rb_vm_control_frame_id_and_class(const rb_control_frame_t *cfp, ID *idp, ID *called_idp, VALUE *klassp);