refs qorelanguage/qore#4272 fixed a race condition in Process::wait(); the exit code is set from the async on_exit_handler() as well as after wait() if necessary

davidnich · davidnich · commit 764c539623aa · 2021-12-27T16:15:37.000+01:00
diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -8,7 +8,7 @@ option(USE_INTERNAL_BOOST "Enforce using internal copy of boost libraries" OFF)
 
 set (VERSION_MAJOR 1)
 set (VERSION_MINOR 0)
-set (VERSION_PATCH 2)
+set (VERSION_PATCH 3)
 
 # where to look first for cmake modules, before ${CMAKE_ROOT}/Modules/ is checked
 set(CMAKE_MODULE_PATH ${CMAKE_SOURCE_DIR}/cmake )
diff --git a/qore-process-module.spec b/qore-process-module.spec
@@ -44,7 +44,7 @@
 
 Summary: process module for Qore
 Name: qore-process-module
-Version: 1.0.2
+Version: 1.0.3
 Release: 1%{dist}
 License: LGPL
 Group: Development/Languages
@@ -97,5 +97,8 @@ rm -rf $RPM_BUILD_ROOT
 %doc COPYING README test/process.qtest test/test_cwd.q test/test_env.q test/test_false.q test/test_io.q test/test_output.q test/test_sleep.q test/test_true.q test/test_utf8.q
 
 %changelog
+* Mon Dec 27 2021 David Nichols <david.nichols@qoretechnologies.com>
+- updated to version 1.0.3
+
 * Fri Sep 17 2021 David Nichols <david.nichols@qoretechnologies.com>
 - initial spec file
diff --git a/src/process.qpp b/src/process.qpp
@@ -53,6 +53,10 @@ Version 1.0. (See http://www.boost.org/LICENSE_1_0.txt)
 
     @section process_relnotes Process Module Release History
 
+    @subsection process_v1_0_3 process v1.0.3
+    - fixed a race condition in @ref Qore::Process::Process::wait() "Process::wait()"
+      (<a href="https://github.com/qorelanguage/qore/issues/4272">issue 4272</a>)
+
     @subsection process_v1_0_2 process v1.0.2
     - updated builtin boost to 1.71
 
diff --git a/src/processpriv.cpp b/src/processpriv.cpp
@@ -69,7 +69,8 @@ ProcessPriv::ProcessPriv(pid_t pid, ExceptionSink* xsink) :
 // default I/O buffer size
 static constexpr unsigned process_buf_size = 4096;
 
-ProcessPriv::ProcessPriv(const char* command, const QoreListNode* arguments, const QoreHashNode *opts, ExceptionSink* xsink) :
+ProcessPriv::ProcessPriv(const char* command, const QoreListNode* arguments, const QoreHashNode *opts,
+        ExceptionSink* xsink) :
         m_asio_ctx(),
         m_in_pipe(m_asio_ctx),
         m_out_pipe(m_asio_ctx),
@@ -139,15 +140,16 @@ ProcessPriv::ProcessPriv(const char* command, const QoreListNode* arguments, con
 
     // launch child process
     try {
-        QoreProcessHandler handler(xsink,
-                                   on_success,
-                                   on_setup,
-                                   on_error,
-                                   on_fork_error,
-                                   on_exec_setup,
-                                   on_exec_error);
-
-        launchChild(p, exeArgs, env, cwd.c_str(), handler, stdoutFile, stderrFile, xsink);
+        handler = new QoreProcessHandler(xsink,
+            on_success,
+            on_setup,
+            on_error,
+            on_fork_error,
+            on_exec_setup,
+            on_exec_error,
+            exit_code);
+
+        launchChild(p, exeArgs, env, cwd.c_str(), stdoutFile, stderrFile, xsink);
     } catch (const std::exception& ex) {
         xsink->raiseException("PROCESS-CONSTRUCTOR-ERROR", ex.what());
     }
@@ -159,6 +161,9 @@ ProcessPriv::ProcessPriv(const char* command, const QoreListNode* arguments, con
 }
 
 ProcessPriv::~ProcessPriv() {
+    if (handler) {
+        delete handler;
+    }
     // in case the object is obliterated (exception in constructor), the destructor is not run
     delete m_process;
     assert(!bg_xsink);
@@ -524,19 +529,18 @@ void ProcessPriv::prepareClosures() {
 }
 
 void ProcessPriv::launchChild(boost::filesystem::path p,
-                             std::vector<std::string>& args,
-                             bp::environment env,
-                             const char* cwd,
-                             QoreProcessHandler& handler,
-                             FILE* stdoutFile,
-                             FILE* stderrFile,
-                             ExceptionSink* xsink) {
+        std::vector<std::string>& args,
+        bp::environment env,
+        const char* cwd,
+        FILE* stdoutFile,
+        FILE* stderrFile,
+        ExceptionSink* xsink) {
     if (stdoutFile && stderrFile) {
         m_process = new bp::child(bp::exe = p.string(),
                                   bp::args = args,
                                   bp::env = env,
                                   bp::start_dir = cwd,
-                                  handler,
+                                  *handler,
                                   bp::std_out > stdoutFile,
                                   bp::std_err > stderrFile,
                                   bp::std_in < m_in_pipe,
@@ -546,7 +550,7 @@ void ProcessPriv::launchChild(boost::filesystem::path p,
                                   bp::args = args,
                                   bp::env = env,
                                   bp::start_dir = cwd,
-                                  handler,
+                                  *handler,
                                   bp::std_out > stdoutFile,
                                   bp::std_err > m_err_pipe,
                                   bp::std_in < m_in_pipe,
@@ -556,7 +560,7 @@ void ProcessPriv::launchChild(boost::filesystem::path p,
                                   bp::args = args,
                                   bp::env = env,
                                   bp::start_dir = cwd,
-                                  handler,
+                                  *handler,
                                   bp::std_out > m_out_pipe,
                                   bp::std_err > stderrFile,
                                   bp::std_in < m_in_pipe,
@@ -566,7 +570,7 @@ void ProcessPriv::launchChild(boost::filesystem::path p,
                                   bp::args = args,
                                   bp::env = env,
                                   bp::start_dir = cwd,
-                                  handler,
+                                  *handler,
                                   bp::std_out > m_out_pipe,
                                   bp::std_err > m_err_pipe,
                                   bp::std_in < m_in_pipe,
@@ -608,7 +612,8 @@ void ProcessPriv::launchChild(boost::filesystem::path p,
 }
 
 int ProcessPriv::exitCode(ExceptionSink* xsink) {
-    if (!processCheck(xsink)) {        return -1;
+    if (!processCheck(xsink)) {
+        return -1;
     }
 
     return exit_code;
@@ -737,23 +742,24 @@ bool ProcessPriv::wait(ExceptionSink* xsink) {
         return true;
     }
 
+    //printd(5, "ProcessPriv::wait() valid: %d exit_code: %d\n", m_process->valid(), exit_code);
+
     try {
         if (m_process->valid()) {
             std::error_code ec;
             m_process->wait(ec);
-            if (ec) {
+            if (ec && ec != std::errc::no_child_process) {
                 xsink->raiseException("PROCESS-WAIT-ERROR", "cannot wait on process: %s", ec.message().c_str());
                 return false;
             }
 
-            // get exit code if possible
-            getExitCode(xsink);
+            if (exit_code == -1) {
+                // get exit code if possible
+                getExitCode(xsink);
+            }
 
             // rethrows any background exceptions
             finalizeStreams(xsink);
-        } else {
-            // invalidate the exit code since the return value of m_process->exit_code() is without any meaning now
-            exit_code = 0;
         }
         return true;
     } catch (const std::exception& ex) {
@@ -778,22 +784,21 @@ bool ProcessPriv::wait(int64 t, ExceptionSink* xsink) {
         if (m_process->valid()) {
             std::error_code ec;
             bool rv = m_process->wait_for(std::chrono::milliseconds(t), ec);
-            if (ec) {
+            if (ec && ec != std::errc::no_child_process) {
                 xsink->raiseException("PROCESS-WAIT-ERROR", "cannot wait on process: %s", ec.message().c_str());
                 return false;
             }
             if (rv) {
-                // get exit code if possible
-                getExitCode(xsink);
+                if (exit_code == -1) {
+                    // get exit code if possible
+                    getExitCode(xsink);
+                }
 
                 // rethrows any background exceptions
                 finalizeStreams(xsink);
 
                 return true;
             }
-        } else {
-            // invalidate the exit code since the return value of m_process->exit_code() is without any meaning now
-            exit_code = 0;
         }
         return false;
     } catch (const std::exception& ex) {
diff --git a/src/processpriv.h b/src/processpriv.h
@@ -146,7 +146,6 @@ class ProcessPriv : public AbstractPrivateData {
                               std::vector<std::string>& args,
                               bp::environment env,
                               const char* cwd,
-                              QoreProcessHandler& handler,
                               FILE* stdoutFile,
                               FILE* stderrFile,
                               ExceptionSink* xsink);
@@ -474,6 +473,9 @@ class ProcessPriv : public AbstractPrivateData {
         }
     };
 
+    //! async process shandler
+    QoreProcessHandler* handler = nullptr;
+
     //! Child process.
     bp::child* m_process = nullptr;
 
diff --git a/src/qoreprocesshandler.h b/src/qoreprocesshandler.h
@@ -1,7 +1,7 @@
 /*
     Qore Programming Language process Module
 
-    Copyright (C) 2003 - 2020 Qore Technologies, s.r.o.
+    Copyright (C) 2003 - 2021 Qore Technologies, s.r.o.
 
     Permission is hereby granted, free of charge, to any person obtaining a
     copy of this software and associated documentation files (the "Software"),
@@ -33,79 +33,60 @@
 namespace bp = boost::process;
 namespace ex = boost::process::extend;
 
-
 class QoreProcessHandler : public ex::async_handler {
 public:
-    QoreProcessHandler(ExceptionSink* xsink,
-                       ResolvedCallReferenceNode* on_success,
-                       ResolvedCallReferenceNode* on_setup,
-                       ResolvedCallReferenceNode* on_error,
-                       ResolvedCallReferenceNode* on_fork_error,
-                       ResolvedCallReferenceNode* on_exec_setup,
-                       ResolvedCallReferenceNode* on_exec_error) :
+    DLLLOCAL QoreProcessHandler(ExceptionSink* xsink,
+            ResolvedCallReferenceNode* on_success,
+            ResolvedCallReferenceNode* on_setup,
+            ResolvedCallReferenceNode* on_error,
+            ResolvedCallReferenceNode* on_fork_error,
+            ResolvedCallReferenceNode* on_exec_setup,
+            ResolvedCallReferenceNode* on_exec_error,
+            int& exit_code) :
         m_xsink(xsink),
         m_on_success(on_success, xsink),
         m_on_setup(on_setup, xsink),
         m_on_error(on_error, xsink),
         m_on_fork_error(on_fork_error, xsink),
         m_on_exec_setup(on_exec_setup, xsink),
-        m_on_exec_error(on_exec_error, xsink) {
+        m_on_exec_error(on_exec_error, xsink),
+        exit_code(exit_code) {
     }
 
-    QoreProcessHandler(QoreProcessHandler& old) :
-        m_xsink(old.m_xsink),
-        m_on_success(*old.m_on_success, old.m_xsink),
-        m_on_setup(*old.m_on_setup, old.m_xsink),
-        m_on_error(*old.m_on_error, old.m_xsink),
-        m_on_fork_error(*old.m_on_fork_error, old.m_xsink),
-        m_on_exec_setup(*old.m_on_exec_setup, old.m_xsink),
-        m_on_exec_error(*old.m_on_exec_error, old.m_xsink) {
-        if (m_on_success)
-            m_on_success->ref();
-        if (m_on_setup)
-            m_on_setup->ref();
-        if (m_on_error)
-            m_on_error->ref();
-        if (m_on_fork_error)
-            m_on_fork_error->ref();
-        if (m_on_exec_setup)
-            m_on_exec_setup->ref();
-        if (m_on_exec_error)
-            m_on_exec_error->ref();
-    }
+    QoreProcessHandler(QoreProcessHandler& old) = delete;
 
     template<typename Executor>
-    void on_success(Executor& exec) const {
+    DLLLOCAL void on_success(Executor& exec) const {
         call("on_success", *m_on_success, exec, std::error_code());
     }
 
     template<typename Executor>
-    void on_setup(Executor& exec) const {
+    DLLLOCAL void on_setup(Executor& exec) const {
         call("on_setup", *m_on_setup, exec, std::error_code());
     }
 
     template<typename Executor>
-    void on_error(Executor& exec, const std::error_code& ec) const {
+    DLLLOCAL void on_error(Executor& exec, const std::error_code& ec) const {
         call("on_error", *m_on_error, exec, ec);
     }
 
     template<typename Executor>
-    void on_fork_error(Executor& exec, const std::error_code& ec) const {
+    DLLLOCAL void on_fork_error(Executor& exec, const std::error_code& ec) const {
         call("on_fork_error", *m_on_fork_error, exec, ec);
     }
 
     template<typename Executor>
-    void on_exec_setup(Executor& exec) const {
+    DLLLOCAL void on_exec_setup(Executor& exec) const {
         call("on_exec_setup", *m_on_exec_setup, exec, std::error_code());
     }
 
     template<typename Executor>
-    void on_exec_error(Executor& exec, const std::error_code& ec) const {
+    DLLLOCAL void on_exec_error(Executor& exec, const std::error_code& ec) const {
         call("on_exec_error", *m_on_exec_error, exec, ec);
     }
 
     template<typename Executor>
-    void call(const char* info,
+    DLLLOCAL void call(const char* info,
               const ResolvedCallReferenceNode* callref,
               Executor& exec,
               const std::error_code& ec) const {
@@ -135,16 +116,26 @@ class QoreProcessHandler : public ex::async_handler {
     }
 
     template<typename Executor>
-    std::function<void(int, const std::error_code&)> on_exit_handler(Executor& exec) {
+    DLLLOCAL std::function<void(int, const std::error_code&)> on_exit_handler(Executor& exec) {
         boost::asio::io_context& ios = ex::get_io_context(exec.seq);
-        return [&ios](int exit_code, const std::error_code& ec) {
+        return [this, &ios](int exit_code, const std::error_code& ec) {
             ios.stop();
             ios.run();
+
+#if defined(BOOST_POSIX_API)
+            if (WIFEXITED(exit_code)) {
+                this->exit_code = WEXITSTATUS(exit_code);
+                //printd(5, "on_exit_handler() set exit_code: %d\n", exit_code);
+            }
+#else
+            this->exit_code = exit_code;
+#endif
         };
     }
 
 private:
     ExceptionSink* m_xsink;
+    int& exit_code;
 
     ReferenceHolder<ResolvedCallReferenceNode> m_on_success;
     ReferenceHolder<ResolvedCallReferenceNode> m_on_setup;
diff --git a/test/process.qtest b/test/process.qtest
@@ -62,6 +62,7 @@ public class Main inherits QUnit::Test {
                 "simpleTest_exe": "ls",
                 "simpleTest_arg": ("-l", "-s",),
                 "executorsTest_success": "Undefined error: 0",
+                "executorsTest_exit_code_383": 383,
             },
         );
     }

Original file line number	Diff line number	Diff line change
`@@ -62,6 +62,7 @@ public class Main inherits QUnit::Test {`
`62`	`62`	`"simpleTest_exe": "ls",`
`63`	`63`	`"simpleTest_arg": ("-l", "-s",),`
`64`	`64`	`"executorsTest_success": "Undefined error: 0",`
	`65`	`+ "executorsTest_exit_code_383": 383,`
`65`	`66`	`},`
`66`	`67`	`);`
`67`	`68`	`}`