Merge pull request #27458 from mrc0mmand/test-corrupted-journals

test: test journalctl with corrupted journals

src/libsystemd/sd-journal/journal-def.h

@@ -38,7 +38,8 @@ typedef enum ObjectType {
         OBJECT_FIELD_HASH_TABLE,
         OBJECT_ENTRY_ARRAY,
         OBJECT_TAG,
-        _OBJECT_TYPE_MAX
+        _OBJECT_TYPE_MAX,
+        _OBJECT_TYPE_INVALID = -EINVAL,
 } ObjectType;
 
 /* Object flags (note that src/basic/compress.h uses the same values for the compression types) */

src/libsystemd/sd-journal/journal-file.c

@@ -285,6 +285,8 @@ JournalFile* journal_file_close(JournalFile *f) {
         if (!f)
                 return NULL;
 
+        assert(f->newest_boot_id_prioq_idx == PRIOQ_IDX_NULL);
+
         if (f->cache_fd)
                 mmap_cache_fd_free(f->cache_fd);
 
@@ -789,10 +791,10 @@ static int check_object_header(JournalFile *f, Object *o, ObjectType type, uint6
                                        "Attempt to move to overly short object with size %"PRIu64": %" PRIu64,
                                        s, offset);
 
-        if (o->object.type <= OBJECT_UNUSED)
+        if (o->object.type <= OBJECT_UNUSED || o->object.type >= _OBJECT_TYPE_MAX)
                 return log_debug_errno(SYNTHETIC_ERRNO(EBADMSG),
-                                       "Attempt to move to object with invalid type: %" PRIu64,
-                                       offset);
+                                       "Attempt to move to object with invalid type (%u): %" PRIu64,
+                                       o->object.type, offset);
 
         if (type > OBJECT_UNUSED && o->object.type != type)
                 return log_debug_errno(SYNTHETIC_ERRNO(EBADMSG),
@@ -2589,10 +2591,18 @@ static int bump_entry_array(
         assert(offset);
         assert(ret);
 
+        /* Return 1 when a non-zero offset found, 0 when the offset is zero.
+         * Here, we assume that the offset of each entry array object is in strict increasing order. */
+
         if (direction == DIRECTION_DOWN) {
                 assert(o);
-                *ret = le64toh(o->entry_array.next_entry_array_offset);
-                return 0;
+
+                p = le64toh(o->entry_array.next_entry_array_offset);
+                if (p > 0 && p <= offset)
+                        return -EBADMSG;
+
+                *ret = p;
+                return p > 0;
         }
 
         /* Entry array chains are a singly linked list, so to find the previous array in the chain, we have
@@ -2607,6 +2617,8 @@ static int bump_entry_array(
 
                 q = p;
                 p = le64toh(o->entry_array.next_entry_array_offset);
+                if (p <= q)
+                        return -EBADMSG;
         }
 
         /* If we can't find the previous entry array in the entry array chain, we're likely dealing with a
@@ -2615,8 +2627,7 @@ static int bump_entry_array(
                 return -EBADMSG;
 
         *ret = q;
-
-        return 0;
+        return 1; /* found */
 }
 
 static int generic_array_get(
@@ -2627,7 +2638,7 @@ static int generic_array_get(
                 Object **ret_object,
                 uint64_t *ret_offset) {
 
-        uint64_t p = 0, a, t = 0, k;
+        uint64_t a, t = 0, k;
         ChainCacheItem *ci;
         Object *o;
         int r;
@@ -2659,7 +2670,7 @@ static int generic_array_get(
                          * array and start iterating entries from there. */
 
                         r = bump_entry_array(f, NULL, a, first, DIRECTION_UP, &a);
-                        if (r < 0)
+                        if (r <= 0)
                                 return r;
 
                         i = UINT64_MAX;
@@ -2675,7 +2686,10 @@ static int generic_array_get(
 
                 i -= k;
                 t += k;
-                a = le64toh(o->entry_array.next_entry_array_offset);
+
+                r = bump_entry_array(f, o, a, first, DIRECTION_DOWN, &a);
+                if (r <= 0)
+                        return r;
         }
 
         /* If we've found the right location, now look for the first non-corrupt entry object (in the right
@@ -2697,6 +2711,8 @@ static int generic_array_get(
                 }
 
                 do {
+                        uint64_t p;
+
                         p = journal_file_entry_array_item(f, o, i);
 
                         r = journal_file_move_to_object(f, OBJECT_ENTRY, p, ret_object);
@@ -2713,12 +2729,17 @@ static int generic_array_get(
                                 return r;
 
                         /* OK, so this entry is borked. Most likely some entry didn't get synced to
-                        * disk properly, let's see if the next one might work for us instead. */
+                         * disk properly, let's see if the next one might work for us instead. */
                         log_debug_errno(r, "Entry item %" PRIu64 " is bad, skipping over it.", i);
+
+                        r = journal_file_move_to_object(f, OBJECT_ENTRY_ARRAY, a, &o);
+                        if (r < 0)
+                                return r;
+
                 } while (bump_array_index(&i, direction, k) > 0);
 
                 r = bump_entry_array(f, o, a, first, direction, &a);
-                if (r < 0)
+                if (r <= 0)
                         return r;
 
                 t += k;
@@ -4439,14 +4460,14 @@ bool journal_file_rotate_suggested(JournalFile *f, usec_t max_file_usec, int log
 }
 
 static const char * const journal_object_type_table[] = {
-        [OBJECT_UNUSED] = "unused",
-        [OBJECT_DATA] = "data",
-        [OBJECT_FIELD] = "field",
-        [OBJECT_ENTRY] = "entry",
-        [OBJECT_DATA_HASH_TABLE] = "data hash table",
+        [OBJECT_UNUSED]           = "unused",
+        [OBJECT_DATA]             = "data",
+        [OBJECT_FIELD]            = "field",
+        [OBJECT_ENTRY]            = "entry",
+        [OBJECT_DATA_HASH_TABLE]  = "data hash table",
         [OBJECT_FIELD_HASH_TABLE] = "field hash table",
-        [OBJECT_ENTRY_ARRAY] = "entry array",
-        [OBJECT_TAG] = "tag",
+        [OBJECT_ENTRY_ARRAY]      = "entry array",
+        [OBJECT_TAG]              = "tag",
 };
 
 DEFINE_STRING_TABLE_LOOKUP_TO_STRING(journal_object_type, ObjectType);

src/libsystemd/sd-journal/sd-journal.c

@@ -2304,7 +2304,7 @@ static void journal_file_unlink_newest_by_bood_id(sd_journal *j, JournalFile *f)
                 /* There's still a member in the prioq? Then make sure the hashmap key now points to its
                  * .newest_boot_id field (and not ours!). Not we only replace the memory of the key here, the
                  * value of the key (and the data associated with it) remain the same. */
-                assert_se(hashmap_update(j->newest_by_boot_id, &nf->newest_boot_id, p) >= 0);
+                assert_se(hashmap_replace(j->newest_by_boot_id, &nf->newest_boot_id, p) >= 0);
         else {
                 assert_se(hashmap_remove(j->newest_by_boot_id, &f->newest_boot_id) == p);
                 prioq_free(p);
@@ -2353,8 +2353,10 @@ static int journal_file_reshuffle_newest_by_boot_id(sd_journal *j, JournalFile *
                         return r;
 
                 r = hashmap_ensure_put(&j->newest_by_boot_id, &id128_hash_ops, &f->newest_boot_id, q);
-                if (r < 0)
+                if (r < 0) {
+                        f->newest_boot_id_prioq_idx = PRIOQ_IDX_NULL;
                         return r;
+                }
 
                 TAKE_PTR(q);
         }

test/units/testsuite-04.sh

@@ -272,20 +272,19 @@ journalctl --sync
 SEQNUM2=$(journalctl -o export -n 1 | grep -Ea "^__SEQNUM=" | cut -d= -f2)
 test "$SEQNUM2" -gt "$SEQNUM1"
 
-JTMP="/var/tmp/jtmp-$RANDOM"
-mkdir "$JTMP"
-
-( cd /test-journals/1 && for f in *.zst; do unzstd "$f" -o "$JTMP/${f%.zst}"; done )
-
-journalctl --directory="$JTMP" --list-boots --output=json >/tmp/lb1
+# Test for journals without RTC
+# See: https://github.com/systemd/systemd/issues/662
+JOURNAL_DIR="$(mktemp -d)"
+while read -r file; do
+    filename="${file##*/}"
+    unzstd "$file" -o "$JOURNAL_DIR/${filename%*.zst}"
+done < <(find /test-journals/no-rtc -name "*.zst")
 
+journalctl --directory="$JOURNAL_DIR" --list-boots --output=json >/tmp/lb1
 diff -u /tmp/lb1 - <<'EOF'
 [{"index":-3,"boot_id":"5ea5fc4f82a14186b5332a788ef9435e","first_entry":1666569600994371,"last_entry":1666584266223608},{"index":-2,"boot_id":"bea6864f21ad4c9594c04a99d89948b0","first_entry":1666584266731785,"last_entry":1666584347230411},{"index":-1,"boot_id":"4c708e1fd0744336be16f3931aa861fb","first_entry":1666584348378271,"last_entry":1666584354649355},{"index":0,"boot_id":"35e8501129134edd9df5267c49f744a4","first_entry":1666584356661527,"last_entry":1666584438086856}]
 EOF
-
-rm -rf "$JTMP"
-
-rm /tmp/lb1
+rm -rf "$JOURNAL_DIR" /tmp/lb1
 
 # https://bugzilla.redhat.com/show_bug.cgi?id=2183546
 mkdir /run/systemd/system/systemd-journald.service.d
@@ -319,4 +318,39 @@ systemctl daemon-reload
 systemctl restart systemd-journald.service
 journalctl --rotate
 
+# Corrupted journals
+JOURNAL_DIR="$(mktemp -d)"
+REMOTE_OUT="$(mktemp -d)"
+# tar on C8S doesn't support the --zstd option
+unzstd --stdout "/test-journals/afl-corrupted-journals.tar.zst" | tar -xC "$JOURNAL_DIR/"
+# First, try each of them sequentially. Skip this part when running with plain
+# QEMU, as it is excruciatingly slow
+# Note: we care only about exit code 124 (timeout) and special bash exit codes
+# >124 (like signals)
+if [[ "$(systemd-detect-virt -v)" != "qemu" ]]; then
+    while read -r file; do
+        timeout 10 journalctl -b --file="$file" >/dev/null || [[ $? -lt 124 ]]
+        timeout 10 journalctl -o export --file="$file" >/dev/null || [[ $? -lt 124 ]]
+        if [[ -x /usr/lib/systemd/systemd-journal-remote ]]; then
+            timeout 10 /usr/lib/systemd/systemd-journal-remote \
+                            --getter="journalctl -o export --file=$file" \
+                            --split-mode=none \
+                            --output="$REMOTE_OUT/system.journal" || [[ $? -lt 124 ]]
+            timeout 10 journalctl -b --directory="$REMOTE_OUT" >/dev/null || [[ $? -lt 124 ]]
+            rm -f "$REMOTE_OUT"/*
+        fi
+    done < <(find "$JOURNAL_DIR" -type f)
+fi
+# And now all at once
+timeout 30 journalctl -b --directory="$JOURNAL_DIR" >/dev/null || [[ $? -lt 124 ]]
+timeout 30 journalctl -o export --directory="$JOURNAL_DIR" >/dev/null || [[ $? -lt 124 ]]
+if [[ -x /usr/lib/systemd/systemd-journal-remote ]]; then
+    timeout 30 /usr/lib/systemd/systemd-journal-remote \
+                    --getter="journalctl -o export --directory=$JOURNAL_DIR" \
+                    --split-mode=none \
+                    --output="$REMOTE_OUT/system.journal" || [[ $? -lt 124 ]]
+    timeout 10 journalctl -b --directory="$REMOTE_OUT" >/dev/null || [[ $? -lt 124 ]]
+    rm -f "$REMOTE_OUT"/*
+fi
+
 touch /testok