From 85ec1b6efadf3466bd1d265950f3d967a43cb1d0 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 10 Aug 2023 12:09:15 +0200 Subject: [PATCH 1/6] coredump: explicitly document that in order to process a coredump we have to write it to disk first Prompted by: #28740 --- man/coredump.conf.xml | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/man/coredump.conf.xml b/man/coredump.conf.xml index f36a6e1eae..ac8d984670 100644 --- a/man/coredump.conf.xml +++ b/man/coredump.conf.xml @@ -57,18 +57,22 @@ Storage= Controls where to store cores. One of none, - external, and journal. When - none, the core dumps may be logged (including the backtrace if - possible), but not stored permanently. When external (the - default), cores will be stored in /var/lib/systemd/coredump/. - When journal, cores will be stored in the journal and rotated - following normal journal rotation patterns. + external, and journal. When none, the core + dumps may be logged (including the backtrace if possible), but not stored permanently. When + external (the default), cores will be stored in + /var/lib/systemd/coredump/. When journal, cores will be + stored in the journal and rotated following normal journal rotation patterns. - When cores are stored in the journal, they might be - compressed following journal compression settings, see + When cores are stored in the journal, they might be compressed following journal compression + settings, see journald.conf5. - When cores are stored externally, they will be compressed - by default, see below. + When cores are stored externally, they will be compressed by default, see below. + + Note that in order to process a coredump (i.e. extract a stack trace) the core must be written + to disk first. Thus, unless ProcessSizeMax= is set to 0 (see below), the core will + be written to /var/lib/systemd/coredump/ either way (under a temporary filename, + or even in an unlinked file), Storage= thus only controls whether to leave it + there even after it was processed. @@ -84,7 +88,7 @@ ProcessSizeMax= The maximum size in bytes of a core which will be processed. Core dumps exceeding - this size may be stored, but the backtrace will not be generated. Like other sizes in this same + this size may be stored, but the stack trace will not be generated. Like other sizes in this same config file, the usual suffixes to the base of 1024 are allowed (B, K, M, G, T, P, and E). Defaults to 1G on 32-bit systems, 32G on 64-bit systems. From 6257e2fb1a2042434358d7347afd5c98467aa64c Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 10 Aug 2023 12:10:47 +0200 Subject: [PATCH 2/6] coredump: use a cleanup handler for destroying iovw objects --- src/basic/io-util.h | 3 +++ src/coredump/coredump.c | 14 +++++--------- 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/src/basic/io-util.h b/src/basic/io-util.h index 3ad8267962..4f7989e66d 100644 --- a/src/basic/io-util.h +++ b/src/basic/io-util.h @@ -93,6 +93,9 @@ struct iovec_wrapper { struct iovec_wrapper *iovw_new(void); struct iovec_wrapper *iovw_free(struct iovec_wrapper *iovw); 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); int iovw_put(struct iovec_wrapper *iovw, void *data, size_t len); diff --git a/src/coredump/coredump.c b/src/coredump/coredump.c index 8ae4a9f63f..8469ab297c 100644 --- a/src/coredump/coredump.c +++ b/src/coredump/coredump.c @@ -1363,8 +1363,8 @@ static int gather_pid_metadata(struct iovec_wrapper *iovw, Context *context) { } static int process_kernel(int argc, char* argv[]) { + _cleanup_(iovw_free_freep) struct iovec_wrapper *iovw = NULL; Context context = {}; - struct iovec_wrapper *iovw; int r; /* When we're invoked by the kernel, stdout/stderr are closed which is dangerous because the fds @@ -1386,12 +1386,12 @@ static int process_kernel(int argc, char* argv[]) { /* Collect all process metadata passed by the kernel through argv[] */ r = gather_pid_metadata_from_argv(iovw, &context, argc - 1, argv + 1); if (r < 0) - goto finish; + return r; /* Collect the rest of the process metadata retrieved from the runtime */ r = gather_pid_metadata(iovw, &context); if (r < 0) - goto finish; + return r; if (!context.is_journald) /* OK, now we know it's not the journal, hence we can make use of it now. */ @@ -1409,13 +1409,9 @@ static int process_kernel(int argc, char* argv[]) { } if (context.is_journald || context.is_pid1) - r = submit_coredump(&context, iovw, STDIN_FILENO); - else - r = send_iovec(iovw, STDIN_FILENO); + return submit_coredump(&context, iovw, STDIN_FILENO); - finish: - iovw = iovw_free_free(iovw); - return r; + return send_iovec(iovw, STDIN_FILENO); } static int process_backtrace(int argc, char *argv[]) { From e6aa443feb1946abe4253f7d0f99d753e55e2569 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 10 Aug 2023 12:11:33 +0200 Subject: [PATCH 3/6] coredump: add four assert()s --- src/coredump/coredump.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/coredump/coredump.c b/src/coredump/coredump.c index 8469ab297c..4898a6f83d 100644 --- a/src/coredump/coredump.c +++ b/src/coredump/coredump.c @@ -1205,6 +1205,9 @@ static int gather_pid_metadata_from_argv( int r, signo; char *t; + assert(iovw); + assert(context); + /* We gather all metadata that were passed via argv[] into an array of iovecs that * we'll forward to the socket unit */ @@ -1258,6 +1261,9 @@ static int gather_pid_metadata(struct iovec_wrapper *iovw, Context *context) { const char *p; int r; + assert(iovw); + assert(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.) */ From db9ac8016322e3227b0466f35ca0103e0cc8d829 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 10 Aug 2023 12:11:52 +0200 Subject: [PATCH 4/6] =?UTF-8?q?coredump:=20rename=20gather=5Fpid=5Fmetadat?= =?UTF-8?q?a()=20=E2=86=92=20gather=5Fpid=5Fmetadata=5Ffrom=5Fprocfs()?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Let's make clear what this function does, and what it distinguishes with the more precisely named gather_pid_metadata_from_argv(). --- 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 4898a6f83d..08be963b8a 100644 --- a/src/coredump/coredump.c +++ b/src/coredump/coredump.c @@ -1253,7 +1253,7 @@ static int gather_pid_metadata_from_argv( return save_context(context, iovw); } -static int gather_pid_metadata(struct iovec_wrapper *iovw, Context *context) { +static int gather_pid_metadata_from_procfs(struct iovec_wrapper *iovw, Context *context) { uid_t owner_uid; pid_t pid; char *t; @@ -1395,7 +1395,7 @@ static int process_kernel(int argc, char* argv[]) { return r; /* Collect the rest of the process metadata retrieved from the runtime */ - r = gather_pid_metadata(iovw, &context); + r = gather_pid_metadata_from_procfs(iovw, &context); if (r < 0) return r; @@ -1443,7 +1443,7 @@ static int process_backtrace(int argc, char *argv[]) { goto finish; /* Collect the rest of the process metadata retrieved from the runtime */ - r = gather_pid_metadata(iovw, &context); + r = gather_pid_metadata_from_procfs(iovw, &context); if (r < 0) goto finish; From 946dc7c635f050129896d1515c08a81504af2421 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 10 Aug 2023 12:13:07 +0200 Subject: [PATCH 5/6] coredump: fix error path We must go through finish, to undo the destruction of the final elements of the iovw properly. --- 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 08be963b8a..9a2066858d 100644 --- a/src/coredump/coredump.c +++ b/src/coredump/coredump.c @@ -1468,7 +1468,7 @@ static int process_backtrace(int argc, char *argv[]) { r = iovw_put_string_field(iovw, "MESSAGE=", message); if (r < 0) - return r; + goto finish; } else { /* The imported iovecs are not supposed to be freed by us so let's store * them at the end of the array so we can skip them while freeing the From d67a0999aafb8a7f6a7315797561f77a429a4173 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 10 Aug 2023 12:13:46 +0200 Subject: [PATCH 6/6] coredump: let's use FOREACH_ARRAY() at once very obvious place --- src/coredump/coredump.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/coredump/coredump.c b/src/coredump/coredump.c index 9a2066858d..659eae3b3e 100644 --- a/src/coredump/coredump.c +++ b/src/coredump/coredump.c @@ -1473,11 +1473,8 @@ static int process_backtrace(int argc, char *argv[]) { /* The imported iovecs are not supposed to be freed by us so let's store * them at the end of the array so we can skip them while freeing the * rest. */ - for (size_t i = 0; i < importer.iovw.count; i++) { - struct iovec *iovec = importer.iovw.iovec + i; - + FOREACH_ARRAY(iovec, importer.iovw.iovec, importer.iovw.count) iovw_put(iovw, iovec->iov_base, iovec->iov_len); - } } r = sd_journal_sendv(iovw->iovec, iovw->count);