From c1ec06454ddbde863f5f15da51d9bc2e3eda415a Mon Sep 17 00:00:00 2001 From: Stan Lo Date: Thu, 26 Oct 2023 20:30:30 +0100 Subject: [PATCH] Unfreeze threads for object-evaluating commands There are several commands that evaluate objects and call methods on them, such as: - info - ls (outline) - trace object - display If the called method acquires a mutex shared with another thread (e.g. when using Timeout), then it'd cause a deadlock as all threads are stopped. For example, if there's an ActiveRecord Relation object stored as a local variable, and the user runs the `info` command, then it'd call `inspect` on it and trigger a database query, it could cause a deadlock as described in #877. This commit fixes the issue by unfreezing all threads before evaluating those commands and freezing them again after receiving the result event. --- lib/debug/session.rb | 32 +++++++++++++++++++------------- test/console/info_test.rb | 32 ++++++++++++++++++++++++++++++++ test/console/outline_test.rb | 31 +++++++++++++++++++++++++++++++ test/console/trace_test.rb | 32 +++++++++++++++++++++++++++++++- 4 files changed, 113 insertions(+), 14 deletions(-) diff --git a/lib/debug/session.rb b/lib/debug/session.rb index 191b4e4fd..7e1bf5a55 100644 --- a/lib/debug/session.rb +++ b/lib/debug/session.rb @@ -256,6 +256,11 @@ def request_tc(req) @tc << req end + def request_tc_with_freed_threads(req) + restart_all_threads + request_tc(req) + end + def process_event evt # variable `@internal_info` is only used for test tc, output, ev, @internal_info, *ev_args = evt @@ -314,7 +319,7 @@ def process_event evt if @displays.empty? wait_command_loop else - request_tc [:eval, :display, @displays] + request_eval :display, @displays end when :result raise "[BUG] not in subsession" if @subsession_stack.empty? @@ -329,6 +334,7 @@ def process_event evt end end + stop_all_threads when :method_breakpoint, :watch_breakpoint bp = ev_args[1] if bp @@ -342,6 +348,7 @@ def process_event evt obj_inspect = ev_args[2] opt = ev_args[3] add_tracer ObjectTracer.new(@ui, obj_id, obj_inspect, **opt) + stop_all_threads else stop_all_threads end @@ -810,15 +817,15 @@ def register_default_command case sub when nil - request_tc [:show, :default, pat] # something useful + request_tc_with_freed_threads [:show, :default, pat] # something useful when :locals - request_tc [:show, :locals, pat] + request_tc_with_freed_threads [:show, :locals, pat] when :ivars - request_tc [:show, :ivars, pat, opt] + request_tc_with_freed_threads [:show, :ivars, pat, opt] when :consts - request_tc [:show, :consts, pat, opt] + request_tc_with_freed_threads [:show, :consts, pat, opt] when :globals - request_tc [:show, :globals, pat] + request_tc_with_freed_threads [:show, :globals, pat] when :threads thread_list :retry @@ -838,7 +845,7 @@ def register_default_command # * Show you available methods and instance variables of the given object. # * If the object is a class/module, it also lists its constants. register_command 'outline', 'o', 'ls', unsafe: false do |arg| - request_tc [:show, :outline, arg] + request_tc_with_freed_threads [:show, :outline, arg] end # * `display` @@ -848,9 +855,9 @@ def register_default_command register_command 'display', postmortem: false do |arg| if arg && !arg.empty? @displays << arg - request_tc [:eval, :try_display, @displays] + request_eval :try_display, @displays else - request_tc [:eval, :display, @displays] + request_eval :display, @displays end end @@ -864,7 +871,7 @@ def register_default_command if @displays[n = $1.to_i] @displays.delete_at n end - request_tc [:eval, :display, @displays] + request_eval :display, @displays when nil if ask "clear all?", 'N' @displays.clear @@ -983,7 +990,7 @@ def register_default_command :retry when /\Aobject\s+(.+)/ - request_tc [:trace, :object, $1.strip, {pattern: pattern, into: into}] + request_tc_with_freed_threads [:trace, :object, $1.strip, {pattern: pattern, into: into}] when /\Aoff\s+(\d+)\z/ if t = @tracers.values[$1.to_i] @@ -1165,8 +1172,7 @@ def process_command line end def request_eval type, src - restart_all_threads - request_tc [:eval, type, src] + request_tc_with_freed_threads [:eval, type, src] end def step_command type, arg diff --git a/test/console/info_test.rb b/test/console/info_test.rb index c14fe5991..85508dd70 100644 --- a/test/console/info_test.rb +++ b/test/console/info_test.rb @@ -367,4 +367,36 @@ def test_ivar_abbrev end end end + + class InfoThreadLockingTest < ConsoleTestCase + def program + <<~RUBY + 1| th0 = Thread.new{sleep} + 2| $m = Mutex.new + 3| th1 = Thread.new do + 4| $m.lock + 5| sleep 1 + 6| $m.unlock + 7| end + 8| + 9| o = Object.new + 10| def o.inspect + 11| $m.lock + 12| "foo" + 13| end + 14| + 15| sleep 0.5 + 16| debugger + RUBY + end + + def test_info_doesnt_cause_deadlock + debug_code(program) do + type 'c' + type 'info' + assert_line_text(/%self = main/) + type 'c' + end + end + end end diff --git a/test/console/outline_test.rb b/test/console/outline_test.rb index f86350399..73b9a5481 100644 --- a/test/console/outline_test.rb +++ b/test/console/outline_test.rb @@ -67,4 +67,35 @@ def test_outline_aliases end end end + + class OutlineThreadLockingTest < ConsoleTestCase + def program + <<~RUBY + 1| th0 = Thread.new{sleep} + 2| $m = Mutex.new + 3| th1 = Thread.new do + 4| $m.lock + 5| sleep 1 + 6| $m.unlock + 7| end + 8| + 9| def self.constants # overriding constants is only one of the ways to cause deadlock with outline + 10| $m.lock + 11| [] + 12| end + 13| + 14| sleep 0.5 + 15| debugger + RUBY + end + + def test_outline_doesnt_cause_deadlock + debug_code(program) do + type 'c' + type 'ls' + assert_line_text(/locals: th0/) + type 'c' + end + end + end end diff --git a/test/console/trace_test.rb b/test/console/trace_test.rb index 71629e797..68e1df260 100644 --- a/test/console/trace_test.rb +++ b/test/console/trace_test.rb @@ -473,6 +473,36 @@ def test_block_doesnt_break_tracer end end + class ThreadLockingTest < ConsoleTestCase + def program + <<~RUBY + 1| th0 = Thread.new{sleep} + 2| $m = Mutex.new + 3| th1 = Thread.new do + 4| $m.lock + 5| sleep 1 + 6| $m.unlock + 7| end + 8| + 9| def inspect + 10| m.lock + 11| "" + 12| end + 13| + 14| sleep 0.5 + 15| debugger + RUBY + end + + def test_object_tracer_doesnt_cause_deadlock + debug_code(program) do + type 'c' + type 'trace object self' + type 'c' + end + end + end + class TraceCallReceiverTest < ConsoleTestCase def program <<~RUBY @@ -544,7 +574,7 @@ def program 6| p a RUBY end - + def test_1656237686 debug_code(program) do type 'trace line'