From 774f13aecbe1502bed7f9fb5254604d1d5c0be75 Mon Sep 17 00:00:00 2001
From: Cameron Gutman <aicommander@gmail.com>
Date: Mon, 14 Aug 2023 19:31:51 -0500
Subject: [PATCH] Fix race condition inserting new process into our job object

Before this fix, it could spawn a child that would escape our tracking if it ran before we added it to our job object.
---
 src/platform/windows/misc.cpp | 39 ++++++++++++++++++++++++-----------
 1 file changed, 27 insertions(+), 12 deletions(-)

diff --git a/src/platform/windows/misc.cpp b/src/platform/windows/misc.cpp
index f7480855..4b0f7020 100644
--- a/src/platform/windows/misc.cpp
+++ b/src/platform/windows/misc.cpp
@@ -40,6 +40,11 @@
   #define UDP_SEND_MSG_SIZE 2
 #endif
 
+// PROC_THREAD_ATTRIBUTE_JOB_LIST is currently missing from MinGW headers
+#ifndef PROC_THREAD_ATTRIBUTE_JOB_LIST
+  #define PROC_THREAD_ATTRIBUTE_JOB_LIST ProcThreadAttributeValue(13, FALSE, TRUE, FALSE)
+#endif
+
 #include <qos2.h>
 
 #ifndef WLAN_API_MAKE_VERSION
@@ -411,11 +416,10 @@ namespace platf {
    * @param cmd The command that was used to launch the process.
    * @param ec A reference to an `std::error_code` object that will store any error that occurred during the launch.
    * @param process_info A reference to a `PROCESS_INFORMATION` structure that contains information about the new process.
-   * @param group A pointer to a `bp::group` object that will add the new process to its group, if not null.
    * @return A `bp::child` object representing the new process, or an empty `bp::child` object if the launch failed.
    */
   bp::child
-  create_boost_child_from_results(bool process_launched, const std::string &cmd, std::error_code &ec, PROCESS_INFORMATION &process_info, bp::group *group) {
+  create_boost_child_from_results(bool process_launched, const std::string &cmd, std::error_code &ec, PROCESS_INFORMATION &process_info) {
     // Use RAII to ensure the process is closed when we're done with it, even if there was an error.
     auto close_process_handles = util::fail_guard([process_launched, process_info]() {
       if (process_launched) {
@@ -432,11 +436,6 @@ namespace platf {
     if (process_launched) {
       // If the launch was successful, create a new bp::child object representing the new process
       auto child = bp::child((bp::pid_t) process_info.dwProcessId);
-      if (group) {
-        // If a group was provided, add the new process to the group
-        group->add(child);
-      }
-
       BOOST_LOG(info) << cmd << " running with PID "sv << child.id();
       return child;
     }
@@ -491,17 +490,18 @@ namespace platf {
   /**
    * @brief A function to create a `STARTUPINFOEXW` structure for launching a process.
    * @param file A pointer to a `FILE` object that will be used as the standard output and error for the new process, or null if not needed.
+   * @param job A job object handle to insert the new process into. This pointer must remain valid for the life of this startup info!
    * @param ec A reference to a `std::error_code` object that will store any error that occurred during the creation of the structure.
    * @return A `STARTUPINFOEXW` structure that contains information about how to launch the new process.
    */
   STARTUPINFOEXW
-  create_startup_info(FILE *file, std::error_code &ec) {
+  create_startup_info(FILE *file, HANDLE *job, std::error_code &ec) {
     // Initialize a zeroed-out STARTUPINFOEXW structure and set its size
     STARTUPINFOEXW startup_info = {};
     startup_info.StartupInfo.cb = sizeof(startup_info);
 
-    // Allocate a process attribute list with space for 1 element
-    startup_info.lpAttributeList = allocate_proc_thread_attr_list(1);
+    // Allocate a process attribute list with space for 2 elements
+    startup_info.lpAttributeList = allocate_proc_thread_attr_list(2);
     if (startup_info.lpAttributeList == NULL) {
       // If the allocation failed, set ec to an appropriate error code and return the structure
       ec = std::make_error_code(std::errc::not_enough_memory);
@@ -532,6 +532,20 @@ namespace platf {
         NULL);
     }
 
+    if (job) {
+      // Atomically insert the new process into the specified job.
+      //
+      // Note: The value we point to here must be valid for the lifetime of the attribute list,
+      // so we take a HANDLE* instead of just a HANDLE to use the caller's stack storage.
+      UpdateProcThreadAttribute(startup_info.lpAttributeList,
+        0,
+        PROC_THREAD_ATTRIBUTE_JOB_LIST,
+        job,
+        sizeof(*job),
+        NULL,
+        NULL);
+    }
+
     return startup_info;
   }
 
@@ -557,7 +571,8 @@ namespace platf {
     std::wstring wcmd = converter.from_bytes(cmd);
     std::wstring start_dir = converter.from_bytes(working_dir.string());
 
-    STARTUPINFOEXW startup_info = create_startup_info(file, ec);
+    HANDLE job = group ? group->native_handle() : nullptr;
+    STARTUPINFOEXW startup_info = create_startup_info(file, job ? &job : nullptr, ec);
     PROCESS_INFORMATION process_info;
 
     // Clone the environment to create a local copy. Boost.Process (bp) shares the environment with all spawned processes.
@@ -649,7 +664,7 @@ namespace platf {
     }
 
     // Use the results of the launch to create a bp::child object
-    return create_boost_child_from_results(ret, cmd, ec, process_info, group);
+    return create_boost_child_from_results(ret, cmd, ec, process_info);
   }
 
   /**