From e5d8473335ee399b12d803288fcc09d4bd3b120b Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 1 Feb 2022 18:31:12 +0100 Subject: [PATCH 1/2] journal: various fixes to journal_file_read_object() This fixes a bunch of issues: pread() returns ssize_t, and returns errors in 'errno', handle that correctly. More importantly: it might incompletely read data in case we hit EOF. Check for that, and handle it. Finally, rename the function to journal_file_read_object_header(), since it really doesn't read full objects, but only their headers. Follow-up for: 117e21121e857b4b7d81949542e8dd257265970a --- src/journal/journald-file.c | 4 ++-- src/libsystemd/sd-journal/journal-file.c | 29 ++++++++++++++++-------- src/libsystemd/sd-journal/journal-file.h | 2 +- 3 files changed, 23 insertions(+), 12 deletions(-) diff --git a/src/journal/journald-file.c b/src/journal/journald-file.c index 4e095acc93..3315eaf13c 100644 --- a/src/journal/journald-file.c +++ b/src/journal/journald-file.c @@ -45,7 +45,7 @@ static int journald_file_entry_array_punch_hole(JournalFile *f, uint64_t p, uint return 0; for (uint64_t q = p; q != 0; q = le64toh(o.entry_array.next_entry_array_offset)) { - r = journal_file_read_object(f, OBJECT_ENTRY_ARRAY, q, &o); + r = journal_file_read_object_header(f, OBJECT_ENTRY_ARRAY, q, &o); if (r < 0) return r; @@ -119,7 +119,7 @@ static int journald_file_punch_holes(JournalFile *f) { for (uint64_t q = le64toh(items[j].head_hash_offset); q != 0; q = le64toh(o.data.next_hash_offset)) { - r = journal_file_read_object(f, OBJECT_DATA, q, &o); + r = journal_file_read_object_header(f, OBJECT_DATA, q, &o); if (r < 0) { log_debug_errno(r, "Invalid data object: %m, ignoring"); break; diff --git a/src/libsystemd/sd-journal/journal-file.c b/src/libsystemd/sd-journal/journal-file.c index dc866b8905..c85dec89be 100644 --- a/src/libsystemd/sd-journal/journal-file.c +++ b/src/libsystemd/sd-journal/journal-file.c @@ -106,7 +106,7 @@ int journal_file_tail_end(JournalFile *f, uint64_t *ret_offset) { else { uint64_t sz; - r = journal_file_read_object(f, OBJECT_UNUSED, p, &tail); + r = journal_file_read_object_header(f, OBJECT_UNUSED, p, &tail); if (r < 0) return r; @@ -818,10 +818,11 @@ int journal_file_move_to_object(JournalFile *f, ObjectType type, uint64_t offset return 0; } -int journal_file_read_object(JournalFile *f, ObjectType type, uint64_t offset, Object *ret) { - int r; - Object o; +int journal_file_read_object_header(JournalFile *f, ObjectType type, uint64_t offset, Object *ret) { uint64_t s; + ssize_t n; + Object o; + int r; assert(f); @@ -838,17 +839,22 @@ int journal_file_read_object(JournalFile *f, ObjectType type, uint64_t offset, O offset); /* This will likely read too much data but it avoids having to call pread() twice. */ - r = pread(f->fd, &o, sizeof(Object), offset); - if (r < 0) - return r; + n = pread(f->fd, &o, sizeof(o), offset); + if (n < 0) + return log_debug_errno(errno, "Failed to read journal file at offset: %" PRIu64, + offset); + + if ((size_t) n < sizeof(o.object)) + return log_debug_errno(SYNTHETIC_ERRNO(EIO), + "Failed to read short object at offset: %" PRIu64, + offset); s = le64toh(o.object.size); - if (s == 0) return log_debug_errno(SYNTHETIC_ERRNO(EBADMSG), "Attempt to read uninitialized object: %" PRIu64, offset); - if (s < sizeof(ObjectHeader)) + if (s < sizeof(o.object)) return log_debug_errno(SYNTHETIC_ERRNO(EBADMSG), "Attempt to read overly short object: %" PRIu64, offset); @@ -863,6 +869,11 @@ int journal_file_read_object(JournalFile *f, ObjectType type, uint64_t offset, O "Attempt to read truncated object: %" PRIu64, offset); + if ((size_t) n < minimum_header_size(&o)) + return log_debug_errno(SYNTHETIC_ERRNO(EIO), + "Short read while reading object: %" PRIu64, + offset); + if (type > OBJECT_UNUSED && o.object.type != type) return log_debug_errno(SYNTHETIC_ERRNO(EBADMSG), "Attempt to read object of unexpected type: %" PRIu64, diff --git a/src/libsystemd/sd-journal/journal-file.h b/src/libsystemd/sd-journal/journal-file.h index f673e05e72..1e903b23e2 100644 --- a/src/libsystemd/sd-journal/journal-file.h +++ b/src/libsystemd/sd-journal/journal-file.h @@ -185,7 +185,7 @@ static inline bool VALID_EPOCH(uint64_t u) { FLAGS_SET(le32toh((h)->incompatible_flags), HEADER_INCOMPATIBLE_KEYED_HASH) int journal_file_move_to_object(JournalFile *f, ObjectType type, uint64_t offset, Object **ret); -int journal_file_read_object(JournalFile *f, ObjectType type, uint64_t offset, Object *ret); +int journal_file_read_object_header(JournalFile *f, ObjectType type, uint64_t offset, Object *ret); int journal_file_tail_end(JournalFile *f, uint64_t *ret_offset); From e6d4a1106c613cbaa50320782889d002d8c1a58d Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 1 Feb 2022 18:45:35 +0100 Subject: [PATCH 2/2] journal-file: don't use pread() when determining where to append, use mmap as before This partially undoes the effect of ab6e257b3e4e5b95f3750ed019bed6e89989e41b. Originally, we always used the mmap logic to determine the current end of the file. ab6e257b3e4e5b95f3750ed019bed6e89989e41b changed this so that we always used pread(). With this change we'll use pread() from the synchronization thread and mmap otherwise. --- src/journal/journald-file.c | 2 +- src/libsystemd/sd-journal/journal-file.c | 46 ++++++++++++++++++++++-- src/libsystemd/sd-journal/journal-file.h | 3 +- 3 files changed, 46 insertions(+), 5 deletions(-) diff --git a/src/journal/journald-file.c b/src/journal/journald-file.c index 3315eaf13c..d4765b98df 100644 --- a/src/journal/journald-file.c +++ b/src/journal/journald-file.c @@ -23,7 +23,7 @@ static int journald_file_truncate(JournalFile *f) { int r; /* truncate excess from the end of archives */ - r = journal_file_tail_end(f, &p); + r = journal_file_tail_end_by_pread(f, &p); if (r < 0) return log_debug_errno(r, "Failed to determine end of tail object: %m"); diff --git a/src/libsystemd/sd-journal/journal-file.c b/src/libsystemd/sd-journal/journal-file.c index c85dec89be..67eb77be23 100644 --- a/src/libsystemd/sd-journal/journal-file.c +++ b/src/libsystemd/sd-journal/journal-file.c @@ -91,8 +91,7 @@ # pragma GCC diagnostic ignored "-Waddress-of-packed-member" #endif -int journal_file_tail_end(JournalFile *f, uint64_t *ret_offset) { - Object tail; +int journal_file_tail_end_by_pread(JournalFile *f, uint64_t *ret_offset) { uint64_t p; int r; @@ -100,10 +99,14 @@ int journal_file_tail_end(JournalFile *f, uint64_t *ret_offset) { assert(f->header); assert(ret_offset); + /* Same as journal_file_tail_end_by_mmap() below, but operates with pread() to avoid the mmap cache + * (and thus is thread safe) */ + p = le64toh(f->header->tail_object_offset); if (p == 0) p = le64toh(f->header->header_size); else { + Object tail; uint64_t sz; r = journal_file_read_object_header(f, OBJECT_UNUSED, p, &tail); @@ -126,6 +129,43 @@ int journal_file_tail_end(JournalFile *f, uint64_t *ret_offset) { return 0; } +int journal_file_tail_end_by_mmap(JournalFile *f, uint64_t *ret_offset) { + uint64_t p; + int r; + + assert(f); + assert(f->header); + assert(ret_offset); + + /* Same as journal_file_tail_end_by_pread() above, but operates with the usual mmap logic */ + + p = le64toh(f->header->tail_object_offset); + if (p == 0) + p = le64toh(f->header->header_size); + else { + Object *tail; + uint64_t sz; + + r = journal_file_move_to_object(f, OBJECT_UNUSED, p, &tail); + if (r < 0) + return r; + + sz = le64toh(READ_NOW(tail->object.size)); + if (sz > UINT64_MAX - sizeof(uint64_t) + 1) + return -EBADMSG; + + sz = ALIGN64(sz); + if (p > UINT64_MAX - sz) + return -EBADMSG; + + p += sz; + } + + *ret_offset = p; + + return 0; +} + int journal_file_set_offline_thread_join(JournalFile *f) { int r; @@ -941,7 +981,7 @@ int journal_file_append_object( if (r < 0) return r; - r = journal_file_tail_end(f, &p); + r = journal_file_tail_end_by_mmap(f, &p); if (r < 0) return r; diff --git a/src/libsystemd/sd-journal/journal-file.h b/src/libsystemd/sd-journal/journal-file.h index 1e903b23e2..59509deec9 100644 --- a/src/libsystemd/sd-journal/journal-file.h +++ b/src/libsystemd/sd-journal/journal-file.h @@ -187,7 +187,8 @@ static inline bool VALID_EPOCH(uint64_t u) { int journal_file_move_to_object(JournalFile *f, ObjectType type, uint64_t offset, Object **ret); int journal_file_read_object_header(JournalFile *f, ObjectType type, uint64_t offset, Object *ret); -int journal_file_tail_end(JournalFile *f, uint64_t *ret_offset); +int journal_file_tail_end_by_pread(JournalFile *f, uint64_t *ret_offset); +int journal_file_tail_end_by_mmap(JournalFile *f, uint64_t *ret_offset); uint64_t journal_file_entry_n_items(Object *o) _pure_; uint64_t journal_file_entry_array_n_items(Object *o) _pure_;