journal: fix access to munmapped memory in sd_journal_enumerate_unique

sd_j_e_u needs to keep a reference to an object while comparing it
with possibly duplicate objects in other files. Because the size of
mmap cache is limited, with enough files and object to compare to,
at some point the object being compared would be munmapped, resulting
in a segmentation fault.

Fix this issue by turning keep_always into a reference count that can
be increased and decreased. Other callers which set keep_always=true
are unmodified: their references are never released but are ignored
when the whole file is closed, which happens at some point. keep_always
is increased in sd_j_e_u and later on released.

diff --git a/src/journal/journal-file.c b/src/journal/journal-file.c
index 275324b672e554be629af524f3175de73d2671b6..b3747e37e4fbcd58b94b36d6581ab899254be4ad 100644 (file)
--- a/src/journal/journal-file.c
+++ b/src/journal/journal-file.c
@@ -419,7 +419,6 @@ int journal_file_move_to_object(JournalFile *f, int type, uint64_t offset, Objec
         void *t;
         Object *o;
         uint64_t s;
-        unsigned context;
 
         assert(f);
         assert(ret);
@@ -428,10 +427,8 @@ int journal_file_move_to_object(JournalFile *f, int type, uint64_t offset, Objec
         if (!VALID64(offset))
                 return -EFAULT;
 
-        /* One context for each type, plus one catch-all for the rest */
-        context = type > 0 && type < _OBJECT_TYPE_MAX ? type : 0;
 
-        r = journal_file_move_to(f, context, false, offset, sizeof(ObjectHeader), &t);
+        r = journal_file_move_to(f, type_to_context(type), false, offset, sizeof(ObjectHeader), &t);
         if (r < 0)
                 return r;
 

diff --git a/src/journal/journal-file.h b/src/journal/journal-file.h
index 21b0821b6b246fbb5cf98cb82a4cded6cc1f64b4..885b3b1df489c5fedfae12c2c51c551375393d1f 100644 (file)
--- a/src/journal/journal-file.h
+++ b/src/journal/journal-file.h
@@ -127,6 +127,10 @@ int journal_file_open_reliably(
 #define ALIGN64(x) (((x) + 7ULL) & ~7ULL)
 #define VALID64(x) (((x) & 7ULL) == 0ULL)
 
+/* Use six characters to cover the offsets common in smallish journal
+ * files without adding too many zeros. */
+#define OFSfmt "%06"PRIx64
+
 static inline bool VALID_REALTIME(uint64_t u) {
         /* This considers timestamps until the year 3112 valid. That should be plenty room... */
         return u > 0 && u < (1ULL << 55);
@@ -196,3 +200,23 @@ int journal_file_get_cutoff_realtime_usec(JournalFile *f, usec_t *from, usec_t *
 int journal_file_get_cutoff_monotonic_usec(JournalFile *f, sd_id128_t boot, usec_t *from, usec_t *to);
 
 bool journal_file_rotate_suggested(JournalFile *f, usec_t max_file_usec);
+
+
+static unsigned type_to_context(int type) {
+        /* One context for each type, plus one catch-all for the rest */
+        return type > 0 && type < _OBJECT_TYPE_MAX ? type : 0;
+}
+
+static inline int journal_file_object_keep(JournalFile *f, Object *o, uint64_t offset) {
+        unsigned context = type_to_context(o->object.type);
+
+        return mmap_cache_get(f->mmap, f->fd, f->prot, context, true,
+                              offset, o->object.size, &f->last_stat, NULL);
+}
+
+static inline int journal_file_object_release(JournalFile *f, Object *o, uint64_t offset) {
+        unsigned context = type_to_context(o->object.type);
+
+        return mmap_cache_release(f->mmap, f->fd, f->prot, context,
+                                  offset, o->object.size);
+}

diff --git a/src/journal/journal-verify.c b/src/journal/journal-verify.c
index 82b0f0a98bb23a7ffa0bdc657819eabc78f09eb5..f2422ff841c0142aa92e2302e9b959dbf27cb728 100644 (file)
--- a/src/journal/journal-verify.c
+++ b/src/journal/journal-verify.c
@@ -34,10 +34,6 @@
 #include "compress.h"
 #include "fsprg.h"
 
-/* Use six characters to cover the offsets common in smallish journal
- * files without adding to many zeros. */
-#define OFSfmt "%06"PRIx64
-
 static int journal_file_object_verify(JournalFile *f, uint64_t offset, Object *o) {
         uint64_t i;
 

diff --git a/src/journal/mmap-cache.c b/src/journal/mmap-cache.c
index 117dc2f3a5b81c27cf5b8838de001e3cceda3340..7dbbb5ef381b4c6b617441c4a6dae2c74aae9ca3 100644 (file)
--- a/src/journal/mmap-cache.c
+++ b/src/journal/mmap-cache.c
@@ -38,7 +38,7 @@ typedef struct FileDescriptor FileDescriptor;
 struct Window {
         MMapCache *cache;
 
-        bool keep_always;
+        unsigned keep_always;
         bool in_unused;
 
         int prot;
@@ -185,7 +185,7 @@ static void context_detach_window(Context *c) {
         c->window = NULL;
         LIST_REMOVE(by_window, w->contexts, c);
 
-        if (!w->contexts && !w->keep_always) {
+        if (!w->contexts && w->keep_always == 0) {
                 /* Not used anymore? */
                 LIST_PREPEND(unused, c->cache->unused, w);
                 if (!c->cache->last_unused)
@@ -360,7 +360,6 @@ static int try_context(
         assert(m->n_ref > 0);
         assert(fd >= 0);
         assert(size > 0);
-        assert(ret);
 
         c = hashmap_get(m->contexts, UINT_TO_PTR(context+1));
         if (!c)
@@ -378,9 +377,10 @@ static int try_context(
                 return 0;
         }
 
-        c->window->keep_always = c->window->keep_always || keep_always;
+        c->window->keep_always += keep_always;
 
-        *ret = (uint8_t*) c->window->ptr + (offset - c->window->offset);
+        if (ret)
+                *ret = (uint8_t*) c->window->ptr + (offset - c->window->offset);
         return 1;
 }
 
@@ -402,7 +402,6 @@ static int find_mmap(
         assert(m->n_ref > 0);
         assert(fd >= 0);
         assert(size > 0);
-        assert(ret);
 
         f = hashmap_get(m->fds, INT_TO_PTR(fd + 1));
         if (!f)
@@ -422,9 +421,10 @@ static int find_mmap(
                 return -ENOMEM;
 
         context_attach_window(c, w);
-        w->keep_always = w->keep_always || keep_always;
+        w->keep_always += keep_always;
 
-        *ret = (uint8_t*) w->ptr + (offset - w->offset);
+        if (ret)
+                *ret = (uint8_t*) w->ptr + (offset - w->offset);
         return 1;
 }
 
@@ -450,7 +450,6 @@ static int add_mmap(
         assert(m->n_ref > 0);
         assert(fd >= 0);
         assert(size > 0);
-        assert(ret);
 
         woffset = offset & ~((uint64_t) page_size() - 1ULL);
         wsize = size + (offset - woffset);
@@ -520,7 +519,8 @@ static int add_mmap(
         c->window = w;
         LIST_PREPEND(by_window, w->contexts, c);
 
-        *ret = (uint8_t*) w->ptr + (offset - w->offset);
+        if (ret)
+                *ret = (uint8_t*) w->ptr + (offset - w->offset);
         return 1;
 }
 
@@ -541,7 +541,6 @@ int mmap_cache_get(
         assert(m->n_ref > 0);
         assert(fd >= 0);
         assert(size > 0);
-        assert(ret);
 
         /* Check whether the current context is the right one already */
         r = try_context(m, fd, prot, context, keep_always, offset, size, ret);
@@ -563,6 +562,42 @@ int mmap_cache_get(
         return add_mmap(m, fd, prot, context, keep_always, offset, size, st, ret);
 }
 
+int mmap_cache_release(
+                MMapCache *m,
+                int fd,
+                int prot,
+                unsigned context,
+                uint64_t offset,
+                size_t size) {
+
+        FileDescriptor *f;
+        Window *w;
+
+        assert(m);
+        assert(m->n_ref > 0);
+        assert(fd >= 0);
+        assert(size > 0);
+
+        f = hashmap_get(m->fds, INT_TO_PTR(fd + 1));
+        if (!f)
+                return -EBADF;
+
+        assert(f->fd == fd);
+
+        LIST_FOREACH(by_fd, w, f->windows)
+                if (window_matches(w, fd, prot, offset, size))
+                        break;
+
+        if (!w)
+                return -ENOENT;
+
+        if (w->keep_always == 0)
+                return -ENOLCK;
+
+        w->keep_always -= 1;
+        return 0;
+}
+
 void mmap_cache_close_fd(MMapCache *m, int fd) {
         FileDescriptor *f;
 

diff --git a/src/journal/mmap-cache.h b/src/journal/mmap-cache.h
index 912336d7c233c19f72e3b5a8fe569c0d0833a1e7..647555a73e5e50767e3d543b38d4dd08b9bc9c6a 100644 (file)
--- a/src/journal/mmap-cache.h
+++ b/src/journal/mmap-cache.h
@@ -31,7 +31,23 @@ MMapCache* mmap_cache_new(void);
 MMapCache* mmap_cache_ref(MMapCache *m);
 MMapCache* mmap_cache_unref(MMapCache *m);
 
-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);
+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);
+int mmap_cache_release(
+        MMapCache *m,
+        int fd,
+        int prot,
+        unsigned context,
+        uint64_t offset,
+        size_t size);
 void mmap_cache_close_fd(MMapCache *m, int fd);
 void mmap_cache_close_context(MMapCache *m, unsigned context);
 

diff --git a/src/journal/sd-journal.c b/src/journal/sd-journal.c
index 283d5930630c34c4dbdfa17944a3b6c73cab41eb..7466006752c7af2b092eb690f01a2498061a1a48 100644 (file)
--- a/src/journal/sd-journal.c
+++ b/src/journal/sd-journal.c
@@ -2481,9 +2481,7 @@ _public_ int sd_journal_query_unique(sd_journal *j, const char *field) {
 }
 
 _public_ int sd_journal_enumerate_unique(sd_journal *j, const void **data, size_t *l) {
-        Object *o;
         size_t k;
-        int r;
 
         assert_return(j, -EINVAL);
         assert_return(!journal_pid_changed(j), -ECHILD);
@@ -2503,9 +2501,11 @@ _public_ int sd_journal_enumerate_unique(sd_journal *j, const void **data, size_
         for (;;) {
                 JournalFile *of;
                 Iterator i;
+                Object *o;
                 const void *odata;
                 size_t ol;
                 bool found;
+                int r;
 
                 /* Proceed to next data object in the field's linked list */
                 if (j->unique_offset == 0) {
@@ -2542,8 +2542,16 @@ _public_ int sd_journal_enumerate_unique(sd_journal *j, const void **data, size_
                         return r;
 
                 /* Let's do the type check by hand, since we used 0 context above. */
-                if (o->object.type != OBJECT_DATA)
+                if (o->object.type != OBJECT_DATA) {
+                        log_error("%s:offset " OFSfmt ": object has type %d, expected %d",
+                                  j->unique_file->path, j->unique_offset,
+                                  o->object.type, OBJECT_DATA);
                         return -EBADMSG;
+                }
+
+                r = journal_file_object_keep(j->unique_file, o, j->unique_offset);
+                if (r < 0)
+                        return r;
 
                 r = return_data(j, j->unique_file, o, &odata, &ol);
                 if (r < 0)
@@ -2577,6 +2585,10 @@ _public_ int sd_journal_enumerate_unique(sd_journal *j, const void **data, size_
                 if (found)
                         continue;
 
+                r = journal_file_object_release(j->unique_file, o, j->unique_offset);
+                if (r < 0)
+                        return r;
+
                 r = return_data(j, j->unique_file, o, data, l);
                 if (r < 0)
                         return r;