diff --git a/src/libsystemd/sd-journal/sd-journal.c b/src/libsystemd/sd-journal/sd-journal.c index 403256eb81..3208ce9733 100644 --- a/src/libsystemd/sd-journal/sd-journal.c +++ b/src/libsystemd/sd-journal/sd-journal.c @@ -1127,69 +1127,88 @@ static int compare_locations(sd_journal *j, JournalFile *af, JournalFile *bf) { } static int real_journal_next(sd_journal *j, direction_t direction) { - JournalFile *new_file = NULL, *exact_match = NULL; unsigned n_files; const void **files; - Object *o; int r; assert_return(j, -EINVAL); assert_return(!journal_origin_changed(j), -ECHILD); - r = iterated_cache_get(j->files_cache, NULL, &files, &n_files); - if (r < 0) - return r; + for (;;) { + JournalFile *new_file = NULL, *exact_match = NULL; + Object *o; - FOREACH_ARRAY(_f, files, n_files) { - JournalFile *f = (JournalFile*) *_f; - bool found; + r = iterated_cache_get(j->files_cache, NULL, &files, &n_files); + if (r < 0) + return r; - r = next_beyond_location(j, f, direction); - if (r < 0) { - log_debug_errno(r, "Can't iterate through %s, ignoring: %m", f->path); - remove_file_real(j, f); - continue; - } else if (r == 0) { - f->location_type = direction == DIRECTION_DOWN ? LOCATION_TAIL : LOCATION_HEAD; - continue; + FOREACH_ARRAY(_f, files, n_files) { + JournalFile *f = (JournalFile*) *_f; + bool found; + + r = next_beyond_location(j, f, direction); + if (r < 0) { + log_debug_errno(r, "Can't iterate through %s, ignoring: %m", f->path); + remove_file_real(j, f); + continue; + } else if (r == 0) { + f->location_type = direction == DIRECTION_DOWN ? + LOCATION_TAIL : LOCATION_HEAD; + continue; + } + + if (!new_file) + found = true; + else { + r = compare_locations(j, f, new_file); + found = direction == DIRECTION_DOWN ? r < 0 : r > 0; + } + + if (found) + new_file = f; + + /* Track the file that holds the cursor's exact entry (matching seqnum_id and + * seqnum). On systems without a reliable (or missing) RTC, compare_boot_ids() can + * produce incorrect cross-boot ordering. After the loop, we detect when + * compare_locations() preferred the wrong file and override the choice if needed. + * + * See https://github.com/systemd/systemd/issues/31516 */ + if (j->current_location.type == LOCATION_SEEK && + j->current_location.seqnum_set && + sd_id128_equal(f->header->seqnum_id, j->current_location.seqnum_id) && + f->current_seqnum == j->current_location.seqnum) + exact_match = f; } + if (exact_match) + new_file = exact_match; + if (!new_file) - found = true; - else { - r = compare_locations(j, f, new_file); - found = direction == DIRECTION_DOWN ? r < 0 : r > 0; + return 0; + + r = journal_file_move_to_object(new_file, OBJECT_ENTRY, new_file->current_offset, &o); + if (r < 0) { + /* Vacuuming can unlink and deallocate the file we just picked, which surfaces as + * one of these errors. Confirm the file is really gone before dropping it: -EIDRM + * already comes from the journal_file_fstat() inside the lookup, the others need a + * stat of our own. */ + if (!IN_SET(r, -EADDRNOTAVAIL, -EBADMSG, -EIDRM, -EIO)) + return r; + if (r != -EIDRM && journal_file_fstat(new_file) != -EIDRM) + return r; + + log_debug_errno(r, "Can't read selected entry from removed journal file '%s', ignoring: %m", + new_file->path); + remove_file_real(j, new_file); + + /* Removing the selected file guarantees that the next iteration makes progress. */ + continue; } - if (found) - new_file = f; + set_location(j, new_file, o); - /* Track the file that holds the cursor's exact entry (matching seqnum_id and seqnum). On - * systems without a reliable (or missing) RTC, compare_boot_ids() can produce incorrect - * cross-boot ordering causing compare_locations() above to prefer a wrong file. We detect - * this after the loop and override the choice if needed. - * - * See https://github.com/systemd/systemd/issues/31516 */ - if (j->current_location.type == LOCATION_SEEK && - j->current_location.seqnum_set && - sd_id128_equal(f->header->seqnum_id, j->current_location.seqnum_id) && - f->current_seqnum == j->current_location.seqnum) - exact_match = f; + return 1; } - - if (exact_match) - new_file = exact_match; - - if (!new_file) - return 0; - - r = journal_file_move_to_object(new_file, OBJECT_ENTRY, new_file->current_offset, &o); - if (r < 0) - return r; - - set_location(j, new_file, o); - - return 1; } _public_ int sd_journal_next(sd_journal *j) { diff --git a/src/libsystemd/sd-journal/test-journal-interleaving.c b/src/libsystemd/sd-journal/test-journal-interleaving.c index 8d3725d01e..1f74dce81c 100644 --- a/src/libsystemd/sd-journal/test-journal-interleaving.c +++ b/src/libsystemd/sd-journal/test-journal-interleaving.c @@ -3,20 +3,28 @@ #include #include +#if HAVE_VALGRIND_VALGRIND_H +# include +#endif + #include "sd-id128.h" #include "sd-journal.h" #include "alloc-util.h" #include "argv-util.h" #include "chattr-util.h" +#include "fd-util.h" +#include "hashmap.h" #include "iovec-util.h" #include "journal-file-util.h" +#include "journal-internal.h" #include "journal-vacuum.h" #include "log.h" #include "logs-show.h" #include "parse-util.h" #include "random-util.h" #include "rm-rf.h" +#include "sigbus.h" #include "strv.h" #include "tests.h" #include "time-util.h" @@ -447,6 +455,118 @@ TEST(skip) { test_skip_one(setup_interleaved); } +/* Read the first entry so that two.journal becomes the next candidate with LOCATION_SEEK, then simulate + * vacuuming removing it while it is still open and mapped. */ +static void test_remove_unlinked_selected_file_one(bool truncate, bool refresh_stat) { + _cleanup_(test_donep) char *t = NULL; + _cleanup_(sd_journal_closep) sd_journal *j = NULL; + _cleanup_close_ int fd = -EBADF; + uint8_t type = OBJECT_UNUSED; + JournalFile *f; + struct stat st; + + ASSERT_TRUE(!refresh_stat || truncate); + + mkdtemp_chdir_chattr("/var/tmp/journal-unlinked-XXXXXX", &t); + setup_interleaved(); + + ASSERT_OK(sd_journal_open_directory(&j, t, SD_JOURNAL_ASSUME_IMMUTABLE)); + ASSERT_OK(sd_journal_seek_head(j)); + ASSERT_OK_POSITIVE(sd_journal_next(j)); + test_check_number(j, 1); + + f = ASSERT_NOT_NULL(ordered_hashmap_get(j->files, strjoina(t, "/two.journal"))); + ASSERT_EQ((int) f->location_type, LOCATION_SEEK); + + if (truncate && !refresh_stat) + sigbus_install(); + + /* Keep the file open and mapped while the name goes away, like vacuuming does. */ + fd = ASSERT_OK_ERRNO(open("two.journal", O_WRONLY|O_CLOEXEC)); + ASSERT_OK_ERRNO(unlink("two.journal")); + + if (truncate) { + /* The lookup only stats the file when the selected entry falls outside the cached size, and + * journal_file_fstat() then reports the unlinked file as -EIDRM. Truncating to the selected + * offset and refreshing the stat takes that path. Truncating the whole file without + * refreshing leaves the cached size stale, so touching the mapping raises SIGBUS and the + * lookup fails with -EIO. */ + ASSERT_OK_ERRNO(ftruncate(fd, refresh_stat ? f->current_offset : 0)); + if (refresh_stat) + ASSERT_ERROR(journal_file_fstat(f), EIDRM); + else { + ASSERT_EQ(READ_NOW(f->header->state), STATE_OFFLINE); + ASSERT_TRUE(mmap_cache_fd_got_sigbus(f->cache_fd)); + } + } else { + /* Emulate a deallocated range, which reads back as zeroes, by zeroing the selected entry's + * object type. */ + ASSERT_OK_EQ_ERRNO(pwrite(fd, &type, sizeof(type), f->current_offset), (ssize_t) sizeof(type)); + ASSERT_OK_ERRNO(fsync(fd)); + } + + ASSERT_OK_ERRNO(fstat(f->fd, &st)); + ASSERT_EQ(st.st_nlink, 0U); + if (refresh_stat) + ASSERT_EQ((uint64_t) st.st_size, f->current_offset); + else if (truncate) + ASSERT_EQ(st.st_size, 0); + + ASSERT_OK_POSITIVE(sd_journal_next(j)); + if (truncate && !refresh_stat) + sigbus_reset(); + test_check_number(j, 3); + ASSERT_NULL(ordered_hashmap_get(j->files, strjoina(t, "/two.journal"))); +} + +TEST(remove_unlinked_selected_file) { + test_remove_unlinked_selected_file_one(/* truncate= */ false, /* refresh_stat= */ false); +} + +TEST(remove_truncated_unlinked_selected_file) { +#if HAS_FEATURE_ADDRESS_SANITIZER + return (void) log_tests_skipped("SIGBUS recovery cannot be tested under AddressSanitizer"); +#endif +#if HAVE_VALGRIND_VALGRIND_H + if (RUNNING_ON_VALGRIND) + return (void) log_tests_skipped("SIGBUS recovery cannot be tested under Valgrind"); +#endif + + test_remove_unlinked_selected_file_one(/* truncate= */ true, /* refresh_stat= */ false); +} + +TEST(remove_truncated_unlinked_selected_file_after_fstat) { + test_remove_unlinked_selected_file_one(/* truncate= */ true, /* refresh_stat= */ true); +} + +/* Corruption in a file that is still linked must stay visible to the caller. Whether libsystemd should + * skip such a file, or just the corrupt entry, is a separate question; this only pins today's behaviour. */ +TEST(keep_linked_selected_file_error) { + _cleanup_(test_donep) char *t = NULL; + _cleanup_(sd_journal_closep) sd_journal *j = NULL; + _cleanup_close_ int fd = -EBADF; + JournalFile *f; + uint8_t type = OBJECT_UNUSED; + + mkdtemp_chdir_chattr("/var/tmp/journal-linked-XXXXXX", &t); + setup_interleaved(); + + ASSERT_OK(sd_journal_open_directory(&j, t, SD_JOURNAL_ASSUME_IMMUTABLE)); + ASSERT_OK(sd_journal_seek_head(j)); + ASSERT_OK_POSITIVE(sd_journal_next(j)); + test_check_number(j, 1); + + f = ASSERT_NOT_NULL(ordered_hashmap_get(j->files, strjoina(t, "/two.journal"))); + ASSERT_EQ((int) f->location_type, LOCATION_SEEK); + + fd = ASSERT_OK_ERRNO(open("two.journal", O_WRONLY|O_CLOEXEC)); + ASSERT_OK_EQ_ERRNO(pwrite(fd, &type, sizeof(type), f->current_offset), (ssize_t) sizeof(type)); + ASSERT_OK_ERRNO(fsync(fd)); + + ASSERT_ERROR(sd_journal_next(j), EBADMSG); + ASSERT_NOT_NULL(ordered_hashmap_get(j->files, strjoina(t, "/two.journal"))); +} + static void test_boot_id_one(void (*setup)(void), size_t n_ids_expected) { _cleanup_(test_donep) char *t = NULL; _cleanup_(sd_journal_closep) sd_journal *j = NULL;