From 928ff71c8ed1b7c7be6ff9f8a53bd12d0ae5e069 Mon Sep 17 00:00:00 2001 From: Karol Lewandowski Date: Tue, 17 Dec 2019 14:56:06 +0100 Subject: [PATCH 01/16] crash-service: Stability monitor does not need to own crash-worker's dbus name Change-Id: Ida73ce27ac6ea040dcffb81f3a61e4096dc81940 --- src/crash-service/crash-service.conf | 1 - 1 file changed, 1 deletion(-) diff --git a/src/crash-service/crash-service.conf b/src/crash-service/crash-service.conf index 1bdcfb0..02bd574 100644 --- a/src/crash-service/crash-service.conf +++ b/src/crash-service/crash-service.conf @@ -14,7 +14,6 @@ send_member="livedump_pid"/> - -- 2.7.4 From 83045734d9c70a6b7d13ff504dccd3cacafffdbf Mon Sep 17 00:00:00 2001 From: Karol Lewandowski Date: Fri, 13 Dec 2019 13:11:08 +0100 Subject: [PATCH 02/16] config: Separate config loading from processing Change-Id: Ief38894e03904f9947ee72773f07044280f62b41 --- src/shared/config.c | 123 +++++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 98 insertions(+), 25 deletions(-) diff --git a/src/shared/config.c b/src/shared/config.c index 3154752..1e728c4 100644 --- a/src/shared/config.c +++ b/src/shared/config.c @@ -49,9 +49,55 @@ enum ReportType report_type_from_str(const char *report_type_str) return REP_TYPE_INVALID; } -bool config_init(config_t *c, const char *const path) +static bool config_load_from_dict(config_t *c, dictionary *ini) { assert(c); + + char *str = iniparser_getstring(ini, CRASH_SECTION ":CrashRootPath", NULL); + if (str) { + char *crash_root_path = strdup(str); + if (!crash_root_path) { + _E("config: Unable to set CrashRootPath - aborting"); + return false; + } + + free(c->crash_root_path); + c->crash_root_path = crash_root_path; + } + + /* strdup() can technically fail, but we don't mind. It is better to + * create a report without the extra script than to abort completely. */ + str = iniparser_getstring(ini, CRASH_SECTION ":ExtraScript", NULL); + if (str) { + char *extra_script = strdup(str); + if (extra_script) { + free(c->extra_script); + c->extra_script = extra_script; + } + } + + str = iniparser_getstring(ini, CRASH_SECTION ":ReportType", NULL); + if (str) { + int type = report_type_from_str(str); + if (report_type_to_str(type)) + c->report_type = type; + } + +#define UPDATE(where, type, key) where = iniparser_get##type(ini, CRASH_SECTION ":" key, where) + + UPDATE(c->system_max_use, int, "SystemMaxUse"); + UPDATE(c->max_retention_sec, int, "MaxRetentionSec"); + UPDATE(c->max_crash_dump, int, "MaxCrashDump"); + UPDATE(c->dump_core, boolean, "DumpCore"); + UPDATE(c->allow_zip, boolean, "AllowZip"); + +#undef UPDATE + + return true; +} + +static bool config_load_from_path(config_t *c, const char *const path) +{ assert(path); dictionary *ini = iniparser_load(path); @@ -60,37 +106,64 @@ bool config_init(config_t *c, const char *const path) return false; } - bool ret = false; + bool ret = config_load_from_dict(c, ini); + iniparser_freedict(ini); -#define GET(type, key, defval) iniparser_get##type(ini, CRASH_SECTION ":" key, defval) + return ret; +} - c->crash_root_path = strdup(GET(string, "CrashRootPath", CRASH_ROOT_PATH)); - if (!c->crash_root_path) - goto out; +static bool config_apply_defaults(config_t *c) +{ + assert(c); - /* strdup() can technically fail, but we don't mind. It is better to - * create a report without the extra script than to abort completely. */ - char *extrascript = GET(string, "ExtraScript", NULL); - c->extra_script = extrascript ? strdup(extrascript) : NULL; + memset(c, 0, sizeof(*c)); - char *reptype = GET(string, "ReportType", (char *)report_type_strmap[REP_TYPE_FULL]); - c->report_type = report_type_from_str(reptype); - if (!report_type_to_str(c->report_type)) - goto out; + c->crash_root_path = strdup(CRASH_ROOT_PATH); + c->report_type = REP_TYPE_FULL; + c->system_max_use = SYSTEM_MAX_USE; + c->system_keep_free = SYSTEM_KEEP_FREE; + c->max_retention_sec = MAX_RETENTION_SEC; + c->max_crash_dump = MAX_CRASH_DUMP; + c->dump_core = DUMP_CORE; + c->allow_zip = ALLOW_ZIP; - c->system_max_use = GET(int, "SystemMaxUse", SYSTEM_MAX_USE); - c->system_keep_free = GET(int, "SystemKeepFree", SYSTEM_KEEP_FREE); - c->max_retention_sec = GET(int, "MaxRetentionSec", MAX_RETENTION_SEC); - c->max_crash_dump = GET(int, "MaxCrashDump", MAX_CRASH_DUMP); - c->dump_core = GET(boolean, "DumpCore", DUMP_CORE); - c->allow_zip = GET(boolean, "AllowZip", ALLOW_ZIP); + return c->crash_root_path != NULL; +} -#undef GET +static void config_dump(config_t *c) +{ + assert(c); - ret = true; -out: - iniparser_freedict(ini); - return ret; + _D("config: crash_root_path = %s\n" + "config: extra_script = %s\n" + "config: report_type = %s\n" + "config: system_max_use = %d\n" + "config: system_keep_free = %d\n" + "config: max_retention_sec = %d\n" + "config: max_crash_dump = %d\n" + "config: dump_core = %d\n" + "config: allow_zip = %d\n", + c->crash_root_path, + c->extra_script, + report_type_to_str(c->report_type), + c->system_max_use, + c->system_keep_free, + c->max_retention_sec, + c->max_crash_dump, + c->dump_core, + c->allow_zip); +} + +bool config_init(config_t *c, const char *const path) +{ + if (!config_apply_defaults(c) || !config_load_from_path(c, path)) { + _E("config: Unable to initialize configuration"); + return false; + } + + config_dump(c); + + return true; } void config_free(config_t *c) -- 2.7.4 From cdca0baf4ce0cda8a866db7e25d544636e4e23ec Mon Sep 17 00:00:00 2001 From: Karol Lewandowski Date: Wed, 18 Dec 2019 10:41:09 +0100 Subject: [PATCH 03/16] config: Search for configuration also under crash-manager.conf.d directory Change-Id: I9c5680eafb467a0514feec5225cb794609ba1823 --- packaging/crash-worker.spec | 1 + src/crash-manager/CMakeLists.txt | 5 ++ src/crash-manager/crash-manager.conf.d.example | 3 ++ src/shared/config.c | 63 ++++++++++++++++++++++ .../crash_root_path/crash_root_path.sh.template | 17 ++++-- 5 files changed, 84 insertions(+), 5 deletions(-) create mode 100644 src/crash-manager/crash-manager.conf.d.example diff --git a/packaging/crash-worker.spec b/packaging/crash-worker.spec index 08bcafb..1557f20 100644 --- a/packaging/crash-worker.spec +++ b/packaging/crash-worker.spec @@ -161,6 +161,7 @@ mkdir -p %{buildroot}%{crash_temp} %dir %{crash_path} %dir %{crash_temp} %{_sysconfdir}/crash-manager.conf +%{_sysconfdir}/crash-manager.conf.d/crash-manager.conf.example %attr(-,root,root) %{_prefix}/lib/sysctl.d/70-crash-manager.conf %attr(0750,crash_worker,crash_worker) %{_bindir}/crash-manager %attr(0750,crash_worker,crash_worker) %{_bindir}/dump_systemstate diff --git a/src/crash-manager/CMakeLists.txt b/src/crash-manager/CMakeLists.txt index 591a3ad..2fdf2a2 100644 --- a/src/crash-manager/CMakeLists.txt +++ b/src/crash-manager/CMakeLists.txt @@ -69,6 +69,11 @@ INSTALL(FILES ${CMAKE_SOURCE_DIR}/src/${PROJECT_NAME}/crash-manager.conf DESTINATION /etc PERMISSIONS OWNER_READ OWNER_WRITE GROUP_READ WORLD_READ) +INSTALL(FILES ${CMAKE_SOURCE_DIR}/src/${PROJECT_NAME}/crash-manager.conf.d.example + DESTINATION /etc/crash-manager.conf.d/ + PERMISSIONS OWNER_READ OWNER_WRITE GROUP_READ WORLD_READ + RENAME crash-manager.conf.example) + INSTALL(FILES ${CMAKE_SOURCE_DIR}/src/${PROJECT_NAME}/crash-manager.pc DESTINATION ${CMAKE_INSTALL_LIBDIR}/pkgconfig PERMISSIONS OWNER_READ OWNER_WRITE GROUP_READ WORLD_READ) diff --git a/src/crash-manager/crash-manager.conf.d.example b/src/crash-manager/crash-manager.conf.d.example new file mode 100644 index 0000000..b50dbff --- /dev/null +++ b/src/crash-manager/crash-manager.conf.d.example @@ -0,0 +1,3 @@ +# Rename this file to end with .conf for it to be read by crash-manager +[CrashManager] +DumpCore=0 diff --git a/src/shared/config.c b/src/shared/config.c index 1e728c4..901bab0 100644 --- a/src/shared/config.c +++ b/src/shared/config.c @@ -15,10 +15,14 @@ * limitations under the License. */ #include +#include +#include #include #include #include #include +#include +#include #include "config.h" #include "defs.h" @@ -112,6 +116,63 @@ static bool config_load_from_path(config_t *c, const char *const path) return ret; } +static int entry_filter(const struct dirent *e) +{ + assert(e); + + const char *const conf_suffix = ".conf"; + const char *const name = e->d_name; + int len = strlen(name); + + if (e->d_type != DT_REG || len <= strlen(conf_suffix)) + return 0; + + // accept only files ending with predefined suffix + return strcmp(name + len - strlen(conf_suffix), conf_suffix) == 0; +} + +static bool config_load_from_dir_prefix(config_t *c, const char *const dir_prefix) +{ + assert(dir_prefix); + + char dir_path[PATH_MAX]; + int ret = snprintf(dir_path, sizeof(dir_path), "%s.d", dir_prefix); + if (ret < 0 || ret >= PATH_MAX) { + _W("config: internal error while trying to prepare config dir path"); + return false; + } + + int fd = open(dir_path, O_RDONLY | O_DIRECTORY); + if (fd < 0 && errno == ENOENT) + return true; + if (fd < 0) { + _E("config: Unable to access config directory at %s: %m", dir_path); + return false; + } + + struct dirent **entries = NULL; + int n = scandirat(fd, ".", &entries, entry_filter, alphasort); + if (n < 0) + goto out; + + for (int i = 0; i < n; ++i) { + char file_path[PATH_MAX]; + const char *fname = entries[i]->d_name; + ret = snprintf(file_path, sizeof(file_path), "%s/%s", dir_path, fname); + if (ret < 0 || ret >= PATH_MAX) { + _W("config: internal error while trying to prepare for reading %s config file", fname); + continue; + } + _D("config: reading additional configuration file from %s", file_path); + (void)config_load_from_path(c, file_path); + } + +out: + free(entries); + close(fd); + return true; +} + static bool config_apply_defaults(config_t *c) { assert(c); @@ -161,6 +222,8 @@ bool config_init(config_t *c, const char *const path) return false; } + (void)config_load_from_dir_prefix(c, path); + config_dump(c); return true; diff --git a/tests/system/crash_root_path/crash_root_path.sh.template b/tests/system/crash_root_path/crash_root_path.sh.template index 0f59b55..7fe5e96 100644 --- a/tests/system/crash_root_path/crash_root_path.sh.template +++ b/tests/system/crash_root_path/crash_root_path.sh.template @@ -8,7 +8,9 @@ fi . ${CRASH_WORKER_SYSTEM_TESTS}/utils/minicore-utils.sh -CRASH_MANAGER_CONF=/etc/crash-manager.conf +CONF_DIR=/etc/crash-manager.conf.d +CONF_FILE=${CONF_DIR}/crash-root-path-test.conf + NEW_PATH=/tmp/crash_path_test/ if [ ! -d ${NEW_PATH} ]; then mkdir ${NEW_PATH} @@ -23,8 +25,13 @@ function check_file_exists { } mount -o rw,remount / -backup_file ${CRASH_MANAGER_CONF} -cat ${CRASH_MANAGER_CONF}.backup | sed "s|#[ ]\+CrashRootPath=.*|CrashRootPath=${NEW_PATH}|" > ${CRASH_MANAGER_CONF} + +mkdir -p $CONF_DIR +rm -f $CONF_FILE || : +cat <$CONF_FILE +[CrashManager] +CrashRootPath=${NEW_PATH} +EOF { ${CRASH_WORKER_SYSTEM_TESTS}/utils/kenny & @@ -34,10 +41,10 @@ cat ${CRASH_MANAGER_CONF}.backup | sed "s|#[ ]\+CrashRootPath=.*|CrashRootPath=$ sleep 2 -restore_file ${CRASH_MANAGER_CONF} - wait_for_app crash-manager +rm -f ${CONF_FILE} || : + pushd ${NEW_PATH}/dump if ! unzip kenny*zip > /dev/null; then popd -- 2.7.4 From 584b89152d0e30db496529eb7da83cb7424882c8 Mon Sep 17 00:00:00 2001 From: Karol Lewandowski Date: Fri, 20 Dec 2019 20:32:46 +0100 Subject: [PATCH 04/16] config: Ensure freed pointers are set to null Change-Id: I779cff27a6873187e5f17415469864d6ccd0d1b7 --- src/shared/config.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/shared/config.c b/src/shared/config.c index 901bab0..d389187 100644 --- a/src/shared/config.c +++ b/src/shared/config.c @@ -235,4 +235,6 @@ void config_free(config_t *c) free(c->crash_root_path); free(c->extra_script); + + memset(c, 0, sizeof(*c)); } -- 2.7.4 From 615efb3d6e5051cdc79eec3b30cec88dacbdf4f9 Mon Sep 17 00:00:00 2001 From: Karol Lewandowski Date: Wed, 18 Dec 2019 15:36:27 +0100 Subject: [PATCH 05/16] Release 6.0.5 This release brings ability to extend configuration via snippets in /etc/crash-manager.conf.d/ directory. Change-Id: Ic01f780a2bb3868e48f02bbe95ac725adcb384cd --- packaging/crash-worker.spec | 2 +- packaging/crash-worker_system-tests.spec | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packaging/crash-worker.spec b/packaging/crash-worker.spec index 1557f20..9b9a62f 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.4 +Version: 6.0.5 Release: 1 Group: Framework/system License: Apache-2.0 and BSD-2-Clause and MIT diff --git a/packaging/crash-worker_system-tests.spec b/packaging/crash-worker_system-tests.spec index e1dc4a0..f0be398 100644 --- a/packaging/crash-worker_system-tests.spec +++ b/packaging/crash-worker_system-tests.spec @@ -8,7 +8,7 @@ Name: crash-worker_system-tests Summary: Package with binaries and scripts for crash-worker system tests -Version: 6.0.4 +Version: 6.0.5 Release: 1 Group: Framework/system License: Apache-2.0 and BSD -- 2.7.4 From 82a4e1e87e0631904804439ff9afa8ece2ef329d Mon Sep 17 00:00:00 2001 From: Karol Lewandowski Date: Fri, 3 Jan 2020 09:16:48 +0100 Subject: [PATCH 06/16] crash-manager: Make permission handling in maps more abstract This also fixes gcc9.2 build error. Change-Id: Iae099a366063cd69cca840bd83121a8930d89088 --- src/crash-manager/so-info.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/src/crash-manager/so-info.c b/src/crash-manager/so-info.c index a0ae2dc..d95a927 100644 --- a/src/crash-manager/so-info.c +++ b/src/crash-manager/so-info.c @@ -169,12 +169,20 @@ static int get_build_id(char *filename, char **build_id) return ret; } -static int get_perms(char *line, char *perms) +enum Perms { + PERM_READ = 1, + PERM_WRITE = 2, + PERM_EXEC = 4 +}; + +/* Parses beginning of the maps file in the form: `addr1-addr2 r-xp size ...` */ +static int get_perms(char *line, enum Perms *perms) { char *first_space = strchr(line, ' '); - if (first_space > 0) { - strncpy(perms, first_space+1, 4); + if (first_space != 0 && strlen(first_space) > 4 /* eg. " r-xp" from example above */) { + char *p = first_space + 1; + *perms = (p[0] == 'r' ? PERM_READ : 0) | (p[1] == 'w' ? PERM_WRITE : 0) | (p[2] == 'x' ? PERM_EXEC : 0); return 0; } else return -1; @@ -186,9 +194,9 @@ static int get_perms(char *line, char *perms) */ static char *get_exe_filename(char *line) { - char perms[4]; + enum Perms perms; - if (get_perms(line, perms) == 0 && perms[2] == 'x') { + if (get_perms(line, &perms) == 0 && (perms & PERM_EXEC)) { char *p = strstr(line, " /"); if (p > 0) return p+1; -- 2.7.4 From f24c53fb208eca0307ab55d1ae9157118b15ab2a Mon Sep 17 00:00:00 2001 From: Karol Lewandowski Date: Tue, 17 Dec 2019 14:58:15 +0100 Subject: [PATCH 07/16] crash-service: Restrict allow livedump API via privilege Use newly created http://tizen.org/privilege/internal/livecoredump privilege to restrict access to crash-service's livedump dbus interface. Change-Id: I9b2f560c061426d561674e3560b60c10e36c384e --- src/crash-service/crash-service.conf | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/crash-service/crash-service.conf b/src/crash-service/crash-service.conf index 02bd574..7aac3d0 100644 --- a/src/crash-service/crash-service.conf +++ b/src/crash-service/crash-service.conf @@ -13,13 +13,12 @@ send_interface="org.tizen.system.crash.livedump" send_member="livedump_pid"/> - - - + -- 2.7.4 From deb008ea555989885a91b19ecec4829b8301be02 Mon Sep 17 00:00:00 2001 From: Karol Lewandowski Date: Tue, 7 Jan 2020 10:24:04 +0100 Subject: [PATCH 08/16] Release 6.0.6 This release brings following changes: - Use designated privilege to limit livedump API. This replaces temporary use of per-used privilege check in dbus policy. - Gcc 9.x build fix Change-Id: I2bad2792af9aefbd210cd1537293823a664d1a8f --- packaging/crash-worker.spec | 2 +- packaging/crash-worker_system-tests.spec | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packaging/crash-worker.spec b/packaging/crash-worker.spec index 9b9a62f..4376521 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.5 +Version: 6.0.6 Release: 1 Group: Framework/system License: Apache-2.0 and BSD-2-Clause and MIT diff --git a/packaging/crash-worker_system-tests.spec b/packaging/crash-worker_system-tests.spec index f0be398..0afd6c9 100644 --- a/packaging/crash-worker_system-tests.spec +++ b/packaging/crash-worker_system-tests.spec @@ -8,7 +8,7 @@ Name: crash-worker_system-tests Summary: Package with binaries and scripts for crash-worker system tests -Version: 6.0.5 +Version: 6.0.6 Release: 1 Group: Framework/system License: Apache-2.0 and BSD -- 2.7.4 From 578274ea612465995c2bda0a874d5c7d6c754276 Mon Sep 17 00:00:00 2001 From: Karol Lewandowski Date: Thu, 9 Jan 2020 11:22:00 +0100 Subject: [PATCH 09/16] Adjust codebase for iniparser upgrade (4.x) Change-Id: I3d8e613b8d1cccbbbf84b7dceac5abde57578fe6 --- src/dump_systemstate/extras.c | 18 +++++++++--------- src/shared/config.c | 4 ++-- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/dump_systemstate/extras.c b/src/dump_systemstate/extras.c index 90625a5..2e6939c 100644 --- a/src/dump_systemstate/extras.c +++ b/src/dump_systemstate/extras.c @@ -62,7 +62,7 @@ static const size_t MAX_INI_KEY_LEN = 5; struct extra_dump_item { // not separate named fields, for convenient iteration - char *fields[COUNT_INI_FIELDS]; + const char *fields[COUNT_INI_FIELDS]; }; int handle_extra_program(int out_fd, struct extra_dump_item *item) @@ -70,10 +70,10 @@ int handle_extra_program(int out_fd, struct extra_dump_item *item) assert(out_fd >= 0); assert(item); - char *const title = item->fields[INI_FIELD_TITLE]; - char *const path = item->fields[INI_FIELD_PATH]; - char *const args = item->fields[INI_FIELD_ARGS] ?: ""; - char *const env = item->fields[INI_FIELD_ENV] ?: ""; + const char *const title = item->fields[INI_FIELD_TITLE]; + const char *const path = item->fields[INI_FIELD_PATH]; + const char *const args = item->fields[INI_FIELD_ARGS] ?: ""; + const char *const env = item->fields[INI_FIELD_ENV] ?: ""; if (!title || !path) { fprintf_fd(out_fd, "\nNo title or path in extra program config"); @@ -109,8 +109,8 @@ int handle_extra_file(int out_fd, struct extra_dump_item *item) assert(out_fd >= 0); assert(item); - char *const title = item->fields[INI_FIELD_TITLE]; - char *const path = item->fields[INI_FIELD_PATH]; + const char *const title = item->fields[INI_FIELD_TITLE]; + const char *const path = item->fields[INI_FIELD_PATH]; if (!title || !path) { fprintf_fd(out_fd, "\nNo title or path in extra file config"); return EXIT_CONFERR; @@ -127,7 +127,7 @@ int handle_extra_file(int out_fd, struct extra_dump_item *item) typedef int (*handle_ini_section_t)(int out_fd, struct extra_dump_item *); -static int handle_ini_Nth_section(int out_fd, dictionary *ini, int n, handle_ini_section_t handle_ini_section) +static int handle_ini_Nth_section(int out_fd, const dictionary *ini, int n, handle_ini_section_t handle_ini_section) { assert(out_fd >= 0); assert(ini); @@ -135,7 +135,7 @@ static int handle_ini_Nth_section(int out_fd, dictionary *ini, int n, handle_ini assert(n < iniparser_getnsec(ini)); assert(handle_ini_section); - char *const secname = iniparser_getsecname(ini, n); + const char *const secname = iniparser_getsecname(ini, n); assert(secname); // can only be NULL if `ini` is NULL or `n` is outta bounds const size_t secname_len = strlen(secname); diff --git a/src/shared/config.c b/src/shared/config.c index d389187..88df6a2 100644 --- a/src/shared/config.c +++ b/src/shared/config.c @@ -53,11 +53,11 @@ enum ReportType report_type_from_str(const char *report_type_str) return REP_TYPE_INVALID; } -static bool config_load_from_dict(config_t *c, dictionary *ini) +static bool config_load_from_dict(config_t *c, const dictionary *ini) { assert(c); - char *str = iniparser_getstring(ini, CRASH_SECTION ":CrashRootPath", NULL); + const char *str = iniparser_getstring(ini, CRASH_SECTION ":CrashRootPath", NULL); if (str) { char *crash_root_path = strdup(str); if (!crash_root_path) { -- 2.7.4 From 11f31feaebfab49492c93d7ea0236bd8686030a0 Mon Sep 17 00:00:00 2001 From: Kunhoon Baik Date: Tue, 21 Jan 2020 16:47:59 +0900 Subject: [PATCH 10/16] Add option to support legacy DBus 'ProcessCrashed' signal Original author: Kun-Hoon Baik Reworked by: Karol Lewandowski Change-Id: I64b950b99d81b2a0a4ba4e67938d865431c1a25e --- src/crash-manager/crash-manager.c | 3 +++ src/crash-manager/crash-manager.conf | 14 ++++++++++++++ src/crash-manager/dbus_notify.c | 21 +++++++++++++++++---- src/shared/config.c | 8 ++++++-- src/shared/config.h | 2 ++ 5 files changed, 42 insertions(+), 6 deletions(-) diff --git a/src/crash-manager/crash-manager.c b/src/crash-manager/crash-manager.c index bd402a1..753d1de 100644 --- a/src/crash-manager/crash-manager.c +++ b/src/crash-manager/crash-manager.c @@ -533,6 +533,8 @@ static void launch_dbus_notify(struct crash_info *cinfo) SNPRINTF_OR_EXIT(tid, "%d"); SNPRINTF_OR_EXIT(sig, "%d"); + char *legacy_notification_str = config.legacy_notification ? "--legacy-sig" : ""; + char *av[] = { CRASH_NOTIFY_BIN_PATH, "--cmdline", cinfo->cmd_line, "--cmdpath", cinfo->cmd_path, @@ -544,6 +546,7 @@ static void launch_dbus_notify(struct crash_info *cinfo) "--prstatus_fd", prstatus_fd_str, "--signal", sig_str, "--tid-comm", tid_comm_str, + legacy_notification_str, NULL }; spawn(av, NULL, NULL, NULL, NULL); diff --git a/src/crash-manager/crash-manager.conf b/src/crash-manager/crash-manager.conf index c485527..acaca35 100644 --- a/src/crash-manager/crash-manager.conf +++ b/src/crash-manager/crash-manager.conf @@ -15,6 +15,20 @@ AllowZip=yes # This option applies to ReportType=FULL only! # DumpCore=1 +# Use Legacy ProcessCrashed DBus signal +# When '0' (default), crash worker emits new-style signal with extensive data: +# 1. cmd name, +# 2. cmd path, +# 3. appid, +# 4. pkgid, +# 5. report path, +# 6. pid, +# 7. tid, +# 8. array of str->variant with additional data +# When '1' (legacy), signal signature is limited to fields 1-4 (cmd name, path, app/pkgid). +# The purpose of this is option is to support software still depending on legacy style. +# UseLegacyNotification=0 + # Extra script to run whose output will be attached to generated report # Script will be called with two arguments, in pseudo-code: # sh -c "$ExtraScript /path/to/report/temp/directory PID" diff --git a/src/crash-manager/dbus_notify.c b/src/crash-manager/dbus_notify.c index 0254092..41126c5 100644 --- a/src/crash-manager/dbus_notify.c +++ b/src/crash-manager/dbus_notify.c @@ -51,6 +51,7 @@ struct RegInfo { }; struct NotifyParams { + bool legacy_notification; int prstatus_fd; pid_t pid; pid_t tid; @@ -148,10 +149,16 @@ strdup_error: goto out; } +static GVariant* build_legacy_message_data(const struct NotifyParams *notify_params) +{ + assert(notify_params); + + return g_variant_new("(ssss)", notify_params->cmd_name, notify_params->cmd_path, notify_params->appid, notify_params->pkgid); +} + static GVariant* build_message_data(const struct NotifyParams *notify_params) { - if (notify_params == NULL) - return NULL; + assert(notify_params); GVariantBuilder md_builder; g_variant_builder_init(&md_builder, G_VARIANT_TYPE("(sssssiia{sv})")); @@ -209,7 +216,9 @@ static bool send_notify(GDBusConnection *conn, const struct NotifyParams *notify int result = false; GError *error = NULL; - GVariant *data = build_message_data(notify_params); + GVariant *data = notify_params->legacy_notification + ? build_legacy_message_data(notify_params) + : build_message_data(notify_params); if (data == NULL) { _E("Error while preparing parameters"); goto out; @@ -256,6 +265,7 @@ static bool parse_cmdline(int ac, char *av[], struct NotifyParams *params) FLAG_PRSTATUS_FD, FLAG_SIGNAL, FLAG_TID_COMM, + FLAG_LEGACY_NOTIFICATION, }; static const struct option options[] = { { .name = "cmdline", .has_arg = required_argument, .flag = NULL, .val = FLAG_CMDLINE }, @@ -268,6 +278,7 @@ static bool parse_cmdline(int ac, char *av[], struct NotifyParams *params) { .name = "prstatus_fd", .has_arg = required_argument, .flag = NULL, .val = FLAG_PRSTATUS_FD }, { .name = "signal", .has_arg = required_argument, .flag = NULL, .val = FLAG_SIGNAL }, { .name = "tid-comm", .has_arg = required_argument, .flag = NULL, .val = FLAG_TID_COMM }, + { .name = "legacy-sig", .has_arg = no_argument, .flag = NULL, .val = FLAG_LEGACY_NOTIFICATION }, { NULL }, }; @@ -295,6 +306,8 @@ static bool parse_cmdline(int ac, char *av[], struct NotifyParams *params) params->signal = atoi(optarg); else if (FLAG_TID_COMM == val) params->tid_comm = optarg; + else if (FLAG_LEGACY_NOTIFICATION == val) + params->legacy_notification = true; } while (val != -1); return params->cmd_name && params->cmd_path && params->appid && params->pkgid && params->prstatus_fd > 0; @@ -304,7 +317,7 @@ static void usage(const char *const progname) { assert(progname); - printf("%s --prstatus_fd N --pid PID --tid TID --cmdline CMDLINE --cmdpath CMDPATH --appid APPID --pkgid PKGID --signal SIGNALNR\n", progname); + printf("%s --prstatus_fd N --pid PID --tid TID --cmdline CMDLINE --cmdpath CMDPATH --appid APPID --pkgid PKGID --signal SIGNALNR [--legacy-sig]\n", progname); } int main(int ac, char *av[]) diff --git a/src/shared/config.c b/src/shared/config.c index d389187..cbf796c 100644 --- a/src/shared/config.c +++ b/src/shared/config.c @@ -94,6 +94,7 @@ static bool config_load_from_dict(config_t *c, dictionary *ini) UPDATE(c->max_crash_dump, int, "MaxCrashDump"); UPDATE(c->dump_core, boolean, "DumpCore"); UPDATE(c->allow_zip, boolean, "AllowZip"); + UPDATE(c->legacy_notification, boolean, "UseLegacyNotification"); #undef UPDATE @@ -187,6 +188,7 @@ static bool config_apply_defaults(config_t *c) c->max_crash_dump = MAX_CRASH_DUMP; c->dump_core = DUMP_CORE; c->allow_zip = ALLOW_ZIP; + c->legacy_notification = LEGACY_NOTIFICATION; return c->crash_root_path != NULL; } @@ -203,7 +205,8 @@ static void config_dump(config_t *c) "config: max_retention_sec = %d\n" "config: max_crash_dump = %d\n" "config: dump_core = %d\n" - "config: allow_zip = %d\n", + "config: allow_zip = %d\n" + "config: legacy_notification = %d\n", c->crash_root_path, c->extra_script, report_type_to_str(c->report_type), @@ -212,7 +215,8 @@ static void config_dump(config_t *c) c->max_retention_sec, c->max_crash_dump, c->dump_core, - c->allow_zip); + c->allow_zip, + c->legacy_notification); } bool config_init(config_t *c, const char *const path) diff --git a/src/shared/config.h b/src/shared/config.h index aea42ea..9343300 100644 --- a/src/shared/config.h +++ b/src/shared/config.h @@ -27,6 +27,7 @@ #define MAX_CRASH_DUMP 0 #define DUMP_CORE 1 #define ALLOW_ZIP 1 +#define LEGACY_NOTIFICATION 0 #define CRASH_SECTION "CrashManager" @@ -40,6 +41,7 @@ enum ReportType { typedef struct config { bool allow_zip; bool dump_core; + bool legacy_notification; int system_max_use; int system_keep_free; int max_retention_sec; -- 2.7.4 From bb69705b4327661a6d9747bc5776c72bf6c79e15 Mon Sep 17 00:00:00 2001 From: Karol Lewandowski Date: Tue, 21 Jan 2020 12:25:41 +0100 Subject: [PATCH 11/16] Add system test for legacy signal Change-Id: If4e2d007ebac2de6a11d64137c9d93e5efc75b6d --- tests/system/CMakeLists.txt | 1 + .../dbus_notify_legacy.sh.template | 80 ++++++++++++++++++++++ 2 files changed, 81 insertions(+) create mode 100644 tests/system/dbus_notify_legacy/dbus_notify_legacy.sh.template diff --git a/tests/system/CMakeLists.txt b/tests/system/CMakeLists.txt index b5423c1..44a1b1c 100644 --- a/tests/system/CMakeLists.txt +++ b/tests/system/CMakeLists.txt @@ -32,6 +32,7 @@ configure_test("dump_systemstate_extras") configure_test("livedumper") configure_test("extra_script") configure_test("dbus_notify") +configure_test("dbus_notify_legacy") configure_test("output_param") configure_test("libcrash-service") configure_test("full_core") diff --git a/tests/system/dbus_notify_legacy/dbus_notify_legacy.sh.template b/tests/system/dbus_notify_legacy/dbus_notify_legacy.sh.template new file mode 100644 index 0000000..946e9c3 --- /dev/null +++ b/tests/system/dbus_notify_legacy/dbus_notify_legacy.sh.template @@ -0,0 +1,80 @@ +#!/bin/bash + +# Check the support for old-style DBus notification signal + +if [ -z "${CRASH_WORKER_SYSTEM_TESTS}" ]; then + CRASH_WORKER_SYSTEM_TESTS="@CRASH_SYSTEM_TESTS_PATH@" +fi + +. ${CRASH_WORKER_SYSTEM_TESTS}/utils/minicore-utils.sh + +# We are looking for following signal - example for `sleep` command: +# +# signal time=1420079847.594970 sender=:1.82 -> destination=(null destination) serial=2 path=/Org/Tizen/System/Crash/Crash; interface=org.tizen.system.crash.Crash; member=ProcessCrashed +# string "sleep" +# string "/usr/bin/sleep" +# string "sleep" +# string "sleep" +# +# Note! Regular programs (Unix sleep, tests-provided kenny) are not enough to check all fields of the signal. +# Complete test, with appid & pkgid checks, would require building proper "Tizen Application". + +CONF_DIR=/etc/crash-manager.conf.d +CONF_FILE=${CONF_DIR}/system-test-dbus-notify-legacy.conf + +mount -o rw,remount / + +mkdir -p $CONF_DIR +rm -f $CONF_FILE || : +cat <$CONF_FILE +[CrashManager] +UseLegacyNotification=true +EOF + +( ${CRASH_WORKER_SYSTEM_TESTS}/utils/kenny & + pid=$! + sleep 2 + kill -6 $pid ) & + +TMPFILE=$(mktemp /tmp/dbus_notify.XXXXXX) +dbus-monitor --system type=signal,path=/Org/Tizen/System/Crash/Crash,interface=org.tizen.system.crash.Crash,member=ProcessCrashed > $TMPFILE & +monpid=$! + +cleanup() +{ + kill $monpid + rm -f $TMPFILE + rm -f $CONF_FILE +} + +trap cleanup 0 + +sleep 3 + +PATTERN='path=/Org/Tizen/System/Crash/Crash; interface=org\.tizen\.system\.crash\.Crash; member=ProcessCrashed' +for i in $(seq 1 10); do + score=0 + if egrep "$PATTERN" $TMPFILE; then + if egrep "string \"kenny" $TMPFILE; then + score=$(($score + 1)) + fi + + # legacy signal must not have the report_path and additional metadata + + if ! egrep "string \"/opt/usr/share/crash/dump.*kenny" $TMPFILE; then + score=$(($score + 1)) + fi + + if ! egrep -A1 "string \"sys.signal" $TMPFILE; then + score=$(($score + 1)) + fi + + if [ $score -eq 3 ]; then + exit_ok + fi + fi + + sleep 1 +done + +fail "legacy dbus signal does not match" -- 2.7.4 From 4cdcd0eb0149e687153f8d2992bec8262ef2ad2d Mon Sep 17 00:00:00 2001 From: Karol Lewandowski Date: Tue, 21 Jan 2020 10:54:05 +0100 Subject: [PATCH 12/16] Release 6.0.7 This version adds support option to support legacy 'ProcessCrashed' signal. Change-Id: I964c7f8a58ac96f3055403d9a5c33aee223747e3 --- packaging/crash-worker.spec | 2 +- packaging/crash-worker_system-tests.spec | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packaging/crash-worker.spec b/packaging/crash-worker.spec index 4376521..4826205 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.6 +Version: 6.0.7 Release: 1 Group: Framework/system License: Apache-2.0 and BSD-2-Clause and MIT diff --git a/packaging/crash-worker_system-tests.spec b/packaging/crash-worker_system-tests.spec index 0afd6c9..9ef7f9c 100644 --- a/packaging/crash-worker_system-tests.spec +++ b/packaging/crash-worker_system-tests.spec @@ -8,7 +8,7 @@ Name: crash-worker_system-tests Summary: Package with binaries and scripts for crash-worker system tests -Version: 6.0.6 +Version: 6.0.7 Release: 1 Group: Framework/system License: Apache-2.0 and BSD -- 2.7.4 From 8cb62d62fc27bd97311521c3b2b69562d4bd81b9 Mon Sep 17 00:00:00 2001 From: Karol Lewandowski Date: Wed, 22 Jan 2020 15:51:26 +0100 Subject: [PATCH 13/16] Add missing system test to spec Change-Id: I7d7b4d2a91e40938e65cb9153ebfc8d0e320876e --- packaging/crash-worker_system-tests.spec | 1 + 1 file changed, 1 insertion(+) diff --git a/packaging/crash-worker_system-tests.spec b/packaging/crash-worker_system-tests.spec index 9ef7f9c..2350dac 100644 --- a/packaging/crash-worker_system-tests.spec +++ b/packaging/crash-worker_system-tests.spec @@ -65,6 +65,7 @@ cd tests/system %{_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 +%{_libdir}/crash-worker_system-tests/dbus_notify_legacy/dbus_notify_legacy.sh %{_libdir}/crash-worker_system-tests/dump_systemstate_extras/dump_systemstate_extras.sh %{_libdir}/crash-worker_system-tests/extra_script/extra_script.sh %{_libdir}/crash-worker_system-tests/info_file/info_file.sh -- 2.7.4 From b88a8a3e7360d9fe12fd8c6e3ed3dc492aac2187 Mon Sep 17 00:00:00 2001 From: Karol Lewandowski Date: Thu, 23 Jan 2020 13:43:46 +0100 Subject: [PATCH 14/16] Fix livedump API DBus access policy This commit partially reverts commit f24c53fb20 ("crash-service: Restrict allow livedump API via privilege") As discussed with SRPOL Security, the 'group membership' of given process does not map directly to process 'having privilege', as verified by Cynara/security-manager. For now the checking has to be done per-user for system services and via-privilege for apps. Change-Id: I2a7b76174adb22be8ffe55e41678b48938d90bbb --- src/crash-service/crash-service.conf | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/crash-service/crash-service.conf b/src/crash-service/crash-service.conf index 7aac3d0..dd38a30 100644 --- a/src/crash-service/crash-service.conf +++ b/src/crash-service/crash-service.conf @@ -13,6 +13,11 @@ send_interface="org.tizen.system.crash.livedump" send_member="livedump_pid"/> + + + Date: Thu, 23 Jan 2020 17:49:04 +0100 Subject: [PATCH 15/16] Release 6.0.8 This release brings minor fixes to dbus policy and system test. Change-Id: I2bd9cc0a7979dd92e0fe819e0eeca0ec37afb28c --- packaging/crash-worker.spec | 2 +- packaging/crash-worker_system-tests.spec | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packaging/crash-worker.spec b/packaging/crash-worker.spec index 4826205..a37523d 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.7 +Version: 6.0.8 Release: 1 Group: Framework/system License: Apache-2.0 and BSD-2-Clause and MIT diff --git a/packaging/crash-worker_system-tests.spec b/packaging/crash-worker_system-tests.spec index 2350dac..22d6e86 100644 --- a/packaging/crash-worker_system-tests.spec +++ b/packaging/crash-worker_system-tests.spec @@ -8,7 +8,7 @@ Name: crash-worker_system-tests Summary: Package with binaries and scripts for crash-worker system tests -Version: 6.0.7 +Version: 6.0.8 Release: 1 Group: Framework/system License: Apache-2.0 and BSD -- 2.7.4 From f3c0b21a8e98a6c297974a9503cf75ace2fbe72a Mon Sep 17 00:00:00 2001 From: Karol Lewandowski Date: Thu, 13 Feb 2020 09:27:41 +0100 Subject: [PATCH 16/16] Drop sys-assert leftovers Change-Id: Ia220cc73a1b1e60f165aedd58cdfe9360c8de63b --- CMakeLists.txt | 5 ----- include/defs.h.in | 1 - src/crash-manager/crash-manager.h | 16 ++++++---------- 3 files changed, 6 insertions(+), 16 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 630e1aa..cf0df7b 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -10,11 +10,6 @@ ADD_SUBDIRECTORY(include) INCLUDE_DIRECTORIES(${CMAKE_SOURCE_DIR}/include) ADD_SUBDIRECTORY(src/crash-manager) - -IF("${SYS_ASSERT}" STREQUAL "ON") - ADD_SUBDIRECTORY(src/sys-assert) -ENDIF("${SYS_ASSERT}" STREQUAL "ON") - ADD_SUBDIRECTORY(src/crash-stack) ADD_SUBDIRECTORY(src/dump_systemstate) diff --git a/include/defs.h.in b/include/defs.h.in index 3ac9871..30b61f4 100644 --- a/include/defs.h.in +++ b/include/defs.h.in @@ -6,7 +6,6 @@ #define CRASH_PATH "@CRASH_PATH@" #define CRASH_ROOT_PATH "@CRASH_ROOT_PATH@" #define CRASH_TEMP "@CRASH_TEMP@" -#define SYS_ASSERT "@SYS_ASSERT@" #define CRASH_STACK_BIN_PATH "@CRASH_STACK_BIN_PATH@" #define CRASH_POPUP_BIN_PATH "@CRASH_POPUP_BIN_PATH@" #define CRASH_NOTIFY_BIN_PATH "@CRASH_NOTIFY_BIN_PATH@" diff --git a/src/crash-manager/crash-manager.h b/src/crash-manager/crash-manager.h index 7c102bc..1e4dbe5 100644 --- a/src/crash-manager/crash-manager.h +++ b/src/crash-manager/crash-manager.h @@ -28,14 +28,17 @@ /* Paths and variables */ struct crash_info { + bool livedump; + bool kill; + bool print_result_path; pid_t pid_info; pid_t tid_info; int uid_info; int gid_info; int sig_info; + int prstatus_fd; char *cmd_line; char *cmd_path; - time_t time_info; char *temp_dir; char *name; char *result_path; @@ -43,17 +46,10 @@ struct crash_info { char *info_path; char *core_path; char *log_path; + char *output_path; char appid[APPID_MAX]; char pkgid[PKGNAME_MAX]; - char *output_path; - bool livedump; - bool kill; - bool print_result_path; -#ifdef SYS_ASSERT - char *sysassert_cs_path; - bool have_sysassert_report; -#endif - int prstatus_fd; + time_t time_info; }; bool crash_manager_direct(struct crash_info *cinfo); -- 2.7.4