[systemd-devel] [RFC PATCH 1/2] Replace mkostemp+unlink with open(O_TMPFILE)
Lucas De Marchi
lucas.de.marchi at gmail.com
Mon Jan 27 05:52:48 PST 2014
On Sun, Jan 26, 2014 at 3:21 AM, Zbigniew Jędrzejewski-Szmek
<zbyszek at in.waw.pl> wrote:
> This will only work on Linux >= 3.11, and probably not on all
> filesystems. Fallback code is provided.
> ---
> Hi,
>
> because on bug https://bugzilla.gnome.org/show_bug.cgi?id=722889, I
> was looking into async signal safety of the journal logging functions.
> All that do any formatting are unsafe, but sd_journal_sendv *almost*
> is. Currently it calls mkostemp and writev, but only in the fallback
> path. So for the purpose of simple logging without multi-megabyte
> messages it already is safe. But it would be nice to turn this into an
> explicit guarantee. When O_TMPFILE is not available, it is hard to
> make it generally safe, so I thought it would nice to provide a mode
> where it simply fails when safety cannot be guaranteed. Because
> sd_j_sendv does not take any flags, this is done by changing the
> length parameter to mean that negative means signal-safe. So on
> newer kernels, everything should just work and the function should be
> async-signal-safe when a negative parameter is used. On old kernels,
> it would cleanly fail for large messages. I think this is a worthy goal,
> and being able to call structured logging from signal handlers should
> lead to better diagnostics in the long run.
>
> Comments? It would be great if somebody could look over sd_j_sendv to
> verify my assumptions that everything in it is signal safe apart from
> the stuff mentioned above.
>
> Zbyszek
>
> src/core/manager.c | 17 +++--------------
> src/journal/journal-send.c | 20 +++++++-------------
> src/journal/journal-verify.c | 12 +++---------
> src/shared/util.c | 22 ++++++++++++++++++++++
> src/shared/util.h | 2 ++
> 5 files changed, 37 insertions(+), 36 deletions(-)
>
> diff --git a/src/core/manager.c b/src/core/manager.c
> index 95fc7e6..7156c38 100644
> --- a/src/core/manager.c
> +++ b/src/core/manager.c
> @@ -1985,28 +1985,17 @@ void manager_dispatch_bus_name_owner_changed(
> }
>
> int manager_open_serialization(Manager *m, FILE **_f) {
> - _cleanup_free_ char *path = NULL;
> + const char *path;
> int fd = -1;
> FILE *f;
>
> assert(_f);
>
> - if (m->running_as == SYSTEMD_SYSTEM)
> - asprintf(&path, "/run/systemd/dump-"PID_FMT"-XXXXXX", getpid());
> - else
> - asprintf(&path, "/tmp/systemd-dump-"PID_FMT"-XXXXXX", getpid());
> -
> - if (!path)
> - return -ENOMEM;
> -
> - RUN_WITH_UMASK(0077) {
> - fd = mkostemp(path, O_RDWR|O_CLOEXEC);
> - }
> -
> + path = m->running_as == SYSTEMD_SYSTEM ? "/run/systemd" : "/tmp";
> + fd = open_tmpfile(path, O_RDWR|O_CLOEXEC);
> if (fd < 0)
> return -errno;
>
> - unlink(path);
> log_debug("Serializing state to %s", path);
>
> f = fdopen(fd, "w+");
> diff --git a/src/journal/journal-send.c b/src/journal/journal-send.c
> index 281e154..ca9199f 100644
> --- a/src/journal/journal-send.c
> +++ b/src/journal/journal-send.c
> @@ -216,10 +216,6 @@ _public_ int sd_journal_sendv(const struct iovec *iov, int n) {
> uint8_t buf[CMSG_SPACE(sizeof(int))];
> } control;
> struct cmsghdr *cmsg;
> - /* We use /dev/shm instead of /tmp here, since we want this to
> - * be a tmpfs, and one that is available from early boot on
> - * and where unprivileged users can create files. */
> - char path[] = "/dev/shm/journal.XXXXXX";
> bool have_syslog_identifier = false;
>
> assert_return(iov, -EINVAL);
> @@ -309,16 +305,14 @@ _public_ int sd_journal_sendv(const struct iovec *iov, int n) {
>
> /* Message doesn't fit... Let's dump the data in a temporary
> * file and just pass a file descriptor of it to the other
> - * side */
> -
> - buffer_fd = mkostemp(path, O_CLOEXEC|O_RDWR);
> + * side.
> + *
> + * We use /dev/shm instead of /tmp here, since we want this to
> + * be a tmpfs, and one that is available from early boot on
> + * and where unprivileged users can create files. */
> + buffer_fd = open_tmpfile("/dev/shm", O_RDWR | O_CLOEXEC);
> if (buffer_fd < 0)
> - return -errno;
> -
> - if (unlink(path) < 0) {
> - close_nointr_nofail(buffer_fd);
> - return -errno;
> - }
> + return buffer_fd;
>
> n = writev(buffer_fd, w, j);
> if (n < 0) {
> diff --git a/src/journal/journal-verify.c b/src/journal/journal-verify.c
> index f2422ff..9434cc9 100644
> --- a/src/journal/journal-verify.c
> +++ b/src/journal/journal-verify.c
> @@ -785,9 +785,6 @@ int journal_file_verify(
> uint64_t n_weird = 0, n_objects = 0, n_entries = 0, n_data = 0, n_fields = 0, n_data_hash_tables = 0, n_field_hash_tables = 0, n_entry_arrays = 0, n_tags = 0;
> usec_t last_usec = 0;
> int data_fd = -1, entry_fd = -1, entry_array_fd = -1;
> - char data_path[] = "/var/tmp/journal-data-XXXXXX",
> - entry_path[] = "/var/tmp/journal-entry-XXXXXX",
> - entry_array_path[] = "/var/tmp/journal-entry-array-XXXXXX";
> unsigned i;
> bool found_last;
> #ifdef HAVE_GCRYPT
> @@ -808,29 +805,26 @@ int journal_file_verify(
> } else if (f->seal)
> return -ENOKEY;
>
> - data_fd = mkostemp(data_path, O_CLOEXEC);
> + data_fd = open_tmpfile("/var/tmp", O_RDWR | O_CLOEXEC);
> if (data_fd < 0) {
> log_error("Failed to create data file: %m");
> r = -errno;
> goto fail;
> }
> - unlink(data_path);
>
> - entry_fd = mkostemp(entry_path, O_CLOEXEC);
> + entry_fd = open_tmpfile("/var/tmp", O_RDWR | O_CLOEXEC);
> if (entry_fd < 0) {
> log_error("Failed to create entry file: %m");
> r = -errno;
> goto fail;
> }
> - unlink(entry_path);
>
> - entry_array_fd = mkostemp(entry_array_path, O_CLOEXEC);
> + entry_array_fd = open_tmpfile("/var/tmp", O_RDWR | O_CLOEXEC);
> if (entry_array_fd < 0) {
> log_error("Failed to create entry array file: %m");
> r = -errno;
> goto fail;
> }
> - unlink(entry_array_path);
>
> #ifdef HAVE_GCRYPT
> if ((le32toh(f->header->compatible_flags) & ~HEADER_COMPATIBLE_SEALED) != 0)
> diff --git a/src/shared/util.c b/src/shared/util.c
> index 5551714..34b3d96 100644
> --- a/src/shared/util.c
> +++ b/src/shared/util.c
> @@ -6105,3 +6105,25 @@ int getpeersec(int fd, char **ret) {
> *ret = s;
> return 0;
> }
> +
> +int open_tmpfile(const char *path, int flags) {
> + int fd;
> + char *p;
> +
> +#ifdef O_TMPFILE
> + fd = open(path, flags | O_TMPFILE, S_IRUSR | S_IWUSR);
> + if (fd >= 0)
> + return fd;
> +#endif
> + p = strappenda(path, "/systemd-tmp-XXXXXX");
> +
> + RUN_WITH_UMASK(0077) {
> + fd = mkostemp(p, O_RDWR|O_CLOEXEC);
> + }
> +
> + if (fd < 0)
> + return -errno;
> +
> + unlink(path);
This unlink() doesn't make much sense with O_TMPFILE.
Lucas De Marchi
More information about the systemd-devel
mailing list