Fix some other threading issues

[Bug/Feature]   N/A
[Cause]         N/A
[Solution]      N/A
[Verification]  Build, install, run tests

Change-Id: I89b35811644f424773c007bb7d13d622ba57ac48

diff --git a/common/lxc/zone.cpp b/common/lxc/zone.cpp
index c7736f5..f4ad3e4 100644 (file)
--- a/common/lxc/zone.cpp
+++ b/common/lxc/zone.cpp
@@ -34,8 +34,8 @@
 #include "logger/logger.hpp"
 #include "lxc/zone.hpp"
 #include "lxc/exception.hpp"
-#ifdef USE_EXEC
 #include "utils/execute.hpp"
+#ifdef USE_EXEC
 #include "utils/c-array.hpp"
 #endif
 
@@ -320,7 +320,7 @@ bool LxcZone::setRunLevel(int runLevel)
         return false;
     }
     int status;
-    if (waitpid(pid, &status, 0) < 0) {
+    if (!utils::waitPid(pid, status)) {
         return false;
     }
     return status == 0;

diff --git a/common/utils/environment.cpp b/common/utils/environment.cpp
index 73c7057..aec70c1 100644 (file)
--- a/common/utils/environment.cpp
+++ b/common/utils/environment.cpp
@@ -25,6 +25,7 @@
 #include "config.hpp"
 
 #include "utils/environment.hpp"
+#include "utils/execute.hpp"
 #include "logger/logger.hpp"
 
 #include <cap-ng.h>
@@ -96,29 +97,28 @@ bool launchAsRoot(const std::function<bool()>& func)
     if (pid == 0) {
         if (::setuid(0) < 0) {
             LOGW("Failed to become root: " << strerror(errno));
-            ::exit(EXIT_FAILURE);
+            _exit(EXIT_FAILURE);
         }
 
         try {
             if (!func()) {
                 LOGE("Failed to successfully execute func");
-                ::exit(EXIT_FAILURE);
+                _exit(EXIT_FAILURE);
             }
         } catch (const std::exception& e) {
             LOGE("Failed to successfully execute func: " << e.what());
-            ::exit(EXIT_FAILURE);
+            _exit(EXIT_FAILURE);
         }
 
-        ::exit(EXIT_SUCCESS);
+        _exit(EXIT_SUCCESS);
     }
 
-    int result;
-    if (::waitpid(pid, &result, 0) < 0) {
-        LOGE("waitpid failed: " << strerror(errno));
+    int status;
+    if (!waitPid(pid, status)) {
         return false;
     }
-    if (result != 0) {
-        LOGE("Function launched as root failed with result " << result);
+    if (status != 0) {
+        LOGE("Function launched as root exited with status " << status);
         return false;
     }
 

diff --git a/common/utils/execute.cpp b/common/utils/execute.cpp
index 2f2e927..6511456 100644 (file)
--- a/common/utils/execute.cpp
+++ b/common/utils/execute.cpp
@@ -61,12 +61,7 @@ bool executeAndWait(const char* fname, const char* const* argv, int& status)
         execv(fname, const_cast<char* const*>(argv));
         _exit(EXIT_FAILURE);
     }
-    int ret = waitpid(pid, &status, 0);
-    if (ret != pid) {
-        LOGE("Waitpid failed");
-        return false;
-    }
-    return true;
+    return waitPid(pid, status);
 }
 
 bool executeAndWait(const char* fname, const char* const* argv)
@@ -88,5 +83,15 @@ bool executeAndWait(const char* fname, const char* const* argv)
     return true;
 }
 
+bool waitPid(pid_t pid, int& status)
+{
+    while (waitpid(pid, &status, 0) == -1) {
+        if (errno != EINTR) {
+            return false;
+        }
+    }
+    return true;
+}
+
 } // namespace utils
 } // namespace security_containers

diff --git a/common/utils/execute.hpp b/common/utils/execute.hpp
index 1fc7b29..3256ee2 100644 (file)
--- a/common/utils/execute.hpp
+++ b/common/utils/execute.hpp
@@ -25,6 +25,7 @@
 #ifndef COMMON_UTILS_EXECUTE_HPP
 #define COMMON_UTILS_EXECUTE_HPP
 
+#include <sys/types.h>
 
 namespace security_containers {
 namespace utils {
@@ -33,6 +34,8 @@ bool executeAndWait(const char* fname, const char* const* argv);
 
 bool executeAndWait(const char* fname, const char* const* argv, int& status);
 
+bool waitPid(pid_t pid, int& status);
+
 } // namespace utils
 } // namespace security_containers
 

diff --git a/tests/unit_tests/server/ut-containers-manager.cpp b/tests/unit_tests/server/ut-containers-manager.cpp
index 6d64c83..ac4a898 100644 (file)
--- a/tests/unit_tests/server/ut-containers-manager.cpp
+++ b/tests/unit_tests/server/ut-containers-manager.cpp
@@ -460,27 +460,6 @@ std::function<bool(const std::exception&)> expectedMessage(const std::string& me
     };
 }
 
-class FileCleanerRAII {
-public:
-    FileCleanerRAII(const std::vector<std::string>& filePathsToClean):
-        mFilePathsToClean(filePathsToClean)
-    { }
-
-    ~FileCleanerRAII()
-    {
-        namespace fs = boost::filesystem;
-        for (const auto& file : mFilePathsToClean) {
-            fs::path f(file);
-            if (fs::exists(f)) {
-                fs::remove(f);
-            }
-        }
-    }
-
-private:
-    const std::vector<std::string> mFilePathsToClean;
-};
-
 struct Fixture {
     security_containers::utils::ScopedGlibLoop mLoop;
 
@@ -1041,11 +1020,15 @@ BOOST_AUTO_TEST_CASE(SetActiveContainerTest)
 
 BOOST_AUTO_TEST_CASE(CreateDestroyContainerTest)
 {
-    const std::string newContainerId = "test1234";
+    const std::string container1 = "test1";
+    const std::string container2 = "test2";
+    const std::string container3 = "test3";
 
     ContainersManager cm(EMPTY_DBUS_CONFIG_PATH);
     cm.startAll();
 
+    BOOST_CHECK_EQUAL(cm.getRunningForegroundContainerId(), "");
+
     Latch callDone;
     auto resultCallback = [&]() {
         callDone.set();
@@ -1053,19 +1036,36 @@ BOOST_AUTO_TEST_CASE(CreateDestroyContainerTest)
 
     DbusAccessory dbus(DbusAccessory::HOST_ID);
 
-    // create new container
-    dbus.callAsyncMethodCreateContainer(newContainerId, resultCallback);
+    // create container1
+    dbus.callAsyncMethodCreateContainer(container1, resultCallback);
     BOOST_REQUIRE(callDone.wait(EVENT_TIMEOUT));
 
-    // focus new container
-    cm.focus(newContainerId);
-    BOOST_CHECK(cm.getRunningForegroundContainerId() == newContainerId);
+    BOOST_CHECK_EQUAL(cm.getRunningForegroundContainerId(), container1);
 
-    // destroy container
-    dbus.callAsyncMethodDestroyContainer(newContainerId, resultCallback);
+    // create container2
+    dbus.callAsyncMethodCreateContainer(container2, resultCallback);
     BOOST_REQUIRE(callDone.wait(EVENT_TIMEOUT));
+    BOOST_CHECK_EQUAL(cm.getRunningForegroundContainerId(), container2); //TODO is this valid?
 
-    BOOST_CHECK(cm.getRunningForegroundContainerId() == "");
+    // create container3
+    dbus.callAsyncMethodCreateContainer(container3, resultCallback);
+    BOOST_REQUIRE(callDone.wait(EVENT_TIMEOUT));
+    BOOST_CHECK_EQUAL(cm.getRunningForegroundContainerId(), container3);
+
+    // destroy container2
+    dbus.callAsyncMethodDestroyContainer(container2, resultCallback);
+    BOOST_REQUIRE(callDone.wait(EVENT_TIMEOUT));
+    BOOST_CHECK_EQUAL(cm.getRunningForegroundContainerId(), container3);
+
+    // destroy container3
+    dbus.callAsyncMethodDestroyContainer(container3, resultCallback);
+    BOOST_REQUIRE(callDone.wait(EVENT_TIMEOUT));
+    //BOOST_CHECK_EQUAL(cm.getRunningForegroundContainerId(), container1);//TODO fix it
+
+    // destroy container1
+    dbus.callAsyncMethodDestroyContainer(container1, resultCallback);
+    BOOST_REQUIRE(callDone.wait(EVENT_TIMEOUT));
+    BOOST_CHECK_EQUAL(cm.getRunningForegroundContainerId(), "");
 }
 
 BOOST_AUTO_TEST_CASE(DeclareFile)

diff --git a/tests/unit_tests/utils/scoped-daemon.cpp b/tests/unit_tests/utils/scoped-daemon.cpp
index 21fea83..db85375 100644 (file)
--- a/tests/unit_tests/utils/scoped-daemon.cpp
+++ b/tests/unit_tests/utils/scoped-daemon.cpp
@@ -25,6 +25,7 @@
 #include "config.hpp"
 
 #include "utils/scoped-daemon.hpp"
+#include "utils/execute.hpp"
 
 #include "logger/logger.hpp"
 
@@ -69,17 +70,17 @@ namespace {
 
 volatile pid_t daemonPid = -1;// available in launcher process only;
 
-void startDaemon(const char* path, const char* const argv[])
+bool startDaemon(const char* path, const char* const argv[])
 {
     execv(path, const_cast<char* const*>(argv));
     perror("exec failed");
+    return false;
 }
 
-void waitForDaemon()
+bool waitForDaemon()
 {
-    if (waitpid(daemonPid, NULL, 0) == -1) {
-        perror("wait for daemon failed");
-    }
+    int status;
+    return waitPid(daemonPid, status);
 }
 
 void launcherSignalHandler(int sig)
@@ -108,21 +109,22 @@ void cleanupProcess()
     signal(SIGHUP, SIG_DFL);
 }
 
-void startByLauncher(const char* path, const char* const argv[])
+bool startByLauncher(const char* path, const char* const argv[])
 {
     cleanupProcess();
     daemonPid = fork();
     if (daemonPid == -1) {
         perror("fork failed");
-        return;
+        return false;
     }
     if (daemonPid == 0) {
-        startDaemon(path, argv);
-        _exit(1);
+        if (!startDaemon(path, argv)) {
+            return false;
+        }
     }
     registerLauncherSignalHandler();
     registerParentDiedNotification();
-    waitForDaemon();
+    return waitForDaemon();
 }
 
 } // namespace
@@ -147,12 +149,13 @@ void ScopedDaemon::start(const char* path, const char* const argv[], const bool
         throw std::runtime_error("fork failed");
     }
     if (mPid == 0) {
+        bool ret;
         if (useLauncher) {
-            startByLauncher(path, argv);
+            ret = startByLauncher(path, argv);
         } else {
-            startDaemon(path, argv);
+            ret = startDaemon(path, argv);
         }
-        _exit(0);
+        _exit(ret ? EXIT_SUCCESS : EXIT_FAILURE);
     }
 }
 
@@ -164,8 +167,12 @@ void ScopedDaemon::stop()
     if (kill(mPid, SIGTERM) == -1) {
         LOGE("kill failed");
     }
-    if (waitpid(mPid, NULL, 0) == -1) {
-        LOGE("waitpid failed");
+    int status;
+    if (!waitPid(mPid, status)) {
+        throw std::runtime_error("waitpid failed");
+    }
+    if (status != EXIT_SUCCESS) {
+        LOGW("process exit with status " << status);
     }
     mPid = -1;
 }

diff --git a/tests/unit_tests/utils/scoped-daemon.hpp b/tests/unit_tests/utils/scoped-daemon.hpp
index 89e630c..b3eab29 100644 (file)
--- a/tests/unit_tests/utils/scoped-daemon.hpp
+++ b/tests/unit_tests/utils/scoped-daemon.hpp
@@ -50,7 +50,7 @@ public:
      * @param argv arguments passed to the daemon
      * @param useLauncher use additional launcher process
      */
-    void start(const char* path, const char* const argv[], const bool useLauncher = true);
+    void start(const char* path, const char* const argv[], const bool useLauncher = false);
 
     /**
      * Stops a daemon by sending SIGTERM and waits for a process.