From 5c1182b73897dcde24a6d5e42295189b6625ee20 Mon Sep 17 00:00:00 2001 From: Lukasz Samson Date: Tue, 11 Jul 2023 18:14:39 +0200 Subject: [PATCH] Handle errors in setting breakpoints Disallow setting breakpoints in Inspect protocol implementations Disallow setting breakpoints in builtin protocols and JasonV.Encoder protocol Fixes https://github.com/elixir-lsp/elixir-ls/issues/900 Fixes https://github.com/elixir-lsp/elixir-ls/issues/903 Fixes https://github.com/elixir-lsp/elixir-ls/issues/942 --- .../elixir_ls_debugger/lib/debugger/server.ex | 72 ++++++++++++--- .../elixir_ls_debugger/test/debugger_test.exs | 92 +++++++++++++++++++ 2 files changed, 151 insertions(+), 13 deletions(-) diff --git a/apps/elixir_ls_debugger/lib/debugger/server.ex b/apps/elixir_ls_debugger/lib/debugger/server.ex index 9611952be..66d7b5f2c 100644 --- a/apps/elixir_ls_debugger/lib/debugger/server.ex +++ b/apps/elixir_ls_debugger/lib/debugger/server.ex @@ -335,7 +335,7 @@ defmodule ElixirLS.Debugger.Server do result = case current[{m, f, a}] do nil -> - case :int.ni(m) do + case interpret(m, false) do {:module, _} -> ModuleInfoCache.store(m) breaks_before = :int.all_breaks(m) @@ -344,16 +344,21 @@ defmodule ElixirLS.Debugger.Server do "Setting function breakpoint in #{inspect(m)}.#{f}/#{a}" ) - :ok = :int.break_in(m, f, a) - breaks_after = :int.all_breaks(m) - lines = for {{^m, line}, _} <- breaks_after -- breaks_before, do: line + case :int.break_in(m, f, a) do + :ok -> + breaks_after = :int.all_breaks(m) + lines = for {{^m, line}, _} <- breaks_after -- breaks_before, do: line - # pass nil as log_message - not supported on function breakpoints as of DAP 1.51 - update_break_condition(m, lines, condition, nil, hit_count) + # pass nil as log_message - not supported on function breakpoints as of DAP 1.51 + update_break_condition(m, lines, condition, nil, hit_count) - {:ok, lines} + {:ok, lines} - _ -> + {:error, :function_not_found} -> + {:error, "Function #{inspect(m)}.#{f}/#{a} not found"} + end + + :error -> {:error, "Cannot interpret module #{inspect(m)}"} end @@ -1153,8 +1158,7 @@ defmodule ElixirLS.Debugger.Server do defp save_and_reload(module, beam_bin) do :ok = File.write(Path.join(@temp_beam_dir, to_string(module) <> ".beam"), beam_bin) true = :code.delete(module) - Output.debugger_console("Interpreting module #{inspect(module)}") - {:module, _} = :int.ni(module) + {:module, _} = interpret(module) ModuleInfoCache.store(module) end @@ -1193,6 +1197,9 @@ defmodule ElixirLS.Debugger.Server do set_breakpoint(modules_to_break, path, line) end end + rescue + error -> + for _line <- lines, do: {:error, Exception.format_exit(error)} end defp set_breakpoint([], path, {line, _}) do @@ -1202,10 +1209,11 @@ defmodule ElixirLS.Debugger.Server do defp set_breakpoint(modules, path, {line, {condition, log_message, hit_count}}) do modules_with_breakpoints = Enum.reduce(modules, [], fn module, added -> - case :int.ni(module) do + case interpret(module, false) do {:module, _} -> ModuleInfoCache.store(module) Output.debugger_console("Setting breakpoint in #{inspect(module)} #{path}:#{line}") + # no need to handle errors here, it can fail only with {:error, :break_exists} :int.break(module, line) update_break_condition(module, line, condition, log_message, hit_count) @@ -1237,8 +1245,7 @@ defmodule ElixirLS.Debugger.Server do defp interpret_module(mod) do try do - Output.debugger_console("Interpreting module #{inspect(mod)}") - {:module, _} = :int.ni(mod) + {:module, _} = interpret(mod) ModuleInfoCache.store(mod) catch _, _ -> @@ -1397,4 +1404,43 @@ defmodule ElixirLS.Debugger.Server do "step" end end + + @exclude_protocols_from_interpreting [ + Enumerable, + Collectable, + List.Chars, + String.Chars, + Inspect, + IEx.Info, + JasonV.Encoder + ] + + @exclude_implementations_from_interpreting [Inspect] + + defp interpret(module, print_message? \\ true) + + defp interpret(module, _print_message?) when module in @exclude_protocols_from_interpreting do + :error + end + + defp interpret(module, print_message?) do + if Code.ensure_loaded?(module) do + module_behaviours = + module.module_info(:attributes) |> Keyword.get_values(:behaviour) |> Enum.concat() + + if Enum.any?(@exclude_implementations_from_interpreting, &(&1 in module_behaviours)) do + # debugger uses Inspect protocol and setting breakpoints in implementations leads to deadlocks + # https://github.com/elixir-lsp/elixir-ls/issues/903 + :error + else + if print_message? do + Output.debugger_console("Interpreting module #{inspect(module)}") + end + + :int.ni(module) + end + else + :error + end + end end diff --git a/apps/elixir_ls_debugger/test/debugger_test.exs b/apps/elixir_ls_debugger/test/debugger_test.exs index 41c040b5b..6e5ed1908 100644 --- a/apps/elixir_ls_debugger/test/debugger_test.exs +++ b/apps/elixir_ls_debugger/test/debugger_test.exs @@ -891,6 +891,54 @@ defmodule ElixirLS.Debugger.ServerTest do end) end + @tag :fixture + test "handles invalid request", %{server: server} do + in_fixture(__DIR__, "mix_project", fn -> + Server.receive_packet(server, initialize_req(1, %{})) + assert_receive(response(_, 1, "initialize", _)) + + Server.receive_packet( + server, + launch_req(2, %{ + "request" => "launch", + "type" => "mix_task", + "task" => "test", + "projectDir" => File.cwd!(), + # disable auto interpret + "debugAutoInterpretAllModules" => false + }) + ) + + assert_receive(response(_, 2, "launch", _), 3000) + assert_receive(event(_, "initialized", %{}), 5000) + + refute :hello in :int.interpreted() + abs_path = Path.absname("src/hello.erl1") + + Server.receive_packet( + server, + set_breakpoints_req(3, %{"path" => abs_path}, [%{"line" => 5}]) + ) + + assert_receive( + response(_, 3, "setBreakpoints", %{"breakpoints" => [%{"verified" => false}]}), + 3000 + ) + + abs_path = Path.absname("lib/mix_project.ex1") + + Server.receive_packet( + server, + set_breakpoints_req(4, %{"path" => abs_path}, [%{"line" => 3}]) + ) + + assert_receive( + response(_, 4, "setBreakpoints", %{"breakpoints" => [%{"verified" => false}]}), + 3000 + ) + end) + end + @tag :fixture test "sets and unsets breakpoints in elixir modules", %{server: server} do in_fixture(__DIR__, "mix_project", fn -> @@ -1671,6 +1719,50 @@ defmodule ElixirLS.Debugger.ServerTest do end) end + test "handles invalid requests", %{server: server} do + in_fixture(__DIR__, "mix_project", fn -> + Server.receive_packet(server, initialize_req(1, %{})) + assert_receive(response(_, 1, "initialize", _)) + + Server.receive_packet( + server, + launch_req(2, %{ + "request" => "launch", + "type" => "mix_task", + "task" => "test", + "projectDir" => File.cwd!(), + # disable auto interpret + "debugAutoInterpretAllModules" => false + }) + ) + + assert_receive(response(_, 2, "launch", _), 3000) + assert_receive(event(_, "initialized", %{}), 5000) + + refute :hello in :int.interpreted() + + Server.receive_packet( + server, + set_function_breakpoints_req(3, [%{"name" => ":hello1.hello_world/0"}]) + ) + + assert_receive( + response(_, 3, "setFunctionBreakpoints", %{"breakpoints" => [%{"verified" => false}]}), + 3000 + ) + + Server.receive_packet( + server, + set_function_breakpoints_req(4, [%{"name" => ":hello.hello_world1/0"}]) + ) + + assert_receive( + response(_, 4, "setFunctionBreakpoints", %{"breakpoints" => [%{"verified" => false}]}), + 3000 + ) + end) + end + test "sets, modifies and unsets conditional function breakpoints", %{server: server} do in_fixture(__DIR__, "mix_project", fn -> Server.receive_packet(server, initialize_req(1, %{}))