From 394623bf081afbb4e02485f2e31365a8b8666ae8 Mon Sep 17 00:00:00 2001
From: Mahmoud Al-Qudsi <mqudsi@neosmart.net>
Date: Tue, 9 Apr 2019 23:29:58 -0500
Subject: [PATCH] Prevent `disown` from directly removing jobs

This prevents the `disown` builtin from directly removing jobs out of
the jobs list to prevent sanity issues, as `disown` may be called within
the context of a subjob (e.g. in a function or block) in which case the
parent job might not yet be done with the reference to the child job.

Instead, a flag is set and the parser removes the job from the list only
after the entire execution chain has completed.

Closes #5720.
---
 src/builtin_disown.cpp  | 18 +++++++-----------
 src/builtin_jobs.cpp    |  4 ++--
 src/parse_execution.cpp |  5 +++++
 src/proc.h              |  6 ++++++
 4 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/src/builtin_disown.cpp b/src/builtin_disown.cpp
index ad965b693..25654d138 100644
--- a/src/builtin_disown.cpp
+++ b/src/builtin_disown.cpp
@@ -17,7 +17,7 @@
 
 /// Helper for builtin_disown.
 static int disown_job(const wchar_t *cmd, parser_t &parser, io_streams_t &streams, job_t *j) {
-    if (j == 0) {
+    if (j == nullptr) {
         streams.err.append_format(_(L"%ls: Unknown job '%ls'\n"), L"bg");
         builtin_print_error_trailer(parser, streams.err, cmd);
         return STATUS_INVALID_ARGS;
@@ -31,17 +31,13 @@ static int disown_job(const wchar_t *cmd, parser_t &parser, io_streams_t &stream
         streams.err.append_format(fmt, cmd, j->job_id, j->command_wcstr());
     }
 
-    for (auto itr = jobs().begin(); itr != jobs().end(); ++itr) {
-        auto job = itr->get();
-        if (job == j) {
-            pid_t pgid = j->pgid;
-            add_disowned_pgid(pgid);
-            jobs().erase(itr);
-            return STATUS_CMD_OK;
-        }
-    }
+    // We cannot directly remove the job from the jobs() list as `disown` might be called
+    // within the context of a subjob which will cause the parent job to crash in exec_job().
+    // Instead, we set a flag and the parser removes the job from the jobs list later.
+    j->set_flag(job_flag_t::PENDING_REMOVAL, true);
+    add_disowned_pgid(j->pgid);
 
-    return STATUS_CMD_ERROR;
+    return STATUS_CMD_OK;
 }
 
 /// Builtin for removing jobs from the job list.
diff --git a/src/builtin_jobs.cpp b/src/builtin_jobs.cpp
index f4dab2411..3d083531d 100644
--- a/src/builtin_jobs.cpp
+++ b/src/builtin_jobs.cpp
@@ -175,7 +175,7 @@ int builtin_jobs(parser_t &parser, io_streams_t &streams, wchar_t **argv) {
     if (print_last) {
         // Ignore unconstructed jobs, i.e. ourself.
         for (const auto &j : jobs()) {
-            if (j->is_constructed() && !j->is_completed()) {
+            if (j->is_visible()) {
                 builtin_jobs_print(j.get(), mode, !streams.out_is_redirected, streams);
                 return STATUS_CMD_ERROR;
             }
@@ -217,7 +217,7 @@ int builtin_jobs(parser_t &parser, io_streams_t &streams, wchar_t **argv) {
         } else {
             for (const auto &j : jobs()) {
                 // Ignore unconstructed jobs, i.e. ourself.
-                if (j->is_constructed() && !j->is_completed()) {
+                if (j->is_visible()) {
                     builtin_jobs_print(j.get(), mode, !found && !streams.out_is_redirected, streams);
                     found = true;
                 }
diff --git a/src/parse_execution.cpp b/src/parse_execution.cpp
index 06ed5bf00..08ca4e658 100644
--- a/src/parse_execution.cpp
+++ b/src/parse_execution.cpp
@@ -1269,6 +1269,11 @@ parse_execution_result_t parse_execution_context_t::run_1_job(tnode_t<g::job> jo
             remove_job(job.get());
         }
 
+        // This job was disowned during its own execution or the execution of its subjobs
+        if (job->get_flag(job_flag_t::PENDING_REMOVAL)) {
+            remove_job(job.get());
+        }
+
         // Only external commands require a new fishd barrier.
         if (job_contained_external_command) {
             set_proc_had_barrier(false);
diff --git a/src/proc.h b/src/proc.h
index 351a30740..3b6186799 100644
--- a/src/proc.h
+++ b/src/proc.h
@@ -257,6 +257,10 @@ enum class job_flag_t {
     JOB_CONTROL,
     /// Whether the job wants to own the terminal when in the foreground.
     TERMINAL,
+    /// This job needs to be removed from the list of jobs for one reason or another (killed,
+    /// completed, disowned, etc). This flag is set rather than directly manipulating the jobs
+    /// list.
+    PENDING_REMOVAL,
 
     JOB_FLAG_COUNT
 };
@@ -396,6 +400,8 @@ class job_t {
     bool is_completed() const;
     /// The job is in a stopped state
     bool is_stopped() const;
+    /// The job is OK to be externally visible, e.g. to the user via `jobs`
+    bool is_visible() const { return !is_completed() && is_constructed() && !get_flag(job_flag_t::PENDING_REMOVAL); };
 
     /// \return the parent job, or nullptr.
     const std::shared_ptr<job_t> get_parent() const { return parent_job; }