From 47cf786c0a13fccd777c334bed4b1e7b02f18d42 Mon Sep 17 00:00:00 2001 From: Franck Bui Date: Fri, 21 Jun 2019 13:12:41 +0200 Subject: [PATCH] coredump: rely on /proc exclusively to get the name of the crashing process I couldn't see any reason why the kernel could provide COMM to the coredump handler via the core_pattern command line but could not make it available in /proc. So let's assume that this info is always available in /proc. For "backtrace" mode (when --backtrace option is passed), I assumed that the crashing process still exists at the time systemd-coredump is called. Also changing the core_pattern line is an API breakage for any users of the backtrace mode but given that systemd-coredump is installed in /usr/lib/systemd, it's a private tool which has no internal users. At least no one complained when the hostname was added to the core_pattern line (f45b8015513)... Indeed it's much easier to get it from /proc since the kernel substitutes '%e' specifier with multiple strings if the process name contains spaces (!). --- src/coredump/coredump.c | 42 ++++++++++++------------------------ sysctl.d/50-coredump.conf.in | 2 +- 2 files changed, 15 insertions(+), 29 deletions(-) diff --git a/src/coredump/coredump.c b/src/coredump/coredump.c index 2898dc2842..0261739a11 100644 --- a/src/coredump/coredump.c +++ b/src/coredump/coredump.c @@ -68,21 +68,14 @@ assert_cc(JOURNAL_SIZE_MAX <= DATA_SIZE_MAX); enum { - /* We use this as array indexes for a couple of special fields we use for - * naming coredump files, and attaching xattrs, and for indexing argv[]. - - * Our pattern for man:systectl(1) kernel.core_pattern is such that the - * kernel passes fields until CONTEXT_RLIMIT as arguments in argv[]. After - * that it gets complicated: the kernel passes "comm" as one or more fields - * starting at index CONTEXT_COMM (in other words, full "comm" is under index - * CONTEXT_COMM when it does not contain spaces, which is the common - * case). This mapping is not reversible, so we prefer to retrieve "comm" - * from /proc. We only fall back to argv[CONTEXT_COMM...] when that fails. + /* We use this as array indexes for a couple of special fields we use for naming + * coredump files, and attaching xattrs, and for indexing argv[]. * - * In the internal context[] array, fields before CONTEXT_COMM are the - * strings from argv[], so they should not be freed. The strings at indices - * CONTEXT_COMM and higher are allocated by us and should be freed at the - * end. + * In the internal context[] array, fields before CONTEXT_COMM are the strings + * from argv[] passed by the kernel according to our pattern defined in + * /proc/sys/kernel/core_pattern (see man:core(5)). So they should not be + * freed. The strings at indices CONTEXT_COMM and higher are allocated by us and + * should be freed at the end. */ CONTEXT_PID, CONTEXT_UID, @@ -1065,15 +1058,12 @@ static char* set_iovec_field_free(struct iovec *iovec, size_t *n_iovec, const ch return x; } -static int gather_pid_metadata( - char* context[_CONTEXT_MAX], - char **comm_fallback, - struct iovec *iovec, size_t *n_iovec) { +static int gather_pid_metadata(char *context[_CONTEXT_MAX], struct iovec *iovec, size_t *n_iovec) { /* We need 27 empty slots in iovec! * - * Note that if we fail on oom later on, we do not roll-back changes to the iovec structure. (It remains valid, - * with the first n_iovec fields initialized.) */ + * Note that if we fail on oom later on, we do not roll-back changes to the iovec + * structure. (It remains valid, with the first n_iovec fields initialized.) */ uid_t owner_uid; pid_t pid; @@ -1086,12 +1076,8 @@ static int gather_pid_metadata( return log_error_errno(r, "Failed to parse PID \"%s\": %m", context[CONTEXT_PID]); r = get_process_comm(pid, &context[CONTEXT_COMM]); - if (r < 0) { - log_warning_errno(r, "Failed to get COMM, falling back to the command line: %m"); - context[CONTEXT_COMM] = strv_join(comm_fallback, " "); - if (!context[CONTEXT_COMM]) - return log_oom(); - } + if (r < 0) + return log_error_errno(r, "Failed to get COMM: %m"); r = get_process_exe(pid, &context[CONTEXT_EXE]); if (r < 0) @@ -1234,7 +1220,7 @@ static int process_kernel(int argc, char* argv[]) { context[CONTEXT_RLIMIT] = argv[1 + CONTEXT_RLIMIT]; context[CONTEXT_HOSTNAME] = argv[1 + CONTEXT_HOSTNAME]; - r = gather_pid_metadata(context, argv + 1 + CONTEXT_COMM, iovec, &n_to_free); + r = gather_pid_metadata(context, iovec, &n_to_free); if (r < 0) goto finish; @@ -1295,7 +1281,7 @@ static int process_backtrace(int argc, char *argv[]) { if (!iovec) return log_oom(); - r = gather_pid_metadata(context, argv + 2 + CONTEXT_COMM, iovec, &n_to_free); + r = gather_pid_metadata(context, iovec, &n_to_free); if (r < 0) goto finish; if (r > 0) { diff --git a/sysctl.d/50-coredump.conf.in b/sysctl.d/50-coredump.conf.in index ccd5c2cc56..47bf847693 100644 --- a/sysctl.d/50-coredump.conf.in +++ b/sysctl.d/50-coredump.conf.in @@ -9,4 +9,4 @@ # and systemd-coredump(8) and core(5) for the explanation of the # setting below. -kernel.core_pattern=|@rootlibexecdir@/systemd-coredump %P %u %g %s %t %c %h %e +kernel.core_pattern=|@rootlibexecdir@/systemd-coredump %P %u %g %s %t %c %h