From bddf2ca3744ee97abb258ddfbc1b95fe18a43047 Mon Sep 17 00:00:00 2001 From: Karol Lewandowski Date: Wed, 3 Oct 2018 15:43:16 +0200 Subject: [PATCH 01/16] Fix: Another instance of indexing array with negative index Issue reported by Coverity. Change-Id: Ida7f3106d650f48d5fff7e976f1bbc0df0fdc308 --- src/shared/util.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/shared/util.c b/src/shared/util.c index e6a2cff..b83dbaa 100644 --- a/src/shared/util.c +++ b/src/shared/util.c @@ -768,12 +768,12 @@ char* get_exe_path(pid_t pid) "/proc/%d/exe", pid); ssize_t ret = readlink(exe_link, buffer, sizeof(buffer) - 1); - buffer[ret] = '\0'; if (ret <= 0) { _E("Failed to read link %s: %m", exe_link); return NULL; } + buffer[ret] = '\0'; if (access(buffer, F_OK) == -1) { _E("Invalid path %s", buffer); -- 2.7.4 From ad7c58644a30ccba6e2cd95d071688f073f0e152 Mon Sep 17 00:00:00 2001 From: Karol Lewandowski Date: Wed, 3 Oct 2018 15:44:20 +0200 Subject: [PATCH 02/16] Release 5.0.7 This release brings one addtional fix for indexing array with negative index bug. Change-Id: I9e49379a68f4839256d1df26e42b144b6ed6a708 --- 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 6104aea..7f1cf41 100644 --- a/packaging/crash-worker.spec +++ b/packaging/crash-worker.spec @@ -10,7 +10,7 @@ Name: crash-worker Summary: Crash-manager -Version: 5.0.6 +Version: 5.0.7 Release: 1 Group: Framework/system License: Apache-2.0 and BSD -- 2.7.4 From 9b9190f5e41257a244b5e254c38e5c595d33f4a8 Mon Sep 17 00:00:00 2001 From: Karol Lewandowski Date: Wed, 3 Oct 2018 13:14:20 +0200 Subject: [PATCH 03/16] crash-manager: Do not make temporary copy of crashed process maps It was needed when crash-worker used (now removed) ptrace -based crash-stack, which woken up crashed process, causing /proc// entries to disappear. Change-Id: Iec344e6c3b9d7d12979173dea530684573a0761d --- src/crash-manager/crash-manager.c | 35 ++++++----------------------------- 1 file changed, 6 insertions(+), 29 deletions(-) diff --git a/src/crash-manager/crash-manager.c b/src/crash-manager/crash-manager.c index af57938..686a13e 100644 --- a/src/crash-manager/crash-manager.c +++ b/src/crash-manager/crash-manager.c @@ -77,7 +77,6 @@ #define MINICOREDUMPER_TIMEOUT DEFAULT_COMMAND_TIMEOUT #define CRASH_STACK_TIMEOUT DEFAULT_COMMAND_TIMEOUT #define ZIP_TIMEOUT DEFAULT_COMMAND_TIMEOUT -#define TEMP_MAPS_FILENAME "maps.temp" enum { RET_EXCEED = 1, @@ -703,35 +702,12 @@ static int dump_system_state(const struct crash_info *cinfo) return system_command_parallel(command); } -static void copy_maps(const struct crash_info *cinfo) -{ - char maps_path[PATH_MAX]; - char temp_maps_path[PATH_MAX]; - snprintf(maps_path, sizeof(maps_path), "/proc/%d/maps", - cinfo->pid_info); - - snprintf(temp_maps_path, sizeof(temp_maps_path), "%s/%s", - cinfo->temp_dir, TEMP_MAPS_FILENAME); - - copy_file(maps_path, temp_maps_path); -} - -static void remove_maps(const struct crash_info *cinfo) -{ - char temp_maps_path[PATH_MAX]; - - snprintf(temp_maps_path, sizeof(temp_maps_path), "%s/%s", - cinfo->temp_dir, TEMP_MAPS_FILENAME); - if (unlink(temp_maps_path) < 0) - _E("Cannot remove %s: %m\n", temp_maps_path); -} - static void save_so_info(const struct crash_info *cinfo) { char maps_path[PATH_MAX]; char so_info_path[PATH_MAX]; - snprintf(maps_path, sizeof(maps_path), "%s/%s", - cinfo->temp_dir, TEMP_MAPS_FILENAME); + snprintf(maps_path, sizeof(maps_path), "/proc/%d/maps", + cinfo->pid_info); snprintf(so_info_path, sizeof(so_info_path), "%s/%s.%s", cinfo->pfx, cinfo->name, "so_info"); @@ -1278,8 +1254,10 @@ int main(int argc, char *argv[]) /* Exec dump_systemstate */ dump_state_pid = dump_system_state(&cinfo); - /* Copy maps file to temp dir */ - copy_maps(&cinfo); + if (dump_state_pid < 0) { + res = EXIT_FAILURE; + goto exit; + } } /* Exec crash modules */ if (execute_crash_modules(&cinfo) < 0) { @@ -1290,7 +1268,6 @@ int main(int argc, char *argv[]) if (report_type >= REP_TYPE_FULL) { /* Save shared objects info (file names, bulid IDs, rpm package names) */ save_so_info(&cinfo); - remove_maps(&cinfo); /* Wait dump_system_state */ wait_system_command(dump_state_pid); -- 2.7.4 From d23978584db9f97fa516c1c64a7bd355962a0467 Mon Sep 17 00:00:00 2001 From: Mateusz Moscicki Date: Mon, 15 Oct 2018 13:43:15 +0200 Subject: [PATCH 04/16] Handle spaces in the crashed app pathname and in the maps file Change-Id: I98ff537cab9c37b78dc3f3056a375fd10461b7ea --- src/crash-manager/crash-manager.c | 2 +- src/crash-stack/crash-stack.c | 7 ++++++- src/crash-stack/unwind.c | 8 +++++++- 3 files changed, 14 insertions(+), 3 deletions(-) diff --git a/src/crash-manager/crash-manager.c b/src/crash-manager/crash-manager.c index 686a13e..a02d59c 100644 --- a/src/crash-manager/crash-manager.c +++ b/src/crash-manager/crash-manager.c @@ -692,7 +692,7 @@ static int dump_system_state(const struct crash_info *cinfo) char command[PATH_MAX]; ret = snprintf(command, sizeof(command), - "/usr/bin/dump_systemstate -d -k -j -f %s", + "/usr/bin/dump_systemstate -d -k -j -f '%s'", cinfo->log_path); if (ret < 0) { _E("Failed to snprintf for dump_systemstate command"); diff --git a/src/crash-stack/crash-stack.c b/src/crash-stack/crash-stack.c index bf1cd38..61b876c 100644 --- a/src/crash-stack/crash-stack.c +++ b/src/crash-stack/crash-stack.c @@ -62,6 +62,11 @@ #define STRING_FORMAT_SPECIFIER_WITH_MACRO(macro) "%"#macro"s" #define STR_FS(macro) STRING_FORMAT_SPECIFIER_WITH_MACRO(macro) +// this macro returns the format specifier to capture a string that can +// contain spaces +#define STRINGWW_FORMAT_SPECIFIER_WITH_MACRO(macro) "%"#macro"[^\t\n]" +#define STRWW_FS(macro) STRINGWW_FORMAT_SPECIFIER_WITH_MACRO(macro) + static FILE *outputfile = NULL; ///< global output stream static FILE *errfile = NULL; ///< global error stream static FILE *bufferfile = NULL; ///< buffer file for ordering @@ -377,7 +382,7 @@ static struct addr_node *get_addr_list_from_maps(int fd) result = sscanf(linebuf, STR_FS(ADDR_LEN) STR_FS(PERM_LEN) "%*s %*s %*s" - STR_FS(PATH_MAX), addr, perm, path); + STRWW_FS(PATH_MAX), addr, perm, path); if (result < 0) continue; perm[PERM_LEN] = 0; diff --git a/src/crash-stack/unwind.c b/src/crash-stack/unwind.c index bce9ef8..d372bbe 100644 --- a/src/crash-stack/unwind.c +++ b/src/crash-stack/unwind.c @@ -861,7 +861,13 @@ void *_TB_create (pid_t pid) { if (fgets(buf, STAT_LINE_MAXSIZE, f) == NULL) goto out_err; - for (i = 0, p = buf; i < 28; i++) { + // The filename of the executable (the second field) is in parentheses and can + // contain whitespaces. Therefore I'm looking for the last ')' character + // and then iterate to the 27 field from here. + p = strrchr(buf, ')'); + if (!p) + goto out_err; + for (i = 0; i < 27; i++) { p = strchr(p, ' '); if (!p) goto out_err; -- 2.7.4 From a6ba03c4c82269ff20c12a277b99b7d765ecdea3 Mon Sep 17 00:00:00 2001 From: Mateusz Moscicki Date: Wed, 17 Oct 2018 13:23:29 +0200 Subject: [PATCH 05/16] dump_systemstate: Continue dumping after failure of a subcommand Change-Id: I7c0481984fbb9c04b26f913fb25208b11f9d6869 --- src/dump_systemstate/dump_systemstate.c | 53 ++++++++++++++------------------- 1 file changed, 23 insertions(+), 30 deletions(-) diff --git a/src/dump_systemstate/dump_systemstate.c b/src/dump_systemstate/dump_systemstate.c index 776ec2b..fb2ee75 100644 --- a/src/dump_systemstate/dump_systemstate.c +++ b/src/dump_systemstate/dump_systemstate.c @@ -84,7 +84,13 @@ static int get_disk_used_percent(const char *path) int main(int argc, char *argv[]) { - int c, ret, i, is_root, dpercent; +#define INFORM_IF_ERROR(ret) \ + if (ret < 0) { \ + exit_code = ret; \ + fprintf_fd(out_fd, "\nCommand failed with error code: %d", ret); \ + } + + int c, ret, i, is_root, dpercent, exit_code = 0; const char *arg_file = NULL; int out_fd = -1; bool arg_dlog = false; @@ -130,7 +136,7 @@ int main(int argc, char *argv[]) out_fd = open(arg_file, O_WRONLY | O_TRUNC | O_CREAT, FILE_PERM); if (out_fd < 0) { perror("couldn't open output file"); - ret = out_fd; + exit_code = out_fd; goto exit; } } @@ -144,66 +150,56 @@ int main(int argc, char *argv[]) fprintf_fd(out_fd, "\n%s(%s)\n", dump_item[i].title, dump_item[i].path); ret = dump_file_write_fd((char *)dump_item[i].path, out_fd); - if (ret < 0) - goto exit_close; + INFORM_IF_ERROR(ret) } fprintf_fd(out_fd, "\n"); fprintf_fd(out_fd, "\n==== System disk space usage (/bin/df -h)\n"); char *df_args[] = {"/bin/df", "-h", NULL}; ret = run_command_write_fd_timeout(df_args[0], df_args, NULL, out_fd, NULL, 0, TIMEOUT_DEFAULT); - if (ret < 0) - goto exit_close; + INFORM_IF_ERROR(ret) 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); char *du_args[] = {"/bin/du", "-ah", "/opt", "--exclude=/opt/usr", NULL}; ret = run_command_write_fd_timeout(du_args[0], du_args, NULL, out_fd, NULL, 0, TIMEOUT_DEFAULT); - if (ret < 0) - goto exit_close; + INFORM_IF_ERROR(ret) } fprintf_fd(out_fd, "\n==== System timezone (ls -al /opt/etc/localtime)\n"); char *ls_args[] = {"/bin/ls", "-al", "/opt/etc/localtime", NULL}; ret = run_command_write_fd_timeout(ls_args[0], ls_args, NULL, out_fd, NULL, 0, TIMEOUT_DEFAULT); - if (ret < 0) - goto exit_close; + INFORM_IF_ERROR(ret) fprintf_fd(out_fd, "\n==== System summary (/usr/bin/top -bcH -n 1)\n"); char *top_args[] = {"/bin/top", "-bcH", "-n", "1", NULL}; char *top_env[] = {"COLUMNS=200", NULL}; ret = run_command_write_fd_timeout(top_args[0], top_args, top_env, out_fd, NULL, 0, TIMEOUT_DEFAULT); - if (ret < 0) - goto exit_close; + INFORM_IF_ERROR(ret) fprintf_fd(out_fd, "\n==== Current processes (/bin/ps auxfw)\n"); char *ps_args[] = {"/bin/ps", "auxfw", NULL}; ret = run_command_write_fd_timeout(ps_args[0], ps_args, NULL, out_fd, NULL, 0, TIMEOUT_DEFAULT); - if (ret < 0) - goto exit_close; + INFORM_IF_ERROR(ret) fprintf_fd(out_fd, "\n==== System memory statistics (/usr/bin/memps -v)\n"); char *memps_args[] = {"/bin/memps", "-v", NULL}; ret = run_command_write_fd_timeout(memps_args[0], memps_args, NULL, out_fd, NULL, 0, TIMEOUT_DEFAULT); - if (ret < 0) - goto exit_close; + INFORM_IF_ERROR(ret) if (is_root) { fprintf_fd(out_fd, "\n==== System configuration (/usr/bin/vconftool get memory, db, file)\n"); char *get_mem_args[] = {"/bin/vconftool", "get", "memory/", "-r", NULL}; ret = run_command_write_fd_timeout(get_mem_args[0], get_mem_args, NULL, out_fd, NULL, 0, TIMEOUT_DEFAULT); - if (ret < 0) - goto exit_close; + INFORM_IF_ERROR(ret) char *get_db_args[] = {"/bin/vconftool", "get", "db/", "-r", NULL}; ret = run_command_write_fd_timeout(get_db_args[0], get_db_args, NULL, out_fd, NULL, 0, TIMEOUT_DEFAULT); - if (ret < 0) - goto exit_close; + INFORM_IF_ERROR(ret) char *get_file_args[] = {"/bin/vconftool", "get", "file/", "-r", NULL}; ret = run_command_write_fd_timeout(get_file_args[0], get_file_args, NULL, out_fd, NULL, 0, TIMEOUT_DEFAULT); - if (ret < 0) - goto exit_close; + INFORM_IF_ERROR(ret) } if (arg_dmesg && is_root) { @@ -211,29 +207,26 @@ int main(int argc, char *argv[]) char *dmesg_args[] = {"/bin/dmesg", "-T", NULL}; char *dmesg_env[] = {"TZ=UTC", NULL}; ret = run_command_write_fd_timeout(dmesg_args[0], dmesg_args, dmesg_env, out_fd, NULL, 0, TIMEOUT_DEFAULT); - if (ret < 0) - goto exit_close; + INFORM_IF_ERROR(ret) } if (arg_dlog) { fprintf_fd(out_fd, "\n==== Log messages\n"); char *dlogutil_args[] = {"/bin/dlogutil", "-d", "-v", "threadtime", "-u", "16384", NULL}; ret = run_command_write_fd_timeout(dlogutil_args[0], dlogutil_args, NULL, out_fd, NULL, 0, TIMEOUT_DEFAULT); - if (ret < 0) - goto exit_close; + INFORM_IF_ERROR(ret) } if (arg_journal) { fprintf_fd(out_fd, "\n==== Journal messages\n"); char *journalctl_args[] = {"/bin/journalctl", "-b", "-n", "1024", NULL}; ret = run_command_write_fd_timeout(journalctl_args[0], journalctl_args, NULL, out_fd, NULL, 0, TIMEOUT_DEFAULT); - if (ret < 0) - goto exit_close; + INFORM_IF_ERROR(ret) } -exit_close: if (arg_file) close(out_fd); exit: - return ret; + return exit_code; +#undef INFORM_IF_ERROR } -- 2.7.4 From 80be530942b9ce1c52367c646fd669622cd06da6 Mon Sep 17 00:00:00 2001 From: Mateusz Moscicki Date: Wed, 31 Oct 2018 14:04:09 +0100 Subject: [PATCH 06/16] Release 5.0.8 Changes: - Handle spaces in the crashed app pathname and in the maps file - Don't interrupt the dump_systemstate if any of subcommands return an error Change-Id: Ifc81df710c4441cbc63b42e719102e50f44b138b --- 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 7f1cf41..3537e86 100644 --- a/packaging/crash-worker.spec +++ b/packaging/crash-worker.spec @@ -10,7 +10,7 @@ Name: crash-worker Summary: Crash-manager -Version: 5.0.7 +Version: 5.0.8 Release: 1 Group: Framework/system License: Apache-2.0 and BSD -- 2.7.4 From 927ace8cc70a51f510d381da2208feaa53bc31e3 Mon Sep 17 00:00:00 2001 From: Mateusz Moscicki Date: Mon, 5 Nov 2018 11:39:40 +0100 Subject: [PATCH 07/16] crash-stack: Fix a /proc//maps file parsing Change-Id: If164f3bf5cfaf7eefc68d992d90ff4110b52ca04 --- src/crash-stack/crash-stack.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/crash-stack/crash-stack.c b/src/crash-stack/crash-stack.c index 61b876c..a348067 100644 --- a/src/crash-stack/crash-stack.c +++ b/src/crash-stack/crash-stack.c @@ -381,7 +381,7 @@ static struct addr_node *get_addr_list_from_maps(int fd) memset(path, 0, PATH_MAX+1); result = sscanf(linebuf, STR_FS(ADDR_LEN) STR_FS(PERM_LEN) - "%*s %*s %*s" + "%*s %*s %*s " STRWW_FS(PATH_MAX), addr, perm, path); if (result < 0) continue; -- 2.7.4 From dca1da43a76959fb8a6cc3bbd621ee08d2f72630 Mon Sep 17 00:00:00 2001 From: Karol Lewandowski Date: Mon, 5 Nov 2018 14:02:41 +0100 Subject: [PATCH 08/16] Release 5.5.1 Version bump due to Tizen 5.0 M2 release & corresponding branching repo to additional tizen_5.0 line. This version contains one fix for /proc//maps parsing (not present in tizen_5.0). Change-Id: Ibbfebbd05b54bd81a0c55dafa357720ddfeb71cb --- 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 3537e86..1a952a8 100644 --- a/packaging/crash-worker.spec +++ b/packaging/crash-worker.spec @@ -10,7 +10,7 @@ Name: crash-worker Summary: Crash-manager -Version: 5.0.8 +Version: 5.5.1 Release: 1 Group: Framework/system License: Apache-2.0 and BSD -- 2.7.4 From ecbca2fe4b8722fd8dce6582abf46c35ae090883 Mon Sep 17 00:00:00 2001 From: Karol Lewandowski Date: Wed, 3 Oct 2018 13:36:17 +0200 Subject: [PATCH 09/16] util: copy_bytes: Multiple changes - Use max possible buffer size for sendfile() The fewer we call sendfile() the better (performance) - Provide read()/write() fallback when sendfile will not work (eg. copying data from pipe to pipe) - Add third argument with number of copied bytes. This is to avoid mixing error code (negative) with transferred size. - Make API available for rest of codebase Change-Id: If80feb578b01fb715008a20e7c000c5ede9b62a1 --- CMakeLists.txt | 2 ++ src/shared/util.c | 68 ++++++++++++++++++++++++++++++++++++++++++++----------- src/shared/util.h | 2 ++ 3 files changed, 59 insertions(+), 13 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 96ecea7..9834ed5 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -3,6 +3,8 @@ PROJECT(crash-worker C) SET(PREFIX ${CMAKE_INSTALL_PREFIX}) +ADD_DEFINITIONS(-D_FILE_OFFSET_BITS=64) + # Sub modules ADD_SUBDIRECTORY(src/crash-manager) diff --git a/src/shared/util.c b/src/shared/util.c index b83dbaa..d6fdd31 100644 --- a/src/shared/util.c +++ b/src/shared/util.c @@ -19,6 +19,7 @@ #endif #include #include +#include #include #include #include @@ -167,19 +168,60 @@ int write_fd(int fd, const void *buf, int len) return total; } -static int copy_bytes(int dfd, int sfd) +static int copy_bytes_sendfile(int destfd, int srcfd, off_t *ncopied) { - int res = 1; - ssize_t bytes_sent; - do - bytes_sent = sendfile(dfd, sfd, NULL, PIPE_BUF); - while (bytes_sent > 0); + ssize_t nsent; + off_t total = 0; - if (bytes_sent == -1) { - _E("sendfile() error: %m\n"); - res = -1; - } - return res; + do { + nsent = sendfile(destfd, srcfd, NULL, INT32_MAX); + if (nsent > 0) + total += nsent; + else if (nsent < 0 && errno == EAGAIN) + continue; + else + break; + } while (1); + + if (ncopied) + *ncopied = total; + + return nsent == -1 ? -1 : 0; +} + +static int copy_bytes_rw(int destfd, int srcfd, off_t *ncopied) +{ + ssize_t n; + char buf[4096]; + off_t total = 0; + + do { + n = read(srcfd, buf, sizeof buf); + if (n < 0) { + if (errno == EAGAIN || errno == EINTR) + continue; + _E("read: %m"); + return -1; + } + + int m = write_fd(destfd, buf, n); + if (m != n) { + _E("failed to write data to destination fd: %m"); + return -1; + } + total += m; + } while (n > 0); + + if (ncopied) + *ncopied = total; + + return 0; +} + +int copy_bytes(int destfd, int srcfd, off_t *ncopied) +{ + int r = copy_bytes_sendfile(destfd, srcfd, ncopied); + return r == 0 ? r : copy_bytes_rw(destfd, srcfd, ncopied); } int copy_file(char *src, char *dst) @@ -204,7 +246,7 @@ int copy_file(char *src, char *dst) return -1; } - res = copy_bytes(dfd, sfd); + res = copy_bytes(dfd, sfd, NULL); close(sfd); close(dfd); @@ -235,7 +277,7 @@ int dump_file_write_fd(char *src, int dfd) return -1; } - res = copy_bytes(dfd, sfd); + res = copy_bytes(dfd, sfd, NULL); close(sfd); return res; diff --git a/src/shared/util.h b/src/shared/util.h index 03de693..a010b7e 100644 --- a/src/shared/util.h +++ b/src/shared/util.h @@ -39,6 +39,8 @@ int wait_system_command(int pid); int write_fd(int fd, const void *buf, int len); +int copy_bytes(int destfd, int srcfd, off_t *ncopied); + int copy_file(char *src, char *dst); int move_file(char *src, char *dst); -- 2.7.4 From a4d1efddefa2114211d736479dd4813ecc1e8014 Mon Sep 17 00:00:00 2001 From: Karol Lewandowski Date: Wed, 7 Nov 2018 12:00:18 +0100 Subject: [PATCH 10/16] util: Unify function calling convention to specify destination first This commit adjusts signature of copy_file(), move_file() and dump_file_write_fd() to specify destination first, source second. This in line with rest of codebase. Change-Id: I81285cda73d43ff2428ea5daf7ad27676e51ca8a Signed-off-by: Karol Lewandowski --- src/crash-manager/crash-manager.c | 2 +- src/dump_systemstate/dump_systemstate.c | 2 +- src/shared/util.c | 8 ++++---- src/shared/util.h | 6 +++--- 4 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/crash-manager/crash-manager.c b/src/crash-manager/crash-manager.c index a02d59c..95cbf8b 100644 --- a/src/crash-manager/crash-manager.c +++ b/src/crash-manager/crash-manager.c @@ -623,7 +623,7 @@ static int get_sysassert_cs(struct crash_info *cinfo) return -1; } - if (move_file(cinfo->sysassert_cs_path, move_path) < 0) { + if (move_file(move_path, cinfo->sysassert_cs_path) < 0) { _E("Failed to move %s to %s", cinfo->sysassert_cs_path, move_path); return -1; diff --git a/src/dump_systemstate/dump_systemstate.c b/src/dump_systemstate/dump_systemstate.c index fb2ee75..d4c0e40 100644 --- a/src/dump_systemstate/dump_systemstate.c +++ b/src/dump_systemstate/dump_systemstate.c @@ -149,7 +149,7 @@ int main(int argc, char *argv[]) fsync(out_fd); fprintf_fd(out_fd, "\n%s(%s)\n", dump_item[i].title, dump_item[i].path); - ret = dump_file_write_fd((char *)dump_item[i].path, out_fd); + ret = dump_file_write_fd(out_fd, (char *)dump_item[i].path); INFORM_IF_ERROR(ret) } fprintf_fd(out_fd, "\n"); diff --git a/src/shared/util.c b/src/shared/util.c index d6fdd31..2367524 100644 --- a/src/shared/util.c +++ b/src/shared/util.c @@ -224,7 +224,7 @@ int copy_bytes(int destfd, int srcfd, off_t *ncopied) return r == 0 ? r : copy_bytes_rw(destfd, srcfd, ncopied); } -int copy_file(char *src, char *dst) +int copy_file(char *dst, char *src) { int res; int sfd; @@ -253,16 +253,16 @@ int copy_file(char *src, char *dst) return res; } -int move_file(char *src, char *dst) +int move_file(char *dst, char *src) { - if (copy_file(src, dst) < 0) + if (copy_file(dst, src) < 0) return -1; if (unlink(src) < 0) return -1; return 1; } -int dump_file_write_fd(char *src, int dfd) +int dump_file_write_fd(int dfd, char *src) { int res; int sfd; diff --git a/src/shared/util.h b/src/shared/util.h index a010b7e..dfe40bf 100644 --- a/src/shared/util.h +++ b/src/shared/util.h @@ -41,11 +41,11 @@ int write_fd(int fd, const void *buf, int len); int copy_bytes(int destfd, int srcfd, off_t *ncopied); -int copy_file(char *src, char *dst); +int copy_file(char *dst, char *src); -int move_file(char *src, char *dst); +int move_file(char *dst, char *src); -int dump_file_write_fd(char *src, int dfd); +int dump_file_write_fd(int dfd, char *src); int run_command_write_fd_timeout(char *path, char *args[], char *env[], int dfd, char *buff, int size, int timeout); -- 2.7.4 From 88120bf4a98462b41b5309017fc2ac0e332cb945 Mon Sep 17 00:00:00 2001 From: Karol Lewandowski Date: Wed, 3 Oct 2018 13:40:13 +0200 Subject: [PATCH 11/16] packaging: Drop unused variables from cmake invocation Ideally, crash-worker codebase should not know about any of TZ_ vars - as all needed paths should be specified explicitly from the .spec. Change-Id: Id80fe0ae5f93940ce49bbe11903723a00f0261f0 --- packaging/crash-worker.spec | 2 -- 1 file changed, 2 deletions(-) diff --git a/packaging/crash-worker.spec b/packaging/crash-worker.spec index 1a952a8..024821a 100644 --- a/packaging/crash-worker.spec +++ b/packaging/crash-worker.spec @@ -102,8 +102,6 @@ export CFLAGS+=" -Werror" -DARCH=%{ARCH} \ -DARCH_BIT=%{ARCH_BIT} \ -DCRASH_MANAGER_CONFIG_PATH=%{_sysconfdir}/crash-manager.conf \ - -DTZ_SYS_ETC=%{TZ_SYS_ETC} \ - -DTZ_SYS_BIN=%{TZ_SYS_BIN} \ -DCRASH_ROOT_PATH=%{crash_root_path} \ -DCRASH_PATH=%{crash_path} \ -DCRASH_TEMP=%{crash_temp} \ -- 2.7.4 From 236600bdad00d1f53b4221e0cac4a47bc64d1645 Mon Sep 17 00:00:00 2001 From: Karol Lewandowski Date: Wed, 3 Oct 2018 13:59:18 +0200 Subject: [PATCH 12/16] Define _GNU_SOURCE once for whole codebase There is no need to repeat this in every file. Change-Id: Ia788840d819b8d72122296a5e5d64aa2a7c3cb40 --- CMakeLists.txt | 2 +- src/crash-manager/crash-manager.c | 2 -- src/crash-manager/dbus_notify.c | 1 - src/crash-stack/CMakeLists.txt | 2 +- src/crash-stack/crash-stack.c | 1 - src/shared/util.c | 3 --- src/sys-assert/arm/backtrace.c | 2 +- src/sys-assert/arm_64/backtrace.c | 2 +- src/sys-assert/sys-assert.c | 1 - src/sys-assert/x86/backtrace.c | 2 +- src/sys-assert/x86/context.c | 2 +- 11 files changed, 6 insertions(+), 14 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 9834ed5..9f3222d 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -3,7 +3,7 @@ PROJECT(crash-worker C) SET(PREFIX ${CMAKE_INSTALL_PREFIX}) -ADD_DEFINITIONS(-D_FILE_OFFSET_BITS=64) +ADD_DEFINITIONS(-D_FILE_OFFSET_BITS=64 -D_GNU_SOURCE=1) # Sub modules ADD_SUBDIRECTORY(src/crash-manager) diff --git a/src/crash-manager/crash-manager.c b/src/crash-manager/crash-manager.c index 95cbf8b..c48e29a 100644 --- a/src/crash-manager/crash-manager.c +++ b/src/crash-manager/crash-manager.c @@ -16,8 +16,6 @@ * limitations under the License. */ -#define _GNU_SOURCE - #include #include #include diff --git a/src/crash-manager/dbus_notify.c b/src/crash-manager/dbus_notify.c index 6a8c5d8..cf78f6a 100644 --- a/src/crash-manager/dbus_notify.c +++ b/src/crash-manager/dbus_notify.c @@ -16,7 +16,6 @@ * Author: Mateusz Moscicki */ -#define _GNU_SOURCE #include #include #include diff --git a/src/crash-stack/CMakeLists.txt b/src/crash-stack/CMakeLists.txt index fd26fac..f996da9 100644 --- a/src/crash-stack/CMakeLists.txt +++ b/src/crash-stack/CMakeLists.txt @@ -21,7 +21,7 @@ else() set(CRASH_STACK_SRCS ${CRASH_STACK_SRCS} crash-stack-stub.c) endif() -set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wall -fPIE -D_GNU_SOURCE=1") +set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wall -fPIE") set(CMAKE_EXE_LINKER_FLAGS "-Wl,--as-needed -pie") # Binary diff --git a/src/crash-stack/crash-stack.c b/src/crash-stack/crash-stack.c index a348067..00e2f3f 100644 --- a/src/crash-stack/crash-stack.c +++ b/src/crash-stack/crash-stack.c @@ -17,7 +17,6 @@ * Łukasz Stelmach * Rafał Pietruch */ -#define _GNU_SOURCE 1 /** * @file crash-stack.c diff --git a/src/shared/util.c b/src/shared/util.c index 2367524..775d6be 100644 --- a/src/shared/util.c +++ b/src/shared/util.c @@ -14,9 +14,6 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -#ifndef _GNU_SOURCE -#define _GNU_SOURCE -#endif #include #include #include diff --git a/src/sys-assert/arm/backtrace.c b/src/sys-assert/arm/backtrace.c index 7520bba..2d1241a 100644 --- a/src/sys-assert/arm/backtrace.c +++ b/src/sys-assert/arm/backtrace.c @@ -14,7 +14,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -#define _GNU_SOURCE + #include #include #include diff --git a/src/sys-assert/arm_64/backtrace.c b/src/sys-assert/arm_64/backtrace.c index 3c8f560..21a10d4 100644 --- a/src/sys-assert/arm_64/backtrace.c +++ b/src/sys-assert/arm_64/backtrace.c @@ -14,7 +14,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -#define _GNU_SOURCE + #include #include #include diff --git a/src/sys-assert/sys-assert.c b/src/sys-assert/sys-assert.c index 75c7c3b..5af44ff 100644 --- a/src/sys-assert/sys-assert.c +++ b/src/sys-assert/sys-assert.c @@ -15,7 +15,6 @@ * limitations under the License. */ -#define _GNU_SOURCE #include #include #include diff --git a/src/sys-assert/x86/backtrace.c b/src/sys-assert/x86/backtrace.c index 0453bbb..4c07d86 100644 --- a/src/sys-assert/x86/backtrace.c +++ b/src/sys-assert/x86/backtrace.c @@ -14,7 +14,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -#define _GNU_SOURCE + #include #include #include diff --git a/src/sys-assert/x86/context.c b/src/sys-assert/x86/context.c index 5644d15..313ad86 100644 --- a/src/sys-assert/x86/context.c +++ b/src/sys-assert/x86/context.c @@ -14,7 +14,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -#define _GNU_SOURCE + #include #include #include -- 2.7.4 From 8f8c0ba615a88de9c78d6b24f3ce0470c7015cbd Mon Sep 17 00:00:00 2001 From: Karol Lewandowski Date: Wed, 7 Nov 2018 00:21:26 +0100 Subject: [PATCH 13/16] util: Fix and replace nullvec2str utility function with concatenate() MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit Multiple issues were found and fixed in previously-unused nullvec2str function, including major problem with possible stack corruption via long parameters. This commit completely rewrites the function to dynamically resize the buffer while appending new parameters to avoid previous problems. Additionally, name is changed to somewhat more developer friendly name. Influenced-by: Mateusz Mościcki and Michał Bloch Change-Id: Ia97e3851bb4b5779a14704098752e3644c487f0b --- src/shared/util.c | 31 ++++++++++++++++++++++++------- src/shared/util.h | 2 +- 2 files changed, 25 insertions(+), 8 deletions(-) diff --git a/src/shared/util.c b/src/shared/util.c index 775d6be..bdb8259 100644 --- a/src/shared/util.c +++ b/src/shared/util.c @@ -828,17 +828,34 @@ char* get_exe_path(pid_t pid) return result; } -char *nullvec2str(char *const vec[]) +/* This function is supposed to accept same data as passed to execve + * (argv and envp), which can be arrays of strings as well as NULL + * pointer. + */ +char* concatenate(char *const vec[]) { - char command[PATH_MAX] = {0, }; + size_t length = 0; + for (char *const *p = vec; p && *p; p++) + length += strlen(*p) + 1; + + if (length == 0) + return strdup(""); + + char *str = (char *)malloc(length); + if (!str) + return NULL; - for (char *const *p = vec; *p; ++p) { - strncat(command, *p, sizeof(command)-1); - strncat(command, " ", sizeof(command)-1); + char *destp = str; + char *const *vecp = vec; + while (*vecp) { + destp = stpcpy(destp, *(vecp++)); + if (*vecp) + destp = stpcpy(destp, " "); } - command[sizeof(command)-1] = 0; - return strdup(command); + + return str; } + /** * @} */ diff --git a/src/shared/util.h b/src/shared/util.h index dfe40bf..53e0c7d 100644 --- a/src/shared/util.h +++ b/src/shared/util.h @@ -67,7 +67,7 @@ char* get_exe_path(pid_t pid); char* get_cmd_line(pid_t pid); -char *nullvec2str(char *const vec[]); +char* concatenate(char *const vec[]); #ifdef __cplusplus } -- 2.7.4 From 6accf65c59124e5a70e6034922ad14f8853fc511 Mon Sep 17 00:00:00 2001 From: Karol Lewandowski Date: Wed, 7 Nov 2018 12:58:19 +0100 Subject: [PATCH 14/16] util: Add fsync_path to selectively flush file buffer to disk Change-Id: I3cd9169dafff33cdfbaf1c21955c37d38a524fae --- src/shared/util.c | 16 ++++++++++++++++ src/shared/util.h | 2 ++ 2 files changed, 18 insertions(+) diff --git a/src/shared/util.c b/src/shared/util.c index bdb8259..85879d6 100644 --- a/src/shared/util.c +++ b/src/shared/util.c @@ -502,6 +502,22 @@ int run_command_timeout(char *path, char *args[], char *env[], int timeout) return res; } +int fsync_path(char *const path) +{ + int fd, ret; + + ret = fd = open(path, O_RDONLY); + if (fd >= 0) { + ret = fsync(fd); + close(fd); + } + + if (ret < 0) + _E("Unable to fsync %s: %m", path); + + return ret; +} + static int remove_dir_internal(int fd) { DIR *dir; diff --git a/src/shared/util.h b/src/shared/util.h index 53e0c7d..819a19c 100644 --- a/src/shared/util.h +++ b/src/shared/util.h @@ -51,6 +51,8 @@ int run_command_write_fd_timeout(char *path, char *args[], char *env[], int dfd, int run_command_timeout(char *path, char *args[], char *env[], int timeout); +int fsync_path(char *const path); + int remove_dir(const char *path, int del_dir); int get_exec_pid(const char *execpath); -- 2.7.4 From 5991e4bda77ecc5b0f219370b2ba7fb5eb6f749d Mon Sep 17 00:00:00 2001 From: Karol Lewandowski Date: Thu, 20 Sep 2018 21:32:11 +0200 Subject: [PATCH 15/16] dump_systemstate: Replace extremely inefficient vconftool with buxton2ctl vconftool and buxton2ctl access the same data. vconftool is shell script which under the hood uses buxton2ctl. However, it does so in very inefficient manner - to get the keys it calls buxton2ctl once, but to get key values it calls buxton2ctl for each key. This causes it to fork On TM1 target with 548 keys in "db/" this means 548 fork & buxton2ctl execs. buxton2ctl gets the data from server via socket connection, meaning that it needs to connect & write request and wait for response. All of this together sums up to absurd - `buxton2ctl dump system` takes ~50ms, while `vconftool get db/ -r` takes more than 12seconds - which is roughly 250 times slower! sh-3.2# time buxton2ctl dump system >/dev/null real 0m0.050s user 0m0.040s sys 0m0.000s sh-3.2# time vconftool get db/ -r >/dev/null real 0m12.344s user 0m0.560s sys 0m1.190s With this change applied, dump_systemstate invocation time drops from 13seconds to 1.5-2sec (without logs), and from 13 seconds to 3-4secs (with logs, ie. -k -d -j options). Change-Id: I0cb042d18e5322d8246bf03ee142117217c83b4c --- packaging/crash-worker.spec | 1 + src/dump_systemstate/dump_systemstate.c | 10 +++------- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/packaging/crash-worker.spec b/packaging/crash-worker.spec index 024821a..1bdc552 100644 --- a/packaging/crash-worker.spec +++ b/packaging/crash-worker.spec @@ -41,6 +41,7 @@ Requires: zip Requires: libelf Requires: libdw Requires: %{_sbindir}/minicoredumper +Requires: %{_bindir}/buxton2ctl %description crash-manager diff --git a/src/dump_systemstate/dump_systemstate.c b/src/dump_systemstate/dump_systemstate.c index d4c0e40..f7f254d 100644 --- a/src/dump_systemstate/dump_systemstate.c +++ b/src/dump_systemstate/dump_systemstate.c @@ -188,18 +188,14 @@ int main(int argc, char *argv[]) INFORM_IF_ERROR(ret) if (is_root) { - fprintf_fd(out_fd, "\n==== System configuration (/usr/bin/vconftool get memory, db, file)\n"); - char *get_mem_args[] = {"/bin/vconftool", "get", "memory/", "-r", NULL}; + fprintf_fd(out_fd, "\n==== System configuration (/usr/bin/buxton2ctl dump memory, system)\n"); + char *get_mem_args[] = {"/usr/bin/buxton2ctl", "dump", "memory", NULL}; ret = run_command_write_fd_timeout(get_mem_args[0], get_mem_args, NULL, out_fd, NULL, 0, TIMEOUT_DEFAULT); INFORM_IF_ERROR(ret) - char *get_db_args[] = {"/bin/vconftool", "get", "db/", "-r", NULL}; + char *get_db_args[] = {"/usr/bin/buxton2ctl", "dump", "system", NULL}; ret = run_command_write_fd_timeout(get_db_args[0], get_db_args, NULL, out_fd, NULL, 0, TIMEOUT_DEFAULT); INFORM_IF_ERROR(ret) - - char *get_file_args[] = {"/bin/vconftool", "get", "file/", "-r", NULL}; - ret = run_command_write_fd_timeout(get_file_args[0], get_file_args, NULL, out_fd, NULL, 0, TIMEOUT_DEFAULT); - INFORM_IF_ERROR(ret) } if (arg_dmesg && is_root) { -- 2.7.4 From 865653d839a318ab3834dd91f36938e8d6ef1d65 Mon Sep 17 00:00:00 2001 From: Karol Lewandowski Date: Fri, 16 Nov 2018 16:34:31 +0100 Subject: [PATCH 16/16] Release 5.5.2 This version brings one major change - from now on buxton2ctl is used instead of vconftool to dump the internal database (buxton is the backend used by vconf anyway). The reason for this change is inefficency of vconftool utility, for details see commit 5991e4b ("dump_systemstate: Replace extremely inefficient vconftool with buxton2ctl"). Minor improvements include: - util: Add fsync_path to selectively flush file buffer to disk - util: Fix and replace nullvec2str utility function with concatenate() - Define _GNU_SOURCE once for whole codebase - packaging: Drop unused variables from cmake invocation - util: Unify function calling convention to specify destination first - util: copy_bytes: Multiple changes Change-Id: I260c34576783ee0dfa79c9b704939b943dfa0515 --- 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 1bdc552..97ebb24 100644 --- a/packaging/crash-worker.spec +++ b/packaging/crash-worker.spec @@ -10,7 +10,7 @@ Name: crash-worker Summary: Crash-manager -Version: 5.5.1 +Version: 5.5.2 Release: 1 Group: Framework/system License: Apache-2.0 and BSD -- 2.7.4