From 0b1af1ace405628f4f506fab6ab01b0b0f1e8db4 Mon Sep 17 00:00:00 2001
From: ridiculousfish <corydoras@ridiculousfish.com>
Date: Tue, 10 Dec 2019 18:32:56 -0800
Subject: [PATCH] Correct the use of the constructed pointer in job lineage

This was always being set to a different pointer. Ensure the root job
shares its constructed pointer with its children.
---
 src/exec.cpp |  4 ++--
 src/proc.cpp | 12 +++++++++++-
 src/proc.h   | 22 +++++++++++++---------
 3 files changed, 26 insertions(+), 12 deletions(-)

diff --git a/src/exec.cpp b/src/exec.cpp
index 04b34f47f..e3869955e 100644
--- a/src/exec.cpp
+++ b/src/exec.cpp
@@ -313,7 +313,7 @@ void internal_exec(env_stack_t &vars, job_t *j, const io_chain_t &all_ios) {
         // launch_process _never_ returns.
         launch_process_nofork(vars, j->processes.front().get());
     } else {
-        j->mut_flags().constructed = true;
+        j->mark_constructed();
         j->processes.front()->completed = true;
         return;
     }
@@ -1225,7 +1225,7 @@ bool exec_job(parser_t &parser, shared_ptr<job_t> j) {
     FLOGF(exec_job_exec, L"Executed job %d from command '%ls' with pgrp %d", j->job_id,
           j->command_wcstr(), j->pgid);
 
-    j->mut_flags().constructed = true;
+    j->mark_constructed();
     if (!j->is_foreground()) {
         parser.vars().set_one(L"last_pid", ENV_GLOBAL, to_string(j->pgid));
     }
diff --git a/src/proc.cpp b/src/proc.cpp
index 603ce08c7..372be6d61 100644
--- a/src/proc.cpp
+++ b/src/proc.cpp
@@ -269,7 +269,12 @@ void process_t::check_generations_before_launch() {
 }
 
 job_t::job_t(job_id_t job_id, const properties_t &props, job_lineage_t lineage)
-    : properties(props), job_lineage(std::move(lineage)), job_id(job_id) {}
+    : properties(props), job_lineage(std::move(lineage)), job_id(job_id) {
+    if (!job_lineage.root_constructed) {
+        // We are the root job, share our constructed pointer.
+        job_lineage.root_constructed = this->constructed;
+    }
+}
 
 job_t::~job_t() { release_job_id(job_id); }
 
@@ -282,6 +287,11 @@ io_chain_t job_t::all_io_redirections() const {
     return result;
 }
 
+void job_t::mark_constructed() {
+    assert(!is_constructed() && "Job was already constructed");
+    *constructed = true;
+}
+
 using process_generation_count_t = unsigned int;
 
 /// A list of pids/pgids that have been disowned. They are kept around until either they exit or
diff --git a/src/proc.h b/src/proc.h
index 5dc918dab..067847f16 100644
--- a/src/proc.h
+++ b/src/proc.h
@@ -278,9 +278,8 @@ struct job_lineage_t {
     io_chain_t block_io{};
 
     /// A shared pointer indicating that the entire tree of jobs is safe to disown.
-    /// This is set by the "root" job after it is constructed.
-    std::shared_ptr<relaxed_atomic_bool_t> root_constructed{
-        std::make_shared<relaxed_atomic_bool_t>(false)};
+    /// This is set to true by the "root" job after it is constructed.
+    std::shared_ptr<relaxed_atomic_bool_t> root_constructed{};
 };
 
 /// A struct represeting a job. A job is basically a pipeline of one or more processes and a couple
@@ -310,7 +309,7 @@ class job_t {
     wcstring command_str;
 
     // The lineage associated with the job.
-    const job_lineage_t job_lineage;
+    job_lineage_t job_lineage;
 
     // No copying.
     job_t(const job_t &rhs) = delete;
@@ -382,6 +381,12 @@ class job_t {
     /// stops.
     struct termios tmodes {};
 
+    /// Whether the specified job is completely constructed, i.e. completely parsed, and every
+    /// process in the job has been forked, etc.
+    /// This is a shared_ptr because it may be passed to child jobs through the lineage.
+    const std::shared_ptr<relaxed_atomic_bool_t> constructed =
+        std::make_shared<relaxed_atomic_bool_t>(false);
+
     /// Flags associated with the job.
     struct flags_t {
         /// Whether the user has been told about stopped job.
@@ -390,10 +395,6 @@ class job_t {
         /// Whether this job is in the foreground.
         bool foreground{false};
 
-        /// Whether the specified job is completely constructed, i.e. completely parsed, and every
-        /// process in the job has been forked, etc.
-        bool constructed{false};
-
         /// Whether the exit status should be negated. This flag can only be set by the not builtin.
         bool negate{false};
 
@@ -426,9 +427,12 @@ class job_t {
     /// Fetch all the IO redirections associated with the job.
     io_chain_t all_io_redirections() const;
 
+    /// Mark this job as constructed. The job must not have previously been marked as constructed.
+    void mark_constructed();
+
     // Helper functions to check presence of flags on instances of jobs
     /// The job has been fully constructed, i.e. all its member processes have been launched
-    bool is_constructed() const { return flags().constructed; }
+    bool is_constructed() const { return *constructed; }
     /// The job was launched in the foreground and has control of the terminal
     bool is_foreground() const { return flags().foreground; }
     /// The job is complete, i.e. all its member processes have been reaped