recon_trace protection improvements

.github/workflows/ci.yml

@@ -15,7 +15,7 @@ jobs:
       image: erlang:${{matrix.otp_vsn}}
     strategy:
       matrix:
-        otp_vsn: ['20.3', '21.3', '22.3', '23.3', '24.3', '25.3', '26.0.2', '27.0.1']
+        otp_vsn: ['20.3', '21.3', '22.3', '23.3', '24.3', '25.3', '26.0.2', '27.0.1', '28.4']
         os: [ubuntu-latest]
     steps:
       - uses: actions/checkout@v2

src/recon_lib.erl

@@ -18,6 +18,8 @@
 %% private exports
 -export([binary_memory/1]).
 
+-dialyzer({nowarn_function, [fold_processes/2, fold_processes_iter/3]}).
+
 -type diff() :: [recon:proc_attrs() | recon:inet_attrs()].
 
 %% @doc Compare two samples and return a list based on some key. The type mentioned

src/recon_trace.erl

@@ -337,13 +337,15 @@ calls(TSpecs = [_|_], Max) ->
 calls({Mod, Fun, Args}, Max, Opts) ->
     calls([{Mod,Fun,Args}], Max, Opts);
 calls(TSpecs = [_|_], {Max, Time}, Opts) ->
+    IOServer = validate_io_server(Opts),
     Pid = setup(rate_tracer, [Max, Time],
-                validate_formatter(Opts), validate_io_server(Opts)),
-    trace_calls(TSpecs, Pid, Opts);
+                validate_formatter(Opts), IOServer),
+    trace_calls(TSpecs, Pid, IOServer, Opts);
 calls(TSpecs = [_|_], Max, Opts) ->
+    IOServer = validate_io_server(Opts),
     Pid = setup(count_tracer, [Max],
-                validate_formatter(Opts), validate_io_server(Opts)),
-    trace_calls(TSpecs, Pid, Opts).
+                validate_formatter(Opts), IOServer),
+    trace_calls(TSpecs, Pid, IOServer, Opts).
 
 %%%%%%%%%%%%%%%%%%%%%%%
 %%% PRIVATE EXPORTS %%%
@@ -421,10 +423,12 @@ setup(TracerFun, TracerArgs, FormatterFun, IOServer) ->
     end.
 
 %% Sets the traces in action
-trace_calls(TSpecs, Pid, Opts) ->
+trace_calls(TSpecs, Pid, IOServer, Opts) ->
     {PidSpecs, TraceOpts, MatchOpts} = validate_opts(Opts),
+    UnsafePids = build_unsafe_pids(Pid, IOServer),
+    PidsOnly = lists:all(fun(P) -> is_safe_pid(P, UnsafePids) end, PidSpecs),
     Matches = [begin
-                {Arity, Spec} = validate_tspec(Mod, Fun, Args),
+                {Arity, Spec} = validate_tspec(Mod, Fun, Args, PidsOnly),
                 erlang:trace_pattern({Mod, Fun, Arity}, Spec, MatchOpts)
                end || {Mod, Fun, Args} <- TSpecs],
     [erlang:trace(PidSpec, true, [call, {tracer, Pid} | TraceOpts])
@@ -480,18 +484,19 @@ validate_pid_specs(PidTerm) ->
     %% has to be `recon:pid_term()'.
     [recon_lib:term_to_pid(PidTerm)].
 
-validate_tspec(Mod, Fun, Args) when is_function(Args) ->
-    validate_tspec(Mod, Fun, fun_to_ms(Args));
+validate_tspec(Mod, Fun, Args, PidsOnly) when is_function(Args) ->
+    validate_tspec(Mod, Fun, fun_to_ms(Args), PidsOnly);
 %% helper to save typing for common actions
-validate_tspec(Mod, Fun, return_trace) ->
-    validate_tspec(Mod, Fun, [{'_', [], [{return_trace}]}]);
-validate_tspec(Mod, Fun, Args) ->
+validate_tspec(Mod, Fun, return_trace, PidsOnly) ->
+    validate_tspec(Mod, Fun, [{'_', [], [{return_trace}]}], PidsOnly);
+validate_tspec(Mod, Fun, Args, PidsOnly) ->
     BannedMods = ['_', ?MODULE, io, lists],
     %% The banned mod check can be bypassed by using
     %% match specs if you really feel like being dumb.
-    case {lists:member(Mod, BannedMods), Args} of
+    case {lists:member(Mod, BannedMods) andalso not PidsOnly, Args} of
         {true, '_'} -> error({dangerous_combo, {Mod,Fun,Args}});
         {true, []} -> error({dangerous_combo, {Mod,Fun,Args}});
+        {true, [{'_', [], [{return_trace}]}]} -> error({dangerous_combo, {Mod,Fun,Args}});
         _ -> ok
     end,
     case Args of
@@ -500,6 +505,17 @@ validate_tspec(Mod, Fun, Args) ->
         _ when Args >= 0, Args =< 255 -> {Args, true}
     end.
 
+is_safe_pid(P, Unsafe) when is_pid(P) -> not lists:member(P, Unsafe);
+is_safe_pid(_, _) -> false.
+
+resolve_io_server(Pid) when is_pid(Pid) -> Pid;
+resolve_io_server(Name) when is_atom(Name) -> whereis(Name).
+
+build_unsafe_pids(TracerPid, IOServer) ->
+    IOPid = resolve_io_server(IOServer),
+    FormatterPid = whereis(recon_trace_formatter),
+    [P || P <- [self(), TracerPid, FormatterPid, IOPid], is_pid(P)].
+
 validate_formatter(Opts) ->
     case proplists:get_value(formatter, Opts) of
         F when is_function(F, 1) -> F;

test/recon_trace_SUITE.erl

@@ -0,0 +1,52 @@
+-module(recon_trace_SUITE).
+-include_lib("common_test/include/ct.hrl").
+-include_lib("eunit/include/eunit.hrl").
+-compile(export_all).
+
+all() -> [dangerous_combo, dangerous_combo_specific_pids,
+          dangerous_combo_self_pid, dangerous_combo_io_server_pid].
+
+%%%%%%%%%%%%%
+%%% TESTS %%%
+%%%%%%%%%%%%%
+
+dangerous_combo(_Config) ->
+    ?assertError({dangerous_combo, {'_', '_', '_'}},
+                  recon_trace:calls([{'_', '_', '_'}], 1)),
+    ?assertError({dangerous_combo, {'_', '_', []}},
+                  recon_trace:calls([{'_', '_', []}], 1)),
+    ?assertError({dangerous_combo, {io, '_', []}},
+                  recon_trace:calls([{io, '_', []}], 1)),
+    ?assertError({dangerous_combo, {lists, '_', []}},
+                  recon_trace:calls([{lists, '_', []}], 1)),
+    ?assertError({dangerous_combo, {'_', '_', [{'_', [], [{return_trace}]}]}},
+                  recon_trace:calls([{'_', '_', return_trace}], 1)),
+    ?assertError({dangerous_combo, {io, '_', [{'_', [], [{return_trace}]}]}},
+                  recon_trace:calls([{io, '_', return_trace}], 1)),
+    ?assertError({dangerous_combo, {lists, '_', [{'_', [], [{return_trace}]}]}},
+                  recon_trace:calls([{lists, '_', return_trace}], 1)),
+    ?assertError({dangerous_combo, {recon_trace, '_', [{'_', [], [{return_trace}]}]}},
+                  recon_trace:calls([{recon_trace, '_', return_trace}], 1)).
+
+dangerous_combo_specific_pids(_Config) ->
+    Pid = spawn(fun() -> timer:sleep(infinity) end),
+    Pid2 = spawn(fun() -> timer:sleep(infinity) end),
+    try
+        recon_trace:calls([{'_', '_', '_'}], 1, [{pid, Pid}]),
+        recon_trace:calls([{'_', '_', '_'}], 1, [{pid, [Pid, Pid2]}]),
+        recon_trace:clear()
+    after
+        exit(Pid, kill),
+        exit(Pid2, kill)
+    end,
+    ?assertError({dangerous_combo, {'_', '_', '_'}},
+        recon_trace:calls([{'_', '_', '_'}], 1, [{pid, [new, Pid, Pid2]}])).
+
+dangerous_combo_self_pid(_Config) ->
+    ?assertError({dangerous_combo, {'_', '_', '_'}},
+        recon_trace:calls([{'_', '_', '_'}], 1, [{pid, self()}])).
+
+dangerous_combo_io_server_pid(_Config) ->
+    GL = group_leader(),
+    ?assertError({dangerous_combo, {'_', '_', '_'}},
+        recon_trace:calls([{'_', '_', '_'}], 1, [{pid, GL}])).