Skip to content

Commit c24cfd7

Browse files
committed
vm: Protect against rb_jump_tag(TAG_RAISE) after errinfo has been cleared
If native code calls rb_protect, then calls rb_funcall before calling rb_jump_tag, and that ruby code has a rescue clause, it may set errinfo to nil. Previously this would cause a SEGV as we weren't checking the type and assuming errinfo was a T_OBJECT. After adding a check to prevent the SEGV the process proceeds to simply exit silently. To avoid that we can identify when we are in this state (TAG_RAISE with nil errinfo) and create an exception to describe what happened.
1 parent 349071b commit c24cfd7

3 files changed

Lines changed: 104 additions & 1 deletion

File tree

ext/-test-/exception/nil_errinfo.c

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
#include <ruby.h>
2+
3+
static VALUE
4+
raise_boom(VALUE _arg)
5+
{
6+
rb_raise(rb_eRuntimeError, "boom");
7+
UNREACHABLE_RETURN(Qnil);
8+
}
9+
10+
/*
11+
* If a native extension catches an exception with rb_protect()
12+
* and then uses rb_funcall to run Ruby code that rescues an exception
13+
* the errinfo will be set to Qnil before the extension calls rb_jump_tag.
14+
* We want to defend against that.
15+
*
16+
* Extensions should save the errinfo before the call and restore it afterward.
17+
*/
18+
static VALUE
19+
raise_after_rescue_cleanup(VALUE self)
20+
{
21+
int state = 0;
22+
23+
rb_protect(raise_boom, Qnil, &state);
24+
25+
if (state) {
26+
/* Any begin/rescue that catches an exception clears ec->errinfo. */
27+
rb_funcall(self, rb_intern("cleanup_with_rescue"), 0);
28+
/* ec->errinfo is now Qnil, but state is still TAG_RAISE. */
29+
rb_jump_tag(state);
30+
}
31+
return Qnil;
32+
}
33+
34+
void
35+
Init_nil_errinfo(VALUE klass)
36+
{
37+
rb_define_singleton_method(klass, "raise_after_rescue_cleanup",
38+
raise_after_rescue_cleanup, 0);
39+
}
Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
# frozen_string_literal: false
2+
require 'test/unit'
3+
require '-test-/exception'
4+
5+
module Bug
6+
class Test_ExceptionNilErrinfo < Test::Unit::TestCase
7+
def test_rescue_cleanup_produces_diagnostic
8+
out, _, status = EnvUtil.invoke_ruby(%w[-W0], <<~'RUBY', true, :merge_to_stdout)
9+
require '-test-/exception'
10+
11+
class Bug::Exception
12+
def self.cleanup_with_rescue
13+
begin
14+
raise "cleanup error"
15+
rescue
16+
# entering this rescue sets ec->errinfo to Qnil
17+
end
18+
end
19+
end
20+
21+
Bug::Exception.raise_after_rescue_cleanup
22+
RUBY
23+
assert !status.signaled?, "process must not crash"
24+
assert_include out, "exception object was lost"
25+
assert_include out, "RuntimeError"
26+
refute_includes out, "cleanup error"
27+
end
28+
29+
def test_rescue_cleanup_raises_latest_error
30+
out, _, status = EnvUtil.invoke_ruby(%w[-W0], <<~'RUBY', true, :merge_to_stdout)
31+
require '-test-/exception'
32+
33+
class Bug::Exception
34+
def self.cleanup_with_rescue
35+
begin
36+
raise "cleanup error"
37+
# no rescue
38+
end
39+
end
40+
end
41+
42+
Bug::Exception.raise_after_rescue_cleanup
43+
RUBY
44+
assert !status.signaled?, "process must not crash"
45+
refute_includes out, "exception object was lost"
46+
assert_include out, "RuntimeError"
47+
assert_include out, "cleanup error"
48+
end
49+
end
50+
end

vm.c

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2637,7 +2637,7 @@ frame_name(const rb_control_frame_t *cfp)
26372637
static void
26382638
hook_before_rewind(rb_execution_context_t *ec, bool cfp_returning_with_value, int state, struct vm_throw_data *err)
26392639
{
2640-
if (state == TAG_RAISE && RBASIC(err)->klass == rb_eSysStackError) {
2640+
if (state == TAG_RAISE && !RB_SPECIAL_CONST_P((VALUE)err) && RBASIC(err)->klass == rb_eSysStackError) {
26412641
return;
26422642
}
26432643
else {
@@ -2976,6 +2976,20 @@ vm_exec_handle_exception(rb_execution_context_t *ec, enum ruby_tag_type state, V
29762976
{
29772977
struct vm_throw_data *err = (struct vm_throw_data *)errinfo;
29782978

2979+
/* If rb_funcall is called between rb_protect and rb_jump_tag and the ruby
2980+
* code has a rescue clause errinfo will be Qnil if not properly preserved.
2981+
* At that point the original exception is lost.
2982+
* Rather than silently exiting we synthesize a RuntimeError that
2983+
* will either end the process or be caught by the caller. */
2984+
if (state == TAG_RAISE && NIL_P(errinfo)) {
2985+
errinfo = rb_exc_new_cstr(rb_eRuntimeError,
2986+
"[Bug] exception object was lost during stack unwinding; "
2987+
"a native extension may have run Ruby code containing a rescue clause "
2988+
"between rb_protect and rb_jump_tag");
2989+
err = (struct vm_throw_data *)errinfo;
2990+
ec->errinfo = errinfo;
2991+
}
2992+
29792993
for (;;) {
29802994
unsigned int i;
29812995
const struct iseq_catch_table_entry *entry;

0 commit comments

Comments
 (0)