Skip to content

Commit

Permalink
Inline RB_VM_SAVE_MACHINE_CONTEXT into BLOCKING_REGION
Browse files Browse the repository at this point in the history
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]
  • Loading branch information
KJTsanaktsidis committed May 19, 2024
1 parent ad63603 commit 7afc16a
Showing 1 changed file with 6 additions and 3 deletions.
9 changes: 6 additions & 3 deletions thread.c
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,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_VM_SAVE_MACHINE_CONTEXT(th); \
thread_sched_to_waiting(TH_SCHED(th), th); \
exec; \
blocking_region_end(th, &__region); \
}; \
Expand Down Expand Up @@ -1482,9 +1486,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("thread_id:%p", (void *)th->nt->thread_id);

RB_VM_SAVE_MACHINE_CONTEXT(th);
thread_sched_to_waiting(TH_SCHED(th), th);
return TRUE;
}
else {
Expand Down Expand Up @@ -1893,6 +1894,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_VM_SAVE_MACHINE_CONTEXT(th);
thread_sched_to_waiting(TH_SCHED(th), th);
return r;
}

Expand Down

0 comments on commit 7afc16a

Please sign in to comment.