From fc6cec86136976488e64b8faec2bad6810acfb68 Mon Sep 17 00:00:00 2001 From: Zbigniew Jędrzejewski-Szmek Date: Mon, 26 Sep 2016 23:40:20 +0200 Subject: coredump: remove Storage=both option Back when external storage was initially added in 34c10968cb, this mode of storage was added. This could have made some sense back when XZ compression was used, and an uncompressed core on disk could be used as short-lived cache file which does require costly decompression. But now fast LZ4 compression is used (by default) both internally and externally, so we have duplicated storage, using the same compression and same default maximum core size in both cases, but with different expiration lifetimes. Even the uncompressed-external, compressed-internal mode is not very useful: for small files, decompression with LZ4 is fast enough not to matter, and for large files, decompression is still relatively fast, but the disk-usage penalty is very big. An additional problem with the two modes of storage is that it complicates the code and makes it much harder to return a useful error message to the user if we cannot find the core file, since if we cannot find the file we have to check the internal storage first. This patch drops "both" storage mode. Effectively this means that if somebody configured coredump this way, they will get a warning about an unsupported value for Storage, and the default of "external" will be used. I'm pretty sure that this mode is very rarely used anyway. --- src/coredump/coredump.c | 6 +- src/coredump/coredumpctl.c | 146 +++++++++++++++++++++------------------------ 2 files changed, 71 insertions(+), 81 deletions(-) (limited to 'src') diff --git a/src/coredump/coredump.c b/src/coredump/coredump.c index 7cc3f3fca2..608b290dd4 100644 --- a/src/coredump/coredump.c +++ b/src/coredump/coredump.c @@ -93,7 +93,6 @@ typedef enum CoredumpStorage { COREDUMP_STORAGE_NONE, COREDUMP_STORAGE_EXTERNAL, COREDUMP_STORAGE_JOURNAL, - COREDUMP_STORAGE_BOTH, _COREDUMP_STORAGE_MAX, _COREDUMP_STORAGE_INVALID = -1 } CoredumpStorage; @@ -102,7 +101,6 @@ static const char* const coredump_storage_table[_COREDUMP_STORAGE_MAX] = { [COREDUMP_STORAGE_NONE] = "none", [COREDUMP_STORAGE_EXTERNAL] = "external", [COREDUMP_STORAGE_JOURNAL] = "journal", - [COREDUMP_STORAGE_BOTH] = "both", }; DEFINE_PRIVATE_STRING_TABLE_LOOKUP(coredump_storage, CoredumpStorage); @@ -247,7 +245,7 @@ static int maybe_remove_external_coredump(const char *filename, uint64_t size) { /* Returns 1 if might remove, 0 if will not remove, < 0 on error. */ - if (IN_SET(arg_storage, COREDUMP_STORAGE_EXTERNAL, COREDUMP_STORAGE_BOTH) && + if (arg_storage == COREDUMP_STORAGE_EXTERNAL && size <= arg_external_size_max) return 0; @@ -740,7 +738,7 @@ log: IOVEC_SET_STRING(iovec[n_iovec++], core_message); /* Optionally store the entire coredump in the journal */ - if (IN_SET(arg_storage, COREDUMP_STORAGE_JOURNAL, COREDUMP_STORAGE_BOTH) && + if (arg_storage == COREDUMP_STORAGE_JOURNAL && coredump_size <= arg_journal_size_max) { size_t sz = 0; diff --git a/src/coredump/coredumpctl.c b/src/coredump/coredumpctl.c index 8ba7c08eed..67bf502e73 100644 --- a/src/coredump/coredumpctl.c +++ b/src/coredump/coredumpctl.c @@ -580,23 +580,21 @@ static int save_core(sd_journal *j, int fd, char **path, bool *unlink_temp) { _cleanup_free_ char *filename = NULL; size_t len; int r; + _cleanup_close_ int fdt = -1; + char *temp = NULL; assert((fd >= 0) != !!path); assert(!!path == !!unlink_temp); - /* Prefer uncompressed file to journal (probably cached) to - * compressed file (probably uncached). */ + /* Look for a coredump on disk first. */ r = sd_journal_get_data(j, "COREDUMP_FILENAME", (const void**) &data, &len); if (r < 0 && r != -ENOENT) - log_warning_errno(r, "Failed to retrieve COREDUMP_FILENAME: %m"); + return log_error_errno(r, "Failed to retrieve COREDUMP_FILENAME: %m"); else if (r == 0) retrieve(data, len, "COREDUMP_FILENAME", &filename); - if (filename && access(filename, R_OK) < 0) { - log_full(errno == ENOENT ? LOG_DEBUG : LOG_WARNING, - "File %s is not readable: %m", filename); - filename = mfree(filename); - } + if (filename && access(filename, R_OK) < 0) + return log_error_errno(errno, "File \"%s\" is not readable: %m", filename); if (filename && !endswith(filename, ".xz") && !endswith(filename, ".lz4")) { if (path) { @@ -605,92 +603,86 @@ static int save_core(sd_journal *j, int fd, char **path, bool *unlink_temp) { } return 0; - } else { - _cleanup_close_ int fdt = -1; - char *temp = NULL; + } - if (fd < 0) { - const char *vt; + if (fd < 0) { + const char *vt; - r = var_tmp_dir(&vt); - if (r < 0) - return log_error_errno(r, "Failed to acquire temporary directory path: %m"); + /* Create a temporary file to write the uncompressed core to. */ - temp = strjoin(vt, "/coredump-XXXXXX", NULL); - if (!temp) - return log_oom(); + r = var_tmp_dir(&vt); + if (r < 0) + return log_error_errno(r, "Failed to acquire temporary directory path: %m"); - fdt = mkostemp_safe(temp); - if (fdt < 0) - return log_error_errno(fdt, "Failed to create temporary file: %m"); - log_debug("Created temporary file %s", temp); + temp = strjoin(vt, "/coredump-XXXXXX", NULL); + if (!temp) + return log_oom(); - fd = fdt; - } + fdt = mkostemp_safe(temp); + if (fdt < 0) + return log_error_errno(fdt, "Failed to create temporary file: %m"); + log_debug("Created temporary file %s", temp); - r = sd_journal_get_data(j, "COREDUMP", (const void**) &data, &len); - if (r == 0) { - ssize_t sz; - - assert(len >= 9); - data += 9; - len -= 9; - - sz = write(fdt, data, len); - if (sz < 0) { - r = log_error_errno(errno, - "Failed to write temporary file: %m"); - goto error; - } - if (sz != (ssize_t) len) { - log_error("Short write to temporary file."); - r = -EIO; - goto error; - } - } else if (filename) { + fd = fdt; + } + + if (filename) { #if defined(HAVE_XZ) || defined(HAVE_LZ4) - _cleanup_close_ int fdf; - - fdf = open(filename, O_RDONLY | O_CLOEXEC); - if (fdf < 0) { - r = log_error_errno(errno, - "Failed to open %s: %m", - filename); - goto error; - } + _cleanup_close_ int fdf; - r = decompress_stream(filename, fdf, fd, -1); - if (r < 0) { - log_error_errno(r, "Failed to decompress %s: %m", filename); - goto error; - } -#else - log_error("Cannot decompress file. Compiled without compression support."); - r = -EOPNOTSUPP; + fdf = open(filename, O_RDONLY | O_CLOEXEC); + if (fdf < 0) { + r = log_error_errno(errno, "Failed to open %s: %m", filename); goto error; -#endif - } else { - if (r == -ENOENT) - log_error("Cannot retrieve coredump from journal or disk."); - else - log_error_errno(r, "Failed to retrieve COREDUMP field: %m"); + } + + r = decompress_stream(filename, fdf, fd, -1); + if (r < 0) { + log_error_errno(r, "Failed to decompress %s: %m", filename); goto error; } +#else + log_error("Cannot decompress file. Compiled without compression support."); + r = -EOPNOTSUPP; + goto error; +#endif + } else { + ssize_t sz; - if (temp) { - *path = temp; - *unlink_temp = true; + r = sd_journal_get_data(j, "COREDUMP", (const void**) &data, &len); + if (r < 0) + return log_error_errno(r, + r == -ENOENT ? "Core file was not saved for this entry." : + "Failed to retrieve COREDUMP field: %m"); + + assert(len >= 9); + data += 9; + len -= 9; + + sz = write(fdt, data, len); + if (sz < 0) { + r = log_error_errno(errno, "Failed to write temporary file: %m"); + goto error; } + if (sz != (ssize_t) len) { + log_error("Short write to temporary file."); + r = -EIO; + goto error; + } + } - return 0; + if (temp) { + *path = temp; + *unlink_temp = true; + } + return 0; error: - if (temp) { - unlink(temp); - log_debug("Removed temporary file %s", temp); - } - return r; + if (temp) { + unlink(temp); + log_debug("Removed temporary file %s", temp); } + return r; } static int dump_core(sd_journal* j) { -- cgit v1.2.3-54-g00ecf From 954d3a51afc0ee23e84b1aa9c4a9073901cfb766 Mon Sep 17 00:00:00 2001 From: Zbigniew Jędrzejewski-Szmek Date: Tue, 27 Sep 2016 00:10:04 +0200 Subject: coredumpctl: fix handling of files written to fd Added in 9fe13294a9 (by me :[```), and later obfuscated in d0c8806d4ab, if an uncompressed external file or an internally stored coredump was supposed to be written to a file descriptor, nothing would be written. --- src/coredump/coredumpctl.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) (limited to 'src') diff --git a/src/coredump/coredumpctl.c b/src/coredump/coredumpctl.c index 67bf502e73..05a097076b 100644 --- a/src/coredump/coredumpctl.c +++ b/src/coredump/coredumpctl.c @@ -593,16 +593,16 @@ static int save_core(sd_journal *j, int fd, char **path, bool *unlink_temp) { else if (r == 0) retrieve(data, len, "COREDUMP_FILENAME", &filename); - if (filename && access(filename, R_OK) < 0) - return log_error_errno(errno, "File \"%s\" is not readable: %m", filename); + if (filename) { + if (access(filename, R_OK) < 0) + return log_error_errno(errno, "File \"%s\" is not readable: %m", filename); - if (filename && !endswith(filename, ".xz") && !endswith(filename, ".lz4")) { - if (path) { + if (path && !endswith(filename, ".xz") && !endswith(filename, ".lz4")) { *path = filename; filename = NULL; - } - return 0; + return 0; + } } if (fd < 0) { @@ -659,13 +659,13 @@ static int save_core(sd_journal *j, int fd, char **path, bool *unlink_temp) { data += 9; len -= 9; - sz = write(fdt, data, len); + sz = write(fd, data, len); if (sz < 0) { - r = log_error_errno(errno, "Failed to write temporary file: %m"); + r = log_error_errno(errno, "Failed to write output: %m"); goto error; } if (sz != (ssize_t) len) { - log_error("Short write to temporary file."); + log_error("Short write to output."); r = -EIO; goto error; } -- cgit v1.2.3-54-g00ecf From cfeead6c77792a8cec83e4d5673698aef3e4d817 Mon Sep 17 00:00:00 2001 From: Zbigniew Jędrzejewski-Szmek Date: Tue, 27 Sep 2016 00:40:55 +0200 Subject: coredumpctl: fix spurious "more than one entry matches" warning sd_journal_previous() returns 0 if it didn't do any move, so the warning was stupidly always printed. --- src/coredump/coredumpctl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src') diff --git a/src/coredump/coredumpctl.c b/src/coredump/coredumpctl.c index 05a097076b..45ce31e9bb 100644 --- a/src/coredump/coredumpctl.c +++ b/src/coredump/coredumpctl.c @@ -706,7 +706,7 @@ static int dump_core(sd_journal* j) { return log_error_errno(r, "Coredump retrieval failed: %m"); r = sd_journal_previous(j); - if (r >= 0) + if (r > 0) log_warning("More than one entry matches, ignoring rest."); return 0; -- cgit v1.2.3-54-g00ecf From 554ed50f90a4cea4ed536b1bea598af045953c67 Mon Sep 17 00:00:00 2001 From: Zbigniew Jędrzejewski-Szmek Date: Tue, 27 Sep 2016 01:18:48 +0200 Subject: coredumpctl: report user unit properly --- src/coredump/coredumpctl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src') diff --git a/src/coredump/coredumpctl.c b/src/coredump/coredumpctl.c index 45ce31e9bb..0640816989 100644 --- a/src/coredump/coredumpctl.c +++ b/src/coredump/coredumpctl.c @@ -477,7 +477,7 @@ static int print_info(FILE *file, sd_journal *j, bool need_space) { if (unit) fprintf(file, " Unit: %s\n", unit); if (user_unit) - fprintf(file, " User Unit: %s\n", unit); + fprintf(file, " User Unit: %s\n", user_unit); if (slice) fprintf(file, " Slice: %s\n", slice); if (session) -- cgit v1.2.3-54-g00ecf From 47f50642075a7a215c9f7b600599cbfee81a2913 Mon Sep 17 00:00:00 2001 From: Zbigniew Jędrzejewski-Szmek Date: Tue, 27 Sep 2016 01:19:01 +0200 Subject: coredumpctl: report corefile presence properly In 'list', show present also for coredumps stored in the journal. In 'status', replace "File" with "Storage" line that is always present. Possible values: Storage: none Storage: journal Storage: /path/to/file (inacessible) Storage: /path/to/file Previously the File field be only present if the file was accessible, so users had to manually extract the file name precisely in the cases where it was needed, i.e. when coredumpctl couldn't access the file. It's much more friendly to always show something. This output is designed for human consumption, so it's better to be a bit verbose. The call to sd_j_set_data_threshold is moved, so that status is always printed with the default of 64k, list uses 4k, and coredump retrieval is done with the limit unset. This should make checking for the presence of the COREDUMP field not too costly. --- src/coredump/coredumpctl.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) (limited to 'src') diff --git a/src/coredump/coredumpctl.c b/src/coredump/coredumpctl.c index 0640816989..15ffd56fd1 100644 --- a/src/coredump/coredumpctl.c +++ b/src/coredump/coredumpctl.c @@ -304,7 +304,7 @@ static int print_list(FILE* file, sd_journal *j, int had_legend) { _cleanup_free_ char *pid = NULL, *uid = NULL, *gid = NULL, *sgnl = NULL, *exe = NULL, *comm = NULL, *cmdline = NULL, - *filename = NULL; + *filename = NULL, *coredump = NULL; const void *d; size_t l; usec_t t; @@ -324,6 +324,7 @@ static int print_list(FILE* file, sd_journal *j, int had_legend) { retrieve(d, l, "COREDUMP_COMM", &comm); retrieve(d, l, "COREDUMP_CMDLINE", &cmdline); retrieve(d, l, "COREDUMP_FILENAME", &filename); + retrieve(d, l, "COREDUMP", &coredump); } if (!pid && !uid && !gid && !sgnl && !exe && !comm && !cmdline && !filename) { @@ -336,7 +337,7 @@ static int print_list(FILE* file, sd_journal *j, int had_legend) { return log_error_errno(r, "Failed to get realtime timestamp: %m"); format_timestamp(buf, sizeof(buf), t); - present = filename && access(filename, F_OK) == 0; + present = (filename && access(filename, F_OK) == 0) || coredump; if (!had_legend && !arg_no_legend) fprintf(file, "%-*s %*s %*s %*s %*s %*s %s\n", @@ -367,7 +368,8 @@ static int print_info(FILE *file, sd_journal *j, bool need_space) { *unit = NULL, *user_unit = NULL, *session = NULL, *boot_id = NULL, *machine_id = NULL, *hostname = NULL, *slice = NULL, *cgroup = NULL, *owner_uid = NULL, - *message = NULL, *timestamp = NULL, *filename = NULL; + *message = NULL, *timestamp = NULL, *filename = NULL, + *coredump = NULL; const void *d; size_t l; int r; @@ -391,6 +393,7 @@ static int print_info(FILE *file, sd_journal *j, bool need_space) { retrieve(d, l, "COREDUMP_CGROUP", &cgroup); retrieve(d, l, "COREDUMP_TIMESTAMP", ×tamp); retrieve(d, l, "COREDUMP_FILENAME", &filename); + retrieve(d, l, "COREDUMP", &coredump); retrieve(d, l, "_BOOT_ID", &boot_id); retrieve(d, l, "_MACHINE_ID", &machine_id); retrieve(d, l, "_HOSTNAME", &hostname); @@ -505,8 +508,13 @@ static int print_info(FILE *file, sd_journal *j, bool need_space) { if (hostname) fprintf(file, " Hostname: %s\n", hostname); - if (filename && access(filename, F_OK) == 0) - fprintf(file, " Coredump: %s\n", filename); + if (filename) + fprintf(file, " Storage: %s%s\n", filename, + access(filename, F_OK) < 0 ? " (inaccessible)" : ""); + else if (coredump) + fprintf(file, " Storage: journal\n"); + else + fprintf(file, " Storage: none\n"); if (message) { _cleanup_free_ char *m = NULL; @@ -586,6 +594,9 @@ static int save_core(sd_journal *j, int fd, char **path, bool *unlink_temp) { assert((fd >= 0) != !!path); assert(!!path == !!unlink_temp); + /* We want full data, nothing truncated. */ + sd_journal_set_data_threshold(j, 0); + /* Look for a coredump on disk first. */ r = sd_journal_get_data(j, "COREDUMP_FILENAME", (const void**) &data, &len); if (r < 0 && r != -ENOENT) @@ -828,9 +839,6 @@ int main(int argc, char *argv[]) { } } - /* We want full data, nothing truncated. */ - sd_journal_set_data_threshold(j, 0); - SET_FOREACH(match, matches, it) { r = sd_journal_add_match(j, match, strlen(match)); if (r != 0) { -- cgit v1.2.3-54-g00ecf From 04de587942054c414beda54af303880851b0fa4c Mon Sep 17 00:00:00 2001 From: Zbigniew Jędrzejewski-Szmek Date: Tue, 27 Sep 2016 01:41:38 +0200 Subject: coredumpctl: rework presence reporting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The column for "present" was easy to miss, especially if somebody had no coredumps present at all, in which case the column of spaces of width one wasn't visually distinguished from the neighbouring columns. Replace this with an explicit text, one of: "missing", "journal", "present", "error". $ coredumpctl TIME PID UID GID SIG COREFILE EXE Mon 2016-09-26 22:46:31 CEST 8623 0 0 11 missing /usr/bin/bash Mon 2016-09-26 22:46:35 CEST 8639 1001 1001 11 missing /usr/bin/bash Tue 2016-09-27 01:10:46 CEST 16110 1001 1001 11 journal /usr/bin/bash Tue 2016-09-27 01:13:20 CEST 16290 1001 1001 11 journal /usr/bin/bash Tue 2016-09-27 01:33:48 CEST 17867 1001 1001 11 present /usr/bin/bash Tue 2016-09-27 01:37:55 CEST 18549 0 0 11 error /usr/bin/bash Also, use access(…, R_OK), so that we can report a present but inaccessible file different than a missing one. --- src/coredump/coredumpctl.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) (limited to 'src') diff --git a/src/coredump/coredumpctl.c b/src/coredump/coredumpctl.c index 15ffd56fd1..57d18006a4 100644 --- a/src/coredump/coredumpctl.c +++ b/src/coredump/coredumpctl.c @@ -310,7 +310,7 @@ static int print_list(FILE* file, sd_journal *j, int had_legend) { usec_t t; char buf[FORMAT_TIMESTAMP_MAX]; int r; - bool present; + const char *present; assert(file); assert(j); @@ -337,7 +337,6 @@ static int print_list(FILE* file, sd_journal *j, int had_legend) { return log_error_errno(r, "Failed to get realtime timestamp: %m"); format_timestamp(buf, sizeof(buf), t); - present = (filename && access(filename, F_OK) == 0) || coredump; if (!had_legend && !arg_no_legend) fprintf(file, "%-*s %*s %*s %*s %*s %*s %s\n", @@ -346,16 +345,28 @@ static int print_list(FILE* file, sd_journal *j, int had_legend) { 5, "UID", 5, "GID", 3, "SIG", - 1, "PRESENT", + 8, "COREFILE", "EXE"); - fprintf(file, "%-*s %*s %*s %*s %*s %*s %s\n", + if (filename) + if (access(filename, R_OK) == 0) + present = "present"; + else if (errno == ENOENT) + present = "missing"; + else + present = "error"; + else if (coredump) + present = "journal"; + else + present = "none"; + + fprintf(file, "%-*s %*s %*s %*s %*s %-*s %s\n", FORMAT_TIMESTAMP_WIDTH, buf, 6, strna(pid), 5, strna(uid), 5, strna(gid), 3, strna(sgnl), - 1, present ? "*" : "", + 8, present, strna(exe ?: (comm ?: cmdline))); return 0; @@ -510,7 +521,7 @@ static int print_info(FILE *file, sd_journal *j, bool need_space) { if (filename) fprintf(file, " Storage: %s%s\n", filename, - access(filename, F_OK) < 0 ? " (inaccessible)" : ""); + access(filename, R_OK) < 0 ? " (inaccessible)" : ""); else if (coredump) fprintf(file, " Storage: journal\n"); else -- cgit v1.2.3-54-g00ecf From 062b99e8be63ba49b80d988da9b00a1d39255496 Mon Sep 17 00:00:00 2001 From: Zbigniew Jędrzejewski-Szmek Date: Tue, 27 Sep 2016 10:52:10 +0200 Subject: coredumpctl: tighten print_field() code Propagate errors properly, so that if we hit oom or an error in the journal, the whole command will fail. This is important when using the output in scripts. Support the output of multiple values for the same field with -F. The journal supports that, and our official commands should too, as far as it makes sense. -F can be used to print user-defined fields (e.g. somebody could use a TAG field with multiple occurences), so we should support that too. That seems better than silently printing the last value found as was done before. We would iterate trying to match the same field with all possible field names. Once we find something, cut the loop short, since we know that nothing else can match. --- src/coredump/coredumpctl.c | 109 +++++++++++++++++++++++++++------------------ 1 file changed, 66 insertions(+), 43 deletions(-) (limited to 'src') diff --git a/src/coredump/coredumpctl.c b/src/coredump/coredumpctl.c index 57d18006a4..a408adf169 100644 --- a/src/coredump/coredumpctl.c +++ b/src/coredump/coredumpctl.c @@ -280,11 +280,10 @@ static int retrieve(const void *data, free(*var); *var = v; - return 0; + return 1; } -static void print_field(FILE* file, sd_journal *j) { - _cleanup_free_ char *value = NULL; +static int print_field(FILE* file, sd_journal *j) { const void *d; size_t l; @@ -293,13 +292,34 @@ static void print_field(FILE* file, sd_journal *j) { assert(arg_field); - SD_JOURNAL_FOREACH_DATA(j, d, l) - retrieve(d, l, arg_field, &value); + /* A (user-specified) field may appear more than once for a given entry. + * We will print all of the occurences. + * This is different below for fields that systemd-coredump uses, + * because they cannot meaningfully appear more than once. + */ + SD_JOURNAL_FOREACH_DATA(j, d, l) { + _cleanup_free_ char *value = NULL; + int r; + + r = retrieve(d, l, arg_field, &value); + if (r < 0) + return r; + if (r > 0) + fprintf(file, "%s\n", value); + } - if (value) - fprintf(file, "%s\n", value); + return 0; } +#define RETRIEVE(d, l, name, arg) \ + { \ + int _r = retrieve(d, l, name, &arg); \ + if (_r < 0) \ + return _r; \ + if (_r > 0) \ + continue; \ + } + static int print_list(FILE* file, sd_journal *j, int had_legend) { _cleanup_free_ char *pid = NULL, *uid = NULL, *gid = NULL, @@ -316,15 +336,15 @@ static int print_list(FILE* file, sd_journal *j, int had_legend) { assert(j); SD_JOURNAL_FOREACH_DATA(j, d, l) { - retrieve(d, l, "COREDUMP_PID", &pid); - retrieve(d, l, "COREDUMP_UID", &uid); - retrieve(d, l, "COREDUMP_GID", &gid); - retrieve(d, l, "COREDUMP_SIGNAL", &sgnl); - retrieve(d, l, "COREDUMP_EXE", &exe); - retrieve(d, l, "COREDUMP_COMM", &comm); - retrieve(d, l, "COREDUMP_CMDLINE", &cmdline); - retrieve(d, l, "COREDUMP_FILENAME", &filename); - retrieve(d, l, "COREDUMP", &coredump); + RETRIEVE(d, l, "COREDUMP_PID", pid); + RETRIEVE(d, l, "COREDUMP_UID", uid); + RETRIEVE(d, l, "COREDUMP_GID", gid); + RETRIEVE(d, l, "COREDUMP_SIGNAL", sgnl); + RETRIEVE(d, l, "COREDUMP_EXE", exe); + RETRIEVE(d, l, "COREDUMP_COMM", comm); + RETRIEVE(d, l, "COREDUMP_CMDLINE", cmdline); + RETRIEVE(d, l, "COREDUMP_FILENAME", filename); + RETRIEVE(d, l, "COREDUMP", coredump); } if (!pid && !uid && !gid && !sgnl && !exe && !comm && !cmdline && !filename) { @@ -389,26 +409,26 @@ static int print_info(FILE *file, sd_journal *j, bool need_space) { assert(j); SD_JOURNAL_FOREACH_DATA(j, d, l) { - retrieve(d, l, "COREDUMP_PID", &pid); - retrieve(d, l, "COREDUMP_UID", &uid); - retrieve(d, l, "COREDUMP_GID", &gid); - retrieve(d, l, "COREDUMP_SIGNAL", &sgnl); - retrieve(d, l, "COREDUMP_EXE", &exe); - retrieve(d, l, "COREDUMP_COMM", &comm); - retrieve(d, l, "COREDUMP_CMDLINE", &cmdline); - retrieve(d, l, "COREDUMP_UNIT", &unit); - retrieve(d, l, "COREDUMP_USER_UNIT", &user_unit); - retrieve(d, l, "COREDUMP_SESSION", &session); - retrieve(d, l, "COREDUMP_OWNER_UID", &owner_uid); - retrieve(d, l, "COREDUMP_SLICE", &slice); - retrieve(d, l, "COREDUMP_CGROUP", &cgroup); - retrieve(d, l, "COREDUMP_TIMESTAMP", ×tamp); - retrieve(d, l, "COREDUMP_FILENAME", &filename); - retrieve(d, l, "COREDUMP", &coredump); - retrieve(d, l, "_BOOT_ID", &boot_id); - retrieve(d, l, "_MACHINE_ID", &machine_id); - retrieve(d, l, "_HOSTNAME", &hostname); - retrieve(d, l, "MESSAGE", &message); + RETRIEVE(d, l, "COREDUMP_PID", pid); + RETRIEVE(d, l, "COREDUMP_UID", uid); + RETRIEVE(d, l, "COREDUMP_GID", gid); + RETRIEVE(d, l, "COREDUMP_SIGNAL", sgnl); + RETRIEVE(d, l, "COREDUMP_EXE", exe); + RETRIEVE(d, l, "COREDUMP_COMM", comm); + RETRIEVE(d, l, "COREDUMP_CMDLINE", cmdline); + RETRIEVE(d, l, "COREDUMP_UNIT", unit); + RETRIEVE(d, l, "COREDUMP_USER_UNIT", user_unit); + RETRIEVE(d, l, "COREDUMP_SESSION", session); + RETRIEVE(d, l, "COREDUMP_OWNER_UID", owner_uid); + RETRIEVE(d, l, "COREDUMP_SLICE", slice); + RETRIEVE(d, l, "COREDUMP_CGROUP", cgroup); + RETRIEVE(d, l, "COREDUMP_TIMESTAMP", timestamp); + RETRIEVE(d, l, "COREDUMP_FILENAME", filename); + RETRIEVE(d, l, "COREDUMP", coredump); + RETRIEVE(d, l, "_BOOT_ID", boot_id); + RETRIEVE(d, l, "_MACHINE_ID", machine_id); + RETRIEVE(d, l, "_HOSTNAME", hostname); + RETRIEVE(d, l, "MESSAGE", message); } if (need_space) @@ -553,15 +573,15 @@ static int focus(sd_journal *j) { return r; } -static void print_entry(sd_journal *j, unsigned n_found) { +static int print_entry(sd_journal *j, unsigned n_found) { assert(j); if (arg_action == ACTION_INFO) - print_info(stdout, j, n_found); + return print_info(stdout, j, n_found); else if (arg_field) - print_field(stdout, j); + return print_field(stdout, j); else - print_list(stdout, j, n_found); + return print_list(stdout, j, n_found); } static int dump_list(sd_journal *j) { @@ -580,10 +600,13 @@ static int dump_list(sd_journal *j) { if (r < 0) return r; - print_entry(j, 0); + return print_entry(j, 0); } else { - SD_JOURNAL_FOREACH(j) - print_entry(j, n_found++); + SD_JOURNAL_FOREACH(j) { + r = print_entry(j, n_found++); + if (r < 0) + return r; + } if (!arg_field && n_found <= 0) { log_notice("No coredumps found."); -- cgit v1.2.3-54-g00ecf From bb7c5bad4a088a18d102c448caaa72e36bfdb84d Mon Sep 17 00:00:00 2001 From: Zbigniew Jędrzejewski-Szmek Date: Tue, 27 Sep 2016 00:32:42 +0200 Subject: coredumpctl: delay the "on tty" refusal until as late as possible For the user, if the core file is missing or inaccessible, it is more interesting that the fact that they forgot to pipe to a file. So delay the failure from the check until after we have verified that the file or the COREDUMP field are present. Partially fixes #4161. Also, error reporting on failure was duplicated. save_core() now always prints an error message (because it knows the paths involved, so can the most useful message), and the callers don't have to. --- src/coredump/coredumpctl.c | 66 ++++++++++++++++++++++++++++++---------------- 1 file changed, 43 insertions(+), 23 deletions(-) (limited to 'src') diff --git a/src/coredump/coredumpctl.c b/src/coredump/coredumpctl.c index a408adf169..0e5351e621 100644 --- a/src/coredump/coredumpctl.c +++ b/src/coredump/coredumpctl.c @@ -617,26 +617,34 @@ static int dump_list(sd_journal *j) { return 0; } -static int save_core(sd_journal *j, int fd, char **path, bool *unlink_temp) { +static int save_core(sd_journal *j, FILE *file, char **path, bool *unlink_temp) { const char *data; _cleanup_free_ char *filename = NULL; size_t len; - int r; + int r, fd; _cleanup_close_ int fdt = -1; char *temp = NULL; - assert((fd >= 0) != !!path); - assert(!!path == !!unlink_temp); - - /* We want full data, nothing truncated. */ - sd_journal_set_data_threshold(j, 0); + assert(!(file && path)); /* At most one can be specified */ + assert(!!path == !!unlink_temp); /* Those must be specified together */ /* Look for a coredump on disk first. */ r = sd_journal_get_data(j, "COREDUMP_FILENAME", (const void**) &data, &len); - if (r < 0 && r != -ENOENT) - return log_error_errno(r, "Failed to retrieve COREDUMP_FILENAME: %m"); - else if (r == 0) + if (r == 0) retrieve(data, len, "COREDUMP_FILENAME", &filename); + else { + if (r != -ENOENT) + return log_error_errno(r, "Failed to retrieve COREDUMP_FILENAME field: %m"); + /* Check that we can have a COREDUMP field. We still haven't set a high + * data threshold, so we'll get a few kilobytes at most. + */ + + r = sd_journal_get_data(j, "COREDUMP", (const void**) &data, &len); + if (r == -ENOENT) + return log_error_errno(r, "Coredump entry has no core attached (neither internally in the journal nor externally on disk)."); + if (r < 0) + return log_error_errno(r, "Failed to retrieve COREDUMP field: %m"); + } if (filename) { if (access(filename, R_OK) < 0) @@ -650,7 +658,7 @@ static int save_core(sd_journal *j, int fd, char **path, bool *unlink_temp) { } } - if (fd < 0) { + if (path) { const char *vt; /* Create a temporary file to write the uncompressed core to. */ @@ -669,6 +677,22 @@ static int save_core(sd_journal *j, int fd, char **path, bool *unlink_temp) { log_debug("Created temporary file %s", temp); fd = fdt; + } else { + /* If neither path or file are specified, we will write to stdout. Let's now check + * if stdout is connected to a tty. We checked that the file exists, or that the + * core might be stored in the journal. In this second case, if we found the entry, + * in all likelyhood we will be able to access the COREDUMP= field. In either case, + * we stop before doing any "real" work, i.e. before starting decompression or + * reading from the file or creating temporary files. + */ + if (!file) { + if (on_tty()) + return log_error_errno(ENOTTY, "Refusing to dump core to tty" + " (use shell redirection or specify --output)."); + file = stdout; + } + + fd = fileno(file); } if (filename) { @@ -694,11 +718,12 @@ static int save_core(sd_journal *j, int fd, char **path, bool *unlink_temp) { } else { ssize_t sz; + /* We want full data, nothing truncated. */ + sd_journal_set_data_threshold(j, 0); + r = sd_journal_get_data(j, "COREDUMP", (const void**) &data, &len); if (r < 0) - return log_error_errno(r, - r == -ENOENT ? "Core file was not saved for this entry." : - "Failed to retrieve COREDUMP field: %m"); + return log_error_errno(r, "Failed to retrieve COREDUMP field: %m"); assert(len >= 9); data += 9; @@ -741,14 +766,9 @@ static int dump_core(sd_journal* j) { print_info(arg_output ? stdout : stderr, j, false); - if (on_tty() && !arg_output) { - log_error("Refusing to dump core to tty."); - return -ENOTTY; - } - - r = save_core(j, arg_output ? fileno(arg_output) : STDOUT_FILENO, NULL, NULL); + r = save_core(j, arg_output, NULL, NULL); if (r < 0) - return log_error_errno(r, "Coredump retrieval failed: %m"); + return r; r = sd_journal_previous(j); if (r > 0) @@ -797,9 +817,9 @@ static int run_gdb(sd_journal *j) { return -ENOENT; } - r = save_core(j, -1, &path, &unlink_path); + r = save_core(j, NULL, &path, &unlink_path); if (r < 0) - return log_error_errno(r, "Failed to retrieve core: %m"); + return r; pid = fork(); if (pid < 0) { -- cgit v1.2.3-54-g00ecf From 6e9ef6038f3971ae42657b98d952ed4c1318b4d7 Mon Sep 17 00:00:00 2001 From: Zbigniew Jędrzejewski-Szmek Date: Tue, 27 Sep 2016 11:32:36 +0200 Subject: coredump: log if the core is too large to store or generate backtrace Another fix for #4161. --- src/coredump/coredump.c | 39 +++++++++++++++++++++++---------------- 1 file changed, 23 insertions(+), 16 deletions(-) (limited to 'src') diff --git a/src/coredump/coredump.c b/src/coredump/coredump.c index 608b290dd4..ecb38bdd8c 100644 --- a/src/coredump/coredump.c +++ b/src/coredump/coredump.c @@ -370,8 +370,7 @@ static int save_external_coredump( #if defined(HAVE_XZ) || defined(HAVE_LZ4) /* If we will remove the coredump anyway, do not compress. */ - if (maybe_remove_external_coredump(NULL, st.st_size) == 0 - && arg_compress) { + if (arg_compress && !maybe_remove_external_coredump(NULL, st.st_size)) { _cleanup_free_ char *fn_compressed = NULL, *tmp_compressed = NULL; _cleanup_close_ int fd_compressed = -1; @@ -703,7 +702,9 @@ static int submit_coredump( coredump_filename = strjoina("COREDUMP_FILENAME=", filename); IOVEC_SET_STRING(iovec[n_iovec++], coredump_filename); - } + } else if (arg_storage == COREDUMP_STORAGE_EXTERNAL) + log_info("The core will not be stored: size %zu is greater than %zu (the configured maximum)", + coredump_size, arg_external_size_max); /* Vacuum again, but exclude the coredump we just created */ (void) coredump_vacuum(coredump_node_fd >= 0 ? coredump_node_fd : coredump_fd, arg_keep_free, arg_max_use); @@ -728,7 +729,9 @@ static int submit_coredump( log_warning("Failed to generate stack trace: %s", dwfl_errmsg(dwfl_errno())); else log_warning_errno(r, "Failed to generate stack trace: %m"); - } + } else + log_debug("Not generating stack trace: core size %zu is greater than %zu (the configured maximum)", + coredump_size, arg_process_size_max); if (!core_message) #endif @@ -738,18 +741,22 @@ log: IOVEC_SET_STRING(iovec[n_iovec++], core_message); /* Optionally store the entire coredump in the journal */ - if (arg_storage == COREDUMP_STORAGE_JOURNAL && - coredump_size <= arg_journal_size_max) { - size_t sz = 0; - - /* Store the coredump itself in the journal */ - - r = allocate_journal_field(coredump_fd, (size_t) coredump_size, &coredump_data, &sz); - if (r >= 0) { - iovec[n_iovec].iov_base = coredump_data; - iovec[n_iovec].iov_len = sz; - n_iovec++; - } + if (arg_storage == COREDUMP_STORAGE_JOURNAL) { + if (coredump_size <= arg_journal_size_max) { + size_t sz = 0; + + /* Store the coredump itself in the journal */ + + r = allocate_journal_field(coredump_fd, (size_t) coredump_size, &coredump_data, &sz); + if (r >= 0) { + iovec[n_iovec].iov_base = coredump_data; + iovec[n_iovec].iov_len = sz; + n_iovec++; + } else + log_warning_errno(r, "Failed to attach the core to the journal entry: %m"); + } else + log_info("The core will not be stored: size %zu is greater than %zu (the configured maximum)", + coredump_size, arg_journal_size_max); } assert(n_iovec <= n_iovec_allocated); -- cgit v1.2.3-54-g00ecf From 73a99163a721d9e96bf7006ecbfb1aefce228c99 Mon Sep 17 00:00:00 2001 From: Zbigniew Jędrzejewski-Szmek Date: Tue, 27 Sep 2016 12:40:54 +0200 Subject: coredump,catalog: give better notice when a core file is truncated coredump had code to check if copy_bytes() hit the max_bytes limit, and refuse further processing in that case. But in 84ee0960443, the return convention for copy_bytes() was changed from -EFBIG to 1 for the case when the limit is hit, so the condition check in coredump couldn't ever trigger. But it seems that *do* want to process such truncated cores [1]. So change the code to detect truncation properly, but instead of returning an error, give a nice log entry. [1] https://github.com/systemd/systemd/issues/3883#issuecomment-239106337 Should fix (or at least alleviate) #3883. --- TODO | 1 + catalog/systemd.catalog.in | 11 +++++++++++ src/coredump/coredump.c | 31 ++++++++++++++++++------------- src/systemd/sd-messages.h | 1 + 4 files changed, 31 insertions(+), 13 deletions(-) (limited to 'src') diff --git a/TODO b/TODO index a47f4c488b..64d530b7d7 100644 --- a/TODO +++ b/TODO @@ -670,6 +670,7 @@ Features: * coredump: - save coredump in Windows/Mozilla minidump format + - when truncating coredumps, also log the full size that the process had, and make a metadata field so we can report truncated coredumps * support crash reporting operation modes (https://live.gnome.org/GnomeOS/Design/Whiteboards/ProblemReporting) diff --git a/catalog/systemd.catalog.in b/catalog/systemd.catalog.in index 8de8597fe9..2c72d31290 100644 --- a/catalog/systemd.catalog.in +++ b/catalog/systemd.catalog.in @@ -88,6 +88,17 @@ Process @COREDUMP_PID@ (@COREDUMP_COMM@) crashed and dumped core. This usually indicates a programming error in the crashing program and should be reported to its vendor as a bug. +-- 5aadd8e954dc4b1a8c954d63fd9e1137 +Subject: Core file was truncated to @SIZE_LIMIT@ bytes. +Defined-By: systemd +Support: %SUPPORT_URL% +Documentation: man:coredump.conf(5) + +The process had more memory mapped than the configured maximum for processing +and storage by systemd-coredump(8). Only the first @SIZE_LIMIT@ bytes were +saved. This core might still be usable, but various tools like gdb(1) will warn +about the file being truncated. + -- fc2e22bc6ee647b6b90729ab34a250b1 de Subject: Speicherabbild für Prozess @COREDUMP_PID@ (@COREDUMP_COMM) generiert Defined-By: systemd diff --git a/src/coredump/coredump.c b/src/coredump/coredump.c index ecb38bdd8c..db60d0af7a 100644 --- a/src/coredump/coredump.c +++ b/src/coredump/coredump.c @@ -28,9 +28,10 @@ #include #endif +#include "sd-daemon.h" #include "sd-journal.h" #include "sd-login.h" -#include "sd-daemon.h" +#include "sd-messages.h" #include "acl-util.h" #include "alloc-util.h" @@ -133,6 +134,10 @@ static int parse_config(void) { false, NULL); } +static inline uint64_t storage_size_max(void) { + return arg_storage == COREDUMP_STORAGE_EXTERNAL ? arg_external_size_max : arg_journal_size_max; +} + static int fix_acl(int fd, uid_t uid) { #ifdef HAVE_ACL @@ -329,12 +334,13 @@ static int save_external_coredump( /* Is coredumping disabled? Then don't bother saving/processing the coredump. * Anything below PAGE_SIZE cannot give a readable coredump (the kernel uses * ELF_EXEC_PAGESIZE which is not easily accessible, but is usually the same as PAGE_SIZE. */ - log_info("Core dumping has been disabled for process %s (%s).", context[CONTEXT_PID], context[CONTEXT_COMM]); + log_info("Resource limits disable core dumping for process %s (%s).", + context[CONTEXT_PID], context[CONTEXT_COMM]); return -EBADSLT; } /* Never store more than the process configured, or than we actually shall keep or process */ - max_size = MIN(rlimit, MAX(arg_process_size_max, arg_external_size_max)); + max_size = MIN(rlimit, MAX(arg_process_size_max, storage_size_max())); r = make_filename(context, &fn); if (r < 0) @@ -347,19 +353,18 @@ static int save_external_coredump( return log_error_errno(fd, "Failed to create temporary file for coredump %s: %m", fn); r = copy_bytes(input_fd, fd, max_size, false); - if (r == -EFBIG) { - log_error("Coredump of %s (%s) is larger than configured processing limit, refusing.", context[CONTEXT_PID], context[CONTEXT_COMM]); - goto fail; - } else if (IN_SET(r, -EDQUOT, -ENOSPC)) { - log_error("Not enough disk space for coredump of %s (%s), refusing.", context[CONTEXT_PID], context[CONTEXT_COMM]); - goto fail; - } else if (r < 0) { - log_error_errno(r, "Failed to dump coredump to file: %m"); + if (r < 0) { + log_error_errno(r, "Cannot store coredump of %s (%s): %m", context[CONTEXT_PID], context[CONTEXT_COMM]); goto fail; - } + } else if (r == 1) + log_struct(LOG_INFO, + LOG_MESSAGE("Core file was truncated to %zu bytes.", max_size), + "SIZE_LIMIT=%zu", max_size, + LOG_MESSAGE_ID(SD_MESSAGE_TRUNCATED_CORE), + NULL); if (fstat(fd, &st) < 0) { - log_error_errno(errno, "Failed to fstat coredump %s: %m", coredump_tmpfile_name(tmp)); + log_error_errno(errno, "Failed to fstat core file %s: %m", coredump_tmpfile_name(tmp)); goto fail; } diff --git a/src/systemd/sd-messages.h b/src/systemd/sd-messages.h index 3c44d63021..79246ae060 100644 --- a/src/systemd/sd-messages.h +++ b/src/systemd/sd-messages.h @@ -40,6 +40,7 @@ _SD_BEGIN_DECLARATIONS; #define SD_MESSAGE_JOURNAL_USAGE SD_ID128_MAKE(ec,38,7f,57,7b,84,4b,8f,a9,48,f3,3c,ad,9a,75,e6) #define SD_MESSAGE_COREDUMP SD_ID128_MAKE(fc,2e,22,bc,6e,e6,47,b6,b9,07,29,ab,34,a2,50,b1) +#define SD_MESSAGE_TRUNCATED_CORE SD_ID128_MAKE(5a,ad,d8,e9,54,dc,4b,1a,8c,95,4d,63,fd,9e,11,37) #define SD_MESSAGE_SESSION_START SD_ID128_MAKE(8d,45,62,0c,1a,43,48,db,b1,74,10,da,57,c6,0c,66) #define SD_MESSAGE_SESSION_STOP SD_ID128_MAKE(33,54,93,94,24,b4,45,6d,98,02,ca,83,33,ed,42,4a) -- cgit v1.2.3-54-g00ecf