From 65b3903ff576488eaabb51d3c4fbf9c73d867d7c Mon Sep 17 00:00:00 2001 From: =?utf8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sat, 25 Jan 2014 23:35:28 -0500 Subject: [PATCH] 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 | 3 ++- Makefile.am | 7 +++++ man/sd_journal_print.xml | 31 +++++++++++++++++++--- src/journal/journal-send.c | 2 +- src/shared/def.h | 1 + src/shared/missing.h | 16 ++++++----- src/shared/util.c | 54 ++++++++++++++++++++++++++++++++++---- src/shared/util.h | 3 +++ src/test/test-tmpfiles.c | 51 +++++++++++++++++++++++++++++++++++ src/test/test-util.c | 26 ++++++++++++++++++ 10 files changed, 178 insertions(+), 16 deletions(-) create mode 100644 src/test/test-tmpfiles.c diff --git a/.gitignore b/.gitignore index 36b91b4a7..271f22546 100644 --- a/.gitignore +++ b/.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 diff --git a/Makefile.am b/Makefile.am index 23f7d2fba..49ac55fee 100644 --- a/Makefile.am +++ b/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 diff --git a/man/sd_journal_print.xml b/man/sd_journal_print.xml index a716cc35e..57d908fe2 100644 --- a/man/sd_journal_print.xml +++ b/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 - array. sd_journal_sendv() is + structures in the array. + sd_journal_sendv() is particularly useful to submit binary objects to the journal where that is necessary. @@ -220,6 +220,19 @@ sd_journal_send("MESSAGE=Hello World, this is PID %lu!", (unsigned long) getpid( variable itself is not altered. + + Async signal safety + sd_journal_sendv() is "async signal + safe" in the meaning of signal7. + + + sd_journal_print, + sd_journal_printv, + sd_journal_send, and + sd_journal_perror are + not async signal safe. + + Notes @@ -233,6 +246,16 @@ sd_journal_send("MESSAGE=Hello World, this is PID %lu!", (unsigned long) getpid( file. + + History + + sd_journal_sendv() 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. + + See Also @@ -243,7 +266,9 @@ sd_journal_send("MESSAGE=Hello World, this is PID %lu!", (unsigned long) getpid( syslog3, perror3, errno3, - systemd.journal-fields7 + systemd.journal-fields7, + signal7, + socket7 diff --git a/src/journal/journal-send.c b/src/journal/journal-send.c index ca9199f71..960c5776b 100644 --- a/src/journal/journal-send.c +++ b/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; diff --git a/src/shared/def.h b/src/shared/def.h index a2304fddd..7777756aa 100644 --- a/src/shared/def.h +++ b/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" diff --git a/src/shared/missing.h b/src/shared/missing.h index 6c038d9f0..4e6210003 100644 --- a/src/shared/missing.h +++ b/src/shared/missing.h @@ -301,25 +301,29 @@ static inline int name_to_handle_at(int fd, const char *name, struct file_handle #endif #ifndef CIFS_MAGIC_NUMBER -#define CIFS_MAGIC_NUMBER 0xFF534D42 +# define CIFS_MAGIC_NUMBER 0xFF534D42 #endif #ifndef TFD_TIMER_CANCEL_ON_SET -#define TFD_TIMER_CANCEL_ON_SET (1 << 1) +# define TFD_TIMER_CANCEL_ON_SET (1 << 1) #endif #ifndef SO_REUSEPORT -#define SO_REUSEPORT 15 +# define SO_REUSEPORT 15 #endif #ifndef EVIOCREVOKE -#define EVIOCREVOKE _IOW('E', 0x91, int) +# define EVIOCREVOKE _IOW('E', 0x91, int) #endif #ifndef DRM_IOCTL_SET_MASTER -#define DRM_IOCTL_SET_MASTER _IO('d', 0x1e) +# define DRM_IOCTL_SET_MASTER _IO('d', 0x1e) #endif #ifndef DRM_IOCTL_DROP_MASTER -#define DRM_IOCTL_DROP_MASTER _IO('d', 0x1f) +# define DRM_IOCTL_DROP_MASTER _IO('d', 0x1f) +#endif + +#ifndef TMP_MAX +# define TMP_MAX 238328 #endif diff --git a/src/shared/util.c b/src/shared/util.c index 27fc9594c..a437d9b12 100644 --- a/src/shared/util.c +++ b/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(p, O_RDWR|O_CLOEXEC); - } - + fd = mkostemp_safe(p, O_RDWR|O_CLOEXEC); if (fd < 0) - return -errno; + return fd; unlink(p); return fd; diff --git a/src/shared/util.h b/src/shared/util.h index 630137a53..1169864c3 100644 --- a/src/shared/util.h +++ b/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); diff --git a/src/test/test-tmpfiles.c b/src/test/test-tmpfiles.c new file mode 100644 index 000000000..f25a0dca5 --- /dev/null +++ b/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 . +***/ + +#include +#include +#include +#include +#include +#include + +#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; +} diff --git a/src/test/test-util.c b/src/test/test-util.c index 05b2294e2..f819589b5 100644 --- a/src/test/test-util.c +++ b/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; } -- 2.30.2