daandemeyer / rpms / systemd

Forked from rpms/systemd 2 years ago
Clone
9fc0f6
From 035c9e559064114e3f7ba19b593a97c4a4d4f060 Mon Sep 17 00:00:00 2001
9fc0f6
From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= <zbyszek@in.waw.pl>
9fc0f6
Date: Sat, 28 Dec 2013 19:33:23 -0500
9fc0f6
Subject: [PATCH] journal: fix access to munmapped memory in
9fc0f6
 sd_journal_enumerate_unique
9fc0f6
9fc0f6
sd_j_e_u needs to keep a reference to an object while comparing it
9fc0f6
with possibly duplicate objects in other files. Because the size of
9fc0f6
mmap cache is limited, with enough files and object to compare to,
9fc0f6
at some point the object being compared would be munmapped, resulting
9fc0f6
in a segmentation fault.
9fc0f6
9fc0f6
Fix this issue by turning keep_always into a reference count that can
9fc0f6
be increased and decreased. Other callers which set keep_always=true
9fc0f6
are unmodified: their references are never released but are ignored
9fc0f6
when the whole file is closed, which happens at some point. keep_always
9fc0f6
is increased in sd_j_e_u and later on released.
9fc0f6
---
9fc0f6
 src/journal/journal-file.c   |  5 +---
9fc0f6
 src/journal/journal-file.h   | 24 +++++++++++++++++++
9fc0f6
 src/journal/journal-verify.c |  4 ----
9fc0f6
 src/journal/mmap-cache.c     | 57 +++++++++++++++++++++++++++++++++++---------
9fc0f6
 src/journal/mmap-cache.h     | 18 +++++++++++++-
9fc0f6
 src/journal/sd-journal.c     | 18 +++++++++++---
9fc0f6
 6 files changed, 103 insertions(+), 23 deletions(-)
9fc0f6
9fc0f6
diff --git a/src/journal/journal-file.c b/src/journal/journal-file.c
9fc0f6
index 748816a..9dbd674 100644
9fc0f6
--- a/src/journal/journal-file.c
9fc0f6
+++ b/src/journal/journal-file.c
9fc0f6
@@ -419,7 +419,6 @@ int journal_file_move_to_object(JournalFile *f, int type, uint64_t offset, Objec
9fc0f6
         void *t;
9fc0f6
         Object *o;
9fc0f6
         uint64_t s;
9fc0f6
-        unsigned context;
9fc0f6
 
9fc0f6
         assert(f);
9fc0f6
         assert(ret);
9fc0f6
@@ -428,10 +427,8 @@ int journal_file_move_to_object(JournalFile *f, int type, uint64_t offset, Objec
9fc0f6
         if (!VALID64(offset))
9fc0f6
                 return -EFAULT;
9fc0f6
 
9fc0f6
-        /* One context for each type, plus one catch-all for the rest */
9fc0f6
-        context = type > 0 && type < _OBJECT_TYPE_MAX ? type : 0;
9fc0f6
 
9fc0f6
-        r = journal_file_move_to(f, context, false, offset, sizeof(ObjectHeader), &t);
9fc0f6
+        r = journal_file_move_to(f, type_to_context(type), false, offset, sizeof(ObjectHeader), &t);
9fc0f6
         if (r < 0)
9fc0f6
                 return r;
9fc0f6
 
9fc0f6
diff --git a/src/journal/journal-file.h b/src/journal/journal-file.h
9fc0f6
index 5cc2c2d..376c3d4 100644
9fc0f6
--- a/src/journal/journal-file.h
9fc0f6
+++ b/src/journal/journal-file.h
9fc0f6
@@ -128,6 +128,10 @@ int journal_file_open_reliably(
9fc0f6
 #define ALIGN64(x) (((x) + 7ULL) & ~7ULL)
9fc0f6
 #define VALID64(x) (((x) & 7ULL) == 0ULL)
9fc0f6
 
9fc0f6
+/* Use six characters to cover the offsets common in smallish journal
9fc0f6
+ * files without adding too many zeros. */
9fc0f6
+#define OFSfmt "%06"PRIx64
9fc0f6
+
9fc0f6
 static inline bool VALID_REALTIME(uint64_t u) {
9fc0f6
         /* This considers timestamps until the year 3112 valid. That should be plenty room... */
9fc0f6
         return u > 0 && u < (1ULL << 55);
9fc0f6
@@ -197,3 +201,23 @@ int journal_file_get_cutoff_realtime_usec(JournalFile *f, usec_t *from, usec_t *
9fc0f6
 int journal_file_get_cutoff_monotonic_usec(JournalFile *f, sd_id128_t boot, usec_t *from, usec_t *to);
9fc0f6
 
9fc0f6
 bool journal_file_rotate_suggested(JournalFile *f, usec_t max_file_usec);
9fc0f6
+
9fc0f6
+
9fc0f6
+static unsigned type_to_context(int type) {
9fc0f6
+        /* One context for each type, plus one catch-all for the rest */
9fc0f6
+        return type > 0 && type < _OBJECT_TYPE_MAX ? type : 0;
9fc0f6
+}
9fc0f6
+
9fc0f6
+static inline int journal_file_object_keep(JournalFile *f, Object *o, uint64_t offset) {
9fc0f6
+        unsigned context = type_to_context(o->object.type);
9fc0f6
+
9fc0f6
+        return mmap_cache_get(f->mmap, f->fd, f->prot, context, true,
9fc0f6
+                              offset, o->object.size, &f->last_stat, NULL);
9fc0f6
+}
9fc0f6
+
9fc0f6
+static inline int journal_file_object_release(JournalFile *f, Object *o, uint64_t offset) {
9fc0f6
+        unsigned context = type_to_context(o->object.type);
9fc0f6
+
9fc0f6
+        return mmap_cache_release(f->mmap, f->fd, f->prot, context,
9fc0f6
+                                  offset, o->object.size);
9fc0f6
+}
9fc0f6
diff --git a/src/journal/journal-verify.c b/src/journal/journal-verify.c
9fc0f6
index 82b0f0a..f2422ff 100644
9fc0f6
--- a/src/journal/journal-verify.c
9fc0f6
+++ b/src/journal/journal-verify.c
9fc0f6
@@ -34,10 +34,6 @@
9fc0f6
 #include "compress.h"
9fc0f6
 #include "fsprg.h"
9fc0f6
 
9fc0f6
-/* Use six characters to cover the offsets common in smallish journal
9fc0f6
- * files without adding to many zeros. */
9fc0f6
-#define OFSfmt "%06"PRIx64
9fc0f6
-
9fc0f6
 static int journal_file_object_verify(JournalFile *f, uint64_t offset, Object *o) {
9fc0f6
         uint64_t i;
9fc0f6
 
9fc0f6
diff --git a/src/journal/mmap-cache.c b/src/journal/mmap-cache.c
9fc0f6
index 03b57be..cfb26da 100644
9fc0f6
--- a/src/journal/mmap-cache.c
9fc0f6
+++ b/src/journal/mmap-cache.c
9fc0f6
@@ -38,7 +38,7 @@ typedef struct FileDescriptor FileDescriptor;
9fc0f6
 struct Window {
9fc0f6
         MMapCache *cache;
9fc0f6
 
9fc0f6
-        bool keep_always;
9fc0f6
+        unsigned keep_always;
9fc0f6
         bool in_unused;
9fc0f6
 
9fc0f6
         int prot;
9fc0f6
@@ -182,7 +182,7 @@ static void context_detach_window(Context *c) {
9fc0f6
         c->window = NULL;
9fc0f6
         LIST_REMOVE(Context, by_window, w->contexts, c);
9fc0f6
 
9fc0f6
-        if (!w->contexts && !w->keep_always) {
9fc0f6
+        if (!w->contexts && w->keep_always == 0) {
9fc0f6
                 /* Not used anymore? */
9fc0f6
                 LIST_PREPEND(Window, unused, c->cache->unused, w);
9fc0f6
                 if (!c->cache->last_unused)
9fc0f6
@@ -357,7 +357,6 @@ static int try_context(
9fc0f6
         assert(m->n_ref > 0);
9fc0f6
         assert(fd >= 0);
9fc0f6
         assert(size > 0);
9fc0f6
-        assert(ret);
9fc0f6
 
9fc0f6
         c = hashmap_get(m->contexts, UINT_TO_PTR(context+1));
9fc0f6
         if (!c)
9fc0f6
@@ -375,9 +374,10 @@ static int try_context(
9fc0f6
                 return 0;
9fc0f6
         }
9fc0f6
 
9fc0f6
-        c->window->keep_always = c->window->keep_always || keep_always;
9fc0f6
+        c->window->keep_always += keep_always;
9fc0f6
 
9fc0f6
-        *ret = (uint8_t*) c->window->ptr + (offset - c->window->offset);
9fc0f6
+        if (ret)
9fc0f6
+                *ret = (uint8_t*) c->window->ptr + (offset - c->window->offset);
9fc0f6
         return 1;
9fc0f6
 }
9fc0f6
 
9fc0f6
@@ -399,7 +399,6 @@ static int find_mmap(
9fc0f6
         assert(m->n_ref > 0);
9fc0f6
         assert(fd >= 0);
9fc0f6
         assert(size > 0);
9fc0f6
-        assert(ret);
9fc0f6
 
9fc0f6
         f = hashmap_get(m->fds, INT_TO_PTR(fd + 1));
9fc0f6
         if (!f)
9fc0f6
@@ -419,9 +418,10 @@ static int find_mmap(
9fc0f6
                 return -ENOMEM;
9fc0f6
 
9fc0f6
         context_attach_window(c, w);
9fc0f6
-        w->keep_always = w->keep_always || keep_always;
9fc0f6
+        w->keep_always += keep_always;
9fc0f6
 
9fc0f6
-        *ret = (uint8_t*) w->ptr + (offset - w->offset);
9fc0f6
+        if (ret)
9fc0f6
+                *ret = (uint8_t*) w->ptr + (offset - w->offset);
9fc0f6
         return 1;
9fc0f6
 }
9fc0f6
 
9fc0f6
@@ -447,7 +447,6 @@ static int add_mmap(
9fc0f6
         assert(m->n_ref > 0);
9fc0f6
         assert(fd >= 0);
9fc0f6
         assert(size > 0);
9fc0f6
-        assert(ret);
9fc0f6
 
9fc0f6
         woffset = offset & ~((uint64_t) page_size() - 1ULL);
9fc0f6
         wsize = size + (offset - woffset);
9fc0f6
@@ -517,7 +516,8 @@ static int add_mmap(
9fc0f6
         c->window = w;
9fc0f6
         LIST_PREPEND(Context, by_window, w->contexts, c);
9fc0f6
 
9fc0f6
-        *ret = (uint8_t*) w->ptr + (offset - w->offset);
9fc0f6
+        if (ret)
9fc0f6
+                *ret = (uint8_t*) w->ptr + (offset - w->offset);
9fc0f6
         return 1;
9fc0f6
 }
9fc0f6
 
9fc0f6
@@ -538,7 +538,6 @@ int mmap_cache_get(
9fc0f6
         assert(m->n_ref > 0);
9fc0f6
         assert(fd >= 0);
9fc0f6
         assert(size > 0);
9fc0f6
-        assert(ret);
9fc0f6
 
9fc0f6
         /* Check whether the current context is the right one already */
9fc0f6
         r = try_context(m, fd, prot, context, keep_always, offset, size, ret);
9fc0f6
@@ -554,6 +553,42 @@ int mmap_cache_get(
9fc0f6
         return add_mmap(m, fd, prot, context, keep_always, offset, size, st, ret);
9fc0f6
 }
9fc0f6
 
9fc0f6
+int mmap_cache_release(
9fc0f6
+                MMapCache *m,
9fc0f6
+                int fd,
9fc0f6
+                int prot,
9fc0f6
+                unsigned context,
9fc0f6
+                uint64_t offset,
9fc0f6
+                size_t size) {
9fc0f6
+
9fc0f6
+        FileDescriptor *f;
9fc0f6
+        Window *w;
9fc0f6
+
9fc0f6
+        assert(m);
9fc0f6
+        assert(m->n_ref > 0);
9fc0f6
+        assert(fd >= 0);
9fc0f6
+        assert(size > 0);
9fc0f6
+
9fc0f6
+        f = hashmap_get(m->fds, INT_TO_PTR(fd + 1));
9fc0f6
+        if (!f)
9fc0f6
+                return -EBADF;
9fc0f6
+
9fc0f6
+        assert(f->fd == fd);
9fc0f6
+
9fc0f6
+        LIST_FOREACH(by_fd, w, f->windows)
9fc0f6
+                if (window_matches(w, fd, prot, offset, size))
9fc0f6
+                        break;
9fc0f6
+
9fc0f6
+        if (!w)
9fc0f6
+                return -ENOENT;
9fc0f6
+
9fc0f6
+        if (w->keep_always == 0)
9fc0f6
+                return -ENOLCK;
9fc0f6
+
9fc0f6
+        w->keep_always -= 1;
9fc0f6
+        return 0;
9fc0f6
+}
9fc0f6
+
9fc0f6
 void mmap_cache_close_fd(MMapCache *m, int fd) {
9fc0f6
         FileDescriptor *f;
9fc0f6
 
9fc0f6
diff --git a/src/journal/mmap-cache.h b/src/journal/mmap-cache.h
9fc0f6
index 0c42fb8..e5e3b38 100644
9fc0f6
--- a/src/journal/mmap-cache.h
9fc0f6
+++ b/src/journal/mmap-cache.h
9fc0f6
@@ -31,6 +31,22 @@ MMapCache* mmap_cache_new(void);
9fc0f6
 MMapCache* mmap_cache_ref(MMapCache *m);
9fc0f6
 MMapCache* mmap_cache_unref(MMapCache *m);
9fc0f6
 
9fc0f6
-int mmap_cache_get(MMapCache *m, int fd, int prot, unsigned context, bool keep_always, uint64_t offset, size_t size, struct stat *st, void **ret);
9fc0f6
+int mmap_cache_get(
9fc0f6
+        MMapCache *m,
9fc0f6
+        int fd,
9fc0f6
+        int prot,
9fc0f6
+        unsigned context,
9fc0f6
+        bool keep_always,
9fc0f6
+        uint64_t offset,
9fc0f6
+        size_t size,
9fc0f6
+        struct stat *st,
9fc0f6
+        void **ret);
9fc0f6
+int mmap_cache_release(
9fc0f6
+        MMapCache *m,
9fc0f6
+        int fd,
9fc0f6
+        int prot,
9fc0f6
+        unsigned context,
9fc0f6
+        uint64_t offset,
9fc0f6
+        size_t size);
9fc0f6
 void mmap_cache_close_fd(MMapCache *m, int fd);
9fc0f6
 void mmap_cache_close_context(MMapCache *m, unsigned context);
9fc0f6
diff --git a/src/journal/sd-journal.c b/src/journal/sd-journal.c
9fc0f6
index 9676f0f..67a77e6 100644
9fc0f6
--- a/src/journal/sd-journal.c
9fc0f6
+++ b/src/journal/sd-journal.c
9fc0f6
@@ -2506,9 +2506,7 @@ _public_ int sd_journal_query_unique(sd_journal *j, const char *field) {
9fc0f6
 }
9fc0f6
 
9fc0f6
 _public_ int sd_journal_enumerate_unique(sd_journal *j, const void **data, size_t *l) {
9fc0f6
-        Object *o;
9fc0f6
         size_t k;
9fc0f6
-        int r;
9fc0f6
 
9fc0f6
         if (!j)
9fc0f6
                 return -EINVAL;
9fc0f6
@@ -2533,9 +2531,11 @@ _public_ int sd_journal_enumerate_unique(sd_journal *j, const void **data, size_
9fc0f6
         for (;;) {
9fc0f6
                 JournalFile *of;
9fc0f6
                 Iterator i;
9fc0f6
+                Object *o;
9fc0f6
                 const void *odata;
9fc0f6
                 size_t ol;
9fc0f6
                 bool found;
9fc0f6
+                int r;
9fc0f6
 
9fc0f6
                 /* Proceed to next data object in the field's linked list */
9fc0f6
                 if (j->unique_offset == 0) {
9fc0f6
@@ -2572,8 +2572,16 @@ _public_ int sd_journal_enumerate_unique(sd_journal *j, const void **data, size_
9fc0f6
                         return r;
9fc0f6
 
9fc0f6
                 /* Let's do the type check by hand, since we used 0 context above. */
9fc0f6
-                if (o->object.type != OBJECT_DATA)
9fc0f6
+                if (o->object.type != OBJECT_DATA) {
9fc0f6
+                        log_error("%s:offset " OFSfmt ": object has type %d, expected %d",
9fc0f6
+                                  j->unique_file->path, j->unique_offset,
9fc0f6
+                                  o->object.type, OBJECT_DATA);
9fc0f6
                         return -EBADMSG;
9fc0f6
+                }
9fc0f6
+
9fc0f6
+                r = journal_file_object_keep(j->unique_file, o, j->unique_offset);
9fc0f6
+                if (r < 0)
9fc0f6
+                        return r;
9fc0f6
 
9fc0f6
                 r = return_data(j, j->unique_file, o, &odata, &ol);
9fc0f6
                 if (r < 0)
9fc0f6
@@ -2607,6 +2615,10 @@ _public_ int sd_journal_enumerate_unique(sd_journal *j, const void **data, size_
9fc0f6
                 if (found)
9fc0f6
                         continue;
9fc0f6
 
9fc0f6
+                r = journal_file_object_release(j->unique_file, o, j->unique_offset);
9fc0f6
+                if (r < 0)
9fc0f6
+                        return r;
9fc0f6
+
9fc0f6
                 r = return_data(j, j->unique_file, o, data, l);
9fc0f6
                 if (r < 0)
9fc0f6
                         return r;