From 6507ecd5e1e40d136c1272ce8b9b80fac82f4f67 Mon Sep 17 00:00:00 2001 From: Karol Lewandowski Date: Wed, 3 Oct 2018 11:27:57 +0200 Subject: [PATCH 01/16] util: Remove unused file_exist() Change-Id: I1e20fc6cd55c9685db304a9aa40b58af56df0c00 --- src/shared/util.c | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/src/shared/util.c b/src/shared/util.c index b47fd02..8273ca2 100644 --- a/src/shared/util.c +++ b/src/shared/util.c @@ -146,17 +146,6 @@ int system_command_with_timeout(int timeout_seconds, char *command) } } -int file_exist(const char *file) -{ - FILE *fp; - - fp = fopen(file, "r"); - if (fp == NULL) - return -1; - fclose(fp); - return 1; -} - int write_fd(int fd, const void *buf, int len) { int count; -- 2.7.4 From 7f46a77f84b58234616885af19255166b273a390 Mon Sep 17 00:00:00 2001 From: Karol Lewandowski Date: Wed, 3 Oct 2018 11:29:30 +0200 Subject: [PATCH 02/16] Fix: Do not index buffer[] with negative index in case of read failure Issue reported by Coverity. Change-Id: Ie9cd66d2801770a096bb1849d11c860533508174 --- src/shared/util.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/shared/util.c b/src/shared/util.c index 8273ca2..e6a2cff 100644 --- a/src/shared/util.c +++ b/src/shared/util.c @@ -742,14 +742,13 @@ char* get_cmd_line(pid_t pid) char buffer[PATH_MAX]; ssize_t ret = read(fd, buffer, sizeof(buffer) - 1); - buffer[ret] = '\0'; close(fd); - if (ret <= 0) { _E("Failed to read %s: %m\n", cmdline_path); return NULL; } + buffer[ret] = '\0'; char *result; if (asprintf(&result, "%s", buffer) == -1) { -- 2.7.4 From e293ed8ad6ee9ee57f5d909de19af5a7f0fff8e3 Mon Sep 17 00:00:00 2001 From: Karol Lewandowski Date: Wed, 3 Oct 2018 11:35:46 +0200 Subject: [PATCH 03/16] Release 5.0.6 This change brings one fix and set of assorted cleanups: - Fix: Do not index buffer[] with negative index in case of read failure - util: Remove unused file_exist() - util: Allow functions to be called from c++ - util: Add nullvec2str - util: Remove unused cat_file() - util: Dead code removal - crash-manager: Use same constant for minicoredumper, crash-stack and zip command timeouts Change-Id: I46fbdea06ad36f06116751c5f14105350569cad5 --- 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 c32e7db..6104aea 100644 --- a/packaging/crash-worker.spec +++ b/packaging/crash-worker.spec @@ -10,7 +10,7 @@ Name: crash-worker Summary: Crash-manager -Version: 5.0.5 +Version: 5.0.6 Release: 1 Group: Framework/system License: Apache-2.0 and BSD -- 2.7.4 From bddf2ca3744ee97abb258ddfbc1b95fe18a43047 Mon Sep 17 00:00:00 2001 From: Karol Lewandowski Date: Wed, 3 Oct 2018 15:43:16 +0200 Subject: [PATCH 04/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 05/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 06/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 07/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 08/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 09/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 10/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 11/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 12/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 13/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 14/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 15/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 16/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