From eb89e5eb5c789826480cb79c17b06588838cc41f Mon Sep 17 00:00:00 2001 From: Lukas Larsson Date: Mon, 7 Jun 2021 15:38:29 +0200 Subject: erts: Fix use after free for ErtsTracerNif* When doing `lookup_tracer_nif` the code assumes that the ErtsTracerNif struct will not be deallocated after the lookup. However, that is not true for unmanaged threads as thread progress may pass after the lookup and before the call to the tracer nif. So, we make sure to delay thread progress before we do the lookup to the ErtsTracerNif and undelay when done. As of the writing of this commit there are 3 different trace flags that can be triggered on a dirty scheduler: - garbage_collect - running - send --- erts/emulator/beam/erl_nif.c | 2 ++ erts/emulator/beam/erl_process.c | 9 ++++++++- erts/emulator/beam/erl_trace.c | 3 +++ erts/emulator/beam/module.c | 1 + erts/emulator/test/dirty_nif_SUITE.erl | 29 +++++++++++++++++++++++++++-- erts/emulator/test/gc_SUITE.erl | 29 ++++++++++++++++++++--------- 6 files changed, 61 insertions(+), 12 deletions(-) diff --git a/erts/emulator/beam/erl_nif.c b/erts/emulator/beam/erl_nif.c index ebef485b04..25b3ce22b3 100644 --- a/erts/emulator/beam/erl_nif.c +++ b/erts/emulator/beam/erl_nif.c @@ -850,6 +850,8 @@ int enif_send(ErlNifEnv* env, const ErlNifPid* to_pid, from = c_p ? c_p->common.id : am_undefined; if (!env || !env->tracee) { + /* This clause is taken when enif_send is called in a nif + that is not a erl_tracer nif. */ if (c_p && IS_TRACED_FL(c_p, F_TRACE_SEND)) { full_flush_env(env); diff --git a/erts/emulator/beam/erl_process.c b/erts/emulator/beam/erl_process.c index 1f464e2e5a..f6e30843fa 100644 --- a/erts/emulator/beam/erl_process.c +++ b/erts/emulator/beam/erl_process.c @@ -9970,9 +9970,12 @@ Process *erts_schedule(ErtsSchedulerData *esdp, Process *p, int calls) static void trace_schedule_in(Process *p, erts_aint32_t state) { + ErtsThrPrgrDelayHandle dhndl; ASSERT(IS_TRACED(p)); ERTS_LC_ASSERT(erts_proc_lc_my_proc_locks(p) == ERTS_PROC_LOCK_MAIN); + dhndl = erts_thr_progress_unmanaged_delay(); + /* Clear tracer if it has been removed */ if (erts_is_tracer_proc_enabled(p, ERTS_PROC_LOCK_MAIN, &p->common)) { @@ -9988,14 +9991,17 @@ trace_schedule_in(Process *p, erts_aint32_t state) if (IS_TRACED_FL(p, F_TRACE_CALLS)) erts_schedule_time_break(p, ERTS_BP_CALL_TIME_SCHEDULE_IN); } - + erts_thr_progress_unmanaged_continue(dhndl); } static void trace_schedule_out(Process *p, erts_aint32_t state) { + ErtsThrPrgrDelayHandle dhndl; ASSERT(IS_TRACED(p)); ERTS_LC_ASSERT(erts_proc_lc_my_proc_locks(p) == ERTS_PROC_LOCK_MAIN); + + dhndl = erts_thr_progress_unmanaged_delay(); if (IS_TRACED_FL(p, F_TRACE_CALLS) && !(state & ERTS_PSFLG_FREE)) erts_schedule_time_break(p, ERTS_BP_CALL_TIME_SCHEDULE_OUT); @@ -10012,6 +10018,7 @@ trace_schedule_out(Process *p, erts_aint32_t state) ARE_TRACE_FLAGS_ON(p, F_TRACE_SCHED_PROCS)) trace_sched(p, ERTS_PROC_LOCK_MAIN, am_out); } + erts_thr_progress_unmanaged_continue(dhndl); } static int diff --git a/erts/emulator/beam/erl_trace.c b/erts/emulator/beam/erl_trace.c index ae7084b7f4..f787d627b0 100644 --- a/erts/emulator/beam/erl_trace.c +++ b/erts/emulator/beam/erl_trace.c @@ -1342,6 +1342,7 @@ trace_gc(Process *p, Eterm what, Uint size, Eterm msg) Eterm* hp; Uint sz = 0; Eterm tup; + ErtsThrPrgrDelayHandle dhndl = erts_thr_progress_unmanaged_delay(); if (is_tracer_enabled(p, ERTS_PROC_LOCK_MAIN, &p->common, &tnif, TRACE_FUN_E_GC, what)) { @@ -1361,6 +1362,7 @@ trace_gc(Process *p, Eterm what, Uint size, Eterm msg) if (o_hp) erts_free(ERTS_ALC_T_TMP, o_hp); } + erts_thr_progress_unmanaged_continue(dhndl); } void @@ -2570,6 +2572,7 @@ lookup_tracer_nif(const ErtsTracer tracer) ErtsTracerNif tnif_tmpl; ErtsTracerNif *tnif; tnif_tmpl.module = ERTS_TRACER_MODULE(tracer); + ERTS_LC_ASSERT(erts_thr_progress_lc_is_delaying() || erts_get_scheduler_id() > 0); erts_rwmtx_rlock(&tracer_mtx); if ((tnif = hash_get(tracer_hash, &tnif_tmpl)) == NULL) { erts_rwmtx_runlock(&tracer_mtx); diff --git a/erts/emulator/beam/module.c b/erts/emulator/beam/module.c index 0642a06123..08d4f26d8d 100644 --- a/erts/emulator/beam/module.c +++ b/erts/emulator/beam/module.c @@ -133,6 +133,7 @@ erts_get_module(Eterm mod, ErtsCodeIndex code_ix) IndexTable* mod_tab; ASSERT(is_atom(mod)); + ERTS_LC_ASSERT(erts_get_scheduler_id() > 0 || erts_thr_progress_lc_is_delaying()); mod_tab = &module_tables[code_ix]; diff --git a/erts/emulator/test/dirty_nif_SUITE.erl b/erts/emulator/test/dirty_nif_SUITE.erl index 93d0ac392c..e56ad670cb 100644 --- a/erts/emulator/test/dirty_nif_SUITE.erl +++ b/erts/emulator/test/dirty_nif_SUITE.erl @@ -442,10 +442,10 @@ dirty_nif_send_traced(Config) when is_list(Config) -> {ok, Rcvr} = send_wait_from_dirty_nif(Rcvr), Parent ! {self(), sent} end), - 1 = erlang:trace(Sndr, true, [send]), + 1 = erlang:trace(Sndr, true, [send, running, exiting]), Start = erlang:monotonic_time(), Sndr ! {self(), go}, - receive {trace, Sndr, send, {ok, Rcvr}, Rcvr} -> ok end, + receive {Rcvr, received} -> ok end, End1 = erlang:monotonic_time(), Time1 = erlang:convert_time_unit(End1-Start, native, 1000), @@ -456,8 +456,33 @@ dirty_nif_send_traced(Config) when is_list(Config) -> Time2 = erlang:convert_time_unit(End2-Start, native, 1000), io:format("Time2: ~p milliseconds~n", [Time2]), true = Time2 >= 1900, + + %% Make sure that the send trace is + %% in between an in and and out trace + (fun F() -> + %% We got an in trace, look for out or send + {trace,Sndr,in,_} = recv_trace_from(Sndr), + case recv_trace_from(Sndr) of + {trace,Sndr,out,_} -> + %% We got an out, look for another in + F(); + {trace,Sndr,send,_,_} -> + %% We got a send, look for another out + {trace,Sndr,out,_} = recv_trace_from(Sndr), + ok + end + end)(), + ok. +recv_trace_from(Sndr) -> + receive + M when element(1, M) =:= trace; + element(1, M) =:= trace_ts, + element(2, M) =:= Sndr -> + M + end. + %% %% Internal... %% diff --git a/erts/emulator/test/gc_SUITE.erl b/erts/emulator/test/gc_SUITE.erl index f3942ef416..9d02102d04 100644 --- a/erts/emulator/test/gc_SUITE.erl +++ b/erts/emulator/test/gc_SUITE.erl @@ -209,6 +209,13 @@ minor_major_gc_option_self(_Config) -> Pid ! {gc, Ref, major} end, [gc_major_start, gc_major_end]), + %% Try as major dirty, the test process will self-trigger GC + check_gc_tracing_around( + fun(Pid, Ref) -> + Pid ! {gc, Ref, major} + end, [gc_major_start, gc_major_end], + lists:seq(1,128 * 1024)), + %% Try as minor, the test process will self-trigger GC check_gc_tracing_around( fun(Pid, Ref) -> @@ -252,17 +259,21 @@ minor_major_gc_option_async(_Config) -> %% it results in any unexpected messages or if the expected trace tags are not %% received. check_gc_tracing_around(Fun, ExpectedTraceTags) -> + check_gc_tracing_around(Fun, ExpectedTraceTags, []). +check_gc_tracing_around(Fun, ExpectedTraceTags, State) -> Ref = erlang:make_ref(), Pid = spawn( - fun Endless() -> - receive - {gc, Ref, Type} -> - erlang:garbage_collect(self(), [{type, Type}]) - after 100 -> - ok - end, - Endless() - end), + fun() -> + (fun Endless(S) -> + receive + {gc, Ref, Type} -> + erlang:garbage_collect(self(), [{type, Type}]) + after 100 -> + ok + end, + Endless(S) + end)(State) + end), erlang:garbage_collect(Pid, []), erlang:trace(Pid, true, [garbage_collection]), Fun(Pid, Ref), -- cgit v1.2.1