Skip to content

Prefer th->ec for stack base/size. #13101

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Apr 17, 2025

Conversation

ioquatix
Copy link
Member

@ioquatix ioquatix commented Apr 11, 2025

Without this PR, the following program exits with SIGBUS on Darwin:

Thread.new do
  Fiber.new do
    # Consume more than the default stack:
    i = 0
    while true
      puts i: i
      Fiber.consume_stack(1024*i)
      i += 1
    end
  end.resume
end.join

Fiber.consume_stack(n) is equivalent to ALLOCA. So it eventually hits guard page.

Since Fiber will set th->ec stack base/size, we should prefer it before using get_stack which may return the original thread's stack details. For the purpose of computing a stack overflow, it is preferable to use the current Fiber's stack.

Test Output

Before PR

samuel@Sakura ~/D/i/ruby-darwin-check_overflow-fix (darwin-check_overflow-fix) [2]> make runruby
compiling thread.c
linking miniruby
ln -sf ../../rbconfig.rb .ext/arm64-darwin24/rbconfig.rb
builtin_binary.inc updated
1092d1a331c3be9c2b03728f93d002c1a71e7a20599d18ac60891c3d2023937e  builtin_binary.inc
compiling builtin.c
linking static-library libruby.3.5-static.a
linking ruby
ld: warning: ignoring duplicate libraries: '-ldl', '-lobjc', '-lpthread'
RUBY_ON_BUG='gdb -x ./.gdbinit -p' ./miniruby -I./lib -I. -I.ext/common  ./tool/runruby.rb --extout=.ext  -- --disable-gems  ./test.rb 
{i: 0}
{i: 1}
{i: 2}
{i: 3}
{i: 4}
{i: 5}
{i: 6}
{i: 7}
{i: 8}
{i: 9}
{i: 10}
{i: 11}
{i: 12}
{i: 13}
{i: 14}
{i: 15}
{i: 16}
{i: 17}
{i: 18}
{i: 19}
{i: 20}
{i: 21}
{i: 22}
{i: 23}
{i: 24}
{i: 25}
{i: 26}
{i: 27}
{i: 28}
{i: 29}
{i: 30}
{i: 31}
{i: 32}
{i: 33}
{i: 34}
{i: 35}
{i: 36}
{i: 37}
{i: 38}
{i: 39}
{i: 40}
{i: 41}
{i: 42}
{i: 43}
{i: 44}
{i: 45}
{i: 46}
{i: 47}
{i: 48}
{i: 49}
{i: 50}
{i: 51}
{i: 52}
{i: 53}
{i: 54}
{i: 55}
{i: 56}
{i: 57}
{i: 58}
{i: 59}
{i: 60}
{i: 61}
{i: 62}
{i: 63}
{i: 64}
{i: 65}
{i: 66}
{i: 67}
{i: 68}
{i: 69}
{i: 70}
{i: 71}
{i: 72}
{i: 73}
{i: 74}
{i: 75}
{i: 76}
{i: 77}
{i: 78}
{i: 79}
{i: 80}
{i: 81}
{i: 82}
{i: 83}
{i: 84}
{i: 85}
{i: 86}
{i: 87}
{i: 88}
{i: 89}
{i: 90}
{i: 91}
{i: 92}
{i: 93}
{i: 94}
{i: 95}
{i: 96}
{i: 97}
{i: 98}
{i: 99}
{i: 100}
{i: 101}
{i: 102}
{i: 103}
{i: 104}
{i: 105}
{i: 106}
{i: 107}
{i: 108}
{i: 109}
{i: 110}
{i: 111}
{i: 112}
{i: 113}
{i: 114}
{i: 115}
{i: 116}
{i: 117}
{i: 118}
{i: 119}
{i: 120}
{i: 121}
{i: 122}
{i: 123}
{i: 124}
{i: 125}
{i: 126}
{i: 127}
{i: 128}
{i: 129}
{i: 130}
{i: 131}
{i: 132}
{i: 133}
{i: 134}
{i: 135}
{i: 136}
{i: 137}
{i: 138}
{i: 139}
{i: 140}
{i: 141}
{i: 142}
{i: 143}
./test.rb:7: [BUG] Bus Error at 0x000000011a93bde0
ruby 3.5.0dev (2025-04-10T23:51:15Z darwin-check_overf.. 1f6edd3a8c) +PRISM [arm64-darwin24]

-- Crash Report log information --------------------------------------------
   See Crash Report log file in one of the following locations:             
     * ~/Library/Logs/DiagnosticReports                                     
     * /Library/Logs/DiagnosticReports                                      
   for more details.                                                        
Don't forget to include the above Crash Report log file in bug reports.     

sh: gdb: command not found
-- Control frame information -----------------------------------------------
c:0003 p:---- s:0012 e:000011 CFUNC  :consume_stack
c:0002 p:0023 s:0007 e:000006 BLOCK  ./test.rb:7 [FINISH]
c:0001 p:---- s:0003 e:000002 DUMMY  [FINISH]

-- Ruby level backtrace information ----------------------------------------
./test.rb:7:in 'block (2 levels) in <main>'
./test.rb:7:in 'consume_stack'

-- Threading information ---------------------------------------------------
Total ractor count: 1
Ruby thread count for this ractor: 2

-- Machine register context ------------------------------------------------
  x0: 0x0000000100bfac88  x1: 0x0000000000023c00  x2: 0x000000011a960040
  x3: 0x0000000100536ee0  x4: 0x000000011a960040  x5: 0x000000011a960038
  x6: 0x7d333431203a697b  x7: 0x0000000000000021 x18: 0x0000000000000000
 x19: 0x00000001467043e0 x20: 0x0000000100c15178 x21: 0x0000000100bfac88
 x22: 0x000000011a97ff90 x23: 0x0000000000000001 x24: 0x000000011a960040
 x25: 0x000000011a960038 x26: 0x0000000055550083 x27: 0x0000000146647010
 x28: 0x0000000000000000  lr: 0x0000000100536f28  fp: 0x000000011a95fa00
  sp: 0x000000011a95f9e0

-- C level backtrace information -------------------------------------------
SEGV received in BUS handler
make: *** [runruby] Abort trap: 6

After PR

samuel@Sakura ~/D/i/ruby-darwin-check_overflow-fix (darwin-check_overflow-fix) [2]> make runruby
compiling thread.c
linking miniruby
ln -sf ../../rbconfig.rb .ext/arm64-darwin24/rbconfig.rb
builtin_binary.inc updated
1092d1a331c3be9c2b03728f93d002c1a71e7a20599d18ac60891c3d2023937e  builtin_binary.inc
compiling builtin.c
linking static-library libruby.3.5-static.a
linking ruby
ld: warning: ignoring duplicate libraries: '-ldl', '-lobjc', '-lpthread'
RUBY_ON_BUG='gdb -x ./.gdbinit -p' ./miniruby -I./lib -I. -I.ext/common  ./tool/runruby.rb --extout=.ext  -- --disable-gems  ./test.rb 
{i: 0}
{i: 1}
{i: 2}
{i: 3}
{i: 4}
{i: 5}
{i: 6}
{i: 7}
{i: 8}
{i: 9}
{i: 10}
{i: 11}
{i: 12}
{i: 13}
{i: 14}
{i: 15}
{i: 16}
{i: 17}
{i: 18}
{i: 19}
{i: 20}
{i: 21}
{i: 22}
{i: 23}
{i: 24}
{i: 25}
{i: 26}
{i: 27}
{i: 28}
{i: 29}
{i: 30}
{i: 31}
{i: 32}
{i: 33}
{i: 34}
{i: 35}
{i: 36}
{i: 37}
{i: 38}
{i: 39}
{i: 40}
{i: 41}
{i: 42}
{i: 43}
{i: 44}
{i: 45}
{i: 46}
{i: 47}
{i: 48}
{i: 49}
{i: 50}
{i: 51}
{i: 52}
{i: 53}
{i: 54}
{i: 55}
{i: 56}
{i: 57}
{i: 58}
{i: 59}
{i: 60}
{i: 61}
{i: 62}
{i: 63}
{i: 64}
{i: 65}
{i: 66}
{i: 67}
{i: 68}
{i: 69}
{i: 70}
{i: 71}
{i: 72}
{i: 73}
{i: 74}
{i: 75}
{i: 76}
{i: 77}
{i: 78}
{i: 79}
{i: 80}
{i: 81}
{i: 82}
{i: 83}
{i: 84}
{i: 85}
{i: 86}
{i: 87}
{i: 88}
{i: 89}
{i: 90}
{i: 91}
{i: 92}
{i: 93}
{i: 94}
{i: 95}
{i: 96}
{i: 97}
{i: 98}
{i: 99}
{i: 100}
{i: 101}
{i: 102}
{i: 103}
{i: 104}
{i: 105}
{i: 106}
{i: 107}
{i: 108}
{i: 109}
{i: 110}
{i: 111}
{i: 112}
{i: 113}
{i: 114}
{i: 115}
{i: 116}
{i: 117}
{i: 118}
{i: 119}
{i: 120}
{i: 121}
{i: 122}
{i: 123}
{i: 124}
{i: 125}
{i: 126}
{i: 127}
{i: 128}
{i: 129}
{i: 130}
{i: 131}
{i: 132}
{i: 133}
{i: 134}
{i: 135}
{i: 136}
{i: 137}
{i: 138}
{i: 139}
{i: 140}
{i: 141}
{i: 142}
{i: 143}
#<Thread:0x000000010503d710 ./test.rb:1 run> terminated with exception (report_on_exception is true):
./test.rb:7:in 'Fiber.consume_stack': stack level too deep (SystemStackError)
  from ./test.rb:7:in 'block (2 levels) in <main>'
./test.rb:7:in 'Fiber.consume_stack': stack level too deep (SystemStackError)
  from ./test.rb:7:in 'block (2 levels) in <main>'
make: *** [runruby] Error 1
samuel@Sakura ~/D/i/ruby-darwin-check_overflow-fix (darwin-check_overflow-fix) [2]> 

@ioquatix ioquatix requested review from Copilot, ko1 and nobu April 11, 2025 00:11
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.

@ioquatix ioquatix force-pushed the darwin-check_overflow-fix branch 4 times, most recently from 35e9888 to 7a9a094 Compare April 16, 2025 03:02
Copy link

launchable-app bot commented Apr 16, 2025

All Tests passed!

✖️no tests failed ✔️61945 tests passed(2 flakes)

@ioquatix ioquatix force-pushed the darwin-check_overflow-fix branch from 7a9a094 to a47ca0f Compare April 16, 2025 05:45
@ioquatix ioquatix force-pushed the darwin-check_overflow-fix branch from a47ca0f to c632cc5 Compare April 16, 2025 13:26
@ioquatix ioquatix enabled auto-merge (squash) April 16, 2025 13:27
@nobu
Copy link
Member

nobu commented Apr 17, 2025

Retrying after the machine stack overflowed may not work well on all systems.
I think assert_separately will be safer.

@ioquatix ioquatix force-pushed the darwin-check_overflow-fix branch from c632cc5 to 89fa2d9 Compare April 17, 2025 03:15
@ioquatix ioquatix force-pushed the darwin-check_overflow-fix branch from e7abc44 to 4e32ab3 Compare April 17, 2025 08:04
@ioquatix
Copy link
Member Author

@nobu I plan to merge this PR, and skip Windows on the tests. We can follow up with another PR to fix Windows stack overflow handling.

@ioquatix ioquatix force-pushed the darwin-check_overflow-fix branch from 4e32ab3 to 00d4f38 Compare April 17, 2025 08:31
@ioquatix
Copy link
Member Author

It looks like it's also possible for the GC itself to try and scan "out of bounds" of the machine stack:

  | -- C level backtrace information -------------------------------------------
  | /home/runner/work/ruby/ruby/build/ruby(rb_print_backtrace+0x8) [0x55b1a92d5106] ../src/vm_dump.c:839
  | /home/runner/work/ruby/ruby/build/ruby(rb_vm_bugreport) ../src/vm_dump.c:1171
  | /home/runner/work/ruby/ruby/build/ruby(rb_bug_for_fatal_signal+0xf8) [0x55b1a928bd68] ../src/error.c:1130
  | /home/runner/work/ruby/ruby/build/ruby(sigsegv+0x46) [0x55b1a8ff3566] ../src/signal.c:933
  | /lib/x86_64-linux-gnu/libc.so.6(0x7fc4fd442520) [0x7fc4fd442520]
  | /home/runner/work/ruby/ruby/build/ruby(each_location+0x30) [0x55b1a8ef9630] ../src/gc.c:2293
  | /home/runner/work/ruby/ruby/build/ruby(each_location_ptr+0xc) [0x55b1a8ef993a] ../src/gc.c:2307
  | /home/runner/work/ruby/ruby/build/ruby(each_location_ptr) ../src/gc.c:2304
  | /home/runner/work/ruby/ruby/build/ruby(rb_gc_mark_machine_context) ../src/gc.c:2545

@ioquatix ioquatix merged commit c4ae6cb into ruby:master Apr 17, 2025
76 checks passed
@ioquatix ioquatix deleted the darwin-check_overflow-fix branch April 17, 2025 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants