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.

1 files changed, 53 insertions, 37 deletions

diff --git a/src/coredump/coredump.c b/src/coredump/coredump.c
index 463e0abef4..e38cc59a36 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;
 }