Merge pull request #15557 from poettering/journal-zero-fix

journal: deal better with reading from zeroed out journal mmaps

man/pam_systemd_home.xml

@@ -73,7 +73,12 @@
         the re-authentication must take place from a component running outside of the user's context, so that
         it does not require access to the user's home directory for operation. Traditionally, most desktop
         environments do not implement screen locking this way, and need to be updated
-        accordingly.</para></listitem>
+        accordingly.</para>
+
+        <para>This setting may also be controlled via the <varname>$SYSTEMD_HOME_SUSPEND</varname>
+        environment variable (see below), which <command>pam_systemd_home</command> reads during initialization and sets
+        for sessions. If both the environment variable is set and the module parameter specified the latter
+        takes precedence.</para></listitem>
       </varlistentry>
 
       <varlistentry>
@@ -105,6 +110,15 @@
         <listitem><para>Indicates that the user's home directory is managed by <filename>systemd-homed.service</filename>.</para></listitem>
       </varlistentry>
 
+      <varlistentry>
+        <term><varname>$SYSTEMD_HOME_SUSPEND=</varname></term>
+
+        <listitem><para>Indicates whether the session has been registered with the suspend mechanism enabled
+        or disabled (see above). The variable's value is either <literal>0</literal> or
+        <literal>1</literal>. Note that the module both reads the variable when initializing, and sets it for
+        sessions.</para></listitem>
+      </varlistentry>
+
     </variablelist>
   </refsect1>
 

man/sd_bus_add_match.xml

@@ -33,7 +33,7 @@
     <funcsynopsis>
       <funcsynopsisinfo>#include &lt;systemd/sd-bus.h&gt;</funcsynopsisinfo>
 
-      <funcprototype>
+      <funcprototype id="sd_bus_message_handler_t">
         <funcdef>typedef int (*<function>sd_bus_message_handler_t</function>)</funcdef>
         <paramdef>sd_bus_message *<parameter>m</parameter></paramdef>
         <paramdef>void *<parameter>userdata</parameter></paramdef>

man/sd_bus_add_object.xml

@@ -41,12 +41,7 @@
     <funcsynopsis>
       <funcsynopsisinfo>#include &lt;systemd/sd-bus-vtable.h&gt;</funcsynopsisinfo>
 
-      <funcprototype>
-        <funcdef>typedef int (*<function>sd_bus_message_handler_t</function>)</funcdef>
-        <paramdef>sd_bus_message *<parameter>m</parameter></paramdef>
-        <paramdef>void *<parameter>userdata</parameter></paramdef>
-        <paramdef>sd_bus_error *<parameter>ret_error</parameter></paramdef>
-      </funcprototype>
+      <xi:include href="sd_bus_add_match.xml" xpointer="sd_bus_message_handler_t"/>
 
       <funcprototype>
         <funcdef>typedef int (*<function>sd_bus_property_get_t</function>)</funcdef>

man/sd_bus_call.xml

@@ -27,6 +27,8 @@
     <funcsynopsis>
       <funcsynopsisinfo>#include &lt;systemd/sd-bus.h&gt;</funcsynopsisinfo>
 
+      <xi:include href="sd_bus_add_match.xml" xpointer="sd_bus_message_handler_t"/>
+
       <funcprototype>
         <funcdef>int <function>sd_bus_call</function></funcdef>
         <paramdef>sd_bus *<parameter>bus</parameter></paramdef>
@@ -59,32 +61,33 @@
     received a D-Bus error reply), <parameter>ret_error</parameter> is initialized to an instance of
     <structname>sd_bus_error</structname> describing the error.</para>
 
-    <para><function>sd_bus_call_async()</function> is like <function>sd_bus_call()</function> but
-    works asynchronously. The <parameter>callback</parameter> indicates the function to call when
-    the response arrives. The <parameter>userdata</parameter> pointer will be passed to the callback
-    function, and may be chosen freely by the caller. If <parameter>slot</parameter> is not
-    <constant>NULL</constant> and <function>sd_bus_call_async()</function> succeeds,
-    <parameter>slot</parameter> is set to a slot object which can be used to cancel the method call
-    at a later time using
+    <para><function>sd_bus_call_async()</function> is like <function>sd_bus_call()</function> but works
+    asynchronously. The <parameter>callback</parameter> indicates the function to call when the response
+    arrives. The <parameter>userdata</parameter> pointer will be passed to the callback function, and may be
+    chosen freely by the caller. If <parameter>slot</parameter> is not <constant>NULL</constant> and
+    <function>sd_bus_call_async()</function> succeeds, <parameter>slot</parameter> is set to a slot object
+    which can be used to cancel the method call at a later time using
     <citerefentry><refentrytitle>sd_bus_slot_unref</refentrytitle><manvolnum>3</manvolnum></citerefentry>.
-    If <parameter>slot</parameter> is <constant>NULL</constant>, the lifetime of the method call is
-    bound to the lifetime of the bus object itself, and it cannot be cancelled independently. See
+    If <parameter>slot</parameter> is <constant>NULL</constant>, the lifetime of the method call is bound to
+    the lifetime of the bus object itself, and it cannot be cancelled independently. See
     <citerefentry><refentrytitle>sd_bus_slot_set_floating</refentrytitle><manvolnum>3</manvolnum></citerefentry>
     for details. <parameter>callback</parameter> is called when a reply arrives with the reply,
-    <parameter>userdata</parameter> and an <structname>sd_bus_error</structname> output
-    parameter as its arguments. Unlike <function>sd_bus_call()</function>, the
-    <structname>sd_bus_error</structname> output parameter passed to the callback will be empty. To
-    determine whether the method call succeeded, use
+    <parameter>userdata</parameter> and an <structname>sd_bus_error</structname> output parameter as its
+    arguments. Unlike <function>sd_bus_call()</function>, the <structname>sd_bus_error</structname> output
+    parameter passed to the callback will be empty. To determine whether the method call succeeded, use
     <citerefentry><refentrytitle>sd_bus_message_is_method_error</refentrytitle><manvolnum>3</manvolnum></citerefentry>
     on the reply message passed to the callback instead. If the callback returns zero and the
-    <structname>sd_bus_error</structname> output parameter is still empty when the callback
-    inishes, other handlers registered with functions such as
+    <structname>sd_bus_error</structname> output parameter is still empty when the callback finishes, other
+    handlers registered with functions such as
     <citerefentry><refentrytitle>sd_bus_add_filter</refentrytitle><manvolnum>3</manvolnum></citerefentry> or
-    <citerefentry><refentrytitle>sd_bus_add_match</refentrytitle><manvolnum>3</manvolnum></citerefentry>
-    are given a chance to process the message. If the callback returns a non-zero value or the
-    <structname>sd_bus_error</structname> output parameter is not empty when the callback finishes,
-    no further processing of the message is done. Generally, you want to return zero from the
-    callback to give other registered handlers a chance to process the reply as well.</para>
+    <citerefentry><refentrytitle>sd_bus_add_match</refentrytitle><manvolnum>3</manvolnum></citerefentry> are
+    given a chance to process the message. If the callback returns a non-zero value or the
+    <structname>sd_bus_error</structname> output parameter is not empty when the callback finishes, no
+    further processing of the message is done. Generally, you want to return zero from the callback to give
+    other registered handlers a chance to process the reply as well. (Note that the
+    <structname>sd_bus_error</structname> parameter is an output parameter of the callback function, not an
+    input parameter; it can be used to propagate errors from the callback handler, it will not receive any
+    error that was received as method reply.)</para>
 
     <para>If <parameter>usec</parameter> is zero, the default D-Bus method call timeout is used. See
     <citerefentry><refentrytitle>sd_bus_get_method_call_timeout</refentrytitle><manvolnum>3</manvolnum></citerefentry>.

man/sd_bus_call_method.xml

@@ -30,6 +30,8 @@
     <funcsynopsis>
       <funcsynopsisinfo>#include &lt;systemd/sd-bus.h&gt;</funcsynopsisinfo>
 
+      <xi:include href="sd_bus_add_match.xml" xpointer="sd_bus_message_handler_t"/>
+
       <funcprototype>
         <funcdef>int <function>sd_bus_call_method</function></funcdef>
         <paramdef>sd_bus *<parameter>bus</parameter></paramdef>

man/sd_bus_get_current_handler.xml

@@ -28,6 +28,8 @@
     <funcsynopsis>
       <funcsynopsisinfo>#include &lt;systemd/sd-bus.h&gt;</funcsynopsisinfo>
 
+      <xi:include href="sd_bus_add_match.xml" xpointer="sd_bus_message_handler_t"/>
+
       <funcprototype>
         <funcdef>sd_bus_message_handler_t <function>sd_bus_get_current_handler</function></funcdef>
         <paramdef>sd_bus *<parameter>bus</parameter></paramdef>

man/sd_bus_request_name.xml

@@ -28,6 +28,8 @@
     <funcsynopsis>
       <funcsynopsisinfo>#include &lt;systemd/sd-bus.h&gt;</funcsynopsisinfo>
 
+      <xi:include href="sd_bus_add_match.xml" xpointer="sd_bus_message_handler_t"/>
+
       <funcprototype>
         <funcdef>int <function>sd_bus_request_name</function></funcdef>
         <paramdef>sd_bus *<parameter>bus</parameter></paramdef>

man/sd_bus_slot_get_bus.xml

@@ -27,6 +27,8 @@
     <funcsynopsis>
       <funcsynopsisinfo>#include &lt;systemd/sd-bus.h&gt;</funcsynopsisinfo>
 
+      <xi:include href="sd_bus_add_match.xml" xpointer="sd_bus_message_handler_t"/>
+
       <funcprototype>
         <funcdef>sd_bus *<function>sd_bus_slot_get_bus</function></funcdef>
         <paramdef>sd_bus_slot *<parameter>slot</parameter></paramdef>

meson.build

@@ -3368,6 +3368,8 @@ foreach tuple : sanitizers
                         if want_tests != 'false' and slow_tests
                                 test('@0@:@1@:@2@'.format(b, c, sanitizer),
                                      env,
+                                     env : ['UBSAN_OPTIONS=print_stacktrace=1:print_summary=1:halt_on_error=1'],
+                                     timeout : 60,
                                      args : [exe.full_path(),
                                              join_paths(project_source_root, p)])
                         endif

src/basic/macro.h

@@ -585,4 +585,17 @@ static inline int __coverity_check_and_return__(int condition) {
         DEFINE_PUBLIC_TRIVIAL_REF_FUNC(type, name);                    \
         DEFINE_PUBLIC_TRIVIAL_UNREF_FUNC(type, name, free_func);
 
+/* A macro to force copying of a variable from memory. This is useful whenever we want to read something from
+ * memory and want to make sure the compiler won't optimize away the destination variable for us. It's not
+ * supposed to be a full CPU memory barrier, i.e. CPU is still allowed to reorder the reads, but it is not
+ * allowed to remove our local copies of the variables. We want this to work for unaligned memory, hence
+ * memcpy() is great for our purposes. */
+#define READ_NOW(x)                                                     \
+        ({                                                              \
+                typeof(x) _copy;                                        \
+                memcpy(&_copy, &(x), sizeof(_copy));                    \
+                asm volatile ("" : : : "memory");                       \
+                _copy;                                                  \
+        })
+
 #include "log.h"

src/basic/memory-util.h

@@ -12,7 +12,7 @@
 
 size_t page_size(void) _pure_;
 #define PAGE_ALIGN(l) ALIGN_TO((l), page_size())
-#define PAGE_ALIGN_DOWN(l) (l & ~(page_size() - 1))
+#define PAGE_ALIGN_DOWN(l) ((l) & ~(page_size() - 1))
 
 /* Normal memcpy requires src to be nonnull. We do nothing if n is 0. */
 static inline void memcpy_safe(void *dst, const void *src, size_t n) {

src/home/pam_systemd_home.c

@@ -60,6 +60,35 @@ static int parse_argv(
         return 0;
 }
 
+static int parse_env(
+                pam_handle_t *handle,
+                bool *please_suspend) {
+
+        const char *v;
+        int r;
+
+        /* Let's read the suspend setting from an env var in addition to the PAM command line. That makes it
+         * easy to declare the features of a display manager in code rather than configuration, and this is
+         * really a feature of code */
+
+        v = pam_getenv(handle, "SYSTEMD_HOME_SUSPEND");
+        if (!v) {
+                /* Also check the process env block, so that people can control this via an env var from the
+                 * outside of our process. */
+                v = secure_getenv("SYSTEMD_HOME_SUSPEND");
+                if (!v)
+                        return 0;
+        }
+
+        r = parse_boolean(v);
+        if (r < 0)
+                pam_syslog(handle, LOG_WARNING, "Failed to parse $SYSTEMD_HOME_SUSPEND argument, ignoring: %s", v);
+        else if (please_suspend)
+                *please_suspend = r;
+
+        return 0;
+}
+
 static int acquire_user_record(
                 pam_handle_t *handle,
                 const char *username,
@@ -636,6 +665,9 @@ _public_ PAM_EXTERN int pam_sm_authenticate(
 
         bool debug = false, suspend_please = false;
 
+        if (parse_env(handle, &suspend_please) < 0)
+                return PAM_AUTH_ERR;
+
         if (parse_argv(handle,
                        argc, argv,
                        &suspend_please,
@@ -660,6 +692,9 @@ _public_ PAM_EXTERN int pam_sm_open_session(
         bool debug = false, suspend_please = false;
         int r;
 
+        if (parse_env(handle, &suspend_please) < 0)
+                return PAM_SESSION_ERR;
+
         if (parse_argv(handle,
                        argc, argv,
                        &suspend_please,
@@ -681,6 +716,12 @@ _public_ PAM_EXTERN int pam_sm_open_session(
                 return r;
         }
 
+        r = pam_putenv(handle, suspend_please ? "SYSTEMD_HOME_SUSPEND=1" : "SYSTEMD_HOME_SUSPEND=0");
+        if (r != PAM_SUCCESS) {
+                pam_syslog(handle, LOG_ERR, "Failed to set PAM environment variable $SYSTEMD_HOME_SUSPEND: %s", pam_strerror(handle, r));
+                return r;
+        }
+
         /* Let's release the D-Bus connection, after all the session might live quite a long time, and we are
          * not going to process the bus connection in that time, so let's better close before the daemon
          * kicks us off because we are not processing anything. */
@@ -764,6 +805,9 @@ _public_ PAM_EXTERN int pam_sm_acct_mgmt(
         usec_t t;
         int r;
 
+        if (parse_env(handle, &please_suspend) < 0)
+                return PAM_AUTH_ERR;
+
         if (parse_argv(handle,
                        argc, argv,
                        &please_suspend,

src/journal/journal-file.c

@@ -533,7 +533,7 @@ static int journal_file_verify_header(JournalFile *f) {
         if (f->header->state >= _STATE_MAX)
                 return -EBADMSG;
 
-        header_size = le64toh(f->header->header_size);
+        header_size = le64toh(READ_NOW(f->header->header_size));
 
         /* The first addition was n_data, so check that we are at least this large */
         if (header_size < HEADER_SIZE_MIN)
@@ -542,7 +542,7 @@ static int journal_file_verify_header(JournalFile *f) {
         if (JOURNAL_HEADER_SEALED(f->header) && !JOURNAL_HEADER_CONTAINS(f->header, n_entry_arrays))
                 return -EBADMSG;
 
-        arena_size = le64toh(f->header->arena_size);
+        arena_size = le64toh(READ_NOW(f->header->arena_size));
 
         if (UINT64_MAX - header_size < arena_size || header_size + arena_size > (uint64_t) f->last_stat.st_size)
                 return -ENODATA;
@@ -625,26 +625,29 @@ int journal_file_fstat(JournalFile *f) {
 }
 
 static int journal_file_allocate(JournalFile *f, uint64_t offset, uint64_t size) {
-        uint64_t old_size, new_size;
+        uint64_t old_size, new_size, old_header_size, old_arena_size;
         int r;
 
         assert(f);
         assert(f->header);
 
-        /* We assume that this file is not sparse, and we know that
-         * for sure, since we always call posix_fallocate()
-         * ourselves */
+        /* We assume that this file is not sparse, and we know that for sure, since we always call
+         * posix_fallocate() ourselves */
+
+        if (size > PAGE_ALIGN_DOWN(UINT64_MAX) - offset)
+                return -EINVAL;
 
         if (mmap_cache_got_sigbus(f->mmap, f->cache_fd))
                 return -EIO;
 
-        old_size =
-                le64toh(f->header->header_size) +
-                le64toh(f->header->arena_size);
+        old_header_size = le64toh(READ_NOW(f->header->header_size));
+        old_arena_size = le64toh(READ_NOW(f->header->arena_size));
+        if (old_arena_size > PAGE_ALIGN_DOWN(UINT64_MAX) - old_header_size)
+                return -EBADMSG;
 
-        new_size = PAGE_ALIGN(offset + size);
-        if (new_size < le64toh(f->header->header_size))
-                new_size = le64toh(f->header->header_size);
+        old_size = old_header_size + old_arena_size;
+
+        new_size = MAX(PAGE_ALIGN(offset + size), old_header_size);
 
         if (new_size <= old_size) {
 
@@ -690,7 +693,7 @@ static int journal_file_allocate(JournalFile *f, uint64_t offset, uint64_t size)
         if (r != 0)
                 return -r;
 
-        f->header->arena_size = htole64(new_size - le64toh(f->header->header_size));
+        f->header->arena_size = htole64(new_size - old_header_size);
 
         return journal_file_fstat(f);
 }
@@ -702,7 +705,15 @@ static unsigned type_to_context(ObjectType type) {
         return type > OBJECT_UNUSED && type < _OBJECT_TYPE_MAX ? type : 0;
 }
 
-static int journal_file_move_to(JournalFile *f, ObjectType type, bool keep_always, uint64_t offset, uint64_t size, void **ret, size_t *ret_size) {
+static int journal_file_move_to(
+                JournalFile *f,
+                ObjectType type,
+                bool keep_always,
+                uint64_t offset,
+                uint64_t size,
+                void **ret,
+                size_t *ret_size) {
+
         int r;
 
         assert(f);
@@ -711,6 +722,9 @@ static int journal_file_move_to(JournalFile *f, ObjectType type, bool keep_alway
         if (size <= 0)
                 return -EINVAL;
 
+        if (size > UINT64_MAX - offset)
+                return -EBADMSG;
+
         /* Avoid SIGBUS on invalid accesses */
         if (offset + size > (uint64_t) f->last_stat.st_size) {
                 /* Hmm, out of range? Let's refresh the fstat() data
@@ -760,7 +774,7 @@ static int journal_file_check_object(JournalFile *f, uint64_t offset, Object *o)
                                                le64toh(o->data.n_entries),
                                                offset);
 
-                if (le64toh(o->object.size) - offsetof(DataObject, payload) <= 0)
+                if (le64toh(o->object.size) <= offsetof(DataObject, payload))
                         return log_debug_errno(SYNTHETIC_ERRNO(EBADMSG),
                                                "Bad object size (<= %zu): %" PRIu64 ": %" PRIu64,
                                                offsetof(DataObject, payload),
@@ -782,7 +796,7 @@ static int journal_file_check_object(JournalFile *f, uint64_t offset, Object *o)
                 break;
 
         case OBJECT_FIELD:
-                if (le64toh(o->object.size) - offsetof(FieldObject, payload) <= 0)
+                if (le64toh(o->object.size) <= offsetof(FieldObject, payload))
                         return log_debug_errno(SYNTHETIC_ERRNO(EBADMSG),
                                                "Bad field size (<= %zu): %" PRIu64 ": %" PRIu64,
                                                offsetof(FieldObject, payload),
@@ -798,18 +812,22 @@ static int journal_file_check_object(JournalFile *f, uint64_t offset, Object *o)
                                                offset);
                 break;
 
-        case OBJECT_ENTRY:
-                if ((le64toh(o->object.size) - offsetof(EntryObject, items)) % sizeof(EntryItem) != 0)
+        case OBJECT_ENTRY: {
+                uint64_t sz;
+
+                sz = le64toh(READ_NOW(o->object.size));
+                if (sz < offsetof(EntryObject, items) ||
+                    (sz - offsetof(EntryObject, items)) % sizeof(EntryItem) != 0)
                         return log_debug_errno(SYNTHETIC_ERRNO(EBADMSG),
                                                "Bad entry size (<= %zu): %" PRIu64 ": %" PRIu64,
                                                offsetof(EntryObject, items),
-                                               le64toh(o->object.size),
+                                               sz,
                                                offset);
 
-                if ((le64toh(o->object.size) - offsetof(EntryObject, items)) / sizeof(EntryItem) <= 0)
+                if ((sz - offsetof(EntryObject, items)) / sizeof(EntryItem) <= 0)
                         return log_debug_errno(SYNTHETIC_ERRNO(EBADMSG),
                                                "Invalid number items in entry: %" PRIu64 ": %" PRIu64,
-                                               (le64toh(o->object.size) - offsetof(EntryObject, items)) / sizeof(EntryItem),
+                                               (sz - offsetof(EntryObject, items)) / sizeof(EntryItem),
                                                offset);
 
                 if (le64toh(o->entry.seqnum) <= 0)
@@ -831,25 +849,35 @@ static int journal_file_check_object(JournalFile *f, uint64_t offset, Object *o)
                                                offset);
 
                 break;
+        }
 
         case OBJECT_DATA_HASH_TABLE:
-        case OBJECT_FIELD_HASH_TABLE:
-                if ((le64toh(o->object.size) - offsetof(HashTableObject, items)) % sizeof(HashItem) != 0 ||
-                    (le64toh(o->object.size) - offsetof(HashTableObject, items)) / sizeof(HashItem) <= 0)
+        case OBJECT_FIELD_HASH_TABLE: {
+                uint64_t sz;
+
+                sz = le64toh(READ_NOW(o->object.size));
+                if (sz < offsetof(HashTableObject, items) ||
+                    (sz - offsetof(HashTableObject, items)) % sizeof(HashItem) != 0 ||
+                    (sz - offsetof(HashTableObject, items)) / sizeof(HashItem) <= 0)
                         return log_debug_errno(SYNTHETIC_ERRNO(EBADMSG),
                                                "Invalid %s hash table size: %" PRIu64 ": %" PRIu64,
                                                o->object.type == OBJECT_DATA_HASH_TABLE ? "data" : "field",
-                                               le64toh(o->object.size),
+                                               sz,
                                                offset);
 
                 break;
+        }
 
-        case OBJECT_ENTRY_ARRAY:
-                if ((le64toh(o->object.size) - offsetof(EntryArrayObject, items)) % sizeof(le64_t) != 0 ||
-                    (le64toh(o->object.size) - offsetof(EntryArrayObject, items)) / sizeof(le64_t) <= 0)
+        case OBJECT_ENTRY_ARRAY: {
+                uint64_t sz;
+
+                sz = le64toh(READ_NOW(o->object.size));
+                if (sz < offsetof(EntryArrayObject, items) ||
+                    (sz - offsetof(EntryArrayObject, items)) % sizeof(le64_t) != 0 ||
+                    (sz - offsetof(EntryArrayObject, items)) / sizeof(le64_t) <= 0)
                         return log_debug_errno(SYNTHETIC_ERRNO(EBADMSG),
                                                "Invalid object entry array size: %" PRIu64 ": %" PRIu64,
-                                               le64toh(o->object.size),
+                                               sz,
                                                offset);
 
                 if (!VALID64(le64toh(o->entry_array.next_entry_array_offset)))
@@ -859,6 +887,7 @@ static int journal_file_check_object(JournalFile *f, uint64_t offset, Object *o)
                                                offset);
 
                 break;
+        }
 
         case OBJECT_TAG:
                 if (le64toh(o->object.size) != sizeof(TagObject))
@@ -905,7 +934,7 @@ int journal_file_move_to_object(JournalFile *f, ObjectType type, uint64_t offset
                 return r;
 
         o = (Object*) t;
-        s = le64toh(o->object.size);
+        s = le64toh(READ_NOW(o->object.size));
 
         if (s == 0)
                 return log_debug_errno(SYNTHETIC_ERRNO(EBADMSG),
@@ -995,11 +1024,21 @@ int journal_file_append_object(JournalFile *f, ObjectType type, uint64_t size, O
         if (p == 0)
                 p = le64toh(f->header->header_size);
         else {
+                uint64_t sz;
+
                 r = journal_file_move_to_object(f, OBJECT_UNUSED, p, &tail);
                 if (r < 0)
                         return r;
 
-                p += ALIGN64(le64toh(tail->object.size));
+                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;
         }
 
         r = journal_file_allocate(f, p, size);
@@ -1011,10 +1050,10 @@ int journal_file_append_object(JournalFile *f, ObjectType type, uint64_t size, O
                 return r;
 
         o = (Object*) t;
-
-        zero(o->object);
-        o->object.type = type;
-        o->object.size = htole64(size);
+        o->object = (ObjectHeader) {
+                .type = type,
+                .size = htole64(size),
+        };
 
         f->header->tail_object_offset = htole64(p);
         f->header->n_objects = htole64(le64toh(f->header->n_objects) + 1);
@@ -1156,7 +1195,7 @@ static int journal_file_link_field(
         if (o->object.type != OBJECT_FIELD)
                 return -EINVAL;
 
-        m = le64toh(f->header->field_hash_table_size) / sizeof(HashItem);
+        m = le64toh(READ_NOW(f->header->field_hash_table_size)) / sizeof(HashItem);
         if (m <= 0)
                 return -EBADMSG;
 
@@ -1201,7 +1240,7 @@ static int journal_file_link_data(
         if (o->object.type != OBJECT_DATA)
                 return -EINVAL;
 
-        m = le64toh(f->header->data_hash_table_size) / sizeof(HashItem);
+        m = le64toh(READ_NOW(f->header->data_hash_table_size)) / sizeof(HashItem);
         if (m <= 0)
                 return -EBADMSG;
 
@@ -1257,7 +1296,7 @@ int journal_file_find_field_object_with_hash(
 
         osize = offsetof(Object, field.payload) + size;
 
-        m = le64toh(f->header->field_hash_table_size) / sizeof(HashItem);
+        m = le64toh(READ_NOW(f->header->field_hash_table_size)) / sizeof(HashItem);
         if (m <= 0)
                 return -EBADMSG;
 
@@ -1329,7 +1368,7 @@ int journal_file_find_data_object_with_hash(
 
         osize = offsetof(Object, data.payload) + size;
 
-        m = le64toh(f->header->data_hash_table_size) / sizeof(HashItem);
+        m = le64toh(READ_NOW(f->header->data_hash_table_size)) / sizeof(HashItem);
         if (m <= 0)
                 return -EBADMSG;
 
@@ -1351,7 +1390,7 @@ int journal_file_find_data_object_with_hash(
                         uint64_t l;
                         size_t rsize = 0;
 
-                        l = le64toh(o->object.size);
+                        l = le64toh(READ_NOW(o->object.size));
                         if (l <= offsetof(Object, data.payload))
                                 return -EBADMSG;
 
@@ -1576,30 +1615,47 @@ static int journal_file_append_data(
 }
 
 uint64_t journal_file_entry_n_items(Object *o) {
+        uint64_t sz;
         assert(o);
 
         if (o->object.type != OBJECT_ENTRY)
                 return 0;
 
-        return (le64toh(o->object.size) - offsetof(Object, entry.items)) / sizeof(EntryItem);
+        sz = le64toh(READ_NOW(o->object.size));
+        if (sz < offsetof(Object, entry.items))
+                return 0;
+
+        return (sz - offsetof(Object, entry.items)) / sizeof(EntryItem);
 }
 
 uint64_t journal_file_entry_array_n_items(Object *o) {
+        uint64_t sz;
+
         assert(o);
 
         if (o->object.type != OBJECT_ENTRY_ARRAY)
                 return 0;
 
-        return (le64toh(o->object.size) - offsetof(Object, entry_array.items)) / sizeof(uint64_t);
+        sz = le64toh(READ_NOW(o->object.size));
+        if (sz < offsetof(Object, entry_array.items))
+                return 0;
+
+        return (sz - offsetof(Object, entry_array.items)) / sizeof(uint64_t);
 }
 
 uint64_t journal_file_hash_table_n_items(Object *o) {
+        uint64_t sz;
+
         assert(o);
 
         if (!IN_SET(o->object.type, OBJECT_DATA_HASH_TABLE, OBJECT_FIELD_HASH_TABLE))
                 return 0;
 
-        return (le64toh(o->object.size) - offsetof(Object, hash_table.items)) / sizeof(HashItem);
+        sz = le64toh(READ_NOW(o->object.size));
+        if (sz < offsetof(Object, hash_table.items))
+                return 0;
+
+        return (sz - offsetof(Object, hash_table.items)) / sizeof(HashItem);
 }
 
 static int link_entry_into_array(JournalFile *f,
@@ -1617,7 +1673,7 @@ static int link_entry_into_array(JournalFile *f,
         assert(p > 0);
 
         a = le64toh(*first);
-        i = hidx = le64toh(*idx);
+        i = hidx = le64toh(READ_NOW(*idx));
         while (a > 0) {
 
                 r = journal_file_move_to_object(f, OBJECT_ENTRY_ARRAY, a, &o);
@@ -1682,6 +1738,7 @@ static int link_entry_into_array_plus_one(JournalFile *f,
                                           le64_t *idx,
                                           uint64_t p) {
 
+        uint64_t hidx;
         int r;
 
         assert(f);
@@ -1690,32 +1747,33 @@ static int link_entry_into_array_plus_one(JournalFile *f,
         assert(idx);
         assert(p > 0);
 
-        if (*idx == 0)
+        hidx = le64toh(READ_NOW(*idx));
+        if (hidx == UINT64_MAX)
+                return -EBADMSG;
+        if (hidx == 0)
                 *extra = htole64(p);
         else {
                 le64_t i;
 
-                i = htole64(le64toh(*idx) - 1);
+                i = htole64(hidx - 1);
                 r = link_entry_into_array(f, first, &i, p);
                 if (r < 0)
                         return r;
         }
 
-        *idx = htole64(le64toh(*idx) + 1);
+        *idx = htole64(hidx + 1);
         return 0;
 }
 
 static int journal_file_link_entry_item(JournalFile *f, Object *o, uint64_t offset, uint64_t i) {
         uint64_t p;
         int r;
+
         assert(f);
         assert(o);
         assert(offset > 0);
 
         p = le64toh(o->entry.items[i].object_offset);
-        if (p == 0)
-                return -EINVAL;
-
         r = journal_file_move_to_object(f, OBJECT_DATA, p, &o);
         if (r < 0)
                 return r;
@@ -2435,6 +2493,7 @@ _pure_ static int test_object_offset(JournalFile *f, uint64_t p, uint64_t needle
 }
 
 static int test_object_seqnum(JournalFile *f, uint64_t p, uint64_t needle) {
+        uint64_t sq;
         Object *o;
         int r;
 
@@ -2445,9 +2504,10 @@ static int test_object_seqnum(JournalFile *f, uint64_t p, uint64_t needle) {
         if (r < 0)
                 return r;
 
-        if (le64toh(o->entry.seqnum) == needle)
+        sq = le64toh(READ_NOW(o->entry.seqnum));
+        if (sq == needle)
                 return TEST_FOUND;
-        else if (le64toh(o->entry.seqnum) < needle)
+        else if (sq < needle)
                 return TEST_LEFT;
         else
                 return TEST_RIGHT;
@@ -2473,6 +2533,7 @@ int journal_file_move_to_entry_by_seqnum(
 
 static int test_object_realtime(JournalFile *f, uint64_t p, uint64_t needle) {
         Object *o;
+        uint64_t rt;
         int r;
 
         assert(f);
@@ -2482,9 +2543,10 @@ static int test_object_realtime(JournalFile *f, uint64_t p, uint64_t needle) {
         if (r < 0)
                 return r;
 
-        if (le64toh(o->entry.realtime) == needle)
+        rt = le64toh(READ_NOW(o->entry.realtime));
+        if (rt == needle)
                 return TEST_FOUND;
-        else if (le64toh(o->entry.realtime) < needle)
+        else if (rt < needle)
                 return TEST_LEFT;
         else
                 return TEST_RIGHT;
@@ -2510,6 +2572,7 @@ int journal_file_move_to_entry_by_realtime(
 
 static int test_object_monotonic(JournalFile *f, uint64_t p, uint64_t needle) {
         Object *o;
+        uint64_t m;
         int r;
 
         assert(f);
@@ -2519,9 +2582,10 @@ static int test_object_monotonic(JournalFile *f, uint64_t p, uint64_t needle) {
         if (r < 0)
                 return r;
 
-        if (le64toh(o->entry.monotonic) == needle)
+        m = le64toh(READ_NOW(o->entry.monotonic));
+        if (m == needle)
                 return TEST_FOUND;
-        else if (le64toh(o->entry.monotonic) < needle)
+        else if (m < needle)
                 return TEST_LEFT;
         else
                 return TEST_RIGHT;
@@ -2679,7 +2743,7 @@ int journal_file_next_entry(
         assert(f);
         assert(f->header);
 
-        n = le64toh(f->header->n_entries);
+        n = le64toh(READ_NOW(f->header->n_entries));
         if (n <= 0)
                 return 0;
 
@@ -2752,7 +2816,7 @@ int journal_file_next_entry_for_data(
         if (r < 0)
                 return r;
 
-        n = le64toh(d->data.n_entries);
+        n = le64toh(READ_NOW(d->data.n_entries));
         if (n <= 0)
                 return n;
 
@@ -2981,7 +3045,7 @@ void journal_file_dump(JournalFile *f) {
 
         journal_file_print_header(f);
 
-        p = le64toh(f->header->header_size);
+        p = le64toh(READ_NOW(f->header->header_size));
         while (p != 0) {
                 r = journal_file_move_to_object(f, OBJECT_UNUSED, p, &o);
                 if (r < 0)
@@ -3038,7 +3102,7 @@ void journal_file_dump(JournalFile *f) {
                 if (p == le64toh(f->header->tail_object_offset))
                         p = 0;
                 else
-                        p = p + ALIGN64(le64toh(o->object.size));
+                        p += ALIGN64(le64toh(o->object.size));
         }
 
         return;
@@ -3659,7 +3723,11 @@ int journal_file_copy_entry(JournalFile *from, JournalFile *to, Object *o, uint6
                 if (le_hash != o->data.hash)
                         return -EBADMSG;
 
-                l = le64toh(o->object.size) - offsetof(Object, data.payload);
+                l = le64toh(READ_NOW(o->object.size));
+                if (l < offsetof(Object, data.payload))
+                        return -EBADMSG;
+
+                l -= offsetof(Object, data.payload);
                 t = (size_t) l;
 
                 /* We hit the limit on 32bit machines */

src/journal/journal-internal.h

@@ -41,10 +41,10 @@ struct Match {
 struct Location {
         LocationType type;
 
-        bool seqnum_set;
-        bool realtime_set;
-        bool monotonic_set;
-        bool xor_hash_set;
+        bool seqnum_set:1;
+        bool realtime_set:1;
+        bool monotonic_set:1;
+        bool xor_hash_set:1;
 
         uint64_t seqnum;
         sd_id128_t seqnum_id;

src/journal/sd-journal.c

@@ -115,28 +115,24 @@ static void detach_location(sd_journal *j) {
                 journal_file_reset_location(f);
 }
 
-static void reset_location(sd_journal *j) {
-        assert(j);
-
-        detach_location(j);
-        zero(j->current_location);
-}
-
 static void init_location(Location *l, LocationType type, JournalFile *f, Object *o) {
         assert(l);
         assert(IN_SET(type, LOCATION_DISCRETE, LOCATION_SEEK));
         assert(f);
-        assert(o->object.type == OBJECT_ENTRY);
 
-        l->type = type;
-        l->seqnum = le64toh(o->entry.seqnum);
-        l->seqnum_id = f->header->seqnum_id;
-        l->realtime = le64toh(o->entry.realtime);
-        l->monotonic = le64toh(o->entry.monotonic);
-        l->boot_id = o->entry.boot_id;
-        l->xor_hash = le64toh(o->entry.xor_hash);
-
-        l->seqnum_set = l->realtime_set = l->monotonic_set = l->xor_hash_set = true;
+        *l = (Location) {
+                .type = type,
+                .seqnum = le64toh(o->entry.seqnum),
+                .seqnum_id = f->header->seqnum_id,
+                .realtime = le64toh(o->entry.realtime),
+                .monotonic = le64toh(o->entry.monotonic),
+                .boot_id = o->entry.boot_id,
+                .xor_hash = le64toh(o->entry.xor_hash),
+                .seqnum_set = true,
+                .realtime_set = true,
+                .monotonic_set = true,
+                .xor_hash_set = true,
+        };
 }
 
 static void set_location(sd_journal *j, JournalFile *f, Object *o) {
@@ -1014,9 +1010,10 @@ _public_ int sd_journal_seek_cursor(sd_journal *j, const char *cursor) {
             !realtime_set)
                 return -EINVAL;
 
-        reset_location(j);
-
-        j->current_location.type = LOCATION_SEEK;
+        detach_location(j);
+        j->current_location = (Location) {
+                .type = LOCATION_SEEK,
+        };
 
         if (realtime_set) {
                 j->current_location.realtime = (uint64_t) realtime;
@@ -1129,11 +1126,14 @@ _public_ int sd_journal_seek_monotonic_usec(sd_journal *j, sd_id128_t boot_id, u
         assert_return(j, -EINVAL);
         assert_return(!journal_pid_changed(j), -ECHILD);
 
-        reset_location(j);
-        j->current_location.type = LOCATION_SEEK;
-        j->current_location.boot_id = boot_id;
-        j->current_location.monotonic = usec;
-        j->current_location.monotonic_set = true;
+        detach_location(j);
+
+        j->current_location = (Location) {
+                .type = LOCATION_SEEK,
+                .boot_id = boot_id,
+                .monotonic = usec,
+                .monotonic_set = true,
+        };
 
         return 0;
 }
@@ -1142,10 +1142,13 @@ _public_ int sd_journal_seek_realtime_usec(sd_journal *j, uint64_t usec) {
         assert_return(j, -EINVAL);
         assert_return(!journal_pid_changed(j), -ECHILD);
 
-        reset_location(j);
-        j->current_location.type = LOCATION_SEEK;
-        j->current_location.realtime = usec;
-        j->current_location.realtime_set = true;
+        detach_location(j);
+
+        j->current_location = (Location) {
+                .type = LOCATION_SEEK,
+                .realtime = usec,
+                .realtime_set = true,
+        };
 
         return 0;
 }
@@ -1154,8 +1157,11 @@ _public_ int sd_journal_seek_head(sd_journal *j) {
         assert_return(j, -EINVAL);
         assert_return(!journal_pid_changed(j), -ECHILD);
 
-        reset_location(j);
-        j->current_location.type = LOCATION_HEAD;
+        detach_location(j);
+
+        j->current_location = (Location) {
+                .type = LOCATION_HEAD,
+        };
 
         return 0;
 }
@@ -1164,8 +1170,11 @@ _public_ int sd_journal_seek_tail(sd_journal *j) {
         assert_return(j, -EINVAL);
         assert_return(!journal_pid_changed(j), -ECHILD);
 
-        reset_location(j);
-        j->current_location.type = LOCATION_TAIL;
+        detach_location(j);
+
+        j->current_location = (Location) {
+                .type = LOCATION_TAIL,
+        };
 
         return 0;
 }
@@ -2357,7 +2366,10 @@ static int return_data(sd_journal *j, JournalFile *f, Object *o, const void **da
         uint64_t l;
         int compression;
 
-        l = le64toh(o->object.size) - offsetof(Object, data.payload);
+        l = le64toh(READ_NOW(o->object.size));
+        if (l < offsetof(Object, data.payload))
+                return -EBADMSG;
+        l -= offsetof(Object, data.payload);
         t = (size_t) l;
 
         /* We can't read objects larger than 4G on a 32bit machine */

src/login/pam_systemd.c

@@ -647,10 +647,6 @@ _public_ PAM_EXTERN int pam_sm_open_session(
 
         assert(handle);
 
-        /* Make this a NOP on non-logind systems */
-        if (!logind_running())
-                return PAM_SUCCESS;
-
         if (parse_argv(handle,
                        argc, argv,
                        &class_pam,
@@ -666,6 +662,10 @@ _public_ PAM_EXTERN int pam_sm_open_session(
         if (r != PAM_SUCCESS)
                 return r;
 
+        /* Make most of this a NOP on non-logind systems */
+        if (!logind_running())
+                goto success;
+
         /* Make sure we don't enter a loop by talking to
          * systemd-logind when it is actually waiting for the
          * background to finish start-up. If the service is
@@ -689,11 +689,7 @@ _public_ PAM_EXTERN int pam_sm_open_session(
                 if (r != PAM_SUCCESS)
                         return r;
 
-                r = apply_user_record_settings(handle, ur, debug);
-                if (r != PAM_SUCCESS)
-                        return r;
-
-                return PAM_SUCCESS;
+                goto success;
         }
 
         /* Otherwise, we ask logind to create a session for us */
@@ -847,7 +843,9 @@ _public_ PAM_EXTERN int pam_sm_open_session(
                 if (sd_bus_error_has_name(&error, BUS_ERROR_SESSION_BUSY)) {
                         if (debug)
                                 pam_syslog(handle, LOG_DEBUG, "Not creating session: %s", bus_error_message(&error, r));
-                        return PAM_SUCCESS;
+
+                        /* We are already in a session, don't do anything */
+                        goto success;
                 } else {
                         pam_syslog(handle, LOG_ERR, "Failed to create session: %s", bus_error_message(&error, r));
                         return PAM_SESSION_ERR;
@@ -944,6 +942,7 @@ _public_ PAM_EXTERN int pam_sm_open_session(
                 }
         }
 
+success:
         r = apply_user_record_settings(handle, ur, debug);
         if (r != PAM_SUCCESS)
                 return r;

test/fuzz/meson.build

@@ -1,17 +1,17 @@
 # SPDX-License-Identifier: LGPL-2.1+
 
-sanitize_address = custom_target(
-        'sanitize-address-fuzzers',
-        output : 'sanitize-address-fuzzers',
+sanitize_address_undefined = custom_target(
+        'sanitize-address-undefined-fuzzers',
+        output : 'sanitize-address-undefined-fuzzers',
         command : [meson_build_sh,
                    project_source_root,
                    '@OUTPUT@',
                    'fuzzers',
-                   '-Db_lundef=false -Db_sanitize=address',
+                   '-Db_lundef=false -Db_sanitize=address,undefined',
                    ' '.join(cc.cmd_array()),
                    cxx_cmd])
 
-sanitizers = [['address', sanitize_address]]
+sanitizers = [['address,undefined', sanitize_address_undefined]]
 
 if git.found()
         out = run_command(

travis-ci/managers/fuzzit.sh

@@ -22,11 +22,10 @@ cd $REPO_ROOT
 export PATH="$HOME/.local/bin/:$PATH"
 
 # We use a subset of https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html#available-checks instead of "undefined"
-# because our fuzzers crash with "pointer-overflow" and "float-cast-overflow":
-# https://github.com/systemd/systemd/pull/12771#issuecomment-502139157
+# because our fuzzers crash with "float-cast-overflow":
 # https://github.com/systemd/systemd/pull/12812#issuecomment-502780455
 # TODO: figure out what to do about unsigned-integer-overflow: https://github.com/google/oss-fuzz/issues/910
-export SANITIZER="address -fsanitize=alignment,array-bounds,bool,bounds,builtin,enum,float-divide-by-zero,function,integer-divide-by-zero,nonnull-attribute,null,object-size,return,returns-nonnull-attribute,shift,signed-integer-overflow,unreachable,unsigned-integer-overflow,vla-bound,vptr -fno-sanitize-recover=alignment,array-bounds,bool,bounds,builtin,enum,float-divide-by-zero,function,integer-divide-by-zero,nonnull-attribute,null,object-size,return,returns-nonnull-attribute,shift,signed-integer-overflow,unreachable,vla-bound,vptr"
+export SANITIZER="address -fsanitize=alignment,array-bounds,bool,bounds,builtin,enum,float-divide-by-zero,function,integer-divide-by-zero,nonnull-attribute,null,object-size,pointer-overflow,return,returns-nonnull-attribute,shift,signed-integer-overflow,unreachable,unsigned-integer-overflow,vla-bound,vptr -fno-sanitize-recover=alignment,array-bounds,bool,bounds,builtin,enum,float-divide-by-zero,function,integer-divide-by-zero,nonnull-attribute,null,object-size,pointer-overflow,return,returns-nonnull-attribute,shift,signed-integer-overflow,unreachable,vla-bound,vptr"
 tools/oss-fuzz.sh
 
 FUZZING_TYPE=${1:-regression}