recon_trace protection improvements#119
Conversation
v0idpwn
force-pushed
the
recon_trace_protection_improvements
branch
from
0fe8d31 to
ec8dece
Compare
|
I also pushed a commit with an ignore on dialyzer, which was broken for pre-OTP 28 releases due to #118. I tried a conditional compilation approach but it only worked past OTP 21, and there's OTP 20 on CI, so I went with the ignore. I took the opportunity to add OTP 28 to the CI too. |
ferd
left a comment
ferd
left a comment
There was a problem hiding this comment.
I think this is worth it but I have a minor suggestion to avoid obvious deadly combinations.
| %% Sets the traces in action | ||
| trace_calls(TSpecs, Pid, Opts) -> | ||
| {PidSpecs, TraceOpts, MatchOpts} = validate_opts(Opts), | ||
| PidsOnly = lists:all(fun is_pid/1, PidSpecs), |
There was a problem hiding this comment.
If we're doing this, we might want to replace fun is_pid/1 by something like a call to fun(P) -> safe_is_pid(P, [Pid, self()]) end with a definition like:
safe_is_pid(P, Unsafe) when is_pid(P) -> not lists:member(P, Unsafe);
safe_is_pid(_, _) -> false.
this would allow all pids, but avoid tracing the tracer or the current process without limits by accident, which are obvious ones to capture, and if we find other risky processes to trace (eg. maybe the current io server would eventually be one) they could be added easily to the block list.
There was a problem hiding this comment.
Sounds good, will do!
There was a problem hiding this comment.
I've done it in 6a3779d . I included the recon formatter and the io server as resolved by recon.
2 participants
This PR proposes two improvements:
(1) Reject
:recon_trace:calls({_, _, return_trace}, 10)-- This was allowed, I believe due to an oversight(2) Accept
:recon_trace:calls(DangerousCombination, 10, [{pid, SomePid}])-- Most often, tracing every call from a specific pid is safe. I can think of a few situations where doing this would be dangerous, but these are exceptions.