File 0380-Don-t-enqueue-system-tasks-if-target-process-is-in-f.patch of Package erlang
From 8ca58cf82e430775ab1c2adfab31f77f649c3f96 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?John=20H=C3=B6gberg?= <john@erlang.org>
Date: Thu, 14 Jun 2018 08:26:07 +0200
Subject: [PATCH 4/4] Don't enqueue system tasks if target process is in
fail_state
The fail state wasn't re-checked in the state change loop; only
the FREE state was checked. In addition to that, we would leave
the task in the queue when bailing out which could lead to a
double-free.
This commit backports active_sys_enqueue from master to make it
easier to merge onwards.
---
erts/emulator/beam/erl_process.c | 228 ++++++++++++++++++-----------------
erts/emulator/test/process_SUITE.erl | 54 ++++++++-
2 files changed, 171 insertions(+), 111 deletions(-)
diff --git a/erts/emulator/beam/erl_process.c b/erts/emulator/beam/erl_process.c
index b0db2da243..dad4006beb 100644
--- a/erts/emulator/beam/erl_process.c
+++ b/erts/emulator/beam/erl_process.c
@@ -7156,129 +7156,72 @@ erts_schedule_process(Process *p, erts_aint32_t state, ErtsProcLocks locks)
schedule_process(p, state, locks);
}
-static int
-schedule_process_sys_task(Process *p, erts_aint32_t prio, ErtsProcSysTask *st,
- erts_aint32_t *fail_state_p)
-{
- int res;
- int locked;
- ErtsProcSysTaskQs *stqs, *free_stqs;
- erts_aint32_t fail_state, state, a, n, enq_prio;
+/* Enqueues the given sys task on the process and schedules it. The task may be
+ * NULL if only scheduling is desired. */
+static ERTS_INLINE erts_aint32_t
+active_sys_enqueue(Process *p, ErtsProcSysTask *sys_task,
+ erts_aint32_t task_prio, erts_aint32_t enable_flags,
+ erts_aint32_t state, erts_aint32_t *fail_state_p)
+{
+ int runnable_procs = erts_system_profile_flags.runnable_procs;
+ erts_aint32_t n, a, enq_prio, fail_state;
+ int already_scheduled;
+ int status_locked;
int enqueue; /* < 0 -> use proxy */
- unsigned int prof_runnable_procs;
+ enable_flags |= ERTS_PSFLG_ACTIVE_SYS;
fail_state = *fail_state_p;
-
- res = 1; /* prepare for success */
- st->next = st->prev = st; /* Prep for empty prio queue */
- state = erts_smp_atomic32_read_nob(&p->state);
- prof_runnable_procs = erts_system_profile_flags.runnable_procs;
- locked = 0;
- free_stqs = NULL;
- if (state & ERTS_PSFLG_ACTIVE_SYS)
- stqs = NULL;
- else {
- alloc_qs:
- stqs = proc_sys_task_queues_alloc();
- stqs->qmask = 1 << prio;
- stqs->ncount = 0;
- stqs->q[PRIORITY_MAX] = NULL;
- stqs->q[PRIORITY_HIGH] = NULL;
- stqs->q[PRIORITY_NORMAL] = NULL;
- stqs->q[PRIORITY_LOW] = NULL;
- stqs->q[prio] = st;
- }
-
- if (!locked) {
- locked = 1;
- erts_smp_proc_lock(p, ERTS_PROC_LOCK_STATUS);
-
- state = erts_smp_atomic32_read_nob(&p->state);
- if (state & fail_state) {
- *fail_state_p = (state & fail_state);
- free_stqs = stqs;
- res = 0;
- goto cleanup;
- }
- }
-
- if (!p->sys_task_qs) {
- if (stqs)
- p->sys_task_qs = stqs;
- else
- goto alloc_qs;
- }
- else {
- free_stqs = stqs;
- stqs = p->sys_task_qs;
- if (!stqs->q[prio]) {
- stqs->q[prio] = st;
- stqs->qmask |= 1 << prio;
- }
- else {
- st->next = stqs->q[prio];
- st->prev = stqs->q[prio]->prev;
- st->next->prev = st;
- st->prev->next = st;
- ASSERT(stqs->qmask & (1 << prio));
- }
- }
-
- if (ERTS_PSFLGS_GET_ACT_PRIO(state) > prio) {
- erts_aint32_t n, a, e;
- /* Need to elevate actual prio */
-
- a = state;
- do {
- if (ERTS_PSFLGS_GET_ACT_PRIO(a) <= prio) {
- n = a;
- break;
- }
- n = e = a;
- n &= ~ERTS_PSFLGS_ACT_PRIO_MASK;
- n |= (prio << ERTS_PSFLGS_ACT_PRIO_OFFSET);
- a = erts_smp_atomic32_cmpxchg_nob(&p->state, n, e);
- } while (a != e);
- state = n;
- }
-
-
- a = state;
+ already_scheduled = 0;
+ status_locked = 0;
enq_prio = -1;
+ a = state;
- /* Status lock prevents out of order "runnable proc" trace msgs */
- ERTS_SMP_LC_ASSERT(ERTS_PROC_LOCK_STATUS & erts_proc_lc_my_proc_locks(p));
+ ERTS_SMP_LC_ASSERT(!(ERTS_PROC_LOCK_STATUS & erts_proc_lc_my_proc_locks(p)));
+ ASSERT(fail_state & (ERTS_PSFLG_EXITING | ERTS_PSFLG_FREE));
+ ASSERT(!(fail_state & enable_flags));
+ ASSERT(!(state & ERTS_PSFLG_PROXY));
- if (!prof_runnable_procs) {
- erts_smp_proc_unlock(p, ERTS_PROC_LOCK_STATUS);
- locked = 0;
+ /* When runnable_procs is enabled, we need to take the status lock to
+ * prevent trace messages from being sent in the wrong order. The lock must
+ * be held over the call to add2runq.
+ *
+ * Otherwise, we only need to take it when we're enqueuing a task and can
+ * safely release it before add2runq. */
+ if (sys_task || runnable_procs) {
+ erts_smp_proc_lock(p, ERTS_PROC_LOCK_STATUS);
+ status_locked = 1;
}
- ASSERT(!(state & ERTS_PSFLG_PROXY));
-
while (1) {
erts_aint32_t e;
n = e = a;
- if (a & ERTS_PSFLG_FREE)
- goto cleanup; /* We don't want to schedule free processes... */
+ if (a & fail_state) {
+ *fail_state_p = a & fail_state;
+ goto cleanup;
+ }
enqueue = ERTS_ENQUEUE_NOT;
- n |= ERTS_PSFLG_ACTIVE_SYS;
+ n |= enable_flags;
+
if (!(a & (ERTS_PSFLG_RUNNING
| ERTS_PSFLG_RUNNING_SYS
| ERTS_PSFLG_DIRTY_RUNNING
- | ERTS_PSFLG_DIRTY_RUNNING_SYS)))
+ | ERTS_PSFLG_DIRTY_RUNNING_SYS))) {
enqueue = check_enqueue_in_prio_queue(p, &enq_prio, &n, a);
+ }
+
a = erts_smp_atomic32_cmpxchg_mb(&p->state, n, e);
- if (a == e)
+ if (a == e) {
break;
- if (a == n && enqueue == ERTS_ENQUEUE_NOT)
- goto cleanup;
+ }
+ else if (a == n && enqueue == ERTS_ENQUEUE_NOT) {
+ already_scheduled = 1;
+ break;
+ }
}
- if (prof_runnable_procs) {
-
+ if (!already_scheduled && runnable_procs) {
if (!(a & (ERTS_PSFLG_ACTIVE_SYS
| ERTS_PSFLG_RUNNING
| ERTS_PSFLG_RUNNING_SYS
@@ -7288,24 +7231,89 @@ schedule_process_sys_task(Process *p, erts_aint32_t prio, ErtsProcSysTask *st,
/* We activated a prevously inactive process */
profile_runnable_proc(p, am_active);
}
+ }
- erts_smp_proc_unlock(p, ERTS_PROC_LOCK_STATUS);
- locked = 0;
+ if (sys_task) {
+ ErtsProcSysTaskQs *stqs = p->sys_task_qs;
+
+ if (!stqs) {
+ sys_task->next = sys_task->prev = sys_task;
+
+ stqs = proc_sys_task_queues_alloc();
+
+ stqs->qmask = 1 << task_prio;
+ stqs->ncount = 0;
+ stqs->q[PRIORITY_MAX] = NULL;
+ stqs->q[PRIORITY_HIGH] = NULL;
+ stqs->q[PRIORITY_NORMAL] = NULL;
+ stqs->q[PRIORITY_LOW] = NULL;
+ stqs->q[task_prio] = sys_task;
+
+ p->sys_task_qs = stqs;
+ }
+ else {
+ if (!stqs->q[task_prio]) {
+ sys_task->next = sys_task->prev = sys_task;
+
+ stqs->q[task_prio] = sys_task;
+ stqs->qmask |= 1 << task_prio;
+ }
+ else {
+ sys_task->next = stqs->q[task_prio];
+ sys_task->prev = stqs->q[task_prio]->prev;
+ sys_task->next->prev = sys_task;
+ sys_task->prev->next = sys_task;
+ ASSERT(stqs->qmask & (1 << task_prio));
+ }
+ }
+ }
+
+ if (status_locked && !runnable_procs) {
+ erts_smp_proc_unlock(p, ERTS_PROC_LOCK_STATUS);
+ status_locked = 0;
}
- add2runq(enqueue, enq_prio, p, n, NULL);
+ if (!already_scheduled) {
+ add2runq(enqueue, enq_prio, p, n, NULL);
+ }
cleanup:
+ if (status_locked) {
+ erts_smp_proc_unlock(p, ERTS_PROC_LOCK_STATUS);
+ }
- if (locked)
- erts_smp_proc_unlock(p, ERTS_PROC_LOCK_STATUS);
+ return n;
+}
+
+static int
+schedule_process_sys_task(Process *p, erts_aint32_t prio, ErtsProcSysTask *st,
+ erts_aint32_t *fail_state_p)
+{
+ erts_aint32_t fail_state, state;
- if (free_stqs)
- proc_sys_task_queues_free(free_stqs);
+ /* Elevate priority if needed. */
+ state = erts_smp_atomic32_read_nob(&p->state);
+ if (ERTS_PSFLGS_GET_ACT_PRIO(state) > prio) {
+ erts_aint32_t n, a, e;
- ERTS_SMP_LC_ASSERT(!(ERTS_PROC_LOCK_STATUS & erts_proc_lc_my_proc_locks(p)));
+ a = state;
+ do {
+ if (ERTS_PSFLGS_GET_ACT_PRIO(a) <= prio) {
+ n = a;
+ break;
+ }
+ n = e = a;
+ n &= ~ERTS_PSFLGS_ACT_PRIO_MASK;
+ n |= (prio << ERTS_PSFLGS_ACT_PRIO_OFFSET);
+ a = erts_smp_atomic32_cmpxchg_nob(&p->state, n, e);
+ } while (a != e);
- return res;
+ state = n;
+ }
+
+ fail_state = *fail_state_p;
+
+ return !(active_sys_enqueue(p, st, prio, 0, state, fail_state_p) & fail_state);
}
static ERTS_INLINE int
diff --git a/erts/emulator/test/process_SUITE.erl b/erts/emulator/test/process_SUITE.erl
index a8bcfac84d..899a5c26bd 100644
--- a/erts/emulator/test/process_SUITE.erl
+++ b/erts/emulator/test/process_SUITE.erl
@@ -58,6 +58,7 @@
no_priority_inversion2/1,
system_task_blast/1,
system_task_on_suspended/1,
+ system_task_failed_enqueue/1,
gc_request_when_gc_disabled/1,
gc_request_blast_when_gc_disabled/1]).
-export([prio_server/2, prio_client/2, init/1, handle_event/2]).
@@ -104,7 +105,7 @@ groups() ->
otp_7738_resume]},
{system_task, [],
[no_priority_inversion, no_priority_inversion2,
- system_task_blast, system_task_on_suspended,
+ system_task_blast, system_task_on_suspended, system_task_failed_enqueue,
gc_request_when_gc_disabled, gc_request_blast_when_gc_disabled]}].
init_per_suite(Config) ->
@@ -2531,6 +2532,57 @@ system_task_on_suspended(Config) when is_list(Config) ->
ok
end.
+%% When a system task couldn't be enqueued due to the process being in an
+%% incompatible state, it would linger in the system task list and get executed
+%% anyway the next time the process was scheduled. This would result in a
+%% double-free at best.
+%%
+%% This test continuously purges modules while other processes run dirty code,
+%% which will provoke this error as ERTS_PSTT_CPC can't be enqueued while a
+%% process is running dirty code.
+system_task_failed_enqueue(Config) when is_list(Config) ->
+ case erlang:system_info(dirty_cpu_schedulers) of
+ N when N > 0 ->
+ system_task_failed_enqueue_1(Config);
+ _ ->
+ {skipped, "No dirty scheduler support"}
+ end.
+
+system_task_failed_enqueue_1(Config) ->
+ Priv = proplists:get_value(priv_dir, Config),
+
+ Purgers = [spawn_link(fun() -> purge_loop(Priv, Id) end)
+ || Id <- lists:seq(1, erlang:system_info(schedulers))],
+ Hogs = [spawn_link(fun() -> dirty_loop() end)
+ || _ <- lists:seq(1, erlang:system_info(dirty_cpu_schedulers))],
+
+ ct:sleep(5000),
+
+ [begin
+ unlink(Pid),
+ exit(Pid, kill)
+ end || Pid <- (Purgers ++ Hogs)],
+
+ ok.
+
+purge_loop(PrivDir, Id) ->
+ Mod = "failed_enq_" ++ integer_to_list(Id),
+ Path = PrivDir ++ "/" ++ Mod,
+ file:write_file(Path ++ ".erl",
+ "-module('" ++ Mod ++ "').\n" ++
+ "-export([t/0]).\n" ++
+ "t() -> ok."),
+ purge_loop_1(Path).
+purge_loop_1(Path) ->
+ {ok, Mod} = compile:file(Path, []),
+ erlang:delete_module(Mod),
+ erts_code_purger:purge(Mod),
+ purge_loop_1(Path).
+
+dirty_loop() ->
+ ok = erts_debug:dirty_cpu(reschedule, 10000),
+ dirty_loop().
+
gc_request_when_gc_disabled(Config) when is_list(Config) ->
AIS = erts_debug:set_internal_state(available_internal_state, true),
gc_request_when_gc_disabled_do(ref),
--
2.16.4
