From 2425ccc8329dc99d0046e45e3408ca3973ebfded Mon Sep 17 00:00:00 2001
From: TheAssassin <theassassin@assassinate-you.net>
Date: Fri, 7 Aug 2020 13:45:52 +0200
Subject: [PATCH] Extract process handling into separate class

Needed for plugins' subprocess handling.
---
 include/linuxdeploy/subprocess/process.h    |  71 ++++++++
 include/linuxdeploy/subprocess/subprocess.h |   6 -
 src/subprocess/CMakeLists.txt               |   4 +-
 src/subprocess/process.cpp                  | 188 ++++++++++++++++++++
 src/subprocess/subprocess.cpp               | 162 ++---------------
 src/subprocess/subprocess_demo.cpp          |   2 +-
 src/subprocess/subprocess_result.cpp        |   2 +-
 7 files changed, 275 insertions(+), 160 deletions(-)
 create mode 100644 include/linuxdeploy/subprocess/process.h
 create mode 100644 src/subprocess/process.cpp

diff --git a/include/linuxdeploy/subprocess/process.h b/include/linuxdeploy/subprocess/process.h
new file mode 100644
index 0000000..fbc903e
--- /dev/null
+++ b/include/linuxdeploy/subprocess/process.h
@@ -0,0 +1,71 @@
+// system headers
+#include <unordered_map>
+#include <vector>
+
+// local headers
+#include "linuxdeploy/subprocess/subprocess.h"
+
+namespace linuxdeploy {
+    namespace subprocess {
+        class process {
+        private:
+            // child process ID
+            int child_pid_;
+
+            // pipes to child process's stdout/stderr
+            int stdout_fd_;
+            int stderr_fd_;
+
+            // process exited
+            bool exited_ = false;
+            // exit code -- will be initialized by close()
+            int exit_code_;
+
+            // these constants help make the pipe code more readable
+            static constexpr int READ_END_ = 0, WRITE_END_ = 1;
+
+            static std::vector<char*> make_args_vector_(const std::vector<std::string>& args) ;
+
+            static std::vector<char*> make_env_vector_(const subprocess_env_map_t& env) ;
+
+        public:
+            /**
+             * Create a child process.
+             * @param args parameters for process
+             * @param env additional environment variables (current environment will be copied)
+             */
+            process(std::initializer_list<std::string> args, const subprocess_env_map_t& env);
+
+            /**
+             * Create a child process.
+             * @param args parameters for process
+             * @param env additional environment variables (current environment will be copied)
+             */
+            process(const std::vector<std::string>& args, const subprocess_env_map_t& env);
+
+            ~process();
+
+            /**
+             * @return child process's ID
+             */
+            int pid() const;
+
+            /**
+             * @return child process's stdout file descriptor ID
+             */
+            int stdout_fd() const;
+
+
+            /**
+             * @return child process's stderr file descriptor ID
+             */
+            int stderr_fd() const;
+
+            /**
+             * Close all pipes and wait for process to exit. If process was closed already, just returns exit code.
+             * @return child process's exit code
+             */
+            int close();
+        };
+    }
+}
diff --git a/include/linuxdeploy/subprocess/subprocess.h b/include/linuxdeploy/subprocess/subprocess.h
index f607fe7..8d0a508 100644
--- a/include/linuxdeploy/subprocess/subprocess.h
+++ b/include/linuxdeploy/subprocess/subprocess.h
@@ -19,12 +19,6 @@ namespace linuxdeploy {
             std::vector<std::string> args_{};
             std::unordered_map<std::string, std::string> env_{};
 
-            void assert_args_not_empty_() const;
-
-            std::vector<char*> make_args_vector_() const;
-
-            std::vector<char*> make_env_vector_() const;
-
         public:
             subprocess(std::initializer_list<std::string> args, subprocess_env_map_t env = {});
 
diff --git a/src/subprocess/CMakeLists.txt b/src/subprocess/CMakeLists.txt
index 139ccc9..4aaf045 100644
--- a/src/subprocess/CMakeLists.txt
+++ b/src/subprocess/CMakeLists.txt
@@ -3,10 +3,12 @@ set(headers_dir ${PROJECT_SOURCE_DIR}/include/linuxdeploy/subprocess)
 add_library(linuxdeploy_subprocess STATIC
     subprocess.cpp
     subprocess_result.cpp
+    process.cpp
     ${headers_dir}/subprocess.h
     ${headers_dir}/subprocess_result.h
+    ${headers_dir}/process.h
 )
-target_include_directories(linuxdeploy_subprocess PUBLIC ${headers_dir})
+target_include_directories(linuxdeploy_subprocess PUBLIC ${PROJECT_SOURCE_DIR}/include)
 
 add_executable(subprocess_demo subprocess_demo.cpp)
 target_link_libraries(subprocess_demo PUBLIC linuxdeploy_subprocess)
diff --git a/src/subprocess/process.cpp b/src/subprocess/process.cpp
new file mode 100644
index 0000000..d1ebb38
--- /dev/null
+++ b/src/subprocess/process.cpp
@@ -0,0 +1,188 @@
+// system headers
+#include <algorithm>
+#include <iostream>
+#include <memory>
+#include <sstream>
+#include <stdexcept>
+#include <utility>
+#include <unistd.h>
+#include <memory.h>
+#include <wait.h>
+
+// local headers
+#include "linuxdeploy/subprocess/process.h"
+#include "linuxdeploy/subprocess/subprocess.h"
+#include "linuxdeploy/util/assert.h"
+
+// shorter than using namespace ...
+using namespace linuxdeploy::subprocess;
+
+int process::pid() const {
+    return child_pid_;
+}
+
+int process::stdout_fd() const {
+    return stdout_fd_;
+}
+
+int process::stderr_fd() const {
+    return stderr_fd_;
+}
+
+process::process(std::initializer_list<std::string> args, const subprocess_env_map_t& env)
+    : process(std::vector<std::string>(args), env) {}
+
+process::process(const std::vector<std::string>& args, const subprocess_env_map_t& env)
+    : exited_(false), exit_code_(0) {
+    // preconditions
+    util::assert::assert_not_empty(args);
+
+    // pipes for both stdout and stderr
+    // the order is, as seen from the child: [read, write]
+    int stdout_pipe_fds[2];
+    int stderr_pipe_fds[2];
+
+    // create actual pipes
+    if (pipe(stdout_pipe_fds) != 0 || pipe(stderr_pipe_fds) != 0) {
+        throw std::logic_error{"failed to create pipes"};
+    }
+
+    // create child process
+    child_pid_ = fork();
+
+    if (child_pid_ < 0) {
+        throw std::runtime_error{"fork() failed"};
+    }
+
+    std::cout << "child pid: " << std::to_string(child_pid_) << std::endl;
+
+    if (child_pid_ == 0) {
+        // we're in the child process
+
+        // first step: close the read end of both pipes
+        ::close(stdout_pipe_fds[READ_END_]);
+        ::close(stderr_pipe_fds[READ_END_]);
+
+        auto connect_fd = [](int fds[], int fileno) {
+            for (;;) {
+                if (dup2(fds[WRITE_END_], fileno) == -1) {
+                    if (errno != EINTR) {
+                        throw std::logic_error{"failed to connect pipes"};
+                    }
+                    continue;
+                }
+
+                break;
+            }
+        };
+
+        connect_fd(stdout_pipe_fds, STDOUT_FILENO);
+        connect_fd(stderr_pipe_fds, STDERR_FILENO);
+
+        // now, we also have to close the write end of both pipes
+        ::close(stdout_pipe_fds[WRITE_END_]);
+        ::close(stderr_pipe_fds[WRITE_END_]);
+
+        // prepare arguments for exec*
+        auto exec_args = make_args_vector_(args);
+        auto exec_env = make_env_vector_(env);
+
+        // call subprocess
+        execvpe(args.front().c_str(), exec_args.data(), exec_env.data());
+
+        // only reached if exec* fails
+        throw std::runtime_error{"exec() failed: " + std::string(strerror(errno))};
+    }
+
+    // parent code
+
+    // we do not intend to write to the processes
+    ::close(stdout_pipe_fds[WRITE_END_]);
+    ::close(stderr_pipe_fds[WRITE_END_]);
+
+    // store file descriptors
+    stdout_fd_ = stdout_pipe_fds[READ_END_];
+    stderr_fd_ = stderr_pipe_fds[READ_END_];
+}
+
+int process::close() {
+    if (exited_) {
+        ::close(stdout_fd_);
+        stdout_fd_ = -1;
+
+        ::close(stderr_fd_);
+        stderr_fd_ = -1;
+
+        {
+            int temporary;
+            waitpid(child_pid_, &temporary, 0);
+            exit_code_ = WEXITSTATUS(temporary);
+        }
+    }
+
+    return exit_code_;
+}
+
+process::~process() {
+    (void) close();
+}
+
+std::vector<char*> process::make_args_vector_(const std::vector<std::string>& args) {
+    std::vector<char*> rv{};
+    rv.reserve(args.size());
+
+    for (const auto& arg : args) {
+        rv.emplace_back(strdup(arg.c_str()));
+    }
+
+    // execv* want a nullptr-terminated array
+    rv.emplace_back(nullptr);
+
+    return rv;
+}
+
+std::vector<char*> process::make_env_vector_(const subprocess_env_map_t& env) {
+    std::vector<char*> rv;
+
+    // first, copy existing environment
+    // we cannot reserve space in the vector unfortunately, as we don't know the size of environ before the iteration
+    if (environ != nullptr) {
+        for (auto** current_env_var = environ; *current_env_var != nullptr; ++current_env_var) {
+            rv.emplace_back(strdup(*current_env_var));
+        }
+    }
+
+    // add own environment variables, overwriting existing ones if necessary
+    for (const auto& env_var : env) {
+        const auto& key = env_var.first;
+        const auto& value = env_var.second;
+
+        auto predicate = [&key](char* existing_env_var) {
+            char* equal_sign = strstr(existing_env_var, "=");
+
+            if (equal_sign == nullptr) {
+                throw std::runtime_error{"no equal sign in environment variable"};
+            }
+
+            std::string existing_env_var_name{existing_env_var_name, 0, static_cast<size_t>(existing_env_var - equal_sign)};
+
+            return existing_env_var_name == key;
+        };
+
+        // delete existing env var, if any
+        rv.erase(std::remove_if(rv.begin(), rv.end(), predicate), rv.end());
+
+        // insert new value
+        std::ostringstream oss;
+        oss << key;
+        oss << "=";
+        oss << value;
+
+        rv.emplace_back(strdup(oss.str().c_str()));
+    }
+
+    // exec*e want a nullptr-terminated array
+    rv.emplace_back(nullptr);
+
+    return rv;
+}
diff --git a/src/subprocess/subprocess.cpp b/src/subprocess/subprocess.cpp
index 08878e3..de81e5b 100644
--- a/src/subprocess/subprocess.cpp
+++ b/src/subprocess/subprocess.cpp
@@ -2,94 +2,29 @@
 #include <algorithm>
 #include <iostream>
 #include <memory>
-#include <sstream>
 #include <stdexcept>
 #include <utility>
 #include <unistd.h>
-#include <memory.h>
-#include <wait.h>
 
 // local headers
-#include "subprocess.h"
+#include "linuxdeploy/subprocess/subprocess.h"
+#include "linuxdeploy/subprocess/process.h"
+#include "linuxdeploy/util/assert.h"
 
 // shorter than using namespace ...
 using namespace linuxdeploy::subprocess;
 
 subprocess::subprocess(std::initializer_list<std::string> args, subprocess_env_map_t env)
-    : args_(args), env_(std::move(env)) {
-    assert_args_not_empty_();
-}
+    : subprocess(std::vector<std::string>(args), std::move(env)) {}
 
 subprocess::subprocess(std::vector<std::string> args, subprocess_env_map_t env)
     : args_(std::move(args)), env_(std::move(env)) {
-    assert_args_not_empty_();
+    // preconditions
+    util::assert::assert_not_empty(args_);
 }
 
 subprocess_result subprocess::run() const {
-    // these constants help make the code below more readable
-    static constexpr int READ_END = 0, WRITE_END = 1;
-
-    // pipes for both stdout and stderr
-    // the order is, as seen from the child: [read, write]
-    int stdout_pipe_fds[2];
-    int stderr_pipe_fds[2];
-
-    // create actual pipes
-    if (pipe(stdout_pipe_fds) != 0 || pipe(stderr_pipe_fds) != 0) {
-        throw std::logic_error{"failed to create pipes"};
-    }
-
-    // create child process
-    pid_t child_pid = fork();
-
-    if (child_pid < 0) {
-        throw std::runtime_error{"fork() failed"};
-    }
-
-    std::cout << "child pid: " << std::to_string(child_pid) << std::endl;
-
-    if (child_pid == 0) {
-        // we're in the child process
-
-        // first step: close the read end of both pipes
-        close(stdout_pipe_fds[READ_END]);
-        close(stderr_pipe_fds[READ_END]);
-
-        auto connect_fd = [](int fds[], int fileno) {
-            for (;;) {
-                if (dup2(fds[WRITE_END], fileno) == -1) {
-                    if (errno != EINTR) {
-                        throw std::logic_error{"failed to connect pipes"};
-                    }
-                    continue;
-                }
-
-                break;
-            }
-        };
-
-        connect_fd(stdout_pipe_fds, STDOUT_FILENO);
-        connect_fd(stderr_pipe_fds, STDERR_FILENO);
-
-        // now, we also have to close the write end of both pipes
-        close(stdout_pipe_fds[WRITE_END]);
-        close(stderr_pipe_fds[WRITE_END]);
-
-        // prepare arguments for exec*
-        auto exec_args = make_args_vector_();
-        auto exec_env = make_env_vector_();
-
-        // call subprocess
-        execvpe(args_.front().c_str(), exec_args.data(), exec_env.data());
-
-        throw std::runtime_error{"exec() failed: " + std::string(strerror(errno))};
-    }
-
-    // parent code
-
-    // we do not intend to write to the processes
-    close(stdout_pipe_fds[WRITE_END]);
-    close(stderr_pipe_fds[WRITE_END]);
+    process proc{args_, env_};
 
     subprocess_result_buffer_t stdout_contents{};
     subprocess_result_buffer_t stderr_contents{};
@@ -118,94 +53,19 @@ subprocess_result subprocess::run() const {
         }
     };
 
-    read_into_buffer(stdout_pipe_fds[READ_END], stdout_contents);
-    read_into_buffer(stderr_pipe_fds[READ_END], stderr_contents);
-
-    // done reading -> close read end
-    close(stdout_pipe_fds[READ_END]);
-    close(stderr_pipe_fds[READ_END]);
+    // TODO: make sure neither stderr nor stdout overflow
+    read_into_buffer(proc.stdout_fd(), stdout_contents);
+    read_into_buffer(proc.stderr_fd(), stderr_contents);
 
     // make sure contents are null-terminated
     stdout_contents.emplace_back('\0');
     stderr_contents.emplace_back('\0');
 
-    // wait for child to exit and fetch its exit code
-    int exit_code;
-    {
-        int temporary;
-        waitpid(child_pid, &temporary, 0);
-        exit_code = WEXITSTATUS(temporary);
-    }
+    auto exit_code = proc.close();
 
     return subprocess_result{exit_code, stdout_contents, stderr_contents};
 }
 
-void subprocess::assert_args_not_empty_() const {
-    if(args_.empty()) {
-        throw std::invalid_argument("args may not be empty");
-    }
-}
-
-std::vector<char*> subprocess::make_args_vector_() const {
-    std::vector<char*> rv{};
-    rv.reserve(args_.size());
-
-    for (const auto& arg : args_) {
-        rv.emplace_back(strdup(arg.c_str()));
-    }
-
-    // execv* want a nullptr-terminated array
-    rv.emplace_back(nullptr);
-
-    return rv;
-}
-
-std::vector<char*> subprocess::make_env_vector_() const {
-    std::vector<char*> rv;
-
-    // first, copy existing environment
-    // we cannot reserve space in the vector unfortunately, as we don't know the size of environ before the iteration
-    if (environ != nullptr) {
-        for (auto** current_env_var = environ; *current_env_var != nullptr; ++current_env_var) {
-            rv.emplace_back(strdup(*current_env_var));
-        }
-    }
-
-    // add own environment variables, overwriting existing ones if necessary
-    for (const auto& env_var : env_) {
-        const auto& key = env_var.first;
-        const auto& value = env_var.second;
-
-        auto predicate = [&key](char* existing_env_var) {
-            char* equal_sign = strstr(existing_env_var, "=");
-
-            if (equal_sign == nullptr) {
-                throw std::runtime_error{"no equal sign in environment variable"};
-            }
-
-            std::string existing_env_var_name{existing_env_var_name, 0, static_cast<size_t>(existing_env_var - equal_sign)};
-
-            return existing_env_var_name == key;
-        };
-
-        // delete existing env var, if any
-        rv.erase(std::remove_if(rv.begin(), rv.end(), predicate), rv.end());
-
-        // insert new value
-        std::ostringstream oss;
-        oss << key;
-        oss << "=";
-        oss << value;
-
-        rv.emplace_back(strdup(oss.str().c_str()));
-    }
-
-    // exec*e want a nullptr-terminated array
-    rv.emplace_back(nullptr);
-
-    return rv;
-}
-
 std::string subprocess::check_output() const {
     const auto result = run();
 
diff --git a/src/subprocess/subprocess_demo.cpp b/src/subprocess/subprocess_demo.cpp
index 7768022..57005ef 100644
--- a/src/subprocess/subprocess_demo.cpp
+++ b/src/subprocess/subprocess_demo.cpp
@@ -1,6 +1,6 @@
 #include <iostream>
 
-#include "subprocess.h"
+#include "linuxdeploy/subprocess/subprocess.h"
 
 using namespace linuxdeploy::subprocess;
 
diff --git a/src/subprocess/subprocess_result.cpp b/src/subprocess/subprocess_result.cpp
index e00f98f..031ce51 100644
--- a/src/subprocess/subprocess_result.cpp
+++ b/src/subprocess/subprocess_result.cpp
@@ -1,5 +1,5 @@
 // local headers
-#include "subprocess_result.h"
+#include "linuxdeploy/subprocess/subprocess_result.h"
 
 // shorter than using namespace ...
 using namespace linuxdeploy::subprocess;