Add locking and cleaning temporary directory

This commit solves the problem of leaving many temporary directories
that were not deleted if the crash-worker has not finished working
properly.

Change-Id: I415403a2531b6e41ebf20a3b185790d9e21a8ca3

diff --git a/packaging/crash-worker.spec b/packaging/crash-worker.spec
index 93d577d..736f684 100644 (file)
--- a/packaging/crash-worker.spec
+++ b/packaging/crash-worker.spec
@@ -243,6 +243,7 @@ mkdir -p %{buildroot}%{crash_temp}
 
 %{_libdir}/crash-worker/system-tests/check_minicore_mem/check_minicore_mem.sh
 %{_libdir}/crash-worker/system-tests/check_minicore_mem/cp.sh
+%{_libdir}/crash-worker/system-tests/clean_temp/clean_temp.sh
 %{_libdir}/crash-worker/system-tests/cmp_backtraces/cmp_backtraces.sh
 %{_libdir}/crash-worker/system-tests/cmp_backtraces/cp.sh
 %{_libdir}/crash-worker/system-tests/copy_tizen_manifest/copy_tizen_manifest.sh
@@ -261,6 +262,7 @@ mkdir -p %{buildroot}%{crash_temp}
 %{_libdir}/crash-worker/system-tests/report_basic/report_basic.sh
 %{_libdir}/crash-worker/system-tests/report_type_info/report_type_info.sh
 %{_libdir}/crash-worker/system-tests/so_info_file/so_info_file.sh
+%{_libdir}/crash-worker/system-tests/temp_lock/temp_lock.sh
 %{_libdir}/crash-worker/system-tests/time_test/cp.sh
 %{_libdir}/crash-worker/system-tests/time_test/time_test.sh
 %{_libdir}/crash-worker/system-tests/utils/btee

diff --git a/src/crash-manager/crash-manager.c b/src/crash-manager/crash-manager.c
index c889cdb..3ae4981 100644 (file)
--- a/src/crash-manager/crash-manager.c
+++ b/src/crash-manager/crash-manager.c
@@ -225,6 +225,13 @@ static int prepare_paths(struct crash_info* cinfo)
        return 1;
 }
 
+static void unlock_dir(int fd)
+{
+       if (flock(fd, LOCK_UN) < 0)
+               _E("Failed to unlock file descriptor: %m");
+       close(fd);
+}
+
 static bool make_dump_dir(void)
 {
        const char *dirs[] = {crash_dump_path, crash_temp_path};
@@ -366,6 +373,72 @@ static void set_crash_info_defaults(struct crash_info *cinfo)
        }
 }
 
+static int dump_filter(const struct dirent *de)
+{
+       assert(de);
+
+       if (de->d_name[0] == '.')
+               return 0;
+       return 1;
+}
+
+/* We treat all errors as if the file was locked. This function is used to check
+ * if we can remove the temporary directory. We don't want to remove that
+ * directory if an error has occurred, because the deletion will probably also
+ * fail */
+static bool is_locked(const char *dir_name)
+{
+       assert(dir_name);
+
+       char lock_file[PATH_MAX];
+       if (snprintf(lock_file, sizeof(lock_file), "%s", dir_name) == -1) {
+               _E("Unable to check locking status for %s: %m", dir_name);
+               return true;
+       }
+
+       int fd = open(lock_file, O_RDONLY | O_DIRECTORY);
+       if (fd == -1) {
+               _E("Unable to open file %s to check if it's blocked: %m", lock_file);
+               return true;
+       }
+
+       bool result = flock(fd, LOCK_EX | LOCK_NB) == -1;
+
+       close(fd);
+       return result;
+}
+
+static bool clean_temp(const char *temp_dir)
+{
+       bool result = true;
+       assert(temp_dir);
+
+       struct dirent **scan_list = NULL;
+       int scan_num;
+
+       if ((scan_num = scandir(temp_dir, &scan_list, &dump_filter, NULL)) < 0)
+               return false;
+
+       for (int i = 0; i < scan_num; i++) {
+               char dir_name[PATH_MAX];
+
+               if (snprintf(dir_name, sizeof(dir_name), "%s/%s", temp_dir, scan_list[i]->d_name) == -1) {
+                       _E("Unable to clean temp for %s: %m", dir_name);
+                       result = false;
+                       continue;
+               }
+
+               if (is_locked(dir_name))
+                       _D("Temporary directory %s is locked", dir_name);
+               else if (remove_dir(dir_name, 1) == -1)
+                       _W("Can not remove temporary directory: %s", dir_name);
+               else
+                       _D("Temporary directory %s removed", dir_name);
+       }
+
+       return result;
+}
+
 /* Note: caller of this function is responsible for cleaning up the cinfo on failure */
 bool set_crash_info(struct crash_info *cinfo)
 {
@@ -420,7 +493,9 @@ bool set_crash_info(struct crash_info *cinfo)
        return true;
 
 out_rm_temp:
+       unlock_dir(cinfo->lock_fd);
        remove_dir(cinfo->temp_dir, 1);
+       clean_temp(crash_temp_path);
        return false;
 
 out_oom:
@@ -786,17 +861,24 @@ static bool execute_crash_modules(struct crash_info *cinfo)
        return true;
 }
 
-static int lock_dumpdir(void)
+static int lock_dir(const char *path, bool block)
 {
+       assert(path);
+
        int fd;
 
-       if ((fd = open(crash_dump_path, O_RDONLY | O_DIRECTORY)) < 0) {
-               _E("Failed to open %s: %m", crash_dump_path);
+       if ((fd = open(path, O_RDONLY | O_DIRECTORY)) == -1) {
+               _E("Failed to open %s: %m", path);
                return -1;
        }
 
-       if (flock(fd, LOCK_EX) < 0) {
-               _E("Failed to lock %s for exclusive access: %m", crash_dump_path);
+       int flock_flags = LOCK_EX;
+
+       if (!block)
+               flock_flags |= LOCK_NB;
+
+       if (flock(fd, flock_flags) == -1) {
+               _E("Failed to lock %s for exclusive access: %m", path);
                close(fd);
                return -1;
        }
@@ -804,20 +886,6 @@ static int lock_dumpdir(void)
        return fd;
 }
 
-static void unlock_dumpdir(int fd)
-{
-       if (flock(fd, LOCK_UN) < 0)
-               _E("Failed to unlock file descriptor: %m");
-       close(fd);
-}
-
-static int dump_filter(const struct dirent *de)
-{
-       if (de->d_name[0] == '.')
-               return 0;
-       return 1;
-}
-
 static int mtime_cmp(const void *_a, const void *_b)
 {
        const struct file_info *a = _a;
@@ -1034,7 +1102,7 @@ static bool move_dump_data(const char *from_path, const struct crash_info *cinfo
        int lock_fd;
        bool is_ok = true;
 
-       if ((lock_fd = lock_dumpdir()) < 0)
+       if ((lock_fd = lock_dir(crash_dump_path, false)) < 0)
                return false;
        if (!rename(from_path, cinfo->result_path))
                clean_dump();
@@ -1042,11 +1110,14 @@ static bool move_dump_data(const char *from_path, const struct crash_info *cinfo
                _E("Failed to move %s to %s", from_path, cinfo->result_path);
                is_ok = false;
        }
-       unlock_dumpdir(lock_fd);
+       unlock_dir(lock_fd);
+       unlock_dir(cinfo->lock_fd);
 
        if (remove_dir(cinfo->temp_dir, 1) < 0)
                _E("Failed to delete temp directory");
 
+       clean_temp(crash_temp_path);
+
        return is_ok;
 }
 
@@ -1107,6 +1178,7 @@ void crash_info_init(struct crash_info *cinfo)
        cinfo->zip_path = NULL;
        cinfo->appid = NULL;
        cinfo->pkgid = NULL;
+       cinfo->lock_fd = -1;
 }
 
 static bool run(struct crash_info *cinfo)
@@ -1208,6 +1280,10 @@ static bool crash_manager_prepare(struct crash_info *cinfo)
        if (!set_crash_info(cinfo))
                return false;
 
+       cinfo->lock_fd = lock_dir(cinfo->temp_dir, true);
+       if (cinfo->lock_fd == -1)
+               return false;
+
        return true;
 }
 

diff --git a/src/crash-manager/crash-manager.h b/src/crash-manager/crash-manager.h
index 6e66af1..320ef41 100644 (file)
--- a/src/crash-manager/crash-manager.h
+++ b/src/crash-manager/crash-manager.h
@@ -50,6 +50,7 @@ struct crash_info {
        char *appid;
        char *pkgid;
        time_t time_info;
+       int lock_fd;
 };
 
 bool crash_manager_direct(struct crash_info *cinfo);

diff --git a/tests/system/CMakeLists.txt b/tests/system/CMakeLists.txt
index 37fe6ff..e8661a1 100644 (file)
--- a/tests/system/CMakeLists.txt
+++ b/tests/system/CMakeLists.txt
@@ -18,6 +18,7 @@ macro(CONFIGURE_TEST test_name)
 endmacro()
 
 configure_test("check_minicore_mem")
+configure_test("clean_temp")
 configure_test("cmp_backtraces" "cp")
 configure_test("copy_tizen_manifest")
 configure_test("crash_root_path")
@@ -36,6 +37,7 @@ configure_test("output_param")
 configure_test("report_basic")
 configure_test("report_type_info")
 configure_test("so_info_file")
+configure_test("temp_lock")
 configure_test("time_test")
 configure_test("wait_for_opt_usr")
 configure_test("without_core")

diff --git a/tests/system/clean_temp/clean_temp.sh.template b/tests/system/clean_temp/clean_temp.sh.template
new file mode 100644 (file)
index 0000000..9e6d615
--- /dev/null
+++ b/tests/system/clean_temp/clean_temp.sh.template
@@ -0,0 +1,33 @@
+#!/bin/bash
+
+# Check if the temp directory will be cleaned up
+
+if [ -z "${CRASH_WORKER_SYSTEM_TESTS}" ]; then
+    CRASH_WORKER_SYSTEM_TESTS="@CRASH_SYSTEM_TESTS_PATH@"
+fi
+
+. ${CRASH_WORKER_SYSTEM_TESTS}/utils/minicore-utils.sh
+
+CRASH_MANAGER_CONF=/etc/crash-manager.conf
+
+clean_crash_dump
+clean_temp
+
+TEST_TMP_DIR="${CRASH_TEMP_PATH}"/tmp1
+mkdir "${TEST_TMP_DIR}"
+echo "test" > "${TEST_TMP_DIR}"/some_file
+
+${CRASH_WORKER_SYSTEM_TESTS}/utils/kenny &
+pid=$!
+sleep 2
+kill -6 $pid
+
+sleep 1
+
+wait_for_app crash-manager || fail "crash-manager is still working"
+
+if [ -d "${TEST_TMP_DIR}" ]; then
+    fail "temp directory still exists"
+fi
+
+exit_ok

diff --git a/tests/system/temp_lock/temp_lock.sh.template b/tests/system/temp_lock/temp_lock.sh.template
new file mode 100644 (file)
index 0000000..08b9f1c
--- /dev/null
+++ b/tests/system/temp_lock/temp_lock.sh.template
@@ -0,0 +1,37 @@
+#!/bin/bash
+
+# Check the report type change in the config file
+
+if [ -z "${CRASH_WORKER_SYSTEM_TESTS}" ]; then
+    CRASH_WORKER_SYSTEM_TESTS="@CRASH_SYSTEM_TESTS_PATH@"
+fi
+
+. ${CRASH_WORKER_SYSTEM_TESTS}/utils/minicore-utils.sh
+
+CRASH_MANAGER_CONF=/etc/crash-manager.conf
+
+clean_crash_dump
+clean_temp
+
+${CRASH_WORKER_SYSTEM_TESTS}/utils/kenny &
+pid=$!
+sleep 2
+kill -6 $pid
+
+sleep 1
+
+pushd ${CRASH_TEMP_PATH}
+wait_for_dir $(echo *) || fail "temp directory not exists"
+name=$(echo *)
+
+flock -n -x "${name}" echo
+
+if [ $? == 0 ]; then
+    fail "temp directory is not locked"
+fi
+
+if [ ! -d "${name}" ]; then
+    fail "temp directory no longer exists"
+fi
+
+exit_ok

diff --git a/tests/system/utils/minicore-utils.sh b/tests/system/utils/minicore-utils.sh
index dd64a99..2805d54 100644 (file)
--- a/tests/system/utils/minicore-utils.sh
+++ b/tests/system/utils/minicore-utils.sh
@@ -118,6 +118,19 @@ function wait_for_file {
     return 0
 }
 
+function wait_for_dir {
+    TIMEOUT=240
+    while [ ! -d ${1} ];
+    do
+        if endoftime; then
+            fail "${1} does not exist"
+        else
+            sleep 1
+        fi
+    done
+    return 0
+}
+
 function wait_for_app {
     TIMEOUT=240
     while true;