Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
128 changes: 128 additions & 0 deletions mrbgems/mruby-method/test/method.rb
Original file line number Diff line number Diff line change
Expand Up @@ -512,3 +512,131 @@ def m(x, y, z, *w, u:, v:, **opts, &blk)
assert_raise(ArgumentError) { BasicObject.instance_method(:__id__).bind_call nil, 1 }
assert_raise(ArgumentError) { BasicObject.instance_method(:__id__).bind_call nil, opts: 1 }
end

assert 'Method#parameters and #arity on aliased methods' do
# Regression: an alias proc carries the original method's name in body.mid
# (not an irep), with `upper` pointing at the original proc. Both
# mrb_proc_parameters (mruby-proc-ext) and mrb_proc_arity (core src/proc.c)
# used to fall into their irep branch for alias procs and dereference body.mid
# as an mrb_irep* -> SEGV / misaligned read. Both must resolve through `upper`.
#
# The literals below match CRuby for these positional/optional/rest/block
# signatures (where mruby and CRuby agree), so this is independent ground
# truth, not merely "alias == original".
c = Class.new {
def f0; end
def f1(a); end
def fopt(a, b = 1); end
def frest(a, *b); end
def fblk(a, &b); end
def fmix(a, b = 1, *c, &d); end
alias_method :a0, :f0
alias_method :a1, :f1
alias_method :aopt, :fopt
alias_method :arest, :frest
alias_method :ablk, :fblk
alias_method :amix, :fmix
}
cases = [
# name, parameters, arity
[:a0, [], 0],
[:a1, [[:req, :a]], 1],
[:aopt, [[:req, :a], [:opt, :b]], -2],
[:arest, [[:req, :a], [:rest, :b]], -2],
[:ablk, [[:req, :a], [:block, :b]], 1],
[:amix, [[:req, :a], [:opt, :b], [:rest, :c], [:block, :d]], -2],
]
cases.each do |name, params, arity|
u = c.instance_method(name)
assert_equal params, u.parameters
assert_equal arity, u.arity
# the bound Method goes through the same proc paths and must agree
b = c.new.method(name)
assert_equal params, b.parameters
assert_equal arity, b.arity
end
# alias must equal the original it points at (parameters and arity)
{ a0: :f0, a1: :f1, aopt: :fopt, arest: :frest, ablk: :fblk, amix: :fmix }.each do |al, orig|
assert_equal c.instance_method(orig).parameters, c.instance_method(al).parameters
assert_equal c.instance_method(orig).arity, c.instance_method(al).arity
end

# Alias-of-an-alias collapses to one proc at creation; must still resolve.
chain = Class.new {
def orig(x, y) end
alias_method :a1, :orig
alias_method :a2, :a1
}
assert_equal [[:req, :x], [:req, :y]], chain.instance_method(:a2).parameters
assert_equal 2, chain.instance_method(:a2).arity

# Aliasing a C method does NOT create an alias proc (it reuses the original
# cfunc method), so it must behave exactly like the original and never crash.
c2 = Class.new(String) { alias_method :up2, :upcase }
assert_equal String.instance_method(:upcase).parameters,
c2.instance_method(:up2).parameters
assert_equal String.instance_method(:upcase).arity,
c2.instance_method(:up2).arity
end

assert 'Method/UnboundMethod on C-defined (native) methods' do
# C methods have no irep: arity/parameters come from the packed argument spec
# (caspec) and source_location is always nil. This exercises the
# MRB_PROC_CFUNC_P branches of mrb_proc_arity / mrb_proc_parameters and the
# cfunc path of method_search_vm -- none of which the suite covered before.

# source_location is nil for genuinely C-defined methods (not mrblib ones).
assert_nil "x".method(:upcase).source_location
assert_nil 1.method(:+).source_location
assert_nil [].method(:push).source_location
assert_nil String.instance_method(:upcase).source_location

# arity: documented values for these core C methods.
assert_equal 0, "x".method(:upcase).arity # no args
assert_equal 1, 1.method(:+).arity # one required
assert_equal(-1, [].method(:push).arity) # variadic (rest)
assert_equal(-1, [].method(:first).arity) # optional

# parameters: always an Array of Arrays, never crashes; a no-arg C method
# gives []. C-method parameter *kinds* are an approximation (names are absent,
# and required args surface as :opt for non-strict procs), so we assert the
# stable shape rather than pinning exact kind labels -- except :rest, which is
# meaningful: a variadic C method must expose a rest parameter.
assert_equal [], "x".method(:upcase).parameters
[1.method(:+), [].method(:push), {}.method(:[]), [].method(:first)].each do |m|
ps = m.parameters
assert_true ps.is_a?(Array)
ps.each { |p| assert_true p.is_a?(Array) }
end
assert_true [].method(:push).parameters.any? { |entry| entry[0] == :rest }

# identity / metadata on a C method.
m = "abc".method(:upcase)
assert_equal String, m.owner
assert_equal :upcase, m.name
assert_equal "abc", m.receiver
assert_equal "#<Method: String#upcase>", m.to_s
assert_equal "#<UnboundMethod: String#upcase>", String.instance_method(:upcase).to_s

# behaviour: the C function actually runs via call / [] / bind / bind_call /
# unbind+rebind.
assert_equal 5, 2.method(:+).call(3)
assert_equal 5, 2.method(:+)[3]
assert_equal 5, Integer.instance_method(:+).bind_call(2, 3)
assert_equal 5, Integer.instance_method(:+).bind(2).call(3)
assert_equal 11, 5.method(:+).unbind.bind(10).call(1)

# eql?: equal only when bound to the SAME receiver object and same definition.
s = "cat"
assert_true s.method(:upcase) == s.method(:upcase)
assert_false s.method(:upcase) == "cat".method(:upcase) # distinct receivers
assert_false s.method(:upcase) == s.method(:downcase)

# super_method resolves across C methods (Integer#to_s -> BasicObject#to_s).
sm = 5.method(:to_s).super_method
assert_false sm.nil?
assert_equal :to_s, sm.name

# binding a C UnboundMethod to an incompatible receiver still raises cleanly.
assert_raise(TypeError) { String.instance_method(:upcase).bind(42) }
end
11 changes: 11 additions & 0 deletions mrbgems/mruby-proc-ext/src/proc.c
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,17 @@ mrb_proc_parameters(mrb_state *mrb, mrb_value self)
};
int i;
const struct RProc *proc = mrb_proc_ptr(self);
/* An alias proc carries no irep of its own: body.mid holds the aliased
method's name and `upper` points at the original proc (see mrb_alias_method
in class.c). Without this, the else branch below reads body.mid as an
mrb_irep* and dereferences it -> misaligned read / SEGV. Resolve to the
underlying proc, exactly as method_to_s already does, so an aliased method
reports the original's parameters. Alias chains are collapsed at creation,
but loop (as mruby-method does) and bail to empty on a broken chain. */
while (MRB_PROC_ALIAS_P(proc)) {
proc = proc->upper;
if (!proc) return mrb_ary_new(mrb);
}
Comment on lines +192 to +195

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

While resolving aliases in mrb_proc_parameters successfully prevents a SEGV here, proc_inspect (lines 103–132) in the same file suffers from the exact same vulnerability. In proc_inspect, if !MRB_PROC_CFUNC_P(p) is true, it directly accesses p->body.irep and passes it to mrb_debug_get_position. For an alias proc, body.irep is actually body.mid (a symbol), which will cause a misaligned read or SEGV when dereferenced.

Please consider resolving aliases in proc_inspect as well, for example:

  while (p && MRB_PROC_ALIAS_P(p)) {
    p = p->upper;
  }

mrb_aspec aspec;
mrb_bool has_lv = TRUE;
if (MRB_PROC_CFUNC_P(proc)) {
Expand Down
9 changes: 9 additions & 0 deletions src/proc.c
Original file line number Diff line number Diff line change
Expand Up @@ -419,6 +419,15 @@ mrb_proc_arity(const struct RProc *p)
mrb_aspec aspec;
int ma, op, ra, pa, arity;

/* Resolve alias procs first: an alias carries body.mid (a symbol), not an
irep, with `upper` pointing at the original proc. Without this the irep
branch below reads body.mid as an mrb_irep* and dereferences it -> SEGV.
Mirrors the guard already present in mrb_proc_source_location / mrb_proc_eql. */
while (p && MRB_PROC_ALIAS_P(p)) {
p = p->upper;
}
if (!p) return 0;
Comment on lines +426 to +429

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Resolving aliases in mrb_proc_arity is a great fix. However, there are other functions in src/proc.c that access proc->body.irep directly without resolving aliases:

  1. mrb_proc_local_variables (lines 469–498) accesses proc->body.irep directly in its loop. If an alias proc is passed, it will treat body.mid (a symbol) as an mrb_irep* and dereference irep->lv, leading to a SEGV.
  2. mrb_proc_copy (lines 276–290) accesses b->body.irep and calls mrb_irep_incref on it if !MRB_PROC_CFUNC_P(b) is true. For an alias proc, this will corrupt memory by incrementing a reference count on a symbol value.

Please consider adding alias resolution guards to mrb_proc_local_variables and mrb_proc_copy to prevent these potential crashes and memory corruptions.


if (MRB_PROC_CFUNC_P(p)) {
uint32_t caspec_bits = p->flags & MRB_PROC_CASPEC_MASK;
if (caspec_bits != 0) {
Expand Down
Loading