journal: guarantee async-signal-safety in sd_journald_sendv

signal(7) provides a list of functions which may be called from a
signal handler. Other functions, which only call those functions and
don't access global memory and are reentrant are also safe.
sd_j_sendv was mostly OK, but would call mkostemp and writev in a
fallback path, which are unsafe.

Being able to call sd_j_sendv in a async-signal-safe way is important
because it allows it be used in signal handlers.

Safety is achieved by replacing mkostemp with open(O_TMPFILE) and an
open-coded writev replacement which uses write. Unfortunately,
O_TMPFILE is only available on kernels >= 3.11. When O_TMPFILE is
unavailable, an open-coded mkostemp is used.

https://bugzilla.gnome.org/show_bug.cgi?id=722889

.gitignore

@@ -33,7 +33,7 @@
 /hostnamectl
 /install-tree
 /journalctl
-/libsystemd-login.c
+/libsystemd-*.c
 /libtool
 /localectl
 /loginctl
@@ -184,6 +184,7 @@
 /test-strxcpyx
 /test-tables
 /test-time
+/test-tmpfiles
 /test-udev
 /test-unit-file
 /test-unit-name

Makefile.am

@@ -1126,6 +1126,7 @@ tests += \
 	test-utf8 \
 	test-ellipsize \
 	test-util \
+	test-tmpfiles \
 	test-namespace \
 	test-date \
 	test-sleep \
@@ -1232,6 +1233,12 @@ test_util_SOURCES = \
 test_util_LDADD = \
 	libsystemd-core.la
 
+test_tmpfiles_SOURCES = \
+	src/test/test-tmpfiles.c
+
+test_tmpfiles_LDADD = \
+	libsystemd-shared.la
+
 test_namespace_SOURCES = \
 	src/test/test-namespace.c
 

man/sd_journal_print.xml

@@ -153,8 +153,8 @@
                 for details) instead of the format string. Each
                 structure should reference one field of the entry to
                 submit. The second argument specifies the number of
-                structures in the
+                structures in the array.
-                array. <function>sd_journal_sendv()</function> is
+                <function>sd_journal_sendv()</function> is
                 particularly useful to submit binary objects to the
                 journal where that is necessary.</para>
 
@@ -220,6 +220,19 @@ sd_journal_send("MESSAGE=Hello World, this is PID %lu!", (unsigned long) getpid(
                 variable itself is not altered.</para>
         </refsect1>
 
+        <refsect1>
+                <title>Async signal safety</title>
+                <para><function>sd_journal_sendv()</function> is "async signal
+                safe" in the meaning of <citerefentry><refentrytitle>signal</refentrytitle><manvolnum>7</manvolnum></citerefentry>.
+                </para>
+
+                <para><function>sd_journal_print</function>,
+                <function>sd_journal_printv</function>,
+                <function>sd_journal_send</function>, and
+                <function>sd_journal_perror</function> are
+                not async signal safe.</para>
+        </refsect1>
+
         <refsect1>
                 <title>Notes</title>
 
@@ -233,6 +246,16 @@ sd_journal_send("MESSAGE=Hello World, this is PID %lu!", (unsigned long) getpid(
                 file.</para>
         </refsect1>
 
+        <refsect1>
+                <title>History</title>
+
+                <para><function>sd_journal_sendv()</function> was
+                modified to guarantee async-signal-safety in
+                systemd-209. Before that, it would behave safely only
+                when entry size was small enough to fit in one (large)
+                datagram.</para>
+        </refsect1>
+
         <refsect1>
                 <title>See Also</title>
 
@@ -243,7 +266,9 @@ sd_journal_send("MESSAGE=Hello World, this is PID %lu!", (unsigned long) getpid(
                         <citerefentry><refentrytitle>syslog</refentrytitle><manvolnum>3</manvolnum></citerefentry>,
                         <citerefentry><refentrytitle>perror</refentrytitle><manvolnum>3</manvolnum></citerefentry>,
                         <citerefentry><refentrytitle>errno</refentrytitle><manvolnum>3</manvolnum></citerefentry>,
-                        <citerefentry><refentrytitle>systemd.journal-fields</refentrytitle><manvolnum>7</manvolnum></citerefentry>
+                        <citerefentry><refentrytitle>systemd.journal-fields</refentrytitle><manvolnum>7</manvolnum></citerefentry>,
+                        <citerefentry><refentrytitle>signal</refentrytitle><manvolnum>7</manvolnum></citerefentry>,
+                        <citerefentry><refentrytitle>socket</refentrytitle><manvolnum>7</manvolnum></citerefentry>
                 </para>
         </refsect1>
 

src/journal/journal-send.c

@@ -314,7 +314,7 @@ _public_ int sd_journal_sendv(const struct iovec *iov, int n) {
         if (buffer_fd < 0)
                 return buffer_fd;
 
-        n = writev(buffer_fd, w, j);
+        n = writev_safe(buffer_fd, w, j);
         if (n < 0) {
                 close_nointr_nofail(buffer_fd);
                 return -errno;

src/shared/def.h

@@ -44,6 +44,7 @@
 #define LOWERCASE_LETTERS "abcdefghijklmnopqrstuvwxyz"
 #define UPPERCASE_LETTERS "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
 #define LETTERS LOWERCASE_LETTERS UPPERCASE_LETTERS
+#define ALPHANUMERICAL LETTERS DIGITS
 
 #define REBOOT_PARAM_FILE "/run/systemd/reboot-param"
 

src/shared/missing.h

@@ -323,3 +323,7 @@ static inline int name_to_handle_at(int fd, const char *name, struct file_handle
 #ifndef DRM_IOCTL_DROP_MASTER
 #  define DRM_IOCTL_DROP_MASTER _IO('d', 0x1f)
 #endif
+
+#ifndef TMP_MAX
+# define TMP_MAX 238328
+#endif

src/shared/util.c

@@ -6109,6 +6109,53 @@ int getpeersec(int fd, char **ret) {
         return 0;
 }
 
+int writev_safe(int fd, const struct iovec *w, int j) {
+        for (int i = 0; i < j; i++) {
+                size_t written = 0;
+
+                while (written < w[i].iov_len) {
+                        ssize_t r;
+
+                        r = write(fd, (char*) w[i].iov_base + written, w[i].iov_len - written);
+                        if (r < 0 && errno != -EINTR)
+                                return -errno;
+
+                        written += r;
+                }
+        }
+
+        return 0;
+}
+
+int mkostemp_safe(char *pattern, int flags) {
+        char *s = pattern + strlen(pattern) - 6;
+        uint64_t tries = TMP_MAX;
+        int randfd, fd, i;
+
+        assert(streq(s, "XXXXXX"));
+
+        randfd = open("/dev/urandom", O_RDONLY);
+        if (randfd < 0)
+                return -ENOSYS;
+
+        while (tries--) {
+                fd = read(randfd, s, 6);
+                if (fd == 0)
+                        return -ENOSYS;
+
+                for (i = 0; i < 6; i++)
+                        s[i] = ALPHANUMERICAL[(unsigned) s[i] % strlen(ALPHANUMERICAL)];
+
+                fd = open(pattern, flags|O_EXCL|O_CREAT, S_IRUSR|S_IWUSR);
+                if (fd >= 0)
+                        return fd;
+                if (!IN_SET(errno, EEXIST, EINTR))
+                        return -errno;
+        }
+
+        return -EEXIST;
+}
+
 int open_tmpfile(const char *path, int flags) {
         int fd;
         char *p;
@@ -6120,12 +6167,9 @@ int open_tmpfile(const char *path, int flags) {
 #endif
         p = strappenda(path, "/systemd-tmp-XXXXXX");
 
-        RUN_WITH_UMASK(0077) {
+        fd = mkostemp_safe(p, O_RDWR|O_CLOEXEC);
-                fd = mkostemp(p, O_RDWR|O_CLOEXEC);
-        }
-
         if (fd < 0)
-                return -errno;
+                return fd;
 
         unlink(p);
         return fd;

src/shared/util.h

@@ -850,4 +850,7 @@ bool pid_valid(pid_t pid);
 int getpeercred(int fd, struct ucred *ucred);
 int getpeersec(int fd, char **ret);
 
+int writev_safe(int fd, const struct iovec *w, int j);
+
+int mkostemp_safe(char *pattern, int flags);
 int open_tmpfile(const char *path, int flags);

src/test/test-tmpfiles.c

@@ -0,0 +1,51 @@
+/*-*- Mode: C; c-basic-offset: 8; indent-tabs-mode: nil -*-*/
+
+/***
+  This file is part of systemd.
+
+  Copyright 2014 Zbigniew Jędrzejewski-Szmek
+
+  systemd is free software; you can redistribute it and/or modify it
+  under the terms of the GNU Lesser General Public License as published by
+  the Free Software Foundation; either version 2.1 of the License, or
+  (at your option) any later version.
+
+  systemd is distributed in the hope that it will be useful, but
+  WITHOUT ANY WARRANTY; without even the implied warranty of
+  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+  Lesser General Public License for more details.
+
+  You should have received a copy of the GNU Lesser General Public License
+  along with systemd; If not, see <http://www.gnu.org/licenses/>.
+***/
+
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include <unistd.h>
+#include <stdlib.h>
+#include <stdio.h>
+
+#include "util.h"
+
+int main(int argc, char** argv) {
+        const char *p = argv[1] ?: "/tmp";
+        char *pattern = strappenda(p, "/systemd-test-XXXXXX");
+        _cleanup_close_ int fd, fd2;
+        _cleanup_free_ char *cmd, *cmd2;
+
+        fd = open_tmpfile(p, O_RDWR);
+        assert(fd >= 0);
+
+        assert_se(asprintf(&cmd, "ls -l /proc/"PID_FMT"/fd/%d", getpid(), fd) > 0);
+        system(cmd);
+
+        fd2 = mkostemp_safe(pattern, O_RDWR);
+        assert(fd >= 0);
+        assert_se(unlink(pattern) == 0);
+
+        assert_se(asprintf(&cmd2, "ls -l /proc/"PID_FMT"/fd/%d", getpid(), fd2) > 0);
+        system(cmd2);
+
+        return 0;
+}

src/test/test-util.c

@@ -28,6 +28,8 @@
 
 #include "util.h"
 #include "strv.h"
+#include "def.h"
+#include "fileio.h"
 
 static void test_streq_ptr(void) {
         assert_se(streq_ptr(NULL, NULL));
@@ -578,6 +580,29 @@ static void test_in_set(void) {
         assert_se(!IN_SET(0, 1, 2, 3, 4));
 }
 
+static void test_writev_safe(void) {
+        char name[] = "/tmp/test-writev_safe.XXXXXX";
+        _cleanup_free_ char *contents;
+        size_t size;
+        int fd, r;
+
+        struct iovec iov[3];
+        IOVEC_SET_STRING(iov[0], "abc\n");
+        IOVEC_SET_STRING(iov[1], ALPHANUMERICAL "\n");
+        IOVEC_SET_STRING(iov[2], "");
+
+        fd = mkstemp(name);
+        printf("test_writev_safe: %s", name);
+
+        r = writev_safe(fd, iov, 3);
+        assert(r == 0);
+
+        r = read_full_file(name, &contents, &size);
+        assert(r == 0);
+        printf("contents: %s", contents);
+        assert(streq(contents, "abc\n" ALPHANUMERICAL "\n"));
+}
+
 int main(int argc, char *argv[]) {
         test_streq_ptr();
         test_first_word();
@@ -615,6 +640,7 @@ int main(int argc, char *argv[]) {
         test_fstab_node_to_udev_node();
         test_get_files_in_directory();
         test_in_set();
+        test_writev_safe();
 
         return 0;
 }