From ea5cc2a8f65535a9b3f8ba39a8df13a0c770f41d Mon Sep 17 00:00:00 2001 From: =?utf8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sat, 25 Feb 2017 14:00:39 -0500 Subject: [PATCH] coredump: do not try to access unitialized CONTEXT_COMM field Most of the fields in the context array come from the kernel (passed through argv), but two are special: comm and exe. We allocate them ourselves. We forgot to initialize context[CONTEXT_COMM] with the value we allocated (introduced in 9aa820231414baa28e6bf02a033932cb69ff6b8b). To simplify things, just set context[CONTEXT_COMM] and context[CONTEXT_EXE], and free those two fields at the end. Fixes #5442. --- src/coredump/coredump.c | 90 ++++++++++++++++++++++++----------------- 1 file changed, 53 insertions(+), 37 deletions(-) diff --git a/src/coredump/coredump.c b/src/coredump/coredump.c index 463e0abef46..e38cc59a36f 100644 --- a/src/coredump/coredump.c +++ b/src/coredump/coredump.c @@ -78,8 +78,22 @@ 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 coredumping files, and - * attaching xattrs */ + /* 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. + * + * 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. + */ CONTEXT_PID, CONTEXT_UID, CONTEXT_GID, @@ -784,7 +798,7 @@ log: return 0; } -static void map_context_fields(const struct iovec *iovec, const char *context[]) { +static void map_context_fields(const struct iovec *iovec, const char* context[]) { static const char * const context_field_names[_CONTEXT_MAX] = { [CONTEXT_PID] = "COREDUMP_PID=", @@ -1054,9 +1068,8 @@ static char* set_iovec_field_free(struct iovec iovec[27], size_t *n_iovec, const } static int gather_pid_metadata_and_process_special_crash( - const char *context[_CONTEXT_MAX], + char* context[_CONTEXT_MAX], char **comm_fallback, - char **comm_ret, struct iovec *iovec, size_t *n_iovec) { /* We need 26 empty slots in iovec! @@ -1064,7 +1077,6 @@ static int gather_pid_metadata_and_process_special_crash( * 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.) */ - _cleanup_free_ char *exe = NULL, *comm = NULL; uid_t owner_uid; pid_t pid; char *t; @@ -1075,15 +1087,15 @@ static int gather_pid_metadata_and_process_special_crash( if (r < 0) return log_error_errno(r, "Failed to parse PID \"%s\": %m", context[CONTEXT_PID]); - r = get_process_comm(pid, &comm); + 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"); - comm = strv_join(comm_fallback, " "); - if (!comm) + context[CONTEXT_COMM] = strv_join(comm_fallback, " "); + if (!context[CONTEXT_COMM]) return log_oom(); } - r = get_process_exe(pid, &exe); + r = get_process_exe(pid, &context[CONTEXT_EXE]); if (r < 0) log_warning_errno(r, "Failed to get EXE, ignoring: %m"); @@ -1099,7 +1111,7 @@ static int gather_pid_metadata_and_process_special_crash( * are unlikely to work then. */ if (STR_IN_SET(t, SPECIAL_JOURNALD_SERVICE, SPECIAL_INIT_SCOPE)) { free(t); - r = process_special_crash(context, STDIN_FILENO); + r = process_special_crash((const char**) context, STDIN_FILENO); if (r < 0) return r; @@ -1132,11 +1144,11 @@ static int gather_pid_metadata_and_process_special_crash( if (!set_iovec_field(iovec, n_iovec, "COREDUMP_RLIMIT=", context[CONTEXT_RLIMIT])) return log_oom(); - if (!set_iovec_field(iovec, n_iovec, "COREDUMP_COMM=", comm)) + if (!set_iovec_field(iovec, n_iovec, "COREDUMP_COMM=", context[CONTEXT_COMM])) return log_oom(); - if (exe && - !set_iovec_field(iovec, n_iovec, "COREDUMP_EXE=", exe)) + if (context[CONTEXT_EXE] && + !set_iovec_field(iovec, n_iovec, "COREDUMP_EXE=", context[CONTEXT_EXE])) return log_oom(); if (sd_pid_get_session(pid, &t) >= 0) @@ -1206,17 +1218,12 @@ static int gather_pid_metadata_and_process_special_crash( if (safe_atoi(context[CONTEXT_SIGNAL], &signo) >= 0 && SIGNAL_VALID(signo)) set_iovec_field(iovec, n_iovec, "COREDUMP_SIGNAL_NAME=SIG", signal_to_string(signo)); - if (comm_ret) { - *comm_ret = comm; - comm = NULL; - } - return 0; /* == 0 means: we successfully acquired all metadata */ } static int process_kernel(int argc, char* argv[]) { - const char *context[_CONTEXT_MAX]; + char* context[_CONTEXT_MAX] = {}; struct iovec iovec[28]; size_t i, n_iovec, n_to_free = 0; int r; @@ -1228,14 +1235,14 @@ static int process_kernel(int argc, char* argv[]) { return -EINVAL; } - context[CONTEXT_PID] = argv[CONTEXT_PID + 1]; - context[CONTEXT_UID] = argv[CONTEXT_UID + 1]; - context[CONTEXT_GID] = argv[CONTEXT_GID + 1]; - context[CONTEXT_SIGNAL] = argv[CONTEXT_SIGNAL + 1]; - context[CONTEXT_TIMESTAMP] = argv[CONTEXT_TIMESTAMP + 1]; - context[CONTEXT_RLIMIT] = argv[CONTEXT_RLIMIT + 1]; + context[CONTEXT_PID] = argv[1 + CONTEXT_PID]; + context[CONTEXT_UID] = argv[1 + CONTEXT_UID]; + context[CONTEXT_GID] = argv[1 + CONTEXT_GID]; + context[CONTEXT_SIGNAL] = argv[1 + CONTEXT_SIGNAL]; + context[CONTEXT_TIMESTAMP] = argv[1 + CONTEXT_TIMESTAMP]; + context[CONTEXT_RLIMIT] = argv[1 + CONTEXT_RLIMIT]; - r = gather_pid_metadata_and_process_special_crash(context, argv + CONTEXT_COMM + 1, NULL, iovec, &n_to_free); + r = gather_pid_metadata_and_process_special_crash(context, argv + 1 + CONTEXT_COMM, iovec, &n_to_free); if (r != 0) /* Error, or a a special crash, which has already been processed. */ goto finish; @@ -1255,12 +1262,16 @@ static int process_kernel(int argc, char* argv[]) { for (i = 0; i < n_to_free; i++) free(iovec[i].iov_base); + /* Those fields are allocated by gather_pid_metadata_and_process_special_crash */ + free(context[CONTEXT_COMM]); + free(context[CONTEXT_EXE]); + return r; } static int process_backtrace(int argc, char *argv[]) { - const char *context[_CONTEXT_MAX]; - _cleanup_free_ char *comm = NULL, *message = NULL; + char *context[_CONTEXT_MAX] = {}; + _cleanup_free_ char *message = NULL; _cleanup_free_ struct iovec *iovec = NULL; size_t n_iovec, n_allocated, n_to_free = 0, i; int r; @@ -1275,19 +1286,19 @@ static int process_backtrace(int argc, char *argv[]) { return -EINVAL; } - context[CONTEXT_PID] = argv[CONTEXT_PID + 2]; - context[CONTEXT_UID] = argv[CONTEXT_UID + 2]; - context[CONTEXT_GID] = argv[CONTEXT_GID + 2]; - context[CONTEXT_SIGNAL] = argv[CONTEXT_SIGNAL + 2]; - context[CONTEXT_TIMESTAMP] = argv[CONTEXT_TIMESTAMP + 2]; - context[CONTEXT_RLIMIT] = argv[CONTEXT_RLIMIT + 2]; + context[CONTEXT_PID] = argv[2 + CONTEXT_PID]; + context[CONTEXT_UID] = argv[2 + CONTEXT_UID]; + context[CONTEXT_GID] = argv[2 + CONTEXT_GID]; + context[CONTEXT_SIGNAL] = argv[2 + CONTEXT_SIGNAL]; + context[CONTEXT_TIMESTAMP] = argv[2 + CONTEXT_TIMESTAMP]; + context[CONTEXT_RLIMIT] = argv[2 + CONTEXT_RLIMIT]; n_allocated = 33; /* 25 metadata, 2 static, +unknown input, rounded up */ iovec = new(struct iovec, n_allocated); if (!iovec) return log_oom(); - r = gather_pid_metadata_and_process_special_crash(context, argv + CONTEXT_COMM + 2, &comm, iovec, &n_to_free); + r = gather_pid_metadata_and_process_special_crash(context, argv + 2 + CONTEXT_COMM, iovec, &n_to_free); if (r < 0) goto finish; if (r > 0) { @@ -1313,7 +1324,8 @@ static int process_backtrace(int argc, char *argv[]) { if (journal_importer_eof(&importer)) { log_warning("Did not receive a full journal entry on stdin, ignoring message sent by reporter"); - message = strjoin("MESSAGE=Process ", context[CONTEXT_PID], " (", comm, ")" + message = strjoin("MESSAGE=Process ", context[CONTEXT_PID], + " (", context[CONTEXT_COMM], ")" " of user ", context[CONTEXT_UID], " failed with ", context[CONTEXT_SIGNAL]); if (!message) { @@ -1340,6 +1352,10 @@ static int process_backtrace(int argc, char *argv[]) { for (i = 0; i < n_to_free; i++) free(iovec[i].iov_base); + /* Those fields are allocated by gather_pid_metadata_and_process_special_crash */ + free(context[CONTEXT_COMM]); + free(context[CONTEXT_EXE]); + return r; } -- 2.39.2