From a40c8c5611eb05341f081394574fa07767db44e2 Mon Sep 17 00:00:00 2001 From: Dongkyun Son Date: Mon, 30 Mar 2020 21:23:37 +0900 Subject: [PATCH 01/16] packaging: remove libebl dependency libebl is not needed anymore. Change-Id: If88fa16b361c70c9155e5240a03cbede3e235074 Signed-off-by: Dongkyun Son --- packaging/crash-worker.spec | 1 - 1 file changed, 1 deletion(-) diff --git a/packaging/crash-worker.spec b/packaging/crash-worker.spec index 56a358a..5717544 100644 --- a/packaging/crash-worker.spec +++ b/packaging/crash-worker.spec @@ -29,7 +29,6 @@ BuildRequires: cmake BuildRequires: pkgconfig(pkgmgr-info) BuildRequires: pkgconfig(libunwind-generic) BuildRequires: libelf-devel libelf -BuildRequires: libebl-devel libebl BuildRequires: libdw-devel libdw BuildRequires: libcap-devel -- 2.7.4 From e7b5394c65076a7e91476446e33f4f52962f4eb7 Mon Sep 17 00:00:00 2001 From: Karol Lewandowski Date: Thu, 2 Apr 2020 10:55:30 +0200 Subject: [PATCH 02/16] dump_systemstate: Also list files when reporting /tmp disk usage Change-Id: I0ea99ca0a05a7d620a4462d6dc44257aa6e4bee0 --- src/dump_systemstate/dump_systemstate.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/dump_systemstate/dump_systemstate.c b/src/dump_systemstate/dump_systemstate.c index 8dbe9ce..9b50fa5 100644 --- a/src/dump_systemstate/dump_systemstate.c +++ b/src/dump_systemstate/dump_systemstate.c @@ -185,8 +185,8 @@ int main(int argc, char *argv[]) dpercent = get_disk_used_percent("/tmp"); if (80 < dpercent) { - fprintf_fd(out_fd, "\n==== tmp usage detail - %d%% (/bin/du -h /tmp)\n", dpercent); - char *du_args[] = {"/bin/du", "-h", "/tmp", NULL}; + fprintf_fd(out_fd, "\n==== tmp usage detail - %d%% (/bin/du -ah /tmp)\n", dpercent); + char *du_args[] = {"/bin/du", "-ah", "/tmp", NULL}; spawn_wait_checked(du_args, NULL); } -- 2.7.4 From 296b22a695bc3197a71d125abed926fe779ce4d8 Mon Sep 17 00:00:00 2001 From: Karol Lewandowski Date: Thu, 2 Apr 2020 13:32:36 +0200 Subject: [PATCH 03/16] dump_systemstate: Use compile-time parameter check in get_disk_used_percent() Guard functions with assert as it's programming error to invoke it with null path. Change-Id: I3a312f90c0553c1dd9e9233551c0d42b89c260f7 --- src/dump_systemstate/dump_systemstate.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/dump_systemstate/dump_systemstate.c b/src/dump_systemstate/dump_systemstate.c index 9b50fa5..149bfb0 100644 --- a/src/dump_systemstate/dump_systemstate.c +++ b/src/dump_systemstate/dump_systemstate.c @@ -21,6 +21,7 @@ * @brief dump system states. */ +#include #include #include #include @@ -73,15 +74,13 @@ static void usage() ); } -/* get disk used percentage */ static int get_disk_used_percent(const char *path) { + assert(path); + struct statfs lstatfs; int percent; - if (!path) - return -1; - if (statfs(path, &lstatfs) < 0) return -1; percent = (((lstatfs.f_blocks - lstatfs.f_bfree) * 1000) / (lstatfs.f_blocks)) + 9; -- 2.7.4 From d3a974dd6778c9c7a61c9be840524582c060cfdc Mon Sep 17 00:00:00 2001 From: Karol Lewandowski Date: Thu, 2 Apr 2020 13:37:03 +0200 Subject: [PATCH 04/16] dump_systemstate: Add /opt/usr disk usage section Change-Id: Ib52cba397f7272c14dcb7623efc85535d4d98d8e --- src/dump_systemstate/dump_systemstate.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/src/dump_systemstate/dump_systemstate.c b/src/dump_systemstate/dump_systemstate.c index 149bfb0..814acda 100644 --- a/src/dump_systemstate/dump_systemstate.c +++ b/src/dump_systemstate/dump_systemstate.c @@ -32,6 +32,7 @@ #include #include #include +#include #include #include "dump_systemstate.h" @@ -88,6 +89,17 @@ static int get_disk_used_percent(const char *path) return percent; } +static dev_t get_disk_device(const char *path) +{ + assert(path); + + struct stat st; + if (stat(path, &st) < 0) + return -1; + + return st.st_dev; +} + int main(int argc, char *argv[]) { int c, ret, i, dpercent, exit_code = 0; @@ -182,6 +194,16 @@ int main(int argc, char *argv[]) spawn_wait_checked(du_args, NULL); } + if (get_disk_device("/opt") != get_disk_device("/opt/usr")) { + dpercent = get_disk_used_percent("/opt/usr"); + + if (90 < dpercent) { + fprintf_fd(out_fd, "\n==== System disk space usage detail - %d%% (/bin/du -ah /opt/usr)\n", dpercent); + char *du_args[] = {"/bin/du", "-ah", "/opt/usr", NULL}; + spawn_wait_checked(du_args, NULL); + } + } + dpercent = get_disk_used_percent("/tmp"); if (80 < dpercent) { fprintf_fd(out_fd, "\n==== tmp usage detail - %d%% (/bin/du -ah /tmp)\n", dpercent); -- 2.7.4 From ba0ee769a2a8eaf87bcaa343eea7b41dd0c0ce7c Mon Sep 17 00:00:00 2001 From: Mateusz Moscicki Date: Thu, 26 Mar 2020 14:23:28 +0100 Subject: [PATCH 05/16] Fix cmp_backtraces test Change-Id: Ia628adfb80a4945d34e9847e864c903106dba3e6 --- tests/system/cmp_backtraces/cmp_backtraces.sh.template | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/tests/system/cmp_backtraces/cmp_backtraces.sh.template b/tests/system/cmp_backtraces/cmp_backtraces.sh.template index 417cadd..5795397 100755 --- a/tests/system/cmp_backtraces/cmp_backtraces.sh.template +++ b/tests/system/cmp_backtraces/cmp_backtraces.sh.template @@ -44,8 +44,14 @@ wait_for_app minicoredumper untar_file ${BASE_DIR} ${CORE_MINI}.tar -gdb ${CRASH_WORKER_SYSTEM_TESTS}/utils/kenny ${BASE_DIR}/${CORE_ORIG} -ex "thread apply all bt" -ex "q" 2> /dev/null | grep -e '^#' > ${BASE_DIR}/${THREADS_ORIG} -gdb ${CRASH_WORKER_SYSTEM_TESTS}/utils/kenny ${BASE_DIR}/${CORE_MINI} -ex "thread apply all bt" -ex "q" 2> /dev/null | grep -e '^#' > ${BASE_DIR}/${THREADS_MINI} +# We want to compare if the call stack from the original dump is the same as +# the one from the minidump. GDB can also sometimes display the type of +# argument based on the data stored in the heap. Unfortunately, the minidump does +# not contain heap, and this causes differences in result. Therefore, the GDB +# result is filtered so that we can compare the two call stacks. + +gdb ${CRASH_WORKER_SYSTEM_TESTS}/utils/kenny ${BASE_DIR}/${CORE_ORIG} -ex "thread apply all bt" -ex "q" 2> /dev/null | grep -e '^#' | sed 's/: [0-9]\+\(x[0-9]\+\( <[^>]\+>\)\?\)\?/: 0/g' > ${BASE_DIR}/${THREADS_ORIG} +gdb ${CRASH_WORKER_SYSTEM_TESTS}/utils/kenny ${BASE_DIR}/${CORE_MINI} -ex "thread apply all bt" -ex "q" 2> /dev/null | grep -e '^#' | sed 's/: [0-9]\+\(x[0-9]\+\( <[^>]\+>\)\?\)\?/: 0/g' > ${BASE_DIR}/${THREADS_MINI} if ! diff ${BASE_DIR}/${THREADS_ORIG} ${BASE_DIR}/${THREADS_MINI} > /dev/null; then fail "backtraces are different" -- 2.7.4 From c24000a957f0256275bb9d8da8840d2ee953773e Mon Sep 17 00:00:00 2001 From: Michal Bloch Date: Thu, 2 Apr 2020 14:14:45 +0200 Subject: [PATCH 06/16] dump_systemstate: report ran command faithfully Change-Id: Id4b830804ae420b60a7250c220a56a5695f80cff Signed-off-by: Michal Bloch --- src/dump_systemstate/dump_systemstate.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/dump_systemstate/dump_systemstate.c b/src/dump_systemstate/dump_systemstate.c index 814acda..dd183e6 100644 --- a/src/dump_systemstate/dump_systemstate.c +++ b/src/dump_systemstate/dump_systemstate.c @@ -189,7 +189,7 @@ int main(int argc, char *argv[]) dpercent = get_disk_used_percent("/opt"); if (90 < dpercent) { - fprintf_fd(out_fd, "\n==== System disk space usage detail - %d%% (/bin/du -ah /opt)\n", dpercent); + fprintf_fd(out_fd, "\n==== System disk space usage detail - %d%% (/bin/du -ah /opt --exclude=/opt/usr)\n", dpercent); char *du_args[] = {"/bin/du", "-ah", "/opt", "--exclude=/opt/usr", NULL}; spawn_wait_checked(du_args, NULL); } -- 2.7.4 From cf5606eec31acfc055d6d1a4f963ca875dae326e Mon Sep 17 00:00:00 2001 From: Karol Lewandowski Date: Thu, 2 Apr 2020 13:40:03 +0200 Subject: [PATCH 07/16] Release 6.0.14 This release brings /opt/usr reporting to dump_systemstate utility. Change-Id: I1aada1d58681272703b7c64181d0c7aceccc3d84 --- packaging/crash-worker.spec | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packaging/crash-worker.spec b/packaging/crash-worker.spec index 5717544..c405a49 100644 --- a/packaging/crash-worker.spec +++ b/packaging/crash-worker.spec @@ -13,7 +13,7 @@ Name: crash-worker Summary: Coredump handler and report generator for Tizen -Version: 6.0.13 +Version: 6.0.14 Release: 1 Group: Framework/system License: Apache-2.0 and BSD-2-Clause and MIT -- 2.7.4 From 2ff06d121be26113a27b83874d1ffc3fc8caa141 Mon Sep 17 00:00:00 2001 From: Wiktor Gerstenstein Date: Wed, 19 Feb 2020 17:37:21 +0100 Subject: [PATCH 08/16] Add tizen-manifest.xml to crash-reports Change-Id: I6fc7f723ad33cd825e2c5f85b42c7b134a0f1f8b --- packaging/crash-worker.manifest | 1 + packaging/crash-worker.spec | 1 + src/crash-manager/crash-manager.c | 73 +++++++++++++++++++++- src/crash-manager/crash-manager.h | 1 + tests/system/CMakeLists.txt | 1 + .../copy_tizen_manifest.sh.template | 56 +++++++++++++++++ 6 files changed, 132 insertions(+), 1 deletion(-) create mode 100644 tests/system/copy_tizen_manifest/copy_tizen_manifest.sh.template diff --git a/packaging/crash-worker.manifest b/packaging/crash-worker.manifest index 1ed60c3..0b9ab26 100644 --- a/packaging/crash-worker.manifest +++ b/packaging/crash-worker.manifest @@ -21,6 +21,7 @@ + diff --git a/packaging/crash-worker.spec b/packaging/crash-worker.spec index 5717544..58e9041 100644 --- a/packaging/crash-worker.spec +++ b/packaging/crash-worker.spec @@ -245,6 +245,7 @@ mkdir -p %{buildroot}%{crash_temp} %{_libdir}/crash-worker/system-tests/check_minicore_mem/cp.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 %{_libdir}/crash-worker/system-tests/crash_root_path/crash_root_path.sh %{_libdir}/crash-worker/system-tests/critical_process/critical_process.sh %{_libdir}/crash-worker/system-tests/dbus_notify/dbus_notify.sh diff --git a/src/crash-manager/crash-manager.c b/src/crash-manager/crash-manager.c index 0f4cdae..682da09 100644 --- a/src/crash-manager/crash-manager.c +++ b/src/crash-manager/crash-manager.c @@ -59,6 +59,7 @@ #define CRASH_TEMP_SUBDIR "/temp/" #define CRASH_PATH_SUBDIR "/dump/" #define LIVE_PATH_SUBDIR "/livedump/" +#define APP_PATH_SUBDIR "/app/" #define WAIT_FOR_OPT_TIMEOUT_SEC 60 @@ -152,6 +153,30 @@ out_free: return have_pkgid && *pkgid != NULL; } +static char *get_app_root_path(const char *pkgid) +{ + char *root_path = NULL; + pkgmgrinfo_pkginfo_h handle; + int ret = pkgmgrinfo_pkginfo_get_pkginfo(pkgid, &handle); + if (ret != PMINFO_R_OK) + return NULL; + + ret = pkgmgrinfo_pkginfo_get_root_path(handle, &root_path); + if (ret != PMINFO_R_OK) { + _E("Unable to get app root path for %s", pkgid); + pkgmgrinfo_pkginfo_destroy_pkginfo(handle); + return NULL; + } + + char *path = root_path ? strdup(root_path) : NULL; + if (path == NULL && root_path != NULL) + _E("Failed to get app root path for %s. strdup() error: %m", pkgid); + + pkgmgrinfo_pkginfo_destroy_pkginfo(handle); + + return path; +} + static bool set_appinfo(char *exepath, char **appid, char **pkgid) { char *a = NULL, *p = NULL; @@ -381,6 +406,8 @@ bool set_crash_info(struct crash_info *cinfo) || -1 == asprintf(&cinfo->result_path, "%s/%s%s", crash_dump_path, cinfo->name, suffix)) goto out_oom; + if (is_app) + cinfo->app_root_path = get_app_root_path(cinfo->pkgid); if (mkdir(cinfo->pfx, 0775) < 0) { _E("Failed to mkdir %s: %m", cinfo->pfx); @@ -689,6 +716,47 @@ static bool process_continue(const pid_t pid) return kill_pid(pid, SIGCONT, false); } +static bool copy_application_data(struct crash_info *cinfo, const char *filename) +{ + bool is_ok = false; + char *src_filepath = NULL; + char *dst_filepath = NULL; + char *dst_dirpath = NULL; + + if (cinfo->app_root_path && + (asprintf(&src_filepath, "%s/%s", cinfo->app_root_path, filename) == -1 + || asprintf(&dst_filepath, "%s%s%s", cinfo->pfx, APP_PATH_SUBDIR, filename) == -1)) { + _E("Failed to asprintf for %s file in %s path", filename, + !src_filepath ? cinfo->app_root_path : cinfo->pfx); + goto out; + } + + if (!src_filepath || access(src_filepath, F_OK)) + goto out; + + dst_dirpath = strdup(dst_filepath); + if (dst_dirpath == NULL) { + _E("Failed to copy dst_filepath: %s", dst_filepath); + goto out; + } + dst_dirpath = dirname(dst_dirpath); + if (mkdir(dst_dirpath, 0775) < 0 && errno != EEXIST) { + _E("Failed to mkdir %s (%m)", dst_dirpath); + goto out; + } + + if (copy_file(dst_filepath, src_filepath)) + _E("Cannot copy %s file to: %s", src_filepath, dst_filepath); + else + is_ok = true; +out: + free(src_filepath); + free(dst_filepath); + free(dst_dirpath); + + return is_ok; +} + static bool execute_crash_modules(struct crash_info *cinfo) { int exit_code = 0; @@ -710,6 +778,7 @@ static bool execute_crash_modules(struct crash_info *cinfo) } execute_crash_stack(cinfo, NULL); + (void)copy_application_data(cinfo, "tizen-manifest.xml"); // manifest.xml is optional because not every application has that. if (cinfo->livedump) process_continue(cinfo->pid_info); @@ -1004,6 +1073,7 @@ static void free_crash_info(struct crash_info *cinfo) free(cinfo->cmd_path); free(cinfo->temp_dir); free(cinfo->result_path); + free(cinfo->app_root_path); free(cinfo->pfx); free(cinfo->info_path); free(cinfo->core_path); @@ -1027,7 +1097,8 @@ void crash_info_init(struct crash_info *cinfo) cinfo->cmd_line = NULL; cinfo->cmd_path = NULL; cinfo->temp_dir = NULL; - cinfo->pfx = NULL; + cinfo->app_root_path = NULL; + cinfo->pfx = NULL; cinfo->result_path = NULL; cinfo->info_path = NULL; cinfo->core_path = NULL; diff --git a/src/crash-manager/crash-manager.h b/src/crash-manager/crash-manager.h index 1e26941..6e66af1 100644 --- a/src/crash-manager/crash-manager.h +++ b/src/crash-manager/crash-manager.h @@ -38,6 +38,7 @@ struct crash_info { char *cmd_path; char *temp_dir; char *name; + char *app_root_path; char *result_path; char *pfx; char *info_path; diff --git a/tests/system/CMakeLists.txt b/tests/system/CMakeLists.txt index c17c517..37fe6ff 100644 --- a/tests/system/CMakeLists.txt +++ b/tests/system/CMakeLists.txt @@ -19,6 +19,7 @@ endmacro() configure_test("check_minicore_mem") configure_test("cmp_backtraces" "cp") +configure_test("copy_tizen_manifest") configure_test("crash_root_path") configure_test("critical_process") configure_test("dbus_notify") diff --git a/tests/system/copy_tizen_manifest/copy_tizen_manifest.sh.template b/tests/system/copy_tizen_manifest/copy_tizen_manifest.sh.template new file mode 100644 index 0000000..2e52e6e --- /dev/null +++ b/tests/system/copy_tizen_manifest/copy_tizen_manifest.sh.template @@ -0,0 +1,56 @@ +#!/bin/bash + +# Check the report for tizen-manifest.xml 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 + +pid=0 +psout=$(ps -ewo pid,cmd | awk -v OFS=$' ' '$cmd ~ "/apps/" || $cmd ~ "org."') +if [ $(wc -l <<< "$psout") -le 1 ]; then + skip "no run application found" +fi + +while IFS=$' ' read -r pidd cmd +do + cmd=${cmd%%[[:space:]]*} + for VARIABLE in 1 2 # path levels count to check + do + cmd=$(dirname $cmd) + if [ -f "$cmd/tizen-manifest.xml" ]; then + pid=$pidd + break 2 + fi + done +done <<< "$psout" + +test $pid -ne 0 || fail "no running application has a tizen-manifest file" + +kill -6 $pid + +sleep 2 + +wait_for_app crash-manager + +pushd ${CRASH_DUMP_PATH} +name=$(echo *_${pid}_*) +name=${name%.zip} + +test -f "${name}.zip" || fail "crash report not found" +unzip "${name}.zip" || fail "unable to extract archive" + +test -s "${name}/app/tizen-manifest.xml" || fail "tizen-manifest corrupt or not found" + +for i in ${CRASH_TEMP_PATH}/*; do + test -a "${i}" && fail "temp directory not cleaned up" +done + +exit_ok -- 2.7.4 From 4dbe9071ca99ae3c8f7b9b3cf2085b0648294e97 Mon Sep 17 00:00:00 2001 From: Mateusz Moscicki Date: Tue, 7 Apr 2020 13:27:12 +0200 Subject: [PATCH 09/16] Fix build id reading when the binary don't have it Change-Id: I82db306cad980104e096e8b738db24fc5e3547bb --- src/crash-manager/so-info.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/crash-manager/so-info.c b/src/crash-manager/so-info.c index 678638a..b3f76a9 100644 --- a/src/crash-manager/so-info.c +++ b/src/crash-manager/so-info.c @@ -85,13 +85,13 @@ static unsigned char *map_file(const char *filename, int64_t *size) static Elf##bits##_Off build_id_section_address##bits(const unsigned char *mapped_file, const int64_t size)\ {\ if (mapped_file <= 0)\ - return -1;\ + return 0;\ \ Elf##bits##_Ehdr *elheader = (Elf##bits##_Ehdr*)mapped_file;\ Elf##bits##_Shdr *sec = (Elf##bits##_Shdr*)(mapped_file + elheader->e_shoff);\ \ if (size < (intptr_t)sec + elheader->e_shstrndx*sizeof(Elf##bits##_Shdr) - (intptr_t)mapped_file)\ - return -2;\ + return 0;\ \ char *names = (char *)(mapped_file + sec[elheader->e_shstrndx].sh_offset);\ \ @@ -102,7 +102,7 @@ static Elf##bits##_Off build_id_section_address##bits(const unsigned char *mappe return sec[i].sh_offset;\ } \ \ - return -1;\ + return 0;\ } \ DECLARE_BUILD_ID_SECTION_ADDRESS(32) @@ -157,10 +157,12 @@ static int get_build_id(char *filename, char **build_id) if (mapped_file > 0) { Elf64_Off offset = build_id_section_address(mapped_file, size); - if (offset > 0) + if (offset > 0) { ret = get_build_id_from(mapped_file, offset, build_id); - else + } else { + _W("File %s doesn't contain build-id", filename); ret = -1; + } munmap(mapped_file, size); } else { -- 2.7.4 From 8b7e006c08c8e01d7440fee4ceae20e3dbfd3d94 Mon Sep 17 00:00:00 2001 From: Karol Lewandowski Date: Wed, 8 Apr 2020 16:33:18 +0200 Subject: [PATCH 10/16] crash-manager: Fix free of invalid pointer returned by dirname() This fixes SVACE issue (overwriting dst_dirpath pointer by dirname(3) result, which should not be freed). Change-Id: Iad2ed92c6111f8133e3c6363876ff3fad80557f3 --- src/crash-manager/crash-manager.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/crash-manager/crash-manager.c b/src/crash-manager/crash-manager.c index 682da09..e7ff778 100644 --- a/src/crash-manager/crash-manager.c +++ b/src/crash-manager/crash-manager.c @@ -735,13 +735,14 @@ static bool copy_application_data(struct crash_info *cinfo, const char *filename goto out; dst_dirpath = strdup(dst_filepath); - if (dst_dirpath == NULL) { - _E("Failed to copy dst_filepath: %s", dst_filepath); + char *dst_dirname = dirname(dst_dirpath); + if (dst_dirpath == NULL || dst_dirname == NULL) { + _E("Out of memory"); goto out; } - dst_dirpath = dirname(dst_dirpath); - if (mkdir(dst_dirpath, 0775) < 0 && errno != EEXIST) { - _E("Failed to mkdir %s (%m)", dst_dirpath); + + if (mkdir(dst_dirname, 0775) < 0 && errno != EEXIST) { + _E("Failed to mkdir %s: %m", dst_dirname); goto out; } -- 2.7.4 From 2e5c4e13576fcda223207b2d654c6f0e10a4eed9 Mon Sep 17 00:00:00 2001 From: Karol Lewandowski Date: Wed, 8 Apr 2020 16:33:50 +0200 Subject: [PATCH 11/16] crash-manager: Clarify when tizen-manifest.xml is copied Change-Id: Iba48fc54f9809c077b5ebc0fbe8463186abd767e --- src/crash-manager/crash-manager.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/crash-manager/crash-manager.c b/src/crash-manager/crash-manager.c index e7ff778..c889cdb 100644 --- a/src/crash-manager/crash-manager.c +++ b/src/crash-manager/crash-manager.c @@ -779,7 +779,7 @@ static bool execute_crash_modules(struct crash_info *cinfo) } execute_crash_stack(cinfo, NULL); - (void)copy_application_data(cinfo, "tizen-manifest.xml"); // manifest.xml is optional because not every application has that. + (void)copy_application_data(cinfo, "tizen-manifest.xml"); // manifest is optional because only tizen applications have it if (cinfo->livedump) process_continue(cinfo->pid_info); -- 2.7.4 From 9780ef741d88fa958ecaeb9abf08be3e8b346ae5 Mon Sep 17 00:00:00 2001 From: Mateusz Moscicki Date: Tue, 7 Apr 2020 10:06:06 +0200 Subject: [PATCH 12/16] Fix libcrash-service test Because of second trap, core_pattern didn't have the correct value restored Change-Id: I71c6f2c401c293769caa816fe2e767b30578d7b6 --- tests/system/libcrash-service/libcrash-service.sh.template | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/system/libcrash-service/libcrash-service.sh.template b/tests/system/libcrash-service/libcrash-service.sh.template index 611978a..97b81cf 100755 --- a/tests/system/libcrash-service/libcrash-service.sh.template +++ b/tests/system/libcrash-service/libcrash-service.sh.template @@ -29,8 +29,6 @@ wait_for_file ${LIVE_DUMP_PATH}/kenny*zip kill -9 ${KENNY_PID} -trap popd 0 - pushd ${LIVE_DUMP_PATH} unzip kenny*zip -- 2.7.4 From 1415476247dd8889c0e9c9a53976816c8994d923 Mon Sep 17 00:00:00 2001 From: Mateusz Moscicki Date: Tue, 7 Apr 2020 10:30:24 +0200 Subject: [PATCH 13/16] Fix report_basic test Add a delay to m ake sure that crash-manager has the time to run Change-Id: I26fd47126d1d8b14ed8da6547f88275e2cbba116 --- tests/system/report_basic/report_basic.sh.template | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/system/report_basic/report_basic.sh.template b/tests/system/report_basic/report_basic.sh.template index 18d7836..e78ad12 100644 --- a/tests/system/report_basic/report_basic.sh.template +++ b/tests/system/report_basic/report_basic.sh.template @@ -18,6 +18,8 @@ pid=$! sleep 2 kill -6 $pid +sleep 2 + wait_for_app crash-manager pushd ${CRASH_DUMP_PATH} -- 2.7.4 From 3168078e08c44562c15111eaadcfb8dfae1449c5 Mon Sep 17 00:00:00 2001 From: Mateusz Moscicki Date: Fri, 3 Apr 2020 15:42:39 +0200 Subject: [PATCH 14/16] 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 --- packaging/crash-worker.spec | 2 + src/crash-manager/crash-manager.c | 118 ++++++++++++++++++++----- src/crash-manager/crash-manager.h | 1 + tests/system/CMakeLists.txt | 2 + tests/system/clean_temp/clean_temp.sh.template | 33 +++++++ tests/system/temp_lock/temp_lock.sh.template | 37 ++++++++ tests/system/utils/minicore-utils.sh | 13 +++ 7 files changed, 185 insertions(+), 21 deletions(-) create mode 100644 tests/system/clean_temp/clean_temp.sh.template create mode 100644 tests/system/temp_lock/temp_lock.sh.template diff --git a/packaging/crash-worker.spec b/packaging/crash-worker.spec index 93d577d..736f684 100644 --- 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 --- 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 --- 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 --- 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 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 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 --- 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; -- 2.7.4 From 9274c50f31c9fde4b0bcb297271b49680e24548b Mon Sep 17 00:00:00 2001 From: Mateusz Moscicki Date: Tue, 14 Apr 2020 11:55:41 +0200 Subject: [PATCH 15/16] Move unlocking and removing of the temp dir to a separate function Change-Id: If38fcfda5737e8d39cb496692ca663e2a3d7c6a2 --- src/crash-manager/crash-manager.c | 27 +++++++++++++++++++-------- 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/src/crash-manager/crash-manager.c b/src/crash-manager/crash-manager.c index 3ae4981..8a9ec5a 100644 --- a/src/crash-manager/crash-manager.c +++ b/src/crash-manager/crash-manager.c @@ -1111,12 +1111,6 @@ static bool move_dump_data(const char *from_path, const struct crash_info *cinfo is_ok = false; } 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; } @@ -1299,6 +1293,17 @@ static void write_dump_reason(const char *reason, const char *base_dir, const ch } } +static void crash_manager_cleanup(struct crash_info *cinfo) +{ + assert(cinfo); + unlock_dir(cinfo->lock_fd); + + if (remove_dir(cinfo->temp_dir, 1) < 0) + _E("Failed to delete temp directory"); + + clean_temp(crash_temp_path); +} + void crash_manager_free(struct crash_info *cinfo) { if (cinfo->prstatus_fd >= 0) @@ -1315,7 +1320,10 @@ bool crash_manager_direct(struct crash_info *cinfo) if (!crash_manager_prepare(cinfo)) return false; - return run(cinfo); + bool result = run(cinfo); + crash_manager_cleanup(cinfo); + + return result; } bool crash_manager_livedump_pid(pid_t pid, const char *dump_reason, char *report_path, size_t report_path_len) @@ -1332,7 +1340,10 @@ bool crash_manager_livedump_pid(pid_t pid, const char *dump_reason, char *report write_dump_reason(dump_reason, cinfo.pfx, cinfo.name); - if (!run(&cinfo)) + result = run(&cinfo); + crash_manager_cleanup(&cinfo); + + if (!result) goto exit; strncpy(report_path, cinfo.result_path, report_path_len); -- 2.7.4 From 7f9dfb70a115d383d3a20d0a17cc0dcbd2c2c67e Mon Sep 17 00:00:00 2001 From: Mateusz Moscicki Date: Tue, 14 Apr 2020 13:36:19 +0200 Subject: [PATCH 16/16] Use bool instead of int in remove.*() functions In the modified places the int type was used as a bool, so the function signatures were changed. Change-Id: Ic172432c33c06d40c1f71ca8137606d7ea102acf --- src/crash-manager/crash-manager.c | 24 ++++++++++++------------ src/shared/util.c | 28 +++++++++++++++------------- src/shared/util.h | 2 +- 3 files changed, 28 insertions(+), 26 deletions(-) diff --git a/src/crash-manager/crash-manager.c b/src/crash-manager/crash-manager.c index 8a9ec5a..b64908c 100644 --- a/src/crash-manager/crash-manager.c +++ b/src/crash-manager/crash-manager.c @@ -430,7 +430,7 @@ static bool clean_temp(const char *temp_dir) if (is_locked(dir_name)) _D("Temporary directory %s is locked", dir_name); - else if (remove_dir(dir_name, 1) == -1) + else if (!remove_dir(dir_name, true)) _W("Can not remove temporary directory: %s", dir_name); else _D("Temporary directory %s removed", dir_name); @@ -494,7 +494,7 @@ bool set_crash_info(struct crash_info *cinfo) out_rm_temp: unlock_dir(cinfo->lock_fd); - remove_dir(cinfo->temp_dir, 1); + remove_dir(cinfo->temp_dir, true); clean_temp(crash_temp_path); return false; @@ -539,9 +539,9 @@ static void save_so_info(const struct crash_info *cinfo) /* remove a file whose name begins with 'beggining_of_name'. This is needed * when we don't want to include coredump in report, but we only know the * beginning of the coredump file name. */ -static int remove_file_in_dir(const char *directory, const char *beginning_of_name) +static bool remove_file_in_dir(const char *directory, const char *beginning_of_name) { - int ret = -1; + bool ret = false; int errno_unlink = 0; DIR *dir = opendir(directory); @@ -557,7 +557,7 @@ static int remove_file_in_dir(const char *directory, const char *beginning_of_na struct dirent* file_info; while ((file_info = readdir(dir)) != NULL) { if (strstr(file_info->d_name, beginning_of_name) == file_info->d_name) { - ret = unlinkat(dir_fd, file_info->d_name, 0); + ret = unlinkat(dir_fd, file_info->d_name, 0) != -1; errno_unlink = errno; break; } @@ -680,7 +680,7 @@ static bool execute_minicoredump(struct crash_info *cinfo, int *exit_code) /* Minicoredumper must be executed to dump at least PRSTATUS for other tools, coredump, however, might have been disabled. */ if (!config.dump_core && file_exists_in_dir(cinfo->pfx, coredump_name)) { - if (remove_file_in_dir(cinfo->pfx, coredump_name) != 0) + if (!remove_file_in_dir(cinfo->pfx, coredump_name)) _E("Saving core disabled - removing coredump %s/%s failed: %m", cinfo->pfx, coredump_name); else @@ -1006,12 +1006,12 @@ static int check_disk_available(const char *path, int check_size) return 0; } -static int remove_file_info(struct file_info file) +static bool remove_file_info(struct file_info file) { if (file.isdir) - return remove_dir(file.path, 1); + return remove_dir(file.path, true); else - return unlink(file.path); + return unlink(file.path) != -1; } static void clean_dump(void) @@ -1068,7 +1068,7 @@ static void clean_dump(void) if (!remove_flag) continue; - if (remove_file_info(dump_list[i]) < 0) { + if (!remove_file_info(dump_list[i])) { _E("Failed to remove %s", dump_list[i].path); continue; } @@ -1298,8 +1298,8 @@ static void crash_manager_cleanup(struct crash_info *cinfo) assert(cinfo); unlock_dir(cinfo->lock_fd); - if (remove_dir(cinfo->temp_dir, 1) < 0) - _E("Failed to delete temp directory"); + if (!remove_dir(cinfo->temp_dir, true)) + _E("Failed to delete temp directory: %s", cinfo->temp_dir); clean_temp(crash_temp_path); } diff --git a/src/shared/util.c b/src/shared/util.c index 9e90b64..667f28e 100644 --- a/src/shared/util.c +++ b/src/shared/util.c @@ -205,15 +205,16 @@ int make_dir(const char *path, const char *name, int mode) return r == 0 || (r == -1 && errno == EEXIST) ? 0 : -1; } -static int remove_dir_internal(int fd) +static bool remove_dir_internal(int fd) { DIR *dir; struct dirent *de; - int subfd, ret = 0; + bool ret = true; + int subfd = 0; dir = fdopendir(fd); if (!dir) - return -1; + return false; while ((de = readdir(dir))) { if (de->d_type == DT_DIR) { @@ -222,20 +223,20 @@ static int remove_dir_internal(int fd) subfd = openat(fd, de->d_name, O_RDONLY | O_DIRECTORY); if (subfd < 0) { _E("Couldn't openat %s: %d\n", de->d_name, errno); - ret = -1; + ret = false; continue; } - if (remove_dir_internal(subfd)) - ret = -1; + if (!remove_dir_internal(subfd)) + ret = false; close(subfd); if (unlinkat(fd, de->d_name, AT_REMOVEDIR) < 0) { _E("Couldn't unlinkat %s: %d\n", de->d_name, errno); - ret = -1; + ret = false; } } else { if (unlinkat(fd, de->d_name, 0) < 0) { _E("Couldn't unlinkat %s: %d\n", de->d_name, errno); - ret = -1; + ret = false; } } } @@ -243,16 +244,17 @@ static int remove_dir_internal(int fd) return ret; } -int remove_dir(const char *path, int del_dir) +bool remove_dir(const char *path, bool del_dir) { - int fd, ret = 0; + bool ret = true; + int fd = 0; if (!path) - return -1; + return false; fd = open(path, O_RDONLY | O_NONBLOCK | O_DIRECTORY | O_CLOEXEC | O_NOFOLLOW); if (fd < 0) { _E("Couldn't opendir %s: %d\n", path, errno); - return -errno; + return false; } ret = remove_dir_internal(fd); close(fd); @@ -260,7 +262,7 @@ int remove_dir(const char *path, int del_dir) if (del_dir) { if (rmdir(path)) { _E("Couldn't rmdir %s: %d\n", path, errno); - ret = -1; + ret = false; } } return ret; diff --git a/src/shared/util.h b/src/shared/util.h index a4325ba..3de0ce1 100644 --- a/src/shared/util.h +++ b/src/shared/util.h @@ -46,7 +46,7 @@ int fsync_path(char *const path); int make_dir(const char *path, const char *name, int mode); -int remove_dir(const char *path, int del_dir); +bool remove_dir(const char *path, bool del_dir); int get_exec_pid(const char *execpath); -- 2.7.4