Don't remove a zombie from its pgroup or session before it's reaped

mehfooz09 · Feb 17, 2020 · dcb19ff · dcb19ff
1 parent db29e26
commit dcb19ff
Show file tree

Hide file tree

Showing 2 changed files with 18 additions and 4 deletions.
diff --git a/kernel/exit.c b/kernel/exit.c
@@ -13,13 +13,16 @@ static bool exit_tgroup(struct task *task) {
     list_remove(&task->group_links);
     bool group_dead = list_empty(&group->threads);
     if (group_dead) {
-        // don't need to lock the group since the only pointers it come from:
-        // - other threads current->group, but the other threads have accessed it
+        // don't need to lock the group since the only pointers to it come from:
+        // - other threads' current->group, but there are none left thanks to that list_empty call
         // - locking pids_lock first, which do_exit did
         if (group->timer)
             timer_free(group->timer);
-        task_leave_session(task);
-        list_remove(&group->pgroup);
+
+        // The group will be removed from its group and session by reap_if_zombie,
+        // because fish tries to set the pgid to that of an exited but not reaped
+        // task.
+        // https://github.com/Microsoft/WSL/issues/2786
     }
     return group_dead;
 }
@@ -191,8 +194,13 @@ static bool reap_if_zombie(struct task *task, addr_t status_addr, addr_t rusage_
             return _EFAULT;
 
     unlock(&task->group->lock);
+
+    // tear down group
     cond_destroy(&task->group->child_exit);
+    task_leave_session(task);
+    list_remove(&task->group->pgroup);
     free(task->group);
+
     task_destroy(task);
     return true;
 }

diff --git a/kernel/fork.c b/kernel/fork.c
@@ -148,7 +148,13 @@ dword_t sys_clone(dword_t flags, addr_t stack, addr_t ptid, addr_t tls, addr_t c
         return _ENOMEM;
     int err = copy_task(task, flags, stack, ptid, tls, ctid);
     if (err < 0) {
+        // FIXME: there is a window between task_create_ and task_destroy where
+        // some other thread could get a pointer to the task.
+        // FIXME: task_destroy doesn't free all aspects of the task, which
+        // could cause leaks
+        lock(&pids_lock);
         task_destroy(task);
+        unlock(&pids_lock);
         return err;
     }
     task->cpu.eax = 0;