Merge pull request #25055 from keszybz/coredump-deadlock

Fix coredump deadlock with overly long backtraces

diff --git a/src/shared/elf-util.c b/src/shared/elf-util.c
index c0f540abc50a737a52c95426161f4db37e57a093..181735409d42d796e1dca9d8424e6f2046baa9c7 100644 (file)
--- a/src/shared/elf-util.c
+++ b/src/shared/elf-util.c
@@ -30,6 +30,9 @@
 #define THREADS_MAX 64
 #define ELF_PACKAGE_METADATA_ID 0xcafe1a7e
 
+/* The amount of data we're willing to write to each of the output pipes. */
+#define COREDUMP_PIPE_MAX (1024*1024U)
+
 static void *dw_dl = NULL;
 static void *elf_dl = NULL;
 
@@ -759,13 +762,13 @@ int parse_elf_object(int fd, const char *executable, bool fork_disable_dump, cha
                 return r;
 
         if (ret) {
-                r = RET_NERRNO(pipe2(return_pipe, O_CLOEXEC));
+                r = RET_NERRNO(pipe2(return_pipe, O_CLOEXEC|O_NONBLOCK));
                 if (r < 0)
                         return r;
         }
 
         if (ret_package_metadata) {
-                r = RET_NERRNO(pipe2(json_pipe, O_CLOEXEC));
+                r = RET_NERRNO(pipe2(json_pipe, O_CLOEXEC|O_NONBLOCK));
                 if (r < 0)
                         return r;
         }
@@ -809,8 +812,24 @@ int parse_elf_object(int fd, const char *executable, bool fork_disable_dump, cha
                         goto child_fail;
 
                 if (buf) {
-                        r = loop_write(return_pipe[1], buf, strlen(buf), false);
-                        if (r < 0)
+                        size_t len = strlen(buf);
+
+                        if (len > COREDUMP_PIPE_MAX) {
+                                /* This is iffy. A backtrace can be a few hundred kilobytes, but too much is
+                                 * too much. Let's log a warning and ignore the rest. */
+                                log_warning("Generated backtrace is %zu bytes (more than the limit of %u bytes), backtrace will be truncated.",
+                                            len, COREDUMP_PIPE_MAX);
+                                len = COREDUMP_PIPE_MAX;
+                        }
+
+                        /* Bump the space for the returned string.
+                         * Failure is ignored, because partial output is still useful. */
+                        (void) fcntl(return_pipe[1], F_SETPIPE_SZ, len);
+
+                        r = loop_write(return_pipe[1], buf, len, false);
+                        if (r == -EAGAIN)
+                                log_warning("Write failed, backtrace will be truncated.");
+                        else if (r < 0)
                                 goto child_fail;
 
                         return_pipe[1] = safe_close(return_pipe[1]);
@@ -819,13 +838,19 @@ int parse_elf_object(int fd, const char *executable, bool fork_disable_dump, cha
                 if (package_metadata) {
                         _cleanup_fclose_ FILE *json_out = NULL;
 
+                        /* Bump the space for the returned string. We don't know how much space we'll need in
+                         * advance, so we'll just try to write as much as possible and maybe fail later. */
+                        (void) fcntl(json_pipe[1], F_SETPIPE_SZ, COREDUMP_PIPE_MAX);
+
                         json_out = take_fdopen(&json_pipe[1], "w");
                         if (!json_out) {
                                 r = -errno;
                                 goto child_fail;
                         }
 
-                        json_variant_dump(package_metadata, JSON_FORMAT_FLUSH, json_out, NULL);
+                        r = json_variant_dump(package_metadata, JSON_FORMAT_FLUSH, json_out, NULL);
+                        if (r < 0)
+                                log_warning_errno(r, "Failed to write JSON package metadata, ignoring: %m");
                 }
 
                 _exit(EXIT_SUCCESS);
@@ -859,8 +884,8 @@ int parse_elf_object(int fd, const char *executable, bool fork_disable_dump, cha
                         return -errno;
 
                 r = json_parse_file(json_in, NULL, 0, &package_metadata, NULL, NULL);
-                if (r < 0 && r != -EINVAL) /* EINVAL: json was empty, so we got nothing, but that's ok */
-                        return r;
+                if (r < 0 && r != -ENODATA) /* ENODATA: json was empty, so we got nothing, but that's ok */
+                        log_warning_errno(r, "Failed to read or parse json metadata, ignoring: %m");
         }
 
         if (ret)

diff --git a/src/shared/json.c b/src/shared/json.c
index 950be9485d58193a6dd18147410a107e4c9c969e..eda7bb19563e560b074fe138b54fb7b70bcb83fd 100644 (file)
--- a/src/shared/json.c
+++ b/src/shared/json.c
@@ -1785,9 +1785,9 @@ int json_variant_format(JsonVariant *v, JsonFormatFlags flags, char **ret) {
         return (int) sz - 1;
 }
 
-void json_variant_dump(JsonVariant *v, JsonFormatFlags flags, FILE *f, const char *prefix) {
+int json_variant_dump(JsonVariant *v, JsonFormatFlags flags, FILE *f, const char *prefix) {
         if (!v)
-                return;
+                return 0;
 
         if (!f)
                 f = stdout;
@@ -1813,7 +1813,8 @@ void json_variant_dump(JsonVariant *v, JsonFormatFlags flags, FILE *f, const cha
                 fputc('\n', f); /* In case of SSE add a second newline */
 
         if (flags & JSON_FORMAT_FLUSH)
-                fflush(f);
+                return fflush_and_check(f);
+        return 0;
 }
 
 int json_variant_filter(JsonVariant **v, char **to_remove) {
@@ -3186,7 +3187,6 @@ int json_parse_continue(const char **p, JsonParseFlags flags, JsonVariant **ret,
 int json_parse_file_at(FILE *f, int dir_fd, const char *path, JsonParseFlags flags, JsonVariant **ret, unsigned *ret_line, unsigned *ret_column) {
         _cleanup_(json_source_unrefp) JsonSource *source = NULL;
         _cleanup_free_ char *text = NULL;
-        const char *p;
         int r;
 
         if (f)
@@ -3198,13 +3198,16 @@ int json_parse_file_at(FILE *f, int dir_fd, const char *path, JsonParseFlags fla
         if (r < 0)
                 return r;
 
+        if (isempty(text))
+                return -ENODATA;
+
         if (path) {
                 source = json_source_new(path);
                 if (!source)
                         return -ENOMEM;
         }
 
-        p = text;
+        const char *p = text;
         return json_parse_internal(&p, source, flags, ret, ret_line, ret_column, false);
 }
 

diff --git a/src/shared/json.h b/src/shared/json.h
index 1992170ed7c2a522ce2f421189666010ca838545..5993e05299c6e8c03961647c99d01ccce8f56154 100644 (file)
--- a/src/shared/json.h
+++ b/src/shared/json.h
@@ -197,7 +197,7 @@ typedef enum JsonFormatFlags {
 } JsonFormatFlags;
 
 int json_variant_format(JsonVariant *v, JsonFormatFlags flags, char **ret);
-void json_variant_dump(JsonVariant *v, JsonFormatFlags flags, FILE *f, const char *prefix);
+int json_variant_dump(JsonVariant *v, JsonFormatFlags flags, FILE *f, const char *prefix);
 
 int json_variant_filter(JsonVariant **v, char **to_remove);
 

diff --git a/src/test/test-json.c b/src/test/test-json.c
index 3563d004c8fa69a3e4f17437a0d56129a981f39d..946c827ccf008915b288d60a66f80b02d629c59b 100644 (file)
--- a/src/test/test-json.c
+++ b/src/test/test-json.c
@@ -344,6 +344,24 @@ TEST(build) {
         assert_se(json_variant_equal(a, b));
 }
 
+TEST(json_parse_file_empty) {
+        _cleanup_fclose_ FILE *f = NULL;
+        _cleanup_(json_variant_unrefp) JsonVariant *v = NULL;
+
+        assert_se(fopen_unlocked("/dev/null", "re", &f) >= 0);
+        assert_se(json_parse_file(f, "waldo", 0, &v, NULL, NULL) == -ENODATA);
+        assert_se(v == NULL);
+}
+
+TEST(json_parse_file_invalid) {
+        _cleanup_fclose_ FILE *f = NULL;
+        _cleanup_(json_variant_unrefp) JsonVariant *v = NULL;
+
+        assert_se(f = fmemopen_unlocked((void*) "kookoo", 6, "r"));
+        assert_se(json_parse_file(f, "waldo", 0, &v, NULL, NULL) == -EINVAL);
+        assert_se(v == NULL);
+}
+
 TEST(source) {
         static const char data[] =
                 "\n"