From 2f010f31f1887ad0f429709a2906fc5a4dae8e87 Mon Sep 17 00:00:00 2001 From: KJ Tsanaktsidis Date: Sat, 18 May 2024 18:37:49 +0900 Subject: Inline RB_VM_SAVE_MACHINE_CONTEXT into BLOCKING_REGION There's an exhaustive explanation of this in the linked redmine bug, but the short version is as follows: blocking_region_begin can spill callee-saved registers to the stack for its own use. That means they're not saved to ec->machine by the call to setjmp, since by that point they're already on the stack and new, different values are in the real registers. ec->machine's end-of-stack pointer is also bumped to accomodate this, BUT, after blocking_region_begin returns, that points past the end of the stack! As far as C is concerned, that's fine; the callee-saved registers are restored when blocking_region_begin returns. But, if another thread triggers GC, it is relying on finding references to Ruby objects by walking the stack region pointed to by ec->machine. If the C code in exec; subsequently does things that use that stack memory, then the value will be overwritten and the GC might prematurely collect something it shouldn't. [Bug #20493] --- thread.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) (limited to 'thread.c') diff --git a/thread.c b/thread.c index a1c2780c6f..56cba3d6ee 100644 --- a/thread.c +++ b/thread.c @@ -196,6 +196,10 @@ static inline void blocking_region_end(rb_thread_t *th, struct rb_blocking_regio if (blocking_region_begin(th, &__region, (ubf), (ubfarg), fail_if_interrupted) || \ /* always return true unless fail_if_interrupted */ \ !only_if_constant(fail_if_interrupted, TRUE)) { \ + /* Important that this is inlined into the macro, and not part of \ + * blocking_region_begin - see bug #20493 */ \ + RB_GC_SAVE_MACHINE_CONTEXT(th); \ + thread_sched_to_waiting(TH_SCHED(th)); \ exec; \ blocking_region_end(th, &__region); \ }; \ @@ -1493,9 +1497,6 @@ blocking_region_begin(rb_thread_t *th, struct rb_blocking_region_buffer *region, rb_ractor_blocking_threads_inc(th->ractor, __FILE__, __LINE__); RUBY_DEBUG_LOG(""); - - RB_GC_SAVE_MACHINE_CONTEXT(th); - thread_sched_to_waiting(TH_SCHED(th)); return TRUE; } else { @@ -1788,6 +1789,8 @@ rb_thread_call_with_gvl(void *(*func)(void *), void *data1) /* leave from Ruby world: You can not access Ruby values, etc. */ int released = blocking_region_begin(th, brb, prev_unblock.func, prev_unblock.arg, FALSE); RUBY_ASSERT_ALWAYS(released); + RB_GC_SAVE_MACHINE_CONTEXT(th); + thread_sched_to_waiting(TH_SCHED(th)); return r; } -- cgit v1.2.3