Skip to content

Commit f35c8ea

Browse files
committed
[Backport #22415] Restore ec->errinfo after handling interrupts
Interrupts are checked at arbitrary points, including while a jump (break/next/redo/return/raise/throw) is being propagated, and the payload of that jump lives in ec->errinfo. Handling an interrupt can run Ruby code: rb_signal_exec() runs a trap handler and threadptr_interrupt_exec_exec() runs interrupt_exec tasks. Neither preserves ec->errinfo, and any rescue or ensure clause that code enters sets it to nil. Thread.handle_interrupt checks for interrupts after its block returns but before re-raising the jump that left the block. If a signal handler runs in that window the throw data is replaced with nil and the VM dereferences nil as a vm_throw_data: THROW_DATA_CATCH_FRAME (obj=0x4) at vm_insnhelper.h:217 vm_exec_handle_exception (ec=..., errinfo=4) at vm.c:2880 vm_exec_loop / rb_vm_exec at vm.c:2829 / vm.c:2812 invoke_iseq_block_from_c / rb_yield / rb_ary_each rb_postponed_job_flush() and rb_exec_event_hooks() already save and restore ec->errinfo around the Ruby code they run; do the same for the rest of rb_threadptr_execute_interrupts(). The paths that set errinfo on purpose (rb_exc_raise, rb_threadptr_to_kill) jump out of the function and so do not reach the restore.
1 parent e9cd5b3 commit f35c8ea

2 files changed

Lines changed: 61 additions & 0 deletions

File tree

‎test/ruby/test_thread.rb‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -821,6 +821,59 @@ def test_handle_interrupt_with_break
821821
end
822822
end
823823

824+
# Thread.handle_interrupt checks for interrupts after the block returns,
825+
# before re-raising the jump (break/return/raise) that left the block.
826+
# Running a signal handler there must not clobber ec->errinfo, which still
827+
# holds the payload of that jump.
828+
def test_handle_interrupt_with_return_and_trap_handler
829+
omit "needs fork" unless Process.respond_to?(:fork)
830+
omit "SIGUSR1 is not supported" unless Signal.list.key?("USR1")
831+
832+
assert_separately([], "#{<<~"begin;"}\n#{<<~'end;'}", timeout: 120)
833+
begin;
834+
trap(:USR1) do
835+
begin
836+
raise "entering a rescue clause clears errinfo"
837+
rescue
838+
end
839+
end
840+
841+
def return_from_handle_interrupt(array)
842+
array.each do |i|
843+
Thread.handle_interrupt(Exception => :immediate) { return i }
844+
end
845+
nil
846+
end
847+
848+
# Signal from another process: a thread of this process would only get
849+
# scheduled a few times a second while the main thread spins.
850+
pid = Process.pid
851+
killer = fork do
852+
begin
853+
# Process.ppid changes if the parent crashes.
854+
while Process.ppid == pid
855+
Process.kill(:USR1, pid)
856+
sleep 0.001
857+
end
858+
rescue Errno::ESRCH
859+
end
860+
exit!(0)
861+
end
862+
863+
returned = nil
864+
begin
865+
500_000.times do
866+
returned = return_from_handle_interrupt([1, 2, 3])
867+
break unless returned == 1
868+
end
869+
ensure
870+
Process.kill(:KILL, killer) rescue Errno::ESRCH
871+
Process.waitpid(killer)
872+
end
873+
assert_equal(1, returned)
874+
end;
875+
end
876+
824877
def test_handle_interrupt_blocking
825878
r = nil
826879
q = Thread::Queue.new

‎thread.c‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2466,6 +2466,11 @@ rb_threadptr_execute_interrupts(rb_thread_t *th, int blocking_timing)
24662466

24672467
if (th->ec->raised_flag) return ret;
24682468

2469+
/* Interrupts are checked at arbitrary points, including while a jump
2470+
* (break/next/redo/return/raise/throw) is being propagated.
2471+
* Save errinfo as the interrupt might run Ruby code that could clear it. */
2472+
const VALUE saved_errinfo = th->ec->errinfo;
2473+
24692474
while ((interrupt = threadptr_get_interrupts(th)) != 0) {
24702475
int sig;
24712476
int timer_interrupt;
@@ -2562,6 +2567,9 @@ rb_threadptr_execute_interrupts(rb_thread_t *th, int blocking_timing)
25622567
rb_thread_schedule_limits(limits_us);
25632568
}
25642569
}
2570+
2571+
th->ec->errinfo = saved_errinfo;
2572+
25652573
return ret;
25662574
}
25672575

0 commit comments

Comments
 (0)