From 0b091fdac6ceb33b7379ceddc9a49a79d0e158b2 Mon Sep 17 00:00:00 2001 From: Jeremy Evans Date: Wed, 6 Apr 2022 19:14:03 -0700 Subject: [PATCH] Raise RuntimeError if Kernel#binding is called from a non-Ruby frame Check whether the current or previous frame is a Ruby frame in call_trace_func and rb_tracearg_binding before attempting to create a binding for the frame. Fixes [Bug #18487] Co-authored-by: Alan Wu --- bootstraptest/test_yjit.rb | 31 ------------- spec/ruby/optional/capi/binding_spec.rb | 21 ++++++--- test/ruby/test_proc.rb | 5 +++ test/ruby/test_settracefunc.rb | 60 ++++++++++++++----------- vm.c | 11 ++--- vm_trace.c | 10 ++++- 6 files changed, 65 insertions(+), 73 deletions(-) diff --git a/bootstraptest/test_yjit.rb b/bootstraptest/test_yjit.rb index d124d180d1..a84a9e035a 100644 --- a/bootstraptest/test_yjit.rb +++ b/bootstraptest/test_yjit.rb @@ -1,34 +1,3 @@ -assert_equal '2022', %q{ - def contrivance(hash, key) - # Expect this to compile to an `opt_aref`. - hash[key] - - # The [] call above tracks that the `hash` local has a VALUE that - # is a heap pointer and the guard for the Kernel#itself call below - # doesn't check that it's a heap pointer VALUE. - # - # As you can see from the crash, the call to rb_hash_aref() can set the - # `hash` local, making eliding the heap object guard unsound. - hash.itself - end - - # This is similar to ->(recv, mid) { send(recv, mid).local_variable_set(...) }. - # By composing we avoid creating new Ruby frames and so sending :binding - # captures the environment of the frame that does the missing key lookup. - # We use it to capture the environment inside of `contrivance`. - cap_then_set = - Kernel.instance_method(:send).method(:bind_call).to_proc >> - ->(binding) { binding.local_variable_set(:hash, 2022) } - special_missing = Hash.new(&cap_then_set) - - # Make YJIT speculate that it's a hash and generate code - # that calls rb_hash_aref(). - contrivance({}, :warmup) - contrivance({}, :warmup) - - contrivance(special_missing, :binding) -} - assert_equal '18374962167983112447', %q{ # regression test for incorrectly discarding 32 bits of a pointer when it # comes to default values. diff --git a/spec/ruby/optional/capi/binding_spec.rb b/spec/ruby/optional/capi/binding_spec.rb index 966d650c46..2165705457 100644 --- a/spec/ruby/optional/capi/binding_spec.rb +++ b/spec/ruby/optional/capi/binding_spec.rb @@ -8,12 +8,21 @@ describe "CApiBindingSpecs" do end describe "Kernel#binding" do - it "gives the top-most Ruby binding when called from C" do - foo = 14 - b = @b.get_binding - b.local_variable_get(:foo).should == 14 - b.local_variable_set :foo, 12 - foo.should == 12 + ruby_version_is '3.2' do + it "raises when called from C" do + foo = 14 + -> { @b.get_binding }.should raise_error(RuntimeError) + end + end + + ruby_version_is ''...'3.2' do + it "gives the top-most Ruby binding when called from C" do + foo = 14 + b = @b.get_binding + b.local_variable_get(:foo).should == 14 + b.local_variable_set :foo, 12 + foo.should == 12 + end end end end diff --git a/test/ruby/test_proc.rb b/test/ruby/test_proc.rb index 63c258744a..05c6e03aac 100644 --- a/test/ruby/test_proc.rb +++ b/test/ruby/test_proc.rb @@ -445,6 +445,11 @@ class TestProc < Test::Unit::TestCase assert_equal(@@line_of_source_location_test, lineno, 'Bug #2427') end + def test_binding_error_unless_ruby_frame + define_singleton_method :binding_from_c!, method(:binding).to_proc >> ->(bndg) {bndg} + assert_raise(RuntimeError) { binding_from_c! } + end + def test_proc_lambda assert_raise(ArgumentError) { proc } assert_raise(ArgumentError) { assert_warn(/deprecated/) {lambda} } diff --git a/test/ruby/test_settracefunc.rb b/test/ruby/test_settracefunc.rb index f10272f2ee..b43f9c114d 100644 --- a/test/ruby/test_settracefunc.rb +++ b/test/ruby/test_settracefunc.rb @@ -594,7 +594,7 @@ CODE eval <<-EOF.gsub(/^.*?: /, ""), nil, 'xyzzy' 1: set_trace_func(lambda{|event, file, line, id, binding, klass| - 2: events << [event, line, file, klass, id, binding.eval('self'), binding.eval("_local_var")] if file == 'xyzzy' + 2: events << [event, line, file, klass, id, binding&.eval('self'), binding&.eval("_local_var")] if file == 'xyzzy' 3: }) 4: 1.times{|;_local_var| _local_var = :inner 5: tap{} @@ -619,31 +619,31 @@ CODE answer_events = [ # - [:c_return, 1, "xyzzy", TracePoint, :trace, TracePoint, :outer, trace], + [:c_return, 1, "xyzzy", TracePoint, :trace, TracePoint, nil, nil], [:line, 4, 'xyzzy', self.class, method, self, :outer, :nothing], - [:c_call, 4, 'xyzzy', Integer, :times, 1, :outer, :nothing], + [:c_call, 4, 'xyzzy', Integer, :times, 1, nil, nil], [:line, 4, 'xyzzy', self.class, method, self, nil, :nothing], [:line, 5, 'xyzzy', self.class, method, self, :inner, :nothing], - [:c_return, 4, "xyzzy", Integer, :times, 1, :outer, 1], + [:c_return, 4, "xyzzy", Integer, :times, 1, nil, nil], [:line, 7, 'xyzzy', self.class, method, self, :outer, :nothing], - [:c_call, 7, "xyzzy", Class, :inherited, Object, :outer, :nothing], - [:c_return, 7, "xyzzy", Class, :inherited, Object, :outer, nil], - [:c_call, 7, "xyzzy", Class, :const_added, Object, :outer, :nothing], - [:c_return, 7, "xyzzy", Class, :const_added, Object, :outer, nil], + [:c_call, 7, "xyzzy", Class, :inherited, Object, nil, nil], + [:c_return, 7, "xyzzy", Class, :inherited, Object, nil, nil], + [:c_call, 7, "xyzzy", Class, :const_added, Object, nil, nil], + [:c_return, 7, "xyzzy", Class, :const_added, Object, nil, nil], [:class, 7, "xyzzy", nil, nil, xyzzy.class, nil, :nothing], [:line, 8, "xyzzy", nil, nil, xyzzy.class, nil, :nothing], [:line, 9, "xyzzy", nil, nil, xyzzy.class, :XYZZY_outer, :nothing], - [:c_call, 9, "xyzzy", Module, :method_added, xyzzy.class, :XYZZY_outer, :nothing], - [:c_return, 9, "xyzzy", Module, :method_added, xyzzy.class, :XYZZY_outer, nil], + [:c_call, 9, "xyzzy", Module, :method_added, xyzzy.class, nil, nil], + [:c_return, 9, "xyzzy", Module, :method_added, xyzzy.class, nil, nil], [:line, 13, "xyzzy", nil, nil, xyzzy.class, :XYZZY_outer, :nothing], - [:c_call, 13, "xyzzy", Module, :method_added, xyzzy.class, :XYZZY_outer, :nothing], - [:c_return,13, "xyzzy", Module, :method_added, xyzzy.class, :XYZZY_outer, nil], + [:c_call, 13, "xyzzy", Module, :method_added, xyzzy.class, nil, nil], + [:c_return,13, "xyzzy", Module, :method_added, xyzzy.class, nil, nil], [:end, 17, "xyzzy", nil, nil, xyzzy.class, :XYZZY_outer, :nothing], [:line, 18, "xyzzy", TestSetTraceFunc, method, self, :outer, :nothing], - [:c_call, 18, "xyzzy", Class, :new, xyzzy.class, :outer, :nothing], - [:c_call, 18, "xyzzy", BasicObject, :initialize, xyzzy, :outer, :nothing], - [:c_return,18, "xyzzy", BasicObject, :initialize, xyzzy, :outer, nil], - [:c_return,18, "xyzzy", Class, :new, xyzzy.class, :outer, xyzzy], + [:c_call, 18, "xyzzy", Class, :new, xyzzy.class, nil, nil], + [:c_call, 18, "xyzzy", BasicObject, :initialize, xyzzy, nil, nil], + [:c_return,18, "xyzzy", BasicObject, :initialize, xyzzy, nil, nil], + [:c_return,18, "xyzzy", Class, :new, xyzzy.class, nil, nil], [:line, 19, "xyzzy", TestSetTraceFunc, method, self, :outer, :nothing], [:call, 9, "xyzzy", xyzzy.class, :foo, xyzzy, nil, :nothing], [:line, 10, "xyzzy", xyzzy.class, :foo, xyzzy, nil, :nothing], @@ -654,23 +654,29 @@ CODE [:return, 16, "xyzzy", xyzzy.class, :bar, xyzzy, :XYZZY_bar, xyzzy], [:return, 12, "xyzzy", xyzzy.class, :foo, xyzzy, :XYZZY_foo, xyzzy], [:line, 20, "xyzzy", TestSetTraceFunc, method, self, :outer, :nothing], - [:c_call, 20, "xyzzy", Kernel, :raise, self, :outer, :nothing], - [:c_call, 20, "xyzzy", Exception, :exception, RuntimeError, :outer, :nothing], - [:c_call, 20, "xyzzy", Exception, :initialize, raised_exc, :outer, :nothing], - [:c_return,20, "xyzzy", Exception, :initialize, raised_exc, :outer, raised_exc], - [:c_return,20, "xyzzy", Exception, :exception, RuntimeError, :outer, raised_exc], - [:c_return,20, "xyzzy", Kernel, :raise, self, :outer, nil], - [:c_call, 20, "xyzzy", Exception, :backtrace, raised_exc, :outer, :nothing], - [:c_return,20, "xyzzy", Exception, :backtrace, raised_exc, :outer, nil], + [:c_call, 20, "xyzzy", Kernel, :raise, self, nil, nil], + [:c_call, 20, "xyzzy", Exception, :exception, RuntimeError, nil, nil], + [:c_call, 20, "xyzzy", Exception, :initialize, raised_exc, nil, nil], + [:c_return,20, "xyzzy", Exception, :initialize, raised_exc, nil, nil], + [:c_return,20, "xyzzy", Exception, :exception, RuntimeError, nil, nil], + [:c_return,20, "xyzzy", Kernel, :raise, self, nil, nil], + [:c_call, 20, "xyzzy", Exception, :backtrace, raised_exc, nil, nil], + [:c_return,20, "xyzzy", Exception, :backtrace, raised_exc, nil, nil], [:raise, 20, "xyzzy", TestSetTraceFunc, :trace_by_tracepoint, self, :outer, raised_exc], - [:c_call, 20, "xyzzy", Module, :===, RuntimeError,:outer, :nothing], - [:c_return,20, "xyzzy", Module, :===, RuntimeError,:outer, true], + [:c_call, 20, "xyzzy", Module, :===, RuntimeError, nil, nil], + [:c_return,20, "xyzzy", Module, :===, RuntimeError, nil, nil], [:line, 21, "xyzzy", TestSetTraceFunc, method, self, :outer, :nothing], - [:c_call, 21, "xyzzy", TracePoint, :disable, trace, :outer, :nothing], + [:c_call, 21, "xyzzy", TracePoint, :disable, trace, nil, nil], ] return events, answer_events end + def test_set_trace_func_curry_argument_error + b = lambda {|x, y, z| (x||0) + (y||0) + (z||0) }.curry[1, 2] + set_trace_func(proc {}) + assert_raise(ArgumentError) {b[3, 4]} + end + def test_set_trace_func actual_events, expected_events = trace_by_set_trace_func expected_events.zip(actual_events){|e, a| diff --git a/vm.c b/vm.c index 720d26256e..b8df0d6ee5 100644 --- a/vm.c +++ b/vm.c @@ -1235,15 +1235,12 @@ rb_vm_make_binding(const rb_execution_context_t *ec, const rb_control_frame_t *s if (cfp == 0 || ruby_level_cfp == 0) { rb_raise(rb_eRuntimeError, "Can't create Binding Object on top of Fiber."); } - - while (1) { - envval = vm_make_env_object(ec, cfp); - if (cfp == ruby_level_cfp) { - break; - } - cfp = rb_vm_get_binding_creatable_next_cfp(ec, RUBY_VM_PREVIOUS_CONTROL_FRAME(cfp)); + if (!VM_FRAME_RUBYFRAME_P(src_cfp) && + !VM_FRAME_RUBYFRAME_P(RUBY_VM_PREVIOUS_CONTROL_FRAME(src_cfp))) { + rb_raise(rb_eRuntimeError, "Cannot create Binding object for non-Ruby caller"); } + envval = vm_make_env_object(ec, cfp); bindval = rb_binding_alloc(rb_cBinding); GetBindingPtr(bindval, bind); vm_bind_update_env(bindval, bind, envval); diff --git a/vm_trace.c b/vm_trace.c index 653e9a510a..99d3104b40 100644 --- a/vm_trace.c +++ b/vm_trace.c @@ -721,7 +721,13 @@ call_trace_func(rb_event_flag_t event, VALUE proc, VALUE self, ID id, VALUE klas argv[1] = filename; argv[2] = INT2FIX(line); argv[3] = id ? ID2SYM(id) : Qnil; - argv[4] = (self && (filename != Qnil)) ? rb_binding_new() : Qnil; + argv[4] = Qnil; + if (self && (filename != Qnil) && + event != RUBY_EVENT_C_CALL && + event != RUBY_EVENT_C_RETURN && + (VM_FRAME_RUBYFRAME_P(ec->cfp) && imemo_type_p((VALUE)ec->cfp->iseq, imemo_iseq))) { + argv[4] = rb_binding_new(); + } argv[5] = klass ? klass : Qnil; rb_proc_call_with_block(proc, 6, argv, Qnil); @@ -952,7 +958,7 @@ rb_tracearg_binding(rb_trace_arg_t *trace_arg) rb_control_frame_t *cfp; cfp = rb_vm_get_binding_creatable_next_cfp(trace_arg->ec, trace_arg->cfp); - if (cfp) { + if (cfp && imemo_type_p((VALUE)cfp->iseq, imemo_iseq)) { return rb_vm_make_binding(trace_arg->ec, cfp); } else {