Bug #22019: Set#intersect () segv if the block is called after return

• Status: Open
• Assignee: jhawthorn (John Hawthorn)
• Target version: -
• Backport: 3.3: UNKNOWN, 3.4: UNKNOWN, 4.0: UNKNOWN

[jhawthorn (John Hawthorn)]

class C
  include Enumerable
  def each(&b)
    $b = b
    yield 1
  end
end

Set[1, 2, 3] & C.new
$b.call(1) # [BUG] Segmentation fault at 0x00007f21bfa67f60

The cause is essentially the same as #5801, we're initializing an ifunc pointing to a stack-allocated struct. The solution I think is to only ever use rb_block_call with a GC managed object like an imemo_memo.

In addition to set_intersection_block this likely also affects, lazy_flat_map_i, nmin_i, enum_sum_i, and product_each_i all of which are passed a stack buffer.

It might also be helpful to prevent the ifunc from being called after return. Some iterators (ex. sort_by) will raise a runtime error when this happens, but it's done ad-hoc (and detection depends on the variable still being accessible).

[jhawthorn (John Hawthorn)]

• Related to Bug #5801: Enumerable#take_while の proc を外に出して使うと Segv added

[shugo (Shugo Maeda)]

jhawthorn (John Hawthorn) wrote:

It might also be helpful to prevent the ifunc from being called after return. Some iterators (ex. sort_by) will raise a runtime error when this happens, but it's done ad-hoc (and detection depends on the variable still being accessible).

I've created a pull request for a comprehensive fix: https://github.com/ruby/ruby/pull/17777

When rb_iterate0() returns, and the ifunc's data points into the machine stack of the current execution context, ifunc->func is replaced with a stub that raises a RuntimeError:

$ ./tool/runruby.rb t.rb
t.rb:10:in '<main>': iterator block was called after iteration ended (RuntimeError)

The only user-visible change is that invoking a captured block after the iterating method has returned now raises a RuntimeError instead of crashing. This is consistent with the existing "sort_by reentered" guard; the existing callcc-based "reentered" tests pass unchanged.

[shugo (Shugo Maeda)]

shugo (Shugo Maeda) wrote in #note-2:

I've created a pull request for a comprehensive fix: https://github.com/ruby/ruby/pull/17777

When rb_iterate0() returns, and the ifunc's data points into the machine stack of the current execution context, ifunc->func is replaced with a stub that raises a RuntimeError:

As John commented on the pull request, the above heuristic check of ifunc's data doesn't work because it's not ASAN-safe and data can be a non-VALUE immediate value which looks like a stack pointer.

So I gave up the heuristic approach, and added the following internal functions:

• rb_block_call_noescape()
• rb_check_block_call_noescape()

And I've converted the vulnerable call sites to them.

Always passing a GC managed object would be more natural as an API, but I chose to just forbid calling the block after return, for two reasons:

• There seem to be few use cases for it: the converted call sites have crashed on such calls ever since they were introduced, and nobody has complained.
• Even if we made them memory-safe by passing a GC managed object, the behavior would still be semantically questionable, e.g., calling the block captured from Set#& afterwards would mutate the already-returned Set.

By the way, while working on this pull request I hit a code generation bug of clang 17 with -O1 -fPIC: the local variable cfp of rb_iterate0(), which is never changed after setjmp(), gets clobbered across longjmp(), so breaks escaped rb_iterate0() and btest failed with "unexpected break". The pull request works around it by making the locals read after longjmp() volatile. Since the bug is latent in rb_iterate0() on master (any innocent change to the function can change the stack slot allocation and trigger it, as mine did), it may be worth applying the volatile for cfp even if this pull request is not merged.

@jhawthorn (John Hawthorn)
I hadn't noticed that this issue is assigned to you. If you were planning to fix it yourself, please feel free to ignore my pull request.

[shugo (Shugo Maeda)]

It might be helpful to add the stack-pointer detection as a debug-only assertion (not to gate behavior): https://github.com/shugo/ruby/pull/129

It is ASAN-aware, reusing the fake-stack helpers the conservative GC already uses. Built-in and extension libraries, including bundled gems, don't seem to pass a non-VALUE immediate as data2, so enabling the assertion on CI would be safe (the ASAN job would need to be built with -DRUBY_DEBUG to exercise the fake-stack path).