From 393d0d2b699bd8f5576c9cd12bee560b10714a00 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 31 Oct 2024 14:45:38 +0100 Subject: [PATCH 01/13] =?UTF-8?q?coredump:=20rename=20pid=20=E2=86=92=20le?= =?UTF-8?q?ader=5Fpid?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Let's rename this local variable, since we are not operating on the coredump process here after all, but on the leader of the namespace the coredump process in, which is quite different, hence let's make this very clear via the name. --- src/coredump/coredump.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/coredump/coredump.c b/src/coredump/coredump.c index dc725d1b22..d7a1fc31ff 100644 --- a/src/coredump/coredump.c +++ b/src/coredump/coredump.c @@ -1519,7 +1519,7 @@ static int can_forward_coredump(pid_t pid) { static int forward_coredump_to_container(Context *context) { _cleanup_close_ int pidnsfd = -EBADF, mntnsfd = -EBADF, netnsfd = -EBADF, usernsfd = -EBADF, rootfd = -EBADF; _cleanup_close_pair_ int pair[2] = EBADF_PAIR; - pid_t pid, child; + pid_t leader_pid, child; struct ucred ucred = { .pid = context->pid, .uid = context->uid, @@ -1527,11 +1527,11 @@ static int forward_coredump_to_container(Context *context) { }; int r; - r = namespace_get_leader(context->pid, NAMESPACE_PID, &pid); + r = namespace_get_leader(context->pid, NAMESPACE_PID, &leader_pid); if (r < 0) return log_debug_errno(r, "Failed to get namespace leader: %m"); - r = can_forward_coredump(pid); + r = can_forward_coredump(leader_pid); if (r < 0) return log_debug_errno(r, "Failed to check if coredump can be forwarded: %m"); if (r == 0) @@ -1546,15 +1546,15 @@ static int forward_coredump_to_container(Context *context) { if (r < 0) return log_debug_errno(r, "Failed to set SO_PASSCRED: %m"); - r = namespace_open(pid, &pidnsfd, &mntnsfd, &netnsfd, &usernsfd, &rootfd); + r = namespace_open(leader_pid, &pidnsfd, &mntnsfd, &netnsfd, &usernsfd, &rootfd); if (r < 0) - return log_debug_errno(r, "Failed to join namespaces of PID " PID_FMT ": %m", pid); + return log_debug_errno(r, "Failed to join namespaces of PID " PID_FMT ": %m", leader_pid); r = namespace_fork("(sd-coredumpns)", "(sd-coredump)", NULL, 0, FORK_RESET_SIGNALS|FORK_DEATHSIG_SIGTERM, pidnsfd, mntnsfd, netnsfd, usernsfd, rootfd, &child); if (r < 0) - return log_debug_errno(r, "Failed to fork into namespaces of PID " PID_FMT ": %m", pid); + return log_debug_errno(r, "Failed to fork into namespaces of PID " PID_FMT ": %m", leader_pid); if (r == 0) { _cleanup_(iovw_free_freep) struct iovec_wrapper *iovw = NULL; Context child_context = {}; From ea8eb370e747126a501dc3f2f5f629c0dc74ced0 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 31 Oct 2024 14:50:41 +0100 Subject: [PATCH 02/13] coredump: minor modernizations --- src/coredump/coredump.c | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/src/coredump/coredump.c b/src/coredump/coredump.c index d7a1fc31ff..a7d9d1a65e 100644 --- a/src/coredump/coredump.c +++ b/src/coredump/coredump.c @@ -773,9 +773,12 @@ static int get_process_container_parent_cmdline(pid_t pid, char** cmdline) { } static int change_uid_gid(const Context *context) { + int r; + + assert(context); + uid_t uid = context->uid; gid_t gid = context->gid; - int r; if (uid_is_system(uid)) { const char *user = "systemd-coredump"; @@ -1233,7 +1236,7 @@ static int send_iovec(const struct iovec_wrapper *iovw, int input_fd, int mounts * what we want to send, and the second one contains * the trailing dots. */ copy[0] = iovw->iovec[i]; - copy[1] = IOVEC_MAKE(((char[]){'.', '.', '.'}), 3); + copy[1] = IOVEC_MAKE(((const char[]){'.', '.', '.'}), 3); mh.msg_iov = copy; mh.msg_iovlen = 2; @@ -1265,9 +1268,7 @@ static int gather_pid_metadata_from_argv( Context *context, int argc, char **argv) { - _cleanup_free_ char *free_timestamp = NULL; int r, signo; - char *t; assert(iovw); assert(context); @@ -1281,8 +1282,8 @@ static int gather_pid_metadata_from_argv( argc, _META_ARGV_MAX); for (int i = 0; i < _META_ARGV_MAX; i++) { - - t = argv[i]; + _cleanup_free_ char *buf = NULL; + const char *t = argv[i]; switch (i) { @@ -1291,9 +1292,11 @@ static int gather_pid_metadata_from_argv( * zeroes, so that the kernel-supplied 1s granularity timestamps * becomes 1μs granularity, i.e. the granularity systemd usually * operates in. */ - t = free_timestamp = strjoin(argv[i], "000000"); - if (!t) + buf = strjoin(argv[i], "000000"); + if (!buf) return log_oom(); + + t = buf; break; case META_ARGV_SIGNAL: @@ -1432,7 +1435,7 @@ static int gather_pid_metadata_from_procfs(struct iovec_wrapper *iovw, Context * return save_context(context, iovw); } -static int send_ucred(int transport_fd, struct ucred *ucred) { +static int send_ucred(int transport_fd, const struct ucred *ucred) { CMSG_BUFFER_TYPE(CMSG_SPACE(sizeof(struct ucred))) control = {}; struct msghdr mh = { .msg_control = &control, @@ -1441,6 +1444,7 @@ static int send_ucred(int transport_fd, struct ucred *ucred) { struct cmsghdr *cmsg; assert(transport_fd >= 0); + assert(ucred); cmsg = CMSG_FIRSTHDR(&mh); *cmsg = (struct cmsghdr) { @@ -1463,6 +1467,7 @@ static int receive_ucred(int transport_fd, struct ucred *ret_ucred) { struct ucred *ucred = NULL; ssize_t n; + assert(transport_fd >= 0); assert(ret_ucred); n = recvmsg_safe(transport_fd, &mh, 0); @@ -1588,12 +1593,12 @@ static int forward_coredump_to_container(Context *context) { char buf[DECIMAL_STR_MAX(pid_t)]; const char *t = context->meta[i]; + /* Patch some of the fields with the translated ucred data */ switch (i) { case META_ARGV_PID: xsprintf(buf, PID_FMT, ucred.pid); t = buf; - break; case META_ARGV_UID: @@ -1818,6 +1823,8 @@ static int process_backtrace(int argc, char *argv[]) { char *message; int r; + assert(argc >= 2); + log_debug("Processing backtrace on stdin..."); iovw = iovw_new(); From ecfb4bb05e243800204989400254c5827d0bf01b Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 31 Oct 2024 14:51:06 +0100 Subject: [PATCH 03/13] coredump: correct debug log message --- src/coredump/coredump.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coredump/coredump.c b/src/coredump/coredump.c index a7d9d1a65e..fb96a414c7 100644 --- a/src/coredump/coredump.c +++ b/src/coredump/coredump.c @@ -1061,7 +1061,7 @@ static int process_socket(int fd) { log_setup(); - log_debug("Processing coredump received on stdin..."); + log_debug("Processing coredump received via socket..."); for (;;) { CMSG_BUFFER_TYPE(CMSG_SPACE(sizeof(int))) control; From 313537da6ffdea4049873571202679734d49f0a1 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 31 Oct 2024 14:52:43 +0100 Subject: [PATCH 04/13] coredump: rework protocol between coredump pattern handler and processing service In 68511cebe58977ea68ae4f57c6462e979efd1cff the ability to pass the coredump's mount namespace fd from the coredump patter handler was added to systemd-coredump. For this the protocol was augmented, in attempt to provide both forward and backward compatibility. The protocol as of v256: one or more datagrams with journal log fields about the coredump are sent via an SOCK_SEQPACKET connection. It is finished with a zero length datagram which carries the coredump fd (this last datagram is called "sentinel" sometimes). The protocol after 68511cebe58977ea68ae4f57c6462e979efd1cff is extended so that after the sentinal a 2nd sentinel is sent, with a pair of fds: the coredump fd *again* and a mount fd (acquired via open_tree()) of the container's mount tree. It's a bit ugly to send the coredump fd a 2nd time, but what's more important the implementation didn't work: since on SOCK_SEQPACKET a zero sized datagram cannot be distinguished from EOF (which is a Linux API design mistake), an early EOF would be misunderstood as a zero size datagram lacking any fd, which resulted in protocol termination. Moreover, I think if we touch the protocol we should make the move to pidfs at the same time. All of the above is what this protocol rework addresses. 1. A pidfd is now sent as well 2. The protocol is now payload, followed by the coredump fd datagram (as before). But now followed by a second empty datagram with a pidfd, and a third empty datagram with the mount tree fd. Of this the latter two or last are optional. Thus, it's now a stream of payload datagrams with one, two or three fd-laden datagrams as sentinel. If we read the 2nd or 3rd sentinel without an attached fd we assume this is actually an EOF (whether it actually is one or not doesn't matter here). This should provide nice up and down compatibility. 3. The mount_tree_fd is moved into the Context object. The pidfd is placed there too, as a PidRef. Thus the data we pass around is now the coredump fd plus the context, which is simpler and makes a lot more semantical sense I think. 4. The "first" boolean is replaced by an explicit state engine enum Fixes: #34130 --- src/coredump/coredump.c | 201 +++++++++++++++++++++++++--------------- 1 file changed, 124 insertions(+), 77 deletions(-) diff --git a/src/coredump/coredump.c b/src/coredump/coredump.c index fb96a414c7..fdcb97b803 100644 --- a/src/coredump/coredump.c +++ b/src/coredump/coredump.c @@ -140,15 +140,26 @@ static const char * const meta_field_names[_META_MAX] = { }; typedef struct Context { - const char *meta[_META_MAX]; - size_t meta_size[_META_MAX]; - pid_t pid; + PidRef pidref; uid_t uid; gid_t gid; bool is_pid1; bool is_journald; + int mount_tree_fd; + + /* These point into external memory, are not owned by this object */ + const char *meta[_META_MAX]; + size_t meta_size[_META_MAX]; } Context; +#define CONTEXT_NULL \ + (Context) { \ + .pidref = PIDREF_NULL, \ + .uid = UID_INVALID, \ + .gid = GID_INVALID, \ + .mount_tree_fd = -EBADF, \ + } + typedef enum CoredumpStorage { COREDUMP_STORAGE_NONE, COREDUMP_STORAGE_EXTERNAL, @@ -177,6 +188,13 @@ static uint64_t arg_max_use = UINT64_MAX; static bool arg_enter_namespace = false; #endif +static void context_done(Context *c) { + assert(c); + + pidref_done(&c->pidref); + c->mount_tree_fd = safe_close(c->mount_tree_fd); +} + static int parse_config(void) { static const ConfigTableItem items[] = { { "Coredump", "Storage", config_parse_coredump_storage, 0, &arg_storage }, @@ -824,8 +842,7 @@ static int attach_mount_tree(int mount_tree_fd) { static int submit_coredump( const Context *context, struct iovec_wrapper *iovw, - int input_fd, - int mount_tree_fd) { + int input_fd) { _cleanup_(sd_json_variant_unrefp) sd_json_variant *json_metadata = NULL; _cleanup_close_ int coredump_fd = -EBADF, coredump_node_fd = -EBADF; @@ -866,7 +883,7 @@ static int submit_coredump( (void) coredump_vacuum(coredump_node_fd >= 0 ? coredump_node_fd : coredump_fd, arg_keep_free, arg_max_use); } - if (mount_tree_fd >= 0 && attach_mount_tree(mount_tree_fd) >= 0) + if (context->mount_tree_fd >= 0 && attach_mount_tree(context->mount_tree_fd) >= 0) root = MOUNT_TREE_ROOT; /* Now, let's drop privileges to become the user who owns the segfaulted process and allocate the @@ -1007,7 +1024,8 @@ static int save_context(Context *context, const struct iovec_wrapper *iovw) { assert(iovw); assert(iovw->count >= _META_ARGV_MAX); - /* The context does not allocate any memory on its own */ + /* Converts the data in the iovec array iovw into separate fields. Fills in context->meta[] (for + * which no memory is allocated, it just contains direct pointers into the iovec array memory). */ for (size_t n = 0; n < iovw->count; n++) { struct iovec *iovec = iovw->iovec + n; @@ -1031,9 +1049,18 @@ static int save_context(Context *context, const struct iovec_wrapper *iovw) { return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Failed to find the PID of crashing process"); - r = parse_pid(context->meta[META_ARGV_PID], &context->pid); + pid_t parsed_pid; + r = parse_pid(context->meta[META_ARGV_PID], &parsed_pid); if (r < 0) return log_error_errno(r, "Failed to parse PID \"%s\": %m", context->meta[META_ARGV_PID]); + if (pidref_is_set(&context->pidref)) { + if (context->pidref.pid != parsed_pid) + return log_error_errno(r, "Passed PID " PID_FMT " does not match passed " PID_FMT ": %m", parsed_pid, context->pidref.pid); + } else { + r = pidref_set_pid(&context->pidref, parsed_pid); + if (r < 0) + return log_error_errno(r, "Failed to initialize pidref from pid " PID_FMT ": %m", parsed_pid); + } r = parse_uid(context->meta[META_ARGV_UID], &context->uid); if (r < 0) @@ -1051,10 +1078,14 @@ static int save_context(Context *context, const struct iovec_wrapper *iovw) { } static int process_socket(int fd) { - _cleanup_close_ int input_fd = -EBADF, mount_tree_fd = -EBADF; - Context context = {}; + _cleanup_(context_done) Context context = CONTEXT_NULL; + _cleanup_close_ int input_fd = -EBADF; struct iovec_wrapper iovw = {}; - bool first = true; + enum { + STATE_PAYLOAD, + STATE_INPUT_FD_DONE, + STATE_PID_FD_DONE, + } state = STATE_PAYLOAD; int r; assert(fd >= 0); @@ -1095,71 +1126,77 @@ static int process_socket(int fd) { goto finish; } - /* The final zero-length datagram carries the file descriptors and tells us - * that we're done. */ + /* The final zero-length datagrams ("sentinels") carry file descriptors and tell us that + * we're done. There are three sentinels: one with just the coredump fd, followed by one with + * the pidfd, and finally one with the mount tree fd. The latter two or the last one may be + * omitted (which is supported for compatibility with older systemd version, in particular to + * facilitate cross-container coredumping). */ if (n == 0) { struct cmsghdr *found; - if (first) { - found = cmsg_find(&mh, SOL_SOCKET, SCM_RIGHTS, CMSG_LEN(sizeof(int))); - if (found) { - /* This is the first message that carries file descriptors. Maybe - * there will be one more that actually contains array of two - * descriptors. */ - assert(input_fd < 0); + found = cmsg_find(&mh, SOL_SOCKET, SCM_RIGHTS, CMSG_LEN(sizeof(int))); + if (!found) { + /* This is zero length message but it either doesn't carry a single + * descriptor, or it has more than one. This is a protocol violation so let's + * bail out. + * + * Well, not quite! In practice there's one more complication: EOF on + * SOCK_SEQPACKET is not distinguishable from a zero length datagram. Hence + * if we get a zero length datagram without fds we consider it EOF, and + * that's permissible for the final two fds. Hence let's be strict on the + * first fd, but lenient on the other two. */ - input_fd = *CMSG_TYPED_DATA(found, int); - first = false; + if (!cmsg_find(&mh, SOL_SOCKET, SCM_RIGHTS, (socklen_t) -1) && state != STATE_PAYLOAD) /* no fds, and already got the first fd → we are done */ + break; - continue; - } - - /* This is zero length message but it either doesn't carry a single descriptor, - * or it has more than one. This is a protocol violation so let's bail out. */ cmsg_close_all(&mh); r = log_error_errno(SYNTHETIC_ERRNO(EBADMSG), - "Received zero length message with zero or more than one file descriptor(s)."); + "Received zero length message with zero or more than one file descriptor(s), expected one."); goto finish; } - /* Second zero length message might carry two file descriptors, coredump fd and mount tree fd. */ - found = cmsg_find(&mh, SOL_SOCKET, SCM_RIGHTS, CMSG_LEN(sizeof(int) * 2)); - if (found) { - int fds[2] = EBADF_PAIR; + switch (state) { - memcpy(fds, CMSG_TYPED_DATA(found, int), sizeof(int) * 2); + case STATE_PAYLOAD: + assert(input_fd < 0); + input_fd = *CMSG_TYPED_DATA(found, int); + state = STATE_INPUT_FD_DONE; + continue; - assert(mount_tree_fd < 0); - assert(input_fd >= 0); + case STATE_INPUT_FD_DONE: + assert(!pidref_is_set(&context.pidref)); - /* Let's close input fd we got in the previous iteration. */ - safe_close(input_fd); + r = pidref_set_pidfd_consume(&context.pidref, *CMSG_TYPED_DATA(found, int)); + if (r < 0) { + log_error_errno(r, "Failed to initialize pidref: %m"); + goto finish; + } - input_fd = fds[0]; - mount_tree_fd = fds[1]; + state = STATE_PID_FD_DONE; + continue; - /* We have all FDs we need so let's take a shortcut here. */ + case STATE_PID_FD_DONE: + assert(context.mount_tree_fd < 0); + context.mount_tree_fd = *CMSG_TYPED_DATA(found, int); + /* We have all FDs we need so we are done. */ break; } - /* This is second iteration and we didn't find array of two FDs. */ - found = cmsg_find(&mh, SOL_SOCKET, SCM_RIGHTS, (socklen_t) -1); - cmsg_close_all(&mh); - - if (!found) - /* Hence we either have no FDs which is OK and we can break. */ - break; - - /* Or we have some other number of FDs and somebody is playing games with us. */ - r = log_error_errno(SYNTHETIC_ERRNO(EBADMSG), "Received unexpected file descriptors."); - goto finish; - + break; } + cmsg_close_all(&mh); /* Only zero length messages are allowed after the first message that carried a file descriptor. */ - if (!first) { - r = log_error_errno(SYNTHETIC_ERRNO(EBADMSG), "Received unexpected message with non zero length."); + if (state != STATE_PAYLOAD) { + r = log_error_errno(SYNTHETIC_ERRNO(EBADMSG), "Received unexpected message with non-zero length."); + goto finish; + } + + /* Payload messages should not carry fds */ + if (cmsg_find(&mh, SOL_SOCKET, SCM_RIGHTS, (socklen_t) -1)) { + r = log_error_errno(SYNTHETIC_ERRNO(EBADMSG), + "Received payload message with file descriptor(s), expected none."); goto finish; } @@ -1190,14 +1227,14 @@ static int process_socket(int fd) { goto finish; } - r = submit_coredump(&context, &iovw, input_fd, mount_tree_fd); + r = submit_coredump(&context, &iovw, input_fd); finish: iovw_free_contents(&iovw, true); return r; } -static int send_iovec(const struct iovec_wrapper *iovw, int input_fd, int mounts_fd) { +static int send_iovec(const struct iovec_wrapper *iovw, int input_fd, PidRef *pidref, int mount_tree_fd) { _cleanup_close_ int fd = -EBADF; int r; @@ -1250,15 +1287,26 @@ static int send_iovec(const struct iovec_wrapper *iovw, int input_fd, int mounts } } + /* First sentinel: the coredump fd */ r = send_one_fd(fd, input_fd, 0); if (r < 0) return log_error_errno(r, "Failed to send coredump fd: %m"); - if (mounts_fd >= 0) { - r = send_many_fds(fd, (int[]) { input_fd, mounts_fd }, 2, 0); - if (r < 0) - return log_error_errno(r, "Failed to send coredump fds: %m"); - } + /* The optional second sentinel: the pidfd */ + if (!pidref_is_set(pidref) || pidref->fd < 0) /* If we have no pidfd, stop now */ + return 0; + + r = send_one_fd(fd, pidref->fd, 0); + if (r < 0) + return log_error_errno(r, "Failed to send pidfd: %m"); + + /* The optional third sentinel: the mount tree fd */ + if (mount_tree_fd < 0) /* If we have no mount tree, stop now */ + return 0; + + r = send_one_fd(fd, mount_tree_fd, 0); + if (r < 0) + return log_error_errno(r, "Failed to send mount tree fd: %m"); return 0; } @@ -1334,7 +1382,7 @@ static int gather_pid_metadata_from_procfs(struct iovec_wrapper *iovw, Context * /* 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 iovec fields initialized.) */ - pid = context->pid; + pid = context->pidref.pid; /* The following is mandatory */ r = pid_get_comm(pid, &t); @@ -1526,13 +1574,15 @@ static int forward_coredump_to_container(Context *context) { _cleanup_close_pair_ int pair[2] = EBADF_PAIR; pid_t leader_pid, child; struct ucred ucred = { - .pid = context->pid, + .pid = context->pidref.pid, .uid = context->uid, .gid = context->gid, }; int r; - r = namespace_get_leader(context->pid, NAMESPACE_PID, &leader_pid); + assert(context); + + r = namespace_get_leader(context->pidref.pid, NAMESPACE_PID, &leader_pid); if (r < 0) return log_debug_errno(r, "Failed to get namespace leader: %m"); @@ -1561,9 +1611,6 @@ static int forward_coredump_to_container(Context *context) { if (r < 0) return log_debug_errno(r, "Failed to fork into namespaces of PID " PID_FMT ": %m", leader_pid); if (r == 0) { - _cleanup_(iovw_free_freep) struct iovec_wrapper *iovw = NULL; - Context child_context = {}; - pair[0] = safe_close(pair[0]); r = access_nofollow("/run/systemd/coredump", W_OK); @@ -1578,7 +1625,7 @@ static int forward_coredump_to_container(Context *context) { _exit(EXIT_FAILURE); } - iovw = iovw_new(); + _cleanup_(iovw_free_freep) struct iovec_wrapper *iovw = iovw_new(); if (!iovw) { log_oom(); _exit(EXIT_FAILURE); @@ -1629,6 +1676,7 @@ static int forward_coredump_to_container(Context *context) { } } + _cleanup_(context_done) Context child_context = CONTEXT_NULL; r = save_context(&child_context, iovw); if (r < 0) { log_debug_errno(r, "Failed to save context: %m"); @@ -1641,7 +1689,7 @@ static int forward_coredump_to_container(Context *context) { _exit(EXIT_FAILURE); } - r = send_iovec(iovw, STDIN_FILENO, -EBADF); + r = send_iovec(iovw, STDIN_FILENO, &context->pidref, /* mount_tree_fd= */ -EBADF); if (r < 0) { log_debug_errno(r, "Failed to send iovec to coredump socket: %m"); _exit(EXIT_FAILURE); @@ -1691,7 +1739,7 @@ static int gather_pid_mount_tree_fd(const Context *context, int *ret_fd) { if (socketpair(AF_UNIX, SOCK_DGRAM|SOCK_CLOEXEC, 0, pair) < 0) return log_error_errno(errno, "Failed to create socket pair: %m"); - r = namespace_open(context->pid, + r = namespace_open(context->pidref.pid, /* ret_pidns_fd= */ NULL, &mntns_fd, /* ret_netns_fd= */ NULL, @@ -1744,8 +1792,7 @@ static int gather_pid_mount_tree_fd(const Context *context, int *ret_fd) { static int process_kernel(int argc, char* argv[]) { _cleanup_(iovw_free_freep) struct iovec_wrapper *iovw = NULL; - _cleanup_close_ int mount_tree_fd = -EBADF; - Context context = {}; + _cleanup_(context_done) Context context = CONTEXT_NULL; int r, signo; /* When we're invoked by the kernel, stdout/stderr are closed which is dangerous because the fds @@ -1781,7 +1828,7 @@ static int process_kernel(int argc, char* argv[]) { context.meta[META_ARGV_UID], context.meta[META_ARGV_SIGNAL], strna(safe_atoi(context.meta[META_ARGV_SIGNAL], &signo) >= 0 ? signal_to_string(signo) : NULL)); - r = in_same_namespace(getpid_cached(), context.pid, NAMESPACE_PID); + r = in_same_namespace(getpid_cached(), context.pidref.pid, NAMESPACE_PID); if (r < 0) log_debug_errno(r, "Failed to check pidns of crashing process, ignoring: %m"); if (r == 0) { @@ -1791,7 +1838,7 @@ static int process_kernel(int argc, char* argv[]) { if (r >= 0) return 0; - r = gather_pid_mount_tree_fd(&context, &mount_tree_fd); + r = gather_pid_mount_tree_fd(&context, &context.mount_tree_fd); if (r < 0) log_warning_errno(r, "Failed to access the mount tree of a container, ignoring: %m"); } @@ -1811,15 +1858,15 @@ static int process_kernel(int argc, char* argv[]) { (void) iovw_put_string_field(iovw, "PRIORITY=", STRINGIFY(LOG_CRIT)); if (context.is_journald || context.is_pid1) - return submit_coredump(&context, iovw, STDIN_FILENO, mount_tree_fd); + return submit_coredump(&context, iovw, STDIN_FILENO); - return send_iovec(iovw, STDIN_FILENO, mount_tree_fd); + return send_iovec(iovw, STDIN_FILENO, &context.pidref, context.mount_tree_fd); } static int process_backtrace(int argc, char *argv[]) { _cleanup_(journal_importer_cleanup) JournalImporter importer = JOURNAL_IMPORTER_INIT(STDIN_FILENO); _cleanup_(iovw_free_freep) struct iovec_wrapper *iovw = NULL; - Context context = {}; + _cleanup_(context_done) Context context = CONTEXT_NULL; char *message; int r; From 1f485bc7356c41caff4fb6fb6b9fb400114d8584 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 31 Oct 2024 15:11:39 +0100 Subject: [PATCH 05/13] coredump: acquire some process fields via pidref Use pidref to acquire some fields. This just makes use of the pidref helpers we already have. We acquire a lot of other data via classic pids still, but for that we first have to write race-free pidref getters, hence leave that for another time. --- src/coredump/coredump.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/coredump/coredump.c b/src/coredump/coredump.c index fdcb97b803..6d556a7a1f 100644 --- a/src/coredump/coredump.c +++ b/src/coredump/coredump.c @@ -1385,7 +1385,7 @@ static int gather_pid_metadata_from_procfs(struct iovec_wrapper *iovw, Context * pid = context->pidref.pid; /* The following is mandatory */ - r = pid_get_comm(pid, &t); + r = pidref_get_comm(&context->pidref, &t); if (r < 0) return log_error_errno(r, "Failed to get COMM: %m"); @@ -1400,7 +1400,7 @@ static int gather_pid_metadata_from_procfs(struct iovec_wrapper *iovw, Context * if (r < 0) log_warning_errno(r, "Failed to get EXE, ignoring: %m"); - if (cg_pid_get_unit(pid, &t) >= 0) + if (cg_pidref_get_unit(&context->pidref, &t) >= 0) (void) iovw_put_string_field_free(iovw, "COREDUMP_UNIT=", t); if (cg_pid_get_user_unit(pid, &t) >= 0) @@ -1418,7 +1418,7 @@ static int gather_pid_metadata_from_procfs(struct iovec_wrapper *iovw, Context * if (sd_pid_get_slice(pid, &t) >= 0) (void) iovw_put_string_field_free(iovw, "COREDUMP_SLICE=", t); - if (pid_get_cmdline(pid, SIZE_MAX, PROCESS_CMDLINE_QUOTE_POSIX, &t) >= 0) + if (pidref_get_cmdline(&context->pidref, SIZE_MAX, PROCESS_CMDLINE_QUOTE_POSIX, &t) >= 0) (void) iovw_put_string_field_free(iovw, "COREDUMP_CMDLINE=", t); if (cg_pid_get_path_shifted(pid, NULL, &t) >= 0) From 32756e57ef2f19f353810a87ed9c350e5950fce5 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 31 Oct 2024 15:15:33 +0100 Subject: [PATCH 06/13] =?UTF-8?q?coredump:=20rename=20save=5Fcontext()=20?= =?UTF-8?q?=E2=86=92=20context=5Fparse=5Fiovw()?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The function doesn't "save" anything, it just parses iovw into the individual fields, hence name the function accordingly. --- src/coredump/coredump.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/coredump/coredump.c b/src/coredump/coredump.c index 6d556a7a1f..9b12c25cbd 100644 --- a/src/coredump/coredump.c +++ b/src/coredump/coredump.c @@ -1016,7 +1016,7 @@ static int submit_coredump( return 0; } -static int save_context(Context *context, const struct iovec_wrapper *iovw) { +static int context_parse_iovw(Context *context, const struct iovec_wrapper *iovw) { const char *unit; int r; @@ -1214,7 +1214,7 @@ static int process_socket(int fd) { /* Make sure we got all data we really need */ assert(input_fd >= 0); - r = save_context(&context, &iovw); + r = context_parse_iovw(&context, &iovw); if (r < 0) goto finish; @@ -1365,7 +1365,7 @@ static int gather_pid_metadata_from_argv( /* Cache some of the process metadata we collected so far and that we'll need to * access soon */ - return save_context(context, iovw); + return context_parse_iovw(context, iovw); } static int gather_pid_metadata_from_procfs(struct iovec_wrapper *iovw, Context *context) { @@ -1452,7 +1452,7 @@ static int gather_pid_metadata_from_procfs(struct iovec_wrapper *iovw, Context * if (read_full_virtual_file(p, &t, &size) >= 0) { char *buf = malloc(strlen("COREDUMP_PROC_AUXV=") + size + 1); if (buf) { - /* Add a dummy terminator to make save_context() happy. */ + /* Add a dummy terminator to make context_parse_iovw() happy. */ *mempcpy_typesafe(stpcpy(buf, "COREDUMP_PROC_AUXV="), t, size) = '\0'; (void) iovw_consume(iovw, buf, size + strlen("COREDUMP_PROC_AUXV=")); } @@ -1480,7 +1480,7 @@ static int gather_pid_metadata_from_procfs(struct iovec_wrapper *iovw, Context * (void) iovw_put_string_field_free(iovw, "COREDUMP_ENVIRON=", t); /* we successfully acquired all metadata */ - return save_context(context, iovw); + return context_parse_iovw(context, iovw); } static int send_ucred(int transport_fd, const struct ucred *ucred) { @@ -1677,7 +1677,7 @@ static int forward_coredump_to_container(Context *context) { } _cleanup_(context_done) Context child_context = CONTEXT_NULL; - r = save_context(&child_context, iovw); + r = context_parse_iovw(&child_context, iovw); if (r < 0) { log_debug_errno(r, "Failed to save context: %m"); _exit(EXIT_FAILURE); From 960b045875b11fd695a862912fd4e48fee4d3dd6 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 31 Oct 2024 15:33:44 +0100 Subject: [PATCH 07/13] coredump: parse signal number at the same time as parsing other fields --- src/basic/signal-util.c | 16 +++++++++++++ src/basic/signal-util.h | 2 ++ src/coredump/coredump.c | 53 +++++++++++++++++------------------------ 3 files changed, 40 insertions(+), 31 deletions(-) diff --git a/src/basic/signal-util.c b/src/basic/signal-util.c index 670040f1ad..ad9ed049f1 100644 --- a/src/basic/signal-util.c +++ b/src/basic/signal-util.c @@ -301,3 +301,19 @@ const struct sigaction sigaction_default = { .sa_handler = SIG_DFL, .sa_flags = SA_RESTART, }; + +int parse_signo(const char *s, int *ret) { + int sig, r; + + r = safe_atoi(s, &sig); + if (r < 0) + return r; + + if (!SIGNAL_VALID(sig)) + return -EINVAL; + + if (ret) + *ret = sig; + + return 0; +} diff --git a/src/basic/signal-util.h b/src/basic/signal-util.h index 5739fe0559..559ce42949 100644 --- a/src/basic/signal-util.h +++ b/src/basic/signal-util.h @@ -68,3 +68,5 @@ void propagate_signal(int sig, siginfo_t *siginfo); extern const struct sigaction sigaction_ignore; extern const struct sigaction sigaction_default; + +int parse_signo(const char *s, int *ret); diff --git a/src/coredump/coredump.c b/src/coredump/coredump.c index 9b12c25cbd..bc2a722bbb 100644 --- a/src/coredump/coredump.c +++ b/src/coredump/coredump.c @@ -143,6 +143,7 @@ typedef struct Context { PidRef pidref; uid_t uid; gid_t gid; + int signo; bool is_pid1; bool is_journald; int mount_tree_fd; @@ -1016,7 +1017,7 @@ static int submit_coredump( return 0; } -static int context_parse_iovw(Context *context, const struct iovec_wrapper *iovw) { +static int context_parse_iovw(Context *context, struct iovec_wrapper *iovw) { const char *unit; int r; @@ -1027,9 +1028,8 @@ static int context_parse_iovw(Context *context, const struct iovec_wrapper *iovw /* Converts the data in the iovec array iovw into separate fields. Fills in context->meta[] (for * which no memory is allocated, it just contains direct pointers into the iovec array memory). */ - for (size_t n = 0; n < iovw->count; n++) { - struct iovec *iovec = iovw->iovec + n; - + bool have_signal_name = false; + FOREACH_ARRAY(iovec, iovw->iovec, iovw->count) { for (size_t i = 0; i < ELEMENTSOF(meta_field_names); i++) { /* Note that these strings are NUL terminated, because we made sure that a * trailing NUL byte is in the buffer, though not included in the iov_len @@ -1043,6 +1043,9 @@ static int context_parse_iovw(Context *context, const struct iovec_wrapper *iovw break; } } + + have_signal_name = have_signal_name || + memory_startswith(iovec->iov_base, iovec->iov_len, "COREDUMP_SIGNAL_NAME="); } if (!context->meta[META_ARGV_PID]) @@ -1070,10 +1073,19 @@ static int context_parse_iovw(Context *context, const struct iovec_wrapper *iovw if (r < 0) return log_error_errno(r, "Failed to parse GID \"%s\": %m", context->meta[META_ARGV_GID]); + r = parse_signo(context->meta[META_ARGV_SIGNAL], &context->signo); + if (r < 0) + log_warning_errno(r, "Failed to parse signal number \"%s\", ignoring: %m", context->meta[META_ARGV_SIGNAL]); + unit = context->meta[META_UNIT]; context->is_pid1 = streq(context->meta[META_ARGV_PID], "1") || streq_ptr(unit, SPECIAL_INIT_SCOPE); context->is_journald = streq_ptr(unit, SPECIAL_JOURNALD_SERVICE); + /* After parsing everything, let's also synthesize a new iovw field for the textual signal name if it + * isn't already set. */ + if (SIGNAL_VALID(context->signo) && !have_signal_name) + (void) iovw_put_string_field(iovw, "COREDUMP_SIGNAL_NAME=SIG", signal_to_string(context->signo)); + return 0; } @@ -1316,7 +1328,7 @@ static int gather_pid_metadata_from_argv( Context *context, int argc, char **argv) { - int r, signo; + int r; assert(iovw); assert(context); @@ -1333,9 +1345,7 @@ static int gather_pid_metadata_from_argv( _cleanup_free_ char *buf = NULL; const char *t = argv[i]; - switch (i) { - - case META_ARGV_TIMESTAMP: + if (i == META_ARGV_TIMESTAMP) { /* The journal fields contain the timestamp padded with six * zeroes, so that the kernel-supplied 1s granularity timestamps * becomes 1μs granularity, i.e. the granularity systemd usually @@ -1345,17 +1355,6 @@ static int gather_pid_metadata_from_argv( return log_oom(); t = buf; - break; - - case META_ARGV_SIGNAL: - /* For signal, record its pretty name too */ - if (safe_atoi(argv[i], &signo) >= 0 && SIGNAL_VALID(signo)) - (void) iovw_put_string_field(iovw, "COREDUMP_SIGNAL_NAME=SIG", - signal_to_string(signo)); - break; - - default: - break; } r = iovw_put_string_field(iovw, meta_field_names[i], t); @@ -1636,7 +1635,6 @@ static int forward_coredump_to_container(Context *context) { (void) iovw_put_string_field(iovw, "COREDUMP_FORWARDED=", "1"); for (int i = 0; i < _META_ARGV_MAX; i++) { - int signo; char buf[DECIMAL_STR_MAX(pid_t)]; const char *t = context->meta[i]; @@ -1658,13 +1656,6 @@ static int forward_coredump_to_container(Context *context) { t = buf; break; - case META_ARGV_SIGNAL: - if (safe_atoi(t, &signo) >= 0 && SIGNAL_VALID(signo)) - (void) iovw_put_string_field(iovw, - "COREDUMP_SIGNAL_NAME=SIG", - signal_to_string(signo)); - break; - default: break; } @@ -1793,7 +1784,7 @@ static int gather_pid_mount_tree_fd(const Context *context, int *ret_fd) { static int process_kernel(int argc, char* argv[]) { _cleanup_(iovw_free_freep) struct iovec_wrapper *iovw = NULL; _cleanup_(context_done) Context context = CONTEXT_NULL; - int r, signo; + int r; /* When we're invoked by the kernel, stdout/stderr are closed which is dangerous because the fds * could get reallocated. To avoid hard to debug issues, let's instead bind stdout/stderr to @@ -1824,9 +1815,9 @@ static int process_kernel(int argc, char* argv[]) { /* Log minimal metadata now, so it is not lost if the system is about to shut down. */ log_info("Process %s (%s) of user %s terminated abnormally with signal %s/%s, processing...", - context.meta[META_ARGV_PID], context.meta[META_COMM], - context.meta[META_ARGV_UID], context.meta[META_ARGV_SIGNAL], - strna(safe_atoi(context.meta[META_ARGV_SIGNAL], &signo) >= 0 ? signal_to_string(signo) : NULL)); + context.meta[META_ARGV_PID], context.meta[META_COMM], + context.meta[META_ARGV_UID], context.meta[META_ARGV_SIGNAL], + signal_to_string(context.signo)); r = in_same_namespace(getpid_cached(), context.pidref.pid, NAMESPACE_PID); if (r < 0) From 19455dd600d659d41b1657d1c9d9281c73e58354 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 31 Oct 2024 15:37:09 +0100 Subject: [PATCH 08/13] coredump: parse rlimit field at same place as other fields --- src/coredump/coredump.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/coredump/coredump.c b/src/coredump/coredump.c index bc2a722bbb..f8422b1dbd 100644 --- a/src/coredump/coredump.c +++ b/src/coredump/coredump.c @@ -144,6 +144,7 @@ typedef struct Context { uid_t uid; gid_t gid; int signo; + uint64_t rlimit; bool is_pid1; bool is_journald; int mount_tree_fd; @@ -455,7 +456,7 @@ static int save_external_coredump( _cleanup_(unlink_and_freep) char *tmp = NULL; _cleanup_free_ char *fn = NULL; _cleanup_close_ int fd = -EBADF; - uint64_t rlimit, process_limit, max_size; + uint64_t process_limit, max_size; bool truncated, storage_on_tmpfs; struct stat st; int r; @@ -468,11 +469,7 @@ static int save_external_coredump( assert(ret_compressed_size); assert(ret_truncated); - r = safe_atou64(context->meta[META_ARGV_RLIMIT], &rlimit); - if (r < 0) - return log_error_errno(r, "Failed to parse resource limit '%s': %m", - context->meta[META_ARGV_RLIMIT]); - if (rlimit < page_size()) + if (context->rlimit < page_size()) /* 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 @@ -487,7 +484,7 @@ static int save_external_coredump( "Limits for coredump processing and storage are both 0, not dumping core."); /* Never store more than the process configured, or than we actually shall keep or process */ - max_size = MIN(rlimit, process_limit); + max_size = MIN(context->rlimit, process_limit); r = make_filename(context, &fn); if (r < 0) @@ -1077,6 +1074,10 @@ static int context_parse_iovw(Context *context, struct iovec_wrapper *iovw) { if (r < 0) log_warning_errno(r, "Failed to parse signal number \"%s\", ignoring: %m", context->meta[META_ARGV_SIGNAL]); + r = safe_atou64(context->meta[META_ARGV_RLIMIT], &context->rlimit); + if (r < 0) + log_warning_errno(r, "Failed to parse resource limit \"%s\", ignoring: %m", context->meta[META_ARGV_RLIMIT]); + unit = context->meta[META_UNIT]; context->is_pid1 = streq(context->meta[META_ARGV_PID], "1") || streq_ptr(unit, SPECIAL_INIT_SCOPE); context->is_journald = streq_ptr(unit, SPECIAL_JOURNALD_SERVICE); From 2865561eaa4c3ef8d95b8ec4e02ed77e0d639564 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 31 Oct 2024 15:44:54 +0100 Subject: [PATCH 09/13] coredump: move to _cleanup_ for destroying iovw object --- src/basic/iovec-wrapper.h | 4 +++ src/coredump/coredump.c | 64 +++++++++++++-------------------------- 2 files changed, 25 insertions(+), 43 deletions(-) diff --git a/src/basic/iovec-wrapper.h b/src/basic/iovec-wrapper.h index 05e220c1d0..0c6d760a73 100644 --- a/src/basic/iovec-wrapper.h +++ b/src/basic/iovec-wrapper.h @@ -19,6 +19,10 @@ DEFINE_TRIVIAL_CLEANUP_FUNC(struct iovec_wrapper*, iovw_free_free); void iovw_free_contents(struct iovec_wrapper *iovw, bool free_vectors); +static inline void iovw_done_free(struct iovec_wrapper *iovw) { + iovw_free_contents(iovw, true); +} + int iovw_put(struct iovec_wrapper *iovw, void *data, size_t len); static inline int iovw_consume(struct iovec_wrapper *iovw, void *data, size_t len) { /* Move data into iovw or free on error */ diff --git a/src/coredump/coredump.c b/src/coredump/coredump.c index f8422b1dbd..176e5373b8 100644 --- a/src/coredump/coredump.c +++ b/src/coredump/coredump.c @@ -1091,9 +1091,9 @@ static int context_parse_iovw(Context *context, struct iovec_wrapper *iovw) { } static int process_socket(int fd) { + _cleanup_(iovw_done_free) struct iovec_wrapper iovw = {}; _cleanup_(context_done) Context context = CONTEXT_NULL; _cleanup_close_ int input_fd = -EBADF; - struct iovec_wrapper iovw = {}; enum { STATE_PAYLOAD, STATE_INPUT_FD_DONE, @@ -1117,27 +1117,21 @@ static int process_socket(int fd) { ssize_t n, l; l = next_datagram_size_fd(fd); - if (l < 0) { - r = log_error_errno(l, "Failed to determine datagram size to read: %m"); - goto finish; - } + if (l < 0) + return log_error_errno(l, "Failed to determine datagram size to read: %m"); _cleanup_(iovec_done) struct iovec iovec = { .iov_len = l, .iov_base = malloc(l + 1), }; - if (!iovec.iov_base) { - r = log_oom(); - goto finish; - } + if (!iovec.iov_base) + return log_oom(); mh.msg_iov = &iovec; n = recvmsg_safe(fd, &mh, MSG_CMSG_CLOEXEC); - if (n < 0) { - r = log_error_errno(n, "Failed to receive datagram: %m"); - goto finish; - } + if (n < 0) + return log_error_errno(n, "Failed to receive datagram: %m"); /* The final zero-length datagrams ("sentinels") carry file descriptors and tell us that * we're done. There are three sentinels: one with just the coredump fd, followed by one with @@ -1163,9 +1157,8 @@ static int process_socket(int fd) { break; cmsg_close_all(&mh); - r = log_error_errno(SYNTHETIC_ERRNO(EBADMSG), - "Received zero length message with zero or more than one file descriptor(s), expected one."); - goto finish; + return log_error_errno(SYNTHETIC_ERRNO(EBADMSG), + "Received zero length message with zero or more than one file descriptor(s), expected one."); } switch (state) { @@ -1180,10 +1173,8 @@ static int process_socket(int fd) { assert(!pidref_is_set(&context.pidref)); r = pidref_set_pidfd_consume(&context.pidref, *CMSG_TYPED_DATA(found, int)); - if (r < 0) { - log_error_errno(r, "Failed to initialize pidref: %m"); - goto finish; - } + if (r < 0) + return log_error_errno(r, "Failed to initialize pidref: %m"); state = STATE_PID_FD_DONE; continue; @@ -1201,25 +1192,20 @@ static int process_socket(int fd) { cmsg_close_all(&mh); /* Only zero length messages are allowed after the first message that carried a file descriptor. */ - if (state != STATE_PAYLOAD) { - r = log_error_errno(SYNTHETIC_ERRNO(EBADMSG), "Received unexpected message with non-zero length."); - goto finish; - } + if (state != STATE_PAYLOAD) + return log_error_errno(SYNTHETIC_ERRNO(EBADMSG), "Received unexpected message with non-zero length."); /* Payload messages should not carry fds */ - if (cmsg_find(&mh, SOL_SOCKET, SCM_RIGHTS, (socklen_t) -1)) { - r = log_error_errno(SYNTHETIC_ERRNO(EBADMSG), + if (cmsg_find(&mh, SOL_SOCKET, SCM_RIGHTS, (socklen_t) -1)) + return log_error_errno(SYNTHETIC_ERRNO(EBADMSG), "Received payload message with file descriptor(s), expected none."); - goto finish; - } /* Add trailing NUL byte, in case these are strings */ ((char*) iovec.iov_base)[n] = 0; iovec.iov_len = (size_t) n; - r = iovw_put(&iovw, iovec.iov_base, iovec.iov_len); - if (r < 0) - goto finish; + if (iovw_put(&iovw, iovec.iov_base, iovec.iov_len) < 0) + return log_oom(); TAKE_STRUCT(iovec); } @@ -1229,22 +1215,14 @@ static int process_socket(int fd) { r = context_parse_iovw(&context, &iovw); if (r < 0) - goto finish; + return r; /* Make sure we received at least all fields we need. */ for (int i = 0; i < _META_MANDATORY_MAX; i++) - if (!context.meta[i]) { - r = log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "A mandatory argument (%i) has not been sent, aborting.", - i); - goto finish; - } + if (!context.meta[i]) + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "A mandatory argument (%i) has not been sent, aborting.", i); - r = submit_coredump(&context, &iovw, input_fd); - -finish: - iovw_free_contents(&iovw, true); - return r; + return submit_coredump(&context, &iovw, input_fd); } static int send_iovec(const struct iovec_wrapper *iovw, int input_fd, PidRef *pidref, int mount_tree_fd) { From 811aa36ab6ee4da8daca386f8c3c338a720a4f0d Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 31 Oct 2024 16:00:13 +0100 Subject: [PATCH 10/13] iovw: add simpler iovw_done() destructor --- src/basic/iovec-wrapper.h | 4 ++++ src/shared/journal-importer.c | 4 ++-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/basic/iovec-wrapper.h b/src/basic/iovec-wrapper.h index 0c6d760a73..4754b98fbb 100644 --- a/src/basic/iovec-wrapper.h +++ b/src/basic/iovec-wrapper.h @@ -23,6 +23,10 @@ static inline void iovw_done_free(struct iovec_wrapper *iovw) { iovw_free_contents(iovw, true); } +static inline void iovw_done(struct iovec_wrapper *iovw) { + iovw_free_contents(iovw, false); +} + int iovw_put(struct iovec_wrapper *iovw, void *data, size_t len); static inline int iovw_consume(struct iovec_wrapper *iovw, void *data, size_t len) { /* Move data into iovw or free on error */ diff --git a/src/shared/journal-importer.c b/src/shared/journal-importer.c index 4dc0b8f662..9264638303 100644 --- a/src/shared/journal-importer.c +++ b/src/shared/journal-importer.c @@ -33,7 +33,7 @@ void journal_importer_cleanup(JournalImporter *imp) { free(imp->name); free(imp->buf); - iovw_free_contents(&imp->iovw, false); + iovw_done(&imp->iovw); } static char* realloc_buffer(JournalImporter *imp, size_t size) { @@ -452,7 +452,7 @@ void journal_importer_drop_iovw(JournalImporter *imp) { /* This function drops processed data that along with the iovw that points at it */ - iovw_free_contents(&imp->iovw, false); + iovw_done(&imp->iovw); /* possibly reset buffer position */ remain = imp->filled - imp->offset; From 0e3e075b56659882b0bbb362b1bda5d747a64537 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 31 Oct 2024 16:05:49 +0100 Subject: [PATCH 11/13] iovw: normalize destructors instead of passing a boolean picking the destruction method just have different functions. That's much nicer in context of _cleanup_, and how we usually do things. --- src/basic/iovec-wrapper.c | 21 +++++++++++++-------- src/basic/iovec-wrapper.h | 11 ++--------- 2 files changed, 15 insertions(+), 17 deletions(-) diff --git a/src/basic/iovec-wrapper.c b/src/basic/iovec-wrapper.c index b335acd108..5cc3cc2f93 100644 --- a/src/basic/iovec-wrapper.c +++ b/src/basic/iovec-wrapper.c @@ -9,22 +9,27 @@ struct iovec_wrapper *iovw_new(void) { return new0(struct iovec_wrapper, 1); } -void iovw_free_contents(struct iovec_wrapper *iovw, bool free_vectors) { +void iovw_done(struct iovec_wrapper *iovw) { assert(iovw); - if (free_vectors) - for (size_t i = 0; i < iovw->count; i++) - free(iovw->iovec[i].iov_base); - iovw->iovec = mfree(iovw->iovec); iovw->count = 0; } +void iovw_done_free(struct iovec_wrapper *iovw) { + assert(iovw); + + FOREACH_ARRAY(i, iovw->iovec, iovw->count) + iovec_done(i); + + iovw_done(iovw); +} + struct iovec_wrapper *iovw_free_free(struct iovec_wrapper *iovw) { if (!iovw) return NULL; - iovw_free_contents(iovw, /* free_vectors= */ true); + iovw_done_free(iovw); return mfree(iovw); } @@ -32,7 +37,7 @@ struct iovec_wrapper *iovw_free(struct iovec_wrapper *iovw) { if (!iovw) return NULL; - iovw_free_contents(iovw, /* free_vectors= */ false); + iovw_done(iovw); return mfree(iovw); } @@ -124,7 +129,7 @@ int iovw_append(struct iovec_wrapper *target, const struct iovec_wrapper *source rollback: for (size_t i = original_count; i < target->count; i++) - free(target->iovec[i].iov_base); + iovec_done(target->iovec + i); target->count = original_count; return r; diff --git a/src/basic/iovec-wrapper.h b/src/basic/iovec-wrapper.h index 4754b98fbb..4778fa6b27 100644 --- a/src/basic/iovec-wrapper.h +++ b/src/basic/iovec-wrapper.h @@ -17,15 +17,8 @@ struct iovec_wrapper *iovw_free_free(struct iovec_wrapper *iovw); DEFINE_TRIVIAL_CLEANUP_FUNC(struct iovec_wrapper*, iovw_free_free); -void iovw_free_contents(struct iovec_wrapper *iovw, bool free_vectors); - -static inline void iovw_done_free(struct iovec_wrapper *iovw) { - iovw_free_contents(iovw, true); -} - -static inline void iovw_done(struct iovec_wrapper *iovw) { - iovw_free_contents(iovw, false); -} +void iovw_done_free(struct iovec_wrapper *iovw); +void iovw_done(struct iovec_wrapper *iovw); int iovw_put(struct iovec_wrapper *iovw, void *data, size_t len); static inline int iovw_consume(struct iovec_wrapper *iovw, void *data, size_t len) { From b1694040afe3c33100018d522f091b721b84b87c Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 31 Oct 2024 16:53:39 +0100 Subject: [PATCH 12/13] coredump: use memory_startswith() when looking at a data blob --- src/coredump/coredump.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coredump/coredump.c b/src/coredump/coredump.c index 176e5373b8..f2acf8c37f 100644 --- a/src/coredump/coredump.c +++ b/src/coredump/coredump.c @@ -1033,7 +1033,7 @@ static int context_parse_iovw(Context *context, struct iovec_wrapper *iovw) { * count (see process_socket() and gather_pid_metadata_*()) */ assert(((char*) iovec->iov_base)[iovec->iov_len] == 0); - const char *p = startswith(iovec->iov_base, meta_field_names[i]); + const char *p = memory_startswith(iovec->iov_base, iovec->iov_len, meta_field_names[i]); if (p) { context->meta[i] = p; context->meta_size[i] = iovec->iov_len - strlen(meta_field_names[i]); From 098c3975acb3df61eedfe471fca27c21f13cf04c Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 31 Oct 2024 17:02:59 +0100 Subject: [PATCH 13/13] coredump: make check that all argv[] meta data fields are passed strict Otherwise, if some field is not supplied we might end up parsing a NULL string later. Let's catch that early. --- src/coredump/coredump.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/coredump/coredump.c b/src/coredump/coredump.c index f2acf8c37f..c3e97325e0 100644 --- a/src/coredump/coredump.c +++ b/src/coredump/coredump.c @@ -1045,9 +1045,10 @@ static int context_parse_iovw(Context *context, struct iovec_wrapper *iovw) { memory_startswith(iovec->iov_base, iovec->iov_len, "COREDUMP_SIGNAL_NAME="); } - if (!context->meta[META_ARGV_PID]) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "Failed to find the PID of crashing process"); + /* The basic fields from argv[] should always be there, refuse early if not */ + for (int i = 0; i < _META_ARGV_MAX; i++) + if (!context->meta[i]) + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "A required (%s) has not been sent, aborting.", meta_field_names[i]); pid_t parsed_pid; r = parse_pid(context->meta[META_ARGV_PID], &parsed_pid);