From fdeea3f4f1c0f78f1014582135d047265098fb82 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 4 Apr 2019 10:17:16 +0200 Subject: [PATCH 1/6] Add fopen_unlocked() wrapper --- coccinelle/fopen-unlocked.cocci | 37 ++++++++++++++++++ src/basic/cgroup-util.c | 26 ++++++------- src/basic/fileio.c | 40 ++++++++++++------- src/basic/fileio.h | 2 + src/basic/mountpoint-util.c | 8 ++-- src/basic/process-util.c | 56 ++++++++++----------------- src/cryptsetup/cryptsetup-generator.c | 7 +--- src/shared/mount-util.c | 18 ++++----- 8 files changed, 108 insertions(+), 86 deletions(-) create mode 100644 coccinelle/fopen-unlocked.cocci diff --git a/coccinelle/fopen-unlocked.cocci b/coccinelle/fopen-unlocked.cocci new file mode 100644 index 0000000000..93b993dd55 --- /dev/null +++ b/coccinelle/fopen-unlocked.cocci @@ -0,0 +1,37 @@ +@@ +expression f, path, options; +@@ +- f = fopen(path, options); +- if (!f) +- return -errno; +- (void) __fsetlocking(f, FSETLOCKING_BYCALLER); ++ r = fopen_unlocked(path, options, &f); ++ if (r < 0) ++ return r; +@@ +expression f, path, options; +@@ +- f = fopen(path, options); +- if (!f) { +- if (errno == ENOENT) +- return -ESRCH; +- return -errno; +- } +- (void) __fsetlocking(f, FSETLOCKING_BYCALLER); ++ r = fopen_unlocked(path, options, &f); ++ if (r == -ENOENT) ++ return -ESRCH; ++ if (r < 0) ++ return r; +@@ +expression f, path, options; +@@ +- f = fopen(path, options); +- if (!f) +- return errno == ENOENT ? -ESRCH : -errno; +- (void) __fsetlocking(f, FSETLOCKING_BYCALLER); ++ r = fopen_unlocked(path, options, &f); ++ if (r == -ENOENT) ++ return -ESRCH; ++ if (r < 0) ++ return r; diff --git a/src/basic/cgroup-util.c b/src/basic/cgroup-util.c index fc28109db8..11b4e3fce1 100644 --- a/src/basic/cgroup-util.c +++ b/src/basic/cgroup-util.c @@ -1016,11 +1016,11 @@ int cg_pid_get_path(const char *controller, pid_t pid, char **path) { } fs = procfs_file_alloca(pid, "cgroup"); - f = fopen(fs, "re"); - if (!f) - return errno == ENOENT ? -ESRCH : -errno; - - (void) __fsetlocking(f, FSETLOCKING_BYCALLER); + r = fopen_unlocked(fs, "re", &f); + if (r == -ENOENT) + return -ESRCH; + if (r < 0) + return r; for (;;) { _cleanup_free_ char *line = NULL; @@ -2442,17 +2442,13 @@ int cg_kernel_controllers(Set **ret) { if (!controllers) return -ENOMEM; - f = fopen("/proc/cgroups", "re"); - if (!f) { - if (errno == ENOENT) { - *ret = NULL; - return 0; - } - - return -errno; + r = fopen_unlocked("/proc/cgroups", "re", &f); + if (r == -ENOENT) { + *ret = NULL; + return 0; } - - (void) __fsetlocking(f, FSETLOCKING_BYCALLER); + if (r < 0) + return r; /* Ignore the header line */ (void) read_line(f, (size_t) -1, NULL); diff --git a/src/basic/fileio.c b/src/basic/fileio.c index 9ab2f501c7..4599440b45 100644 --- a/src/basic/fileio.c +++ b/src/basic/fileio.c @@ -29,6 +29,19 @@ #define READ_FULL_BYTES_MAX (4U*1024U*1024U) +int fopen_unlocked(const char *path, const char *options, FILE **ret) { + assert(ret); + + FILE *f = fopen(path, options); + if (!f) + return -errno; + + (void) __fsetlocking(f, FSETLOCKING_BYCALLER); + + *ret = f; + return 0; +} + int write_string_stream_ts( FILE *f, const char *line, @@ -213,15 +226,14 @@ int write_string_filef( int read_one_line_file(const char *fn, char **line) { _cleanup_fclose_ FILE *f = NULL; + int r; assert(fn); assert(line); - f = fopen(fn, "re"); - if (!f) - return -errno; - - (void) __fsetlocking(f, FSETLOCKING_BYCALLER); + r = fopen_unlocked(fn, "re", &f); + if (r < 0) + return r; return read_line(f, LONG_LINE_MAX, line); } @@ -230,6 +242,7 @@ int verify_file(const char *fn, const char *blob, bool accept_extra_nl) { _cleanup_fclose_ FILE *f = NULL; _cleanup_free_ char *buf = NULL; size_t l, k; + int r; assert(fn); assert(blob); @@ -243,11 +256,9 @@ int verify_file(const char *fn, const char *blob, bool accept_extra_nl) { if (!buf) return -ENOMEM; - f = fopen(fn, "re"); - if (!f) - return -errno; - - (void) __fsetlocking(f, FSETLOCKING_BYCALLER); + r = fopen_unlocked(fn, "re", &f); + if (r < 0) + return r; /* We try to read one byte more than we need, so that we know whether we hit eof */ errno = 0; @@ -390,15 +401,14 @@ finalize: int read_full_file_full(const char *filename, ReadFullFileFlags flags, char **contents, size_t *size) { _cleanup_fclose_ FILE *f = NULL; + int r; assert(filename); assert(contents); - f = fopen(filename, "re"); - if (!f) - return -errno; - - (void) __fsetlocking(f, FSETLOCKING_BYCALLER); + r = fopen_unlocked(filename, "re", &f); + if (r < 0) + return r; return read_full_stream_full(f, filename, flags, contents, size); } diff --git a/src/basic/fileio.h b/src/basic/fileio.h index 760e738688..bee3353439 100644 --- a/src/basic/fileio.h +++ b/src/basic/fileio.h @@ -33,6 +33,8 @@ typedef enum { READ_FULL_FILE_UNBASE64 = 1 << 1, } ReadFullFileFlags; +int fopen_unlocked(const char *path, const char *options, FILE **ret); + int write_string_stream_ts(FILE *f, const char *line, WriteStringFileFlags flags, struct timespec *ts); static inline int write_string_stream(FILE *f, const char *line, WriteStringFileFlags flags) { return write_string_stream_ts(f, line, flags, NULL); diff --git a/src/basic/mountpoint-util.c b/src/basic/mountpoint-util.c index 1e946a0bb6..48494320fd 100644 --- a/src/basic/mountpoint-util.c +++ b/src/basic/mountpoint-util.c @@ -378,11 +378,9 @@ int dev_is_devtmpfs(void) { if (r < 0) return r; - proc_self_mountinfo = fopen("/proc/self/mountinfo", "re"); - if (!proc_self_mountinfo) - return -errno; - - (void) __fsetlocking(proc_self_mountinfo, FSETLOCKING_BYCALLER); + r = fopen_unlocked("/proc/self/mountinfo", "re", &proc_self_mountinfo); + if (r < 0) + return r; for (;;) { _cleanup_free_ char *line = NULL; diff --git a/src/basic/process-util.c b/src/basic/process-util.c index f773eeaffd..568f400d97 100644 --- a/src/basic/process-util.c +++ b/src/basic/process-util.c @@ -106,7 +106,7 @@ int get_process_cmdline(pid_t pid, size_t max_length, bool comm_fallback, char * char *k; _cleanup_free_ char *ans = NULL; const char *p; - int c; + int c, r; assert(line); assert(pid >= 0); @@ -121,15 +121,11 @@ int get_process_cmdline(pid_t pid, size_t max_length, bool comm_fallback, char * * comm_fallback is false). Returns 0 and sets *line otherwise. */ p = procfs_file_alloca(pid, "cmdline"); - - f = fopen(p, "re"); - if (!f) { - if (errno == ENOENT) - return -ESRCH; - return -errno; - } - - (void) __fsetlocking(f, FSETLOCKING_BYCALLER); + r = fopen_unlocked(p, "re", &f); + if (r == -ENOENT) + return -ESRCH; + if (r < 0) + return r; if (max_length == 0) { /* This is supposed to be a safety guard against runaway command lines. */ @@ -513,14 +509,11 @@ static int get_process_id(pid_t pid, const char *field, uid_t *uid) { return -EINVAL; p = procfs_file_alloca(pid, "status"); - f = fopen(p, "re"); - if (!f) { - if (errno == ENOENT) - return -ESRCH; - return -errno; - } - - (void) __fsetlocking(f, FSETLOCKING_BYCALLER); + r = fopen_unlocked(p, "re", &f); + if (r == -ENOENT) + return -ESRCH; + if (r < 0) + return r; for (;;) { _cleanup_free_ char *line = NULL; @@ -602,14 +595,11 @@ int get_process_environ(pid_t pid, char **env) { p = procfs_file_alloca(pid, "environ"); - f = fopen(p, "re"); - if (!f) { - if (errno == ENOENT) - return -ESRCH; - return -errno; - } - - (void) __fsetlocking(f, FSETLOCKING_BYCALLER); + r = fopen_unlocked(p, "re", &f); + if (r == -ENOENT) + return -ESRCH; + if (r < 0) + return r; for (;;) { char c; @@ -895,15 +885,11 @@ int getenv_for_pid(pid_t pid, const char *field, char **ret) { path = procfs_file_alloca(pid, "environ"); - f = fopen(path, "re"); - if (!f) { - if (errno == ENOENT) - return -ESRCH; - - return -errno; - } - - (void) __fsetlocking(f, FSETLOCKING_BYCALLER); + r = fopen_unlocked(path, "re", &f); + if (r == -ENOENT) + return -ESRCH; + if (r < 0) + return r; l = strlen(field); for (;;) { diff --git a/src/cryptsetup/cryptsetup-generator.c b/src/cryptsetup/cryptsetup-generator.c index e9b21689c7..fbb443c6e2 100644 --- a/src/cryptsetup/cryptsetup-generator.c +++ b/src/cryptsetup/cryptsetup-generator.c @@ -2,7 +2,6 @@ #include #include -#include #include #include @@ -479,15 +478,13 @@ static int add_crypttab_devices(void) { if (!arg_read_crypttab) return 0; - f = fopen("/etc/crypttab", "re"); - if (!f) { + r = fopen_unlocked("/etc/crypttab", "re", &f); + if (r < 0) { if (errno != ENOENT) log_error_errno(errno, "Failed to open /etc/crypttab: %m"); return 0; } - (void) __fsetlocking(f, FSETLOCKING_BYCALLER); - if (fstat(fileno(f), &st) < 0) { log_error_errno(errno, "Failed to stat /etc/crypttab: %m"); return 0; diff --git a/src/shared/mount-util.c b/src/shared/mount-util.c index 9987b6f80c..1b50716731 100644 --- a/src/shared/mount-util.c +++ b/src/shared/mount-util.c @@ -40,13 +40,10 @@ int umount_recursive(const char *prefix, int flags) { _cleanup_fclose_ FILE *proc_self_mountinfo = NULL; again = false; - r = 0; - proc_self_mountinfo = fopen("/proc/self/mountinfo", "re"); - if (!proc_self_mountinfo) - return -errno; - - (void) __fsetlocking(proc_self_mountinfo, FSETLOCKING_BYCALLER); + r = fopen_unlocked("/proc/self/mountinfo", "re", &proc_self_mountinfo); + if (r < 0) + return r; for (;;) { _cleanup_free_ char *path = NULL, *p = NULL; @@ -302,12 +299,11 @@ int bind_remount_recursive_with_mountinfo( int bind_remount_recursive(const char *prefix, unsigned long new_flags, unsigned long flags_mask, char **blacklist) { _cleanup_fclose_ FILE *proc_self_mountinfo = NULL; + int r; - proc_self_mountinfo = fopen("/proc/self/mountinfo", "re"); - if (!proc_self_mountinfo) - return -errno; - - (void) __fsetlocking(proc_self_mountinfo, FSETLOCKING_BYCALLER); + r = fopen_unlocked("/proc/self/mountinfo", "re", &proc_self_mountinfo); + if (r < 0) + return r; return bind_remount_recursive_with_mountinfo(prefix, new_flags, flags_mask, blacklist, proc_self_mountinfo); } From 41f6e627d7cfdf1ea50d5adbd7e118589dbcf8db Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 4 Apr 2019 11:02:11 +0200 Subject: [PATCH 2/6] Make fopen_temporary and fopen_temporary_label unlocked This is partially a refactoring, but also makes many more places use unlocked operations implicitly, i.e. all users of fopen_temporary(). AFAICT, the uses are always for short-lived files which are not shared externally, and are just used within the same context. Locking is not necessary. --- coccinelle/fopen-unlocked.cocci | 14 ++++++++++++++ src/basic/cgroup-util.c | 1 - src/basic/env-file.c | 3 --- src/basic/fileio.c | 11 ++++------- src/basic/mountpoint-util.c | 1 - src/basic/process-util.c | 1 - src/basic/tmpfile-util.c | 7 +++++++ src/core/dbus-service.c | 1 - src/fstab-generator/fstab-generator.c | 1 - src/libsystemd-network/sd-dhcp-lease.c | 2 -- src/locale/keymap-util.c | 2 -- src/login/logind-seat.c | 2 -- src/login/logind-session.c | 2 -- src/login/logind-user.c | 2 -- src/machine/machine.c | 2 -- src/network/networkd-link.c | 2 -- src/network/networkd-manager.c | 2 -- src/resolve/resolved-link.c | 2 -- src/resolve/resolved-resolv-conf.c | 3 --- src/shared/generator.c | 14 ++++++-------- src/shared/mount-util.c | 1 - 21 files changed, 31 insertions(+), 45 deletions(-) diff --git a/coccinelle/fopen-unlocked.cocci b/coccinelle/fopen-unlocked.cocci index 93b993dd55..e6f2bc5681 100644 --- a/coccinelle/fopen-unlocked.cocci +++ b/coccinelle/fopen-unlocked.cocci @@ -35,3 +35,17 @@ expression f, path, options; + return -ESRCH; + if (r < 0) + return r; +@@ +expression f, path, p; +@@ + r = fopen_temporary(path, &f, &p); + if (r < 0) + return ...; +- (void) __fsetlocking(f, FSETLOCKING_BYCALLER); +@@ +expression f, g, path, p; +@@ + r = fopen_temporary_label(path, g, &f, &p); + if (r < 0) + return ...; +- (void) __fsetlocking(f, FSETLOCKING_BYCALLER); diff --git a/src/basic/cgroup-util.c b/src/basic/cgroup-util.c index 11b4e3fce1..210089688b 100644 --- a/src/basic/cgroup-util.c +++ b/src/basic/cgroup-util.c @@ -6,7 +6,6 @@ #include #include #include -#include #include #include #include diff --git a/src/basic/env-file.c b/src/basic/env-file.c index a1f1308a54..83767b0a24 100644 --- a/src/basic/env-file.c +++ b/src/basic/env-file.c @@ -1,7 +1,5 @@ /* SPDX-License-Identifier: LGPL-2.1+ */ -#include - #include "alloc-util.h" #include "env-file.h" #include "env-util.h" @@ -545,7 +543,6 @@ int write_env_file(const char *fname, char **l) { if (r < 0) return r; - (void) __fsetlocking(f, FSETLOCKING_BYCALLER); (void) fchmod_umask(fileno(f), 0644); STRV_FOREACH(i, l) diff --git a/src/basic/fileio.c b/src/basic/fileio.c index 4599440b45..2318f407b8 100644 --- a/src/basic/fileio.c +++ b/src/basic/fileio.c @@ -108,7 +108,6 @@ static int write_string_file_atomic( if (r < 0) return r; - (void) __fsetlocking(f, FSETLOCKING_BYCALLER); (void) fchmod_umask(fileno(f), 0644); r = write_string_stream_ts(f, line, flags, ts); @@ -154,11 +153,9 @@ int write_string_file_ts( assert(!ts); if (flags & WRITE_STRING_FILE_CREATE) { - f = fopen(fn, "we"); - if (!f) { - r = -errno; + r = fopen_unlocked(fn, "we", &f); + if (r < 0) goto fail; - } } else { int fd; @@ -176,9 +173,9 @@ int write_string_file_ts( safe_close(fd); goto fail; } - } - (void) __fsetlocking(f, FSETLOCKING_BYCALLER); + (void) __fsetlocking(f, FSETLOCKING_BYCALLER); + } if (flags & WRITE_STRING_FILE_DISABLE_BUFFER) setvbuf(f, NULL, _IONBF, 0); diff --git a/src/basic/mountpoint-util.c b/src/basic/mountpoint-util.c index 48494320fd..5ac9293167 100644 --- a/src/basic/mountpoint-util.c +++ b/src/basic/mountpoint-util.c @@ -2,7 +2,6 @@ #include #include -#include #include #include "alloc-util.h" diff --git a/src/basic/process-util.c b/src/basic/process-util.c index 568f400d97..3dc3534e1a 100644 --- a/src/basic/process-util.c +++ b/src/basic/process-util.c @@ -8,7 +8,6 @@ #include #include #include -#include #include #include #include diff --git a/src/basic/tmpfile-util.c b/src/basic/tmpfile-util.c index bc92d6a6de..260443a1d6 100644 --- a/src/basic/tmpfile-util.c +++ b/src/basic/tmpfile-util.c @@ -1,5 +1,7 @@ /* SPDX-License-Identifier: LGPL-2.1+ */ +#include +#include #include #include "alloc-util.h" @@ -37,6 +39,9 @@ int fopen_temporary(const char *path, FILE **_f, char **_temp_path) { return -errno; } + /* This assumes that returned FILE object is short-lived and used within the same single-threaded + * context and never shared externally, hence locking is not necessary. */ + f = fdopen(fd, "w"); if (!f) { unlink_noerrno(t); @@ -45,6 +50,8 @@ int fopen_temporary(const char *path, FILE **_f, char **_temp_path) { return -errno; } + (void) __fsetlocking(f, FSETLOCKING_BYCALLER); + *_f = f; *_temp_path = t; diff --git a/src/core/dbus-service.c b/src/core/dbus-service.c index 0f563a6625..4e6709c42e 100644 --- a/src/core/dbus-service.c +++ b/src/core/dbus-service.c @@ -1,6 +1,5 @@ /* SPDX-License-Identifier: LGPL-2.1+ */ -#include #include #include "alloc-util.h" diff --git a/src/fstab-generator/fstab-generator.c b/src/fstab-generator/fstab-generator.c index d35f2f993e..28ae48d551 100644 --- a/src/fstab-generator/fstab-generator.c +++ b/src/fstab-generator/fstab-generator.c @@ -5,7 +5,6 @@ #include #include #include -#include #include "alloc-util.h" #include "fd-util.h" diff --git a/src/libsystemd-network/sd-dhcp-lease.c b/src/libsystemd-network/sd-dhcp-lease.c index a16314a9d3..c089b4278b 100644 --- a/src/libsystemd-network/sd-dhcp-lease.c +++ b/src/libsystemd-network/sd-dhcp-lease.c @@ -6,7 +6,6 @@ #include #include #include -#include #include #include #include @@ -832,7 +831,6 @@ int dhcp_lease_save(sd_dhcp_lease *lease, const char *lease_file) { if (r < 0) goto fail; - (void) __fsetlocking(f, FSETLOCKING_BYCALLER); (void) fchmod(fileno(f), 0644); fprintf(f, diff --git a/src/locale/keymap-util.c b/src/locale/keymap-util.c index b8bd181c16..e238e5a124 100644 --- a/src/locale/keymap-util.c +++ b/src/locale/keymap-util.c @@ -1,7 +1,6 @@ /* SPDX-License-Identifier: LGPL-2.1+ */ #include -#include #include #include #include @@ -423,7 +422,6 @@ int x11_write_data(Context *c) { if (r < 0) return r; - (void) __fsetlocking(f, FSETLOCKING_BYCALLER); (void) fchmod(fileno(f), 0644); fputs("# Written by systemd-localed(8), read by systemd-localed and Xorg. It's\n" diff --git a/src/login/logind-seat.c b/src/login/logind-seat.c index b4904c37d5..f5ffb68238 100644 --- a/src/login/logind-seat.c +++ b/src/login/logind-seat.c @@ -2,7 +2,6 @@ #include #include -#include #include #include #include @@ -97,7 +96,6 @@ int seat_save(Seat *s) { if (r < 0) goto fail; - (void) __fsetlocking(f, FSETLOCKING_BYCALLER); (void) fchmod(fileno(f), 0644); fprintf(f, diff --git a/src/login/logind-session.c b/src/login/logind-session.c index 3d3bc8ab1c..f1efeb0e01 100644 --- a/src/login/logind-session.c +++ b/src/login/logind-session.c @@ -5,7 +5,6 @@ #include #include #include -#include #include #include #include @@ -214,7 +213,6 @@ int session_save(Session *s) { if (r < 0) goto fail; - (void) __fsetlocking(f, FSETLOCKING_BYCALLER); (void) fchmod(fileno(f), 0644); fprintf(f, diff --git a/src/login/logind-user.c b/src/login/logind-user.c index 045b6f0e17..8356a9089a 100644 --- a/src/login/logind-user.c +++ b/src/login/logind-user.c @@ -3,7 +3,6 @@ #include #include #include -#include #include "alloc-util.h" #include "bus-common-errors.h" @@ -162,7 +161,6 @@ static int user_save_internal(User *u) { if (r < 0) goto fail; - (void) __fsetlocking(f, FSETLOCKING_BYCALLER); (void) fchmod(fileno(f), 0644); fprintf(f, diff --git a/src/machine/machine.c b/src/machine/machine.c index 84454ddd86..b916d038d7 100644 --- a/src/machine/machine.c +++ b/src/machine/machine.c @@ -3,7 +3,6 @@ #include #include #include -#include #include #include "sd-messages.h" @@ -126,7 +125,6 @@ int machine_save(Machine *m) { if (r < 0) goto fail; - (void) __fsetlocking(f, FSETLOCKING_BYCALLER); (void) fchmod(fileno(f), 0644); fprintf(f, diff --git a/src/network/networkd-link.c b/src/network/networkd-link.c index 3e334c8d29..4846b13a8b 100644 --- a/src/network/networkd-link.c +++ b/src/network/networkd-link.c @@ -4,7 +4,6 @@ #include #include #include -#include #include "alloc-util.h" #include "bus-util.h" @@ -4034,7 +4033,6 @@ int link_save(Link *link) { if (r < 0) goto fail; - (void) __fsetlocking(f, FSETLOCKING_BYCALLER); (void) fchmod(fileno(f), 0644); fprintf(f, diff --git a/src/network/networkd-manager.c b/src/network/networkd-manager.c index 9075b0a14b..677f66a478 100644 --- a/src/network/networkd-manager.c +++ b/src/network/networkd-manager.c @@ -3,7 +3,6 @@ #include #include #include -#include #include #include "sd-daemon.h" @@ -1189,7 +1188,6 @@ static int manager_save(Manager *m) { if (r < 0) return r; - (void) __fsetlocking(f, FSETLOCKING_BYCALLER); (void) fchmod(fileno(f), 0644); fprintf(f, diff --git a/src/resolve/resolved-link.c b/src/resolve/resolved-link.c index 50f9309f10..f65ce64d17 100644 --- a/src/resolve/resolved-link.c +++ b/src/resolve/resolved-link.c @@ -2,7 +2,6 @@ #include #include -#include #include #include "sd-network.h" @@ -1178,7 +1177,6 @@ int link_save_user(Link *l) { if (r < 0) goto fail; - (void) __fsetlocking(f, FSETLOCKING_BYCALLER); (void) fchmod(fileno(f), 0644); fputs("# This is private data. Do not parse.\n", f); diff --git a/src/resolve/resolved-resolv-conf.c b/src/resolve/resolved-resolv-conf.c index 0435791ea0..dfc9a948e3 100644 --- a/src/resolve/resolved-resolv-conf.c +++ b/src/resolve/resolved-resolv-conf.c @@ -1,7 +1,6 @@ /* SPDX-License-Identifier: LGPL-2.1+ */ #include -#include #include #include #include @@ -359,14 +358,12 @@ int manager_write_resolv_conf(Manager *m) { if (r < 0) return log_warning_errno(r, "Failed to open private resolv.conf file for writing: %m"); - (void) __fsetlocking(f_uplink, FSETLOCKING_BYCALLER); (void) fchmod(fileno(f_uplink), 0644); r = fopen_temporary_label(PRIVATE_STUB_RESOLV_CONF, PRIVATE_STUB_RESOLV_CONF, &f_stub, &temp_path_stub); if (r < 0) return log_warning_errno(r, "Failed to open private stub-resolv.conf file for writing: %m"); - (void) __fsetlocking(f_stub, FSETLOCKING_BYCALLER); (void) fchmod(fileno(f_stub), 0644); r = write_uplink_resolv_conf_contents(f_uplink, dns, domains); diff --git a/src/shared/generator.c b/src/shared/generator.c index ed7f037e91..403c2a6737 100644 --- a/src/shared/generator.c +++ b/src/shared/generator.c @@ -1,7 +1,6 @@ /* SPDX-License-Identifier: LGPL-2.1+ */ #include -#include #include #include "alloc-util.h" @@ -30,23 +29,22 @@ int generator_open_unit_file( const char *unit; FILE *f; + int r; unit = strjoina(dest, "/", name); - f = fopen(unit, "wxe"); - if (!f) { - if (source && errno == EEXIST) - return log_error_errno(errno, + r = fopen_unlocked(unit, "wxe", &f); + if (r < 0) { + if (source && r == -EEXIST) + return log_error_errno(r, "Failed to create unit file %s, as it already exists. Duplicate entry in %s?", unit, source); else - return log_error_errno(errno, + return log_error_errno(r, "Failed to create unit file %s: %m", unit); } - (void) __fsetlocking(f, FSETLOCKING_BYCALLER); - fprintf(f, "# Automatically generated by %s\n\n", program_invocation_short_name); diff --git a/src/shared/mount-util.c b/src/shared/mount-util.c index 1b50716731..680c4522ad 100644 --- a/src/shared/mount-util.c +++ b/src/shared/mount-util.c @@ -1,7 +1,6 @@ /* SPDX-License-Identifier: LGPL-2.1+ */ #include -#include #include #include #include From 02e23d1a1a8c3baf73d82da5abbab3a4eeb1cbf4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 4 Apr 2019 11:27:08 +0200 Subject: [PATCH 3/6] Add fdopen_unlocked() wrapper --- coccinelle/fopen-unlocked.cocci | 12 ++++++++++++ src/basic/fileio.c | 20 +++++++++++++++----- src/basic/fileio.h | 1 + src/basic/tmpfile-util.c | 11 +++++------ src/portable/portable.c | 10 ++++------ 5 files changed, 37 insertions(+), 17 deletions(-) diff --git a/coccinelle/fopen-unlocked.cocci b/coccinelle/fopen-unlocked.cocci index e6f2bc5681..bbd70a6338 100644 --- a/coccinelle/fopen-unlocked.cocci +++ b/coccinelle/fopen-unlocked.cocci @@ -49,3 +49,15 @@ expression f, g, path, p; if (r < 0) return ...; - (void) __fsetlocking(f, FSETLOCKING_BYCALLER); +@@ +expression f, fd, options; +@@ +- f = fdopen(fd, options); ++ r = fdopen_unlocked(fd, options, &f); ++ if (r < 0) { +- if (!f) { + ... +- return -errno; ++ return r; + } +- (void) __fsetlocking(f, FSETLOCKING_BYCALLER); diff --git a/src/basic/fileio.c b/src/basic/fileio.c index 2318f407b8..af22ec9110 100644 --- a/src/basic/fileio.c +++ b/src/basic/fileio.c @@ -42,6 +42,19 @@ int fopen_unlocked(const char *path, const char *options, FILE **ret) { return 0; } +int fdopen_unlocked(int fd, const char *options, FILE **ret) { + assert(ret); + + FILE *f = fdopen(fd, options); + if (!f) + return -errno; + + (void) __fsetlocking(f, FSETLOCKING_BYCALLER); + + *ret = f; + return 0; +} + int write_string_stream_ts( FILE *f, const char *line, @@ -167,14 +180,11 @@ int write_string_file_ts( goto fail; } - f = fdopen(fd, "w"); - if (!f) { - r = -errno; + r = fdopen_unlocked(fd, "w", &f); + if (r < 0) { safe_close(fd); goto fail; } - - (void) __fsetlocking(f, FSETLOCKING_BYCALLER); } if (flags & WRITE_STRING_FILE_DISABLE_BUFFER) diff --git a/src/basic/fileio.h b/src/basic/fileio.h index bee3353439..7903b57c80 100644 --- a/src/basic/fileio.h +++ b/src/basic/fileio.h @@ -34,6 +34,7 @@ typedef enum { } ReadFullFileFlags; int fopen_unlocked(const char *path, const char *options, FILE **ret); +int fdopen_unlocked(int fd, const char *options, FILE **ret); int write_string_stream_ts(FILE *f, const char *line, WriteStringFileFlags flags, struct timespec *ts); static inline int write_string_stream(FILE *f, const char *line, WriteStringFileFlags flags) { diff --git a/src/basic/tmpfile-util.c b/src/basic/tmpfile-util.c index 260443a1d6..3ed91520b9 100644 --- a/src/basic/tmpfile-util.c +++ b/src/basic/tmpfile-util.c @@ -6,6 +6,7 @@ #include "alloc-util.h" #include "fd-util.h" +#include "fileio.h" #include "fs-util.h" #include "hexdecoct.h" #include "macro.h" @@ -42,16 +43,14 @@ int fopen_temporary(const char *path, FILE **_f, char **_temp_path) { /* This assumes that returned FILE object is short-lived and used within the same single-threaded * context and never shared externally, hence locking is not necessary. */ - f = fdopen(fd, "w"); - if (!f) { - unlink_noerrno(t); + r = fdopen_unlocked(fd, "w", &f); + if (r < 0) { + unlink(t); free(t); safe_close(fd); - return -errno; + return r; } - (void) __fsetlocking(f, FSETLOCKING_BYCALLER); - *_f = f; *_temp_path = t; diff --git a/src/portable/portable.c b/src/portable/portable.c index 9b6cc21d2c..4aa879801f 100644 --- a/src/portable/portable.c +++ b/src/portable/portable.c @@ -1091,12 +1091,10 @@ static int test_chroot_dropin( return log_debug_errno(errno, "Failed to open %s/%s: %m", where, p); } - f = fdopen(fd, "r"); - if (!f) - return log_debug_errno(errno, "Failed to convert file handle: %m"); - fd = -1; - - (void) __fsetlocking(f, FSETLOCKING_BYCALLER); + r = fdopen_unlocked(fd, "r", &f); + if (r < 0) + return log_debug_errno(r, "Failed to convert file handle: %m"); + TAKE_FD(fd); r = read_line(f, LONG_LINE_MAX, &line); if (r < 0) From b636d78aeeea584b7828df18cb636693af365e2f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 4 Apr 2019 11:27:21 +0200 Subject: [PATCH 4/6] core/smack-setup: add helper function for openat+fdopen Unlocked operations are used in all three places. I don't see why just one was special. This also improves logging, since we don't just log the final component of the path, but the full name. --- src/core/smack-setup.c | 93 +++++++++++++++--------------------------- 1 file changed, 34 insertions(+), 59 deletions(-) diff --git a/src/core/smack-setup.c b/src/core/smack-setup.c index cd7fb01416..931e8a5227 100644 --- a/src/core/smack-setup.c +++ b/src/core/smack-setup.c @@ -26,12 +26,36 @@ #if ENABLE_SMACK -static int write_access2_rules(const char* srcdir) { +static int fdopen_unlocked_at(int dfd, const char *dir, const char *name, int *status, FILE **ret_file) { + int fd, r; + FILE *f; + + fd = openat(dfd, name, O_RDONLY|O_CLOEXEC); + if (fd < 0) { + if (*status == 0) + *status = -errno; + + return log_warning_errno(errno, "Failed to open \"%s/%s\": %m", dir, name); + } + + r = fdopen_unlocked(fd, "r", &f); + if (r < 0) { + if (*status == 0) + *status = r; + + safe_close(fd); + return log_error_errno(r, "Failed to open \"%s/%s\": %m", dir, name); + } + + *ret_file = f; + return 0; +} + +static int write_access2_rules(const char *srcdir) { _cleanup_close_ int load2_fd = -1, change_fd = -1; _cleanup_closedir_ DIR *dir = NULL; struct dirent *entry; - int dfd = -1; - int r = 0; + int dfd = -1, r = 0; load2_fd = open("/sys/fs/smackfs/load2", O_RDWR|O_CLOEXEC|O_NONBLOCK|O_NOCTTY); if (load2_fd < 0) { @@ -59,28 +83,13 @@ static int write_access2_rules(const char* srcdir) { assert(dfd >= 0); FOREACH_DIRENT(entry, dir, return 0) { - int fd; _cleanup_fclose_ FILE *policy = NULL; if (!dirent_is_file(entry)) continue; - fd = openat(dfd, entry->d_name, O_RDONLY|O_CLOEXEC); - if (fd < 0) { - if (r == 0) - r = -errno; - log_warning_errno(errno, "Failed to open '%s': %m", entry->d_name); + if (fdopen_unlocked_at(dfd, srcdir, entry->d_name, &r, &policy) < 0) continue; - } - - policy = fdopen(fd, "r"); - if (!policy) { - if (r == 0) - r = -errno; - safe_close(fd); - log_error_errno(errno, "Failed to open '%s': %m", entry->d_name); - continue; - } /* load2 write rules in the kernel require a line buffered stream */ for (;;) { @@ -115,12 +124,11 @@ static int write_access2_rules(const char* srcdir) { return r; } -static int write_cipso2_rules(const char* srcdir) { +static int write_cipso2_rules(const char *srcdir) { _cleanup_close_ int cipso2_fd = -1; _cleanup_closedir_ DIR *dir = NULL; struct dirent *entry; - int dfd = -1; - int r = 0; + int dfd = -1, r = 0; cipso2_fd = open("/sys/fs/smackfs/cipso2", O_RDWR|O_CLOEXEC|O_NONBLOCK|O_NOCTTY); if (cipso2_fd < 0) { @@ -141,28 +149,13 @@ static int write_cipso2_rules(const char* srcdir) { assert(dfd >= 0); FOREACH_DIRENT(entry, dir, return 0) { - int fd; _cleanup_fclose_ FILE *policy = NULL; if (!dirent_is_file(entry)) continue; - fd = openat(dfd, entry->d_name, O_RDONLY|O_CLOEXEC); - if (fd < 0) { - if (r == 0) - r = -errno; - log_error_errno(errno, "Failed to open '%s': %m", entry->d_name); + if (fdopen_unlocked_at(dfd, srcdir, entry->d_name, &r, &policy) < 0) continue; - } - - policy = fdopen(fd, "r"); - if (!policy) { - if (r == 0) - r = -errno; - safe_close(fd); - log_error_errno(errno, "Failed to open '%s': %m", entry->d_name); - continue; - } /* cipso2 write rules in the kernel require a line buffered stream */ for (;;) { @@ -191,12 +184,11 @@ static int write_cipso2_rules(const char* srcdir) { return r; } -static int write_netlabel_rules(const char* srcdir) { +static int write_netlabel_rules(const char *srcdir) { _cleanup_fclose_ FILE *dst = NULL; _cleanup_closedir_ DIR *dir = NULL; struct dirent *entry; - int dfd = -1; - int r = 0; + int dfd = -1, r = 0; dst = fopen("/sys/fs/smackfs/netlabel", "we"); if (!dst) { @@ -217,27 +209,10 @@ static int write_netlabel_rules(const char* srcdir) { assert(dfd >= 0); FOREACH_DIRENT(entry, dir, return 0) { - int fd; _cleanup_fclose_ FILE *policy = NULL; - fd = openat(dfd, entry->d_name, O_RDONLY|O_CLOEXEC); - if (fd < 0) { - if (r == 0) - r = -errno; - log_warning_errno(errno, "Failed to open %s: %m", entry->d_name); + if (fdopen_unlocked_at(dfd, srcdir, entry->d_name, &r, &policy) < 0) continue; - } - - policy = fdopen(fd, "r"); - if (!policy) { - if (r == 0) - r = -errno; - safe_close(fd); - log_error_errno(errno, "Failed to open %s: %m", entry->d_name); - continue; - } - - (void) __fsetlocking(policy, FSETLOCKING_BYCALLER); /* load2 write rules in the kernel require a line buffered stream */ for (;;) { From 2fe21124a6560fcf1ce3b0a3004baa9bb45d1e0b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 4 Apr 2019 11:46:44 +0200 Subject: [PATCH 5/6] Add open_memstream_unlocked() wrapper --- coccinelle/fopen-unlocked.cocci | 8 +++++++ src/basic/fileio.c | 10 +++++++++ src/basic/fileio.h | 1 + src/basic/string-util.c | 16 ++++++-------- src/basic/tmpfile-util.c | 1 - src/busctl/busctl.c | 5 +---- src/core/dbus-cgroup.c | 29 +++++++------------------- src/core/dbus-execute.c | 9 ++------ src/core/manager.c | 5 +---- src/core/smack-setup.c | 1 - src/coredump/coredump.c | 5 +---- src/coredump/stacktrace.c | 6 ++---- src/journal/journal-qrcode.c | 6 ++---- src/libsystemd/sd-bus/bus-introspect.c | 6 +----- src/libsystemd/sd-bus/bus-match.c | 6 +----- src/portable/portable.c | 2 -- src/resolve/resolved-dns-dnssec.c | 7 ++----- src/resolve/resolved-manager.c | 5 +---- src/shared/calendarspec.c | 5 +---- src/shared/format-table.c | 5 +---- src/shared/json.c | 5 +---- 21 files changed, 49 insertions(+), 94 deletions(-) diff --git a/coccinelle/fopen-unlocked.cocci b/coccinelle/fopen-unlocked.cocci index bbd70a6338..7870f8ccea 100644 --- a/coccinelle/fopen-unlocked.cocci +++ b/coccinelle/fopen-unlocked.cocci @@ -61,3 +61,11 @@ expression f, fd, options; + return r; } - (void) __fsetlocking(f, FSETLOCKING_BYCALLER); +@@ +expression f, buf, sz; +@@ +- f = open_memstream(&buf, &sz); ++ f = open_memstream_unlocked(&buf, &sz); + if (!f) + return ...; +- (void) __fsetlocking(f, FSETLOCKING_BYCALLER); diff --git a/src/basic/fileio.c b/src/basic/fileio.c index af22ec9110..99efc2410c 100644 --- a/src/basic/fileio.c +++ b/src/basic/fileio.c @@ -55,6 +55,16 @@ int fdopen_unlocked(int fd, const char *options, FILE **ret) { return 0; } +FILE* open_memstream_unlocked(char **ptr, size_t *sizeloc) { + FILE *f = open_memstream(ptr, sizeloc); + if (!f) + return NULL; + + (void) __fsetlocking(f, FSETLOCKING_BYCALLER); + + return f; +} + int write_string_stream_ts( FILE *f, const char *line, diff --git a/src/basic/fileio.h b/src/basic/fileio.h index 7903b57c80..fe5c8277de 100644 --- a/src/basic/fileio.h +++ b/src/basic/fileio.h @@ -35,6 +35,7 @@ typedef enum { int fopen_unlocked(const char *path, const char *options, FILE **ret); int fdopen_unlocked(int fd, const char *options, FILE **ret); +FILE* open_memstream_unlocked(char **ptr, size_t *sizeloc); int write_string_stream_ts(FILE *f, const char *line, WriteStringFileFlags flags, struct timespec *ts); static inline int write_string_stream(FILE *f, const char *line, WriteStringFileFlags flags) { diff --git a/src/basic/string-util.c b/src/basic/string-util.c index 5001a2be3a..779048904a 100644 --- a/src/basic/string-util.c +++ b/src/basic/string-util.c @@ -4,7 +4,6 @@ #include #include #include -#include #include #include @@ -761,23 +760,20 @@ char *strip_tab_ansi(char **ibuf, size_t *_isz, size_t highlight[2]) { * 2. Strips ANSI color sequences (a subset of CSI), i.e. ESC '[' … 'm' sequences * 3. Strips ANSI operating system sequences (CSO), i.e. ESC ']' … BEL sequences * - * Everything else will be left as it is. In particular other ANSI sequences are left as they are, as are any - * other special characters. Truncated ANSI sequences are left-as is too. This call is supposed to suppress the - * most basic formatting noise, but nothing else. + * Everything else will be left as it is. In particular other ANSI sequences are left as they are, as + * are any other special characters. Truncated ANSI sequences are left-as is too. This call is + * supposed to suppress the most basic formatting noise, but nothing else. * * Why care for CSO sequences? Well, to undo what terminal_urlify() and friends generate. */ isz = _isz ? *_isz : strlen(*ibuf); - f = open_memstream(&obuf, &osz); + /* Note we turn off internal locking on f for performance reasons. It's safe to do so since we + * created f here and it doesn't leave our scope. */ + f = open_memstream_unlocked(&obuf, &osz); if (!f) return NULL; - /* Note we turn off internal locking on f for performance reasons. It's safe to do so since we created f here - * and it doesn't leave our scope. */ - - (void) __fsetlocking(f, FSETLOCKING_BYCALLER); - for (i = *ibuf; i < *ibuf + isz + 1; i++) { switch (state) { diff --git a/src/basic/tmpfile-util.c b/src/basic/tmpfile-util.c index 3ed91520b9..e77af7659f 100644 --- a/src/basic/tmpfile-util.c +++ b/src/basic/tmpfile-util.c @@ -1,7 +1,6 @@ /* SPDX-License-Identifier: LGPL-2.1+ */ #include -#include #include #include "alloc-util.h" diff --git a/src/busctl/busctl.c b/src/busctl/busctl.c index c8125c6606..02f12dc701 100644 --- a/src/busctl/busctl.c +++ b/src/busctl/busctl.c @@ -1,7 +1,6 @@ /* SPDX-License-Identifier: LGPL-2.1+ */ #include -#include #include "sd-bus.h" @@ -997,12 +996,10 @@ static int introspect(int argc, char **argv, void *userdata) { if (r < 0) return bus_log_parse_error(r); - mf = open_memstream(&buf, &sz); + mf = open_memstream_unlocked(&buf, &sz); if (!mf) return log_oom(); - (void) __fsetlocking(mf, FSETLOCKING_BYCALLER); - r = format_cmdline(reply, mf, false); if (r < 0) return bus_log_parse_error(r); diff --git a/src/core/dbus-cgroup.c b/src/core/dbus-cgroup.c index 4615aeaf66..7a5e440e9c 100644 --- a/src/core/dbus-cgroup.c +++ b/src/core/dbus-cgroup.c @@ -1,7 +1,6 @@ /* SPDX-License-Identifier: LGPL-2.1+ */ #include -#include #include "af-list.h" #include "alloc-util.h" @@ -821,12 +820,10 @@ int bus_cgroup_set_property( unit_invalidate_cgroup(u, CGROUP_MASK_IO); - f = open_memstream(&buf, &size); + f = open_memstream_unlocked(&buf, &size); if (!f) return -ENOMEM; - (void) __fsetlocking(f, FSETLOCKING_BYCALLER); - fprintf(f, "%s=\n", name); LIST_FOREACH(device_limits, a, c->io_device_limits) if (a->limits[iol_type] != cgroup_io_limit_defaults[iol_type]) @@ -903,12 +900,10 @@ int bus_cgroup_set_property( unit_invalidate_cgroup(u, CGROUP_MASK_IO); - f = open_memstream(&buf, &size); + f = open_memstream_unlocked(&buf, &size); if (!f) return -ENOMEM; - (void) __fsetlocking(f, FSETLOCKING_BYCALLER); - fputs("IODeviceWeight=\n", f); LIST_FOREACH(device_weights, a, c->io_device_weights) fprintf(f, "IODeviceWeight=%s %" PRIu64 "\n", a->path, a->weight); @@ -982,12 +977,10 @@ int bus_cgroup_set_property( unit_invalidate_cgroup(u, CGROUP_MASK_IO); - f = open_memstream(&buf, &size); + f = open_memstream_unlocked(&buf, &size); if (!f) return -ENOMEM; - (void) __fsetlocking(f, FSETLOCKING_BYCALLER); - fputs("IODeviceLatencyTargetSec=\n", f); LIST_FOREACH(device_latencies, a, c->io_device_latencies) fprintf(f, "IODeviceLatencyTargetSec=%s %s\n", @@ -1077,12 +1070,10 @@ int bus_cgroup_set_property( unit_invalidate_cgroup(u, CGROUP_MASK_BLKIO); - f = open_memstream(&buf, &size); + f = open_memstream_unlocked(&buf, &size); if (!f) return -ENOMEM; - (void) __fsetlocking(f, FSETLOCKING_BYCALLER); - if (read) { fputs("BlockIOReadBandwidth=\n", f); LIST_FOREACH(device_bandwidths, a, c->blockio_device_bandwidths) @@ -1167,12 +1158,10 @@ int bus_cgroup_set_property( unit_invalidate_cgroup(u, CGROUP_MASK_BLKIO); - f = open_memstream(&buf, &size); + f = open_memstream_unlocked(&buf, &size); if (!f) return -ENOMEM; - (void) __fsetlocking(f, FSETLOCKING_BYCALLER); - fputs("BlockIODeviceWeight=\n", f); LIST_FOREACH(device_weights, a, c->blockio_device_weights) fprintf(f, "BlockIODeviceWeight=%s %" PRIu64 "\n", a->path, a->weight); @@ -1275,12 +1264,10 @@ int bus_cgroup_set_property( unit_invalidate_cgroup(u, CGROUP_MASK_DEVICES); - f = open_memstream(&buf, &size); + f = open_memstream_unlocked(&buf, &size); if (!f) return -ENOMEM; - (void) __fsetlocking(f, FSETLOCKING_BYCALLER); - fputs("DeviceAllow=\n", f); LIST_FOREACH(device_allow, a, c->device_allow) fprintf(f, "DeviceAllow=%s %s%s%s\n", a->path, a->r ? "r" : "", a->w ? "w" : "", a->m ? "m" : ""); @@ -1390,12 +1377,10 @@ int bus_cgroup_set_property( *list = ip_address_access_free_all(*list); unit_invalidate_cgroup_bpf(u); - f = open_memstream(&buf, &size); + f = open_memstream_unlocked(&buf, &size); if (!f) return -ENOMEM; - (void) __fsetlocking(f, FSETLOCKING_BYCALLER); - fputs(name, f); fputs("=\n", f); diff --git a/src/core/dbus-execute.c b/src/core/dbus-execute.c index 5532d1ada9..48a2ebc5ef 100644 --- a/src/core/dbus-execute.c +++ b/src/core/dbus-execute.c @@ -2,7 +2,6 @@ #include #include -#include #if HAVE_SECCOMP #include @@ -958,12 +957,10 @@ int bus_set_transient_exec_command( if (n == 0) *exec_command = exec_command_free_list(*exec_command); - f = open_memstream(&buf, &size); + f = open_memstream_unlocked(&buf, &size); if (!f) return -ENOMEM; - (void) __fsetlocking(f, FSETLOCKING_BYCALLER); - fputs("ExecStart=\n", f); LIST_FOREACH(command, c, *exec_command) { @@ -2015,12 +2012,10 @@ int bus_exec_context_set_transient_property( if (r < 0) return r; - f = open_memstream(&joined, &size); + f = open_memstream_unlocked(&joined, &size); if (!f) return -ENOMEM; - (void) __fsetlocking(f, FSETLOCKING_BYCALLER); - fputs("EnvironmentFile=\n", f); STRV_FOREACH(i, c->environment_files) { diff --git a/src/core/manager.c b/src/core/manager.c index 4154aec12f..15df7e5f35 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -4,7 +4,6 @@ #include #include #include -#include #include #include #include @@ -2111,12 +2110,10 @@ int manager_get_dump_string(Manager *m, char **ret) { assert(m); assert(ret); - f = open_memstream(&dump, &size); + f = open_memstream_unlocked(&dump, &size); if (!f) return -errno; - (void) __fsetlocking(f, FSETLOCKING_BYCALLER); - manager_dump(m, f, NULL); r = fflush_and_check(f); diff --git a/src/core/smack-setup.c b/src/core/smack-setup.c index 931e8a5227..b95e6239d4 100644 --- a/src/core/smack-setup.c +++ b/src/core/smack-setup.c @@ -9,7 +9,6 @@ #include #include #include -#include #include #include #include diff --git a/src/coredump/coredump.c b/src/coredump/coredump.c index 023701646b..ac7b972026 100644 --- a/src/coredump/coredump.c +++ b/src/coredump/coredump.c @@ -2,7 +2,6 @@ #include #include -#include #include #include #include @@ -530,12 +529,10 @@ static int compose_open_fds(pid_t pid, char **open_fds) { if (proc_fdinfo_fd < 0) return -errno; - stream = open_memstream(&buffer, &size); + stream = open_memstream_unlocked(&buffer, &size); if (!stream) return -ENOMEM; - (void) __fsetlocking(stream, FSETLOCKING_BYCALLER); - FOREACH_DIRENT(dent, proc_fd_dir, return -errno) { _cleanup_fclose_ FILE *fdinfo = NULL; _cleanup_free_ char *fdname = NULL; diff --git a/src/coredump/stacktrace.c b/src/coredump/stacktrace.c index ab1ac12a23..66b2ec8cca 100644 --- a/src/coredump/stacktrace.c +++ b/src/coredump/stacktrace.c @@ -2,11 +2,11 @@ #include #include -#include #include #include #include "alloc-util.h" +#include "fileio.h" #include "fd-util.h" #include "format-util.h" #include "macro.h" @@ -126,12 +126,10 @@ int coredump_make_stack_trace(int fd, const char *executable, char **ret) { if (lseek(fd, 0, SEEK_SET) == (off_t) -1) return -errno; - c.f = open_memstream(&buf, &sz); + c.f = open_memstream_unlocked(&buf, &sz); if (!c.f) return -ENOMEM; - (void) __fsetlocking(c.f, FSETLOCKING_BYCALLER); - elf_version(EV_CURRENT); c.elf = elf_begin(fd, ELF_C_READ_MMAP, NULL); diff --git a/src/journal/journal-qrcode.c b/src/journal/journal-qrcode.c index 35d6ec4593..678654f773 100644 --- a/src/journal/journal-qrcode.c +++ b/src/journal/journal-qrcode.c @@ -4,9 +4,9 @@ #include #include #include -#include #include +#include "fileio.h" #include "journal-qrcode.h" #include "macro.h" @@ -45,12 +45,10 @@ int print_qr_code( assert(seed); assert(seed_size > 0); - f = open_memstream(&url, &url_size); + f = open_memstream_unlocked(&url, &url_size); if (!f) return -ENOMEM; - (void) __fsetlocking(f, FSETLOCKING_BYCALLER); - fputs("fss://", f); for (i = 0; i < seed_size; i++) { diff --git a/src/libsystemd/sd-bus/bus-introspect.c b/src/libsystemd/sd-bus/bus-introspect.c index 29a5ef715e..022eddb10f 100644 --- a/src/libsystemd/sd-bus/bus-introspect.c +++ b/src/libsystemd/sd-bus/bus-introspect.c @@ -1,7 +1,5 @@ /* SPDX-License-Identifier: LGPL-2.1+ */ -#include - #include "bus-internal.h" #include "bus-introspect.h" #include "bus-objects.h" @@ -18,12 +16,10 @@ int introspect_begin(struct introspect *i, bool trusted) { zero(*i); i->trusted = trusted; - i->f = open_memstream(&i->introspection, &i->size); + i->f = open_memstream_unlocked(&i->introspection, &i->size); if (!i->f) return -ENOMEM; - (void) __fsetlocking(i->f, FSETLOCKING_BYCALLER); - fputs(BUS_INTROSPECT_DOCTYPE "\n", i->f); diff --git a/src/libsystemd/sd-bus/bus-match.c b/src/libsystemd/sd-bus/bus-match.c index 266dd7f1df..14204eeb6b 100644 --- a/src/libsystemd/sd-bus/bus-match.c +++ b/src/libsystemd/sd-bus/bus-match.c @@ -1,7 +1,5 @@ /* SPDX-License-Identifier: LGPL-2.1+ */ -#include - #include "alloc-util.h" #include "bus-internal.h" #include "bus-match.h" @@ -861,12 +859,10 @@ char *bus_match_to_string(struct bus_match_component *components, unsigned n_com assert(components); - f = open_memstream(&buffer, &size); + f = open_memstream_unlocked(&buffer, &size); if (!f) return NULL; - (void) __fsetlocking(f, FSETLOCKING_BYCALLER); - for (i = 0; i < n_components; i++) { char buf[32]; diff --git a/src/portable/portable.c b/src/portable/portable.c index 4aa879801f..1017864b37 100644 --- a/src/portable/portable.c +++ b/src/portable/portable.c @@ -1,7 +1,5 @@ /* SPDX-License-Identifier: LGPL-2.1+ */ -#include - #include "bus-common-errors.h" #include "bus-error.h" #include "conf-files.h" diff --git a/src/resolve/resolved-dns-dnssec.c b/src/resolve/resolved-dns-dnssec.c index a5ded5ada2..18e253bea3 100644 --- a/src/resolve/resolved-dns-dnssec.c +++ b/src/resolve/resolved-dns-dnssec.c @@ -1,9 +1,7 @@ /* SPDX-License-Identifier: LGPL-2.1+ */ -#include - #if HAVE_GCRYPT -#include +# include #endif #include "alloc-util.h" @@ -803,10 +801,9 @@ int dnssec_verify_rrset( /* Bring the RRs into canonical order */ typesafe_qsort(list, n, rr_compare); - f = open_memstream(&sig_data, &sig_size); + f = open_memstream_unlocked(&sig_data, &sig_size); if (!f) return -ENOMEM; - (void) __fsetlocking(f, FSETLOCKING_BYCALLER); fwrite_uint16(f, rrsig->rrsig.type_covered); fwrite_uint8(f, rrsig->rrsig.algorithm); diff --git a/src/resolve/resolved-manager.c b/src/resolve/resolved-manager.c index 9b8239bd09..24a752e3bb 100644 --- a/src/resolve/resolved-manager.c +++ b/src/resolve/resolved-manager.c @@ -3,7 +3,6 @@ #include #include #include -#include #include #include #include @@ -511,12 +510,10 @@ static int manager_sigusr1(sd_event_source *s, const struct signalfd_siginfo *si assert(si); assert(m); - f = open_memstream(&buffer, &size); + f = open_memstream_unlocked(&buffer, &size); if (!f) return log_oom(); - (void) __fsetlocking(f, FSETLOCKING_BYCALLER); - LIST_FOREACH(scopes, scope, m->dns_scopes) dns_scope_dump(scope, f); diff --git a/src/shared/calendarspec.c b/src/shared/calendarspec.c index d83e7962a6..6e318fa265 100644 --- a/src/shared/calendarspec.c +++ b/src/shared/calendarspec.c @@ -6,7 +6,6 @@ #include #include #include -#include #include #include #include @@ -330,12 +329,10 @@ int calendar_spec_to_string(const CalendarSpec *c, char **p) { assert(c); assert(p); - f = open_memstream(&buf, &sz); + f = open_memstream_unlocked(&buf, &sz); if (!f) return -ENOMEM; - (void) __fsetlocking(f, FSETLOCKING_BYCALLER); - if (c->weekdays_bits > 0 && c->weekdays_bits <= BITS_WEEKDAYS) { format_weekdays(f, c); fputc(' ', f); diff --git a/src/shared/format-table.c b/src/shared/format-table.c index a5c0a99b08..74379e8c64 100644 --- a/src/shared/format-table.c +++ b/src/shared/format-table.c @@ -1,7 +1,6 @@ /* SPDX-License-Identifier: LGPL-2.1+ */ #include -#include #include "alloc-util.h" #include "fd-util.h" @@ -1376,12 +1375,10 @@ int table_format(Table *t, char **ret) { size_t sz = 0; int r; - f = open_memstream(&buf, &sz); + f = open_memstream_unlocked(&buf, &sz); if (!f) return -ENOMEM; - (void) __fsetlocking(f, FSETLOCKING_BYCALLER); - r = table_print(t, f); if (r < 0) return r; diff --git a/src/shared/json.c b/src/shared/json.c index db003a41a4..a1559bda4e 100644 --- a/src/shared/json.c +++ b/src/shared/json.c @@ -4,7 +4,6 @@ #include #include #include -#include #include #include #include @@ -1558,12 +1557,10 @@ int json_variant_format(JsonVariant *v, JsonFormatFlags flags, char **ret) { { _cleanup_fclose_ FILE *f = NULL; - f = open_memstream(&s, &sz); + f = open_memstream_unlocked(&s, &sz); if (!f) return -ENOMEM; - (void) __fsetlocking(f, FSETLOCKING_BYCALLER); - json_variant_dump(v, flags, f, NULL); r = fflush_and_check(f); From 673a1e6fb9ea2b61d97be45b8f9852c70a69778c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 4 Apr 2019 12:24:38 +0200 Subject: [PATCH 6/6] Add fmemopen_unlocked() and use unlocked ops in fuzzers and some other tests This might make things marginially faster. I didn't benchmark though. --- src/basic/fileio.c | 10 ++++++++++ src/basic/fileio.h | 1 + src/fuzz/fuzz-bus-message.c | 6 ++---- src/fuzz/fuzz-env-file.c | 5 +++-- src/fuzz/fuzz-hostname-util.c | 5 ++--- src/fuzz/fuzz-json.c | 5 +++-- src/fuzz/fuzz-nspawn-oci.c | 5 ++--- src/fuzz/fuzz-nspawn-settings.c | 5 ++--- src/fuzz/fuzz-unit-file.c | 4 ++-- src/libsystemd/sd-bus/test-bus-marshal.c | 7 ++++--- src/portable/portablectl.c | 4 ++-- src/shared/bootspec.c | 2 +- src/test/test-fileio.c | 8 ++++---- src/test/test-json.c | 3 ++- 14 files changed, 40 insertions(+), 30 deletions(-) diff --git a/src/basic/fileio.c b/src/basic/fileio.c index 99efc2410c..85a49b1f9e 100644 --- a/src/basic/fileio.c +++ b/src/basic/fileio.c @@ -65,6 +65,16 @@ FILE* open_memstream_unlocked(char **ptr, size_t *sizeloc) { return f; } +FILE* fmemopen_unlocked(void *buf, size_t size, const char *mode) { + FILE *f = fmemopen(buf, size, mode); + if (!f) + return NULL; + + (void) __fsetlocking(f, FSETLOCKING_BYCALLER); + + return f; +} + int write_string_stream_ts( FILE *f, const char *line, diff --git a/src/basic/fileio.h b/src/basic/fileio.h index fe5c8277de..ffe900b486 100644 --- a/src/basic/fileio.h +++ b/src/basic/fileio.h @@ -36,6 +36,7 @@ typedef enum { int fopen_unlocked(const char *path, const char *options, FILE **ret); int fdopen_unlocked(int fd, const char *options, FILE **ret); FILE* open_memstream_unlocked(char **ptr, size_t *sizeloc); +FILE* fmemopen_unlocked(void *buf, size_t size, const char *mode); int write_string_stream_ts(FILE *f, const char *line, WriteStringFileFlags flags, struct timespec *ts); static inline int write_string_stream(FILE *f, const char *line, WriteStringFileFlags flags) { diff --git a/src/fuzz/fuzz-bus-message.c b/src/fuzz/fuzz-bus-message.c index 9842c62a6f..aca82edad9 100644 --- a/src/fuzz/fuzz-bus-message.c +++ b/src/fuzz/fuzz-bus-message.c @@ -1,13 +1,11 @@ /* SPDX-License-Identifier: LGPL-2.1+ */ -#include -#include - #include "alloc-util.h" #include "bus-dump.h" #include "bus-message.h" #include "env-util.h" #include "fd-util.h" +#include "fileio.h" #include "fuzz.h" int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) { @@ -36,7 +34,7 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) { TAKE_PTR(buffer); if (getenv_bool("SYSTEMD_FUZZ_OUTPUT") <= 0) - assert_se(g = open_memstream(&out, &out_size)); + assert_se(g = open_memstream_unlocked(&out, &out_size)); bus_message_dump(m, g ?: stdout, BUS_MESSAGE_DUMP_WITH_HEADER); diff --git a/src/fuzz/fuzz-env-file.c b/src/fuzz/fuzz-env-file.c index 3c8ffaa7b2..d945dfc02c 100644 --- a/src/fuzz/fuzz-env-file.c +++ b/src/fuzz/fuzz-env-file.c @@ -1,9 +1,10 @@ /* SPDX-License-Identifier: LGPL-2.1+ */ -#include +#include #include "alloc-util.h" #include "env-file.h" +#include "fileio.h" #include "fd-util.h" #include "fuzz.h" #include "strv.h" @@ -15,7 +16,7 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) { if (size == 0 || size > 65535) return 0; - f = fmemopen((char*) data, size, "re"); + f = fmemopen_unlocked((char*) data, size, "re"); assert_se(f); /* We don't want to fill the logs with messages about parse errors. diff --git a/src/fuzz/fuzz-hostname-util.c b/src/fuzz/fuzz-hostname-util.c index deaf8112ba..2130a4a811 100644 --- a/src/fuzz/fuzz-hostname-util.c +++ b/src/fuzz/fuzz-hostname-util.c @@ -1,9 +1,8 @@ /* SPDX-License-Identifier: LGPL-2.1+ */ -#include - #include "alloc-util.h" #include "fd-util.h" +#include "fileio.h" #include "fuzz.h" #include "hostname-util.h" @@ -14,7 +13,7 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) { if (size == 0) return 0; - f = fmemopen((char*) data, size, "re"); + f = fmemopen_unlocked((char*) data, size, "re"); assert_se(f); /* We don't want to fill the logs with messages about parse errors. diff --git a/src/fuzz/fuzz-json.c b/src/fuzz/fuzz-json.c index 3aa9d089e6..ce7b69dbb9 100644 --- a/src/fuzz/fuzz-json.c +++ b/src/fuzz/fuzz-json.c @@ -1,6 +1,7 @@ /* SPDX-License-Identifier: LGPL-2.1+ */ #include "alloc-util.h" +#include "fileio.h" #include "fd-util.h" #include "fuzz.h" #include "json.h" @@ -14,13 +15,13 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) { if (size == 0) return 0; - f = fmemopen((char*) data, size, "re"); + f = fmemopen_unlocked((char*) data, size, "re"); assert_se(f); if (json_parse_file(f, NULL, &v, NULL, NULL) < 0) return 0; - g = open_memstream(&out, &out_size); + g = open_memstream_unlocked(&out, &out_size); assert_se(g); json_variant_dump(v, 0, g, NULL); diff --git a/src/fuzz/fuzz-nspawn-oci.c b/src/fuzz/fuzz-nspawn-oci.c index f7b59f13ba..004230eafe 100644 --- a/src/fuzz/fuzz-nspawn-oci.c +++ b/src/fuzz/fuzz-nspawn-oci.c @@ -1,9 +1,8 @@ /* SPDX-License-Identifier: LGPL-2.1+ */ -#include - #include "alloc-util.h" #include "fd-util.h" +#include "fileio.h" #include "fuzz.h" #include "nspawn-oci.h" @@ -14,7 +13,7 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) { if (size == 0) return 0; - f = fmemopen((char*) data, size, "re"); + f = fmemopen_unlocked((char*) data, size, "re"); assert_se(f); /* We don't want to fill the logs with messages about parse errors. diff --git a/src/fuzz/fuzz-nspawn-settings.c b/src/fuzz/fuzz-nspawn-settings.c index 6c81eb773a..aa0a8225b4 100644 --- a/src/fuzz/fuzz-nspawn-settings.c +++ b/src/fuzz/fuzz-nspawn-settings.c @@ -1,9 +1,8 @@ /* SPDX-License-Identifier: LGPL-2.1+ */ -#include - #include "alloc-util.h" #include "fd-util.h" +#include "fileio.h" #include "fuzz.h" #include "nspawn-settings.h" @@ -14,7 +13,7 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) { if (size == 0) return 0; - f = fmemopen((char*) data, size, "re"); + f = fmemopen_unlocked((char*) data, size, "re"); assert_se(f); /* We don't want to fill the logs with messages about parse errors. diff --git a/src/fuzz/fuzz-unit-file.c b/src/fuzz/fuzz-unit-file.c index 84b1ea66ec..d3993cf123 100644 --- a/src/fuzz/fuzz-unit-file.c +++ b/src/fuzz/fuzz-unit-file.c @@ -24,7 +24,7 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) { if (size == 0) return 0; - f = fmemopen((char*) data, size, "re"); + f = fmemopen_unlocked((char*) data, size, "re"); assert_se(f); if (read_line(f, LINE_MAX, &p) < 0) @@ -75,7 +75,7 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) { config_item_perf_lookup, load_fragment_gperf_lookup, CONFIG_PARSE_ALLOW_INCLUDE, u); - g = open_memstream(&out, &out_size); + g = open_memstream_unlocked(&out, &out_size); assert_se(g); unit_dump(u, g, ""); diff --git a/src/libsystemd/sd-bus/test-bus-marshal.c b/src/libsystemd/sd-bus/test-bus-marshal.c index 1e9810ce4f..ade16e532d 100644 --- a/src/libsystemd/sd-bus/test-bus-marshal.c +++ b/src/libsystemd/sd-bus/test-bus-marshal.c @@ -20,6 +20,7 @@ #include "bus-util.h" #include "escape.h" #include "fd-util.h" +#include "fileio.h" #include "log.h" #include "tests.h" #include "util.h" @@ -189,7 +190,7 @@ int main(int argc, char *argv[]) { bus_message_dump(m, stdout, BUS_MESSAGE_DUMP_WITH_HEADER); - ms = open_memstream(&first, &first_size); + ms = open_memstream_unlocked(&first, &first_size); bus_message_dump(m, ms, 0); fflush(ms); assert_se(!ferror(ms)); @@ -245,7 +246,7 @@ int main(int argc, char *argv[]) { bus_message_dump(m, stdout, BUS_MESSAGE_DUMP_WITH_HEADER); fclose(ms); - ms = open_memstream(&second, &second_size); + ms = open_memstream_unlocked(&second, &second_size); bus_message_dump(m, ms, 0); fflush(ms); assert_se(!ferror(ms)); @@ -351,7 +352,7 @@ int main(int argc, char *argv[]) { assert_se(r >= 0); fclose(ms); - ms = open_memstream(&third, &third_size); + ms = open_memstream_unlocked(&third, &third_size); bus_message_dump(copy, ms, 0); fflush(ms); assert_se(!ferror(ms)); diff --git a/src/portable/portablectl.c b/src/portable/portablectl.c index bb6cebdf16..2c59f04eb5 100644 --- a/src/portable/portablectl.c +++ b/src/portable/portablectl.c @@ -12,6 +12,7 @@ #include "dirent-util.h" #include "env-file.h" #include "fd-util.h" +#include "fileio.h" #include "format-table.h" #include "fs-util.h" #include "locale-util.h" @@ -272,10 +273,9 @@ static int inspect_image(int argc, char *argv[], void *userdata) { nl = true; } else { _cleanup_free_ char *pretty_portable = NULL, *pretty_os = NULL; - _cleanup_fclose_ FILE *f; - f = fmemopen((void*) data, sz, "re"); + f = fmemopen_unlocked((void*) data, sz, "re"); if (!f) return log_error_errno(errno, "Failed to open /etc/os-release buffer: %m"); diff --git a/src/shared/bootspec.c b/src/shared/bootspec.c index 3bd14d7372..b2f8936038 100644 --- a/src/shared/bootspec.c +++ b/src/shared/bootspec.c @@ -298,7 +298,7 @@ static int boot_entry_load_unified( if (!k) return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Path is not below root: %s", path); - f = fmemopen((void*) osrelease, strlen(osrelease), "r"); + f = fmemopen_unlocked((void*) osrelease, strlen(osrelease), "r"); if (!f) return log_error_errno(errno, "Failed to open os-release buffer: %m"); diff --git a/src/test/test-fileio.c b/src/test/test-fileio.c index 2ff5b9a69d..cd1db3dd62 100644 --- a/src/test/test-fileio.c +++ b/src/test/test-fileio.c @@ -637,7 +637,7 @@ static void test_fgetc(void) { _cleanup_fclose_ FILE *f = NULL; char c; - f = fmemopen((void*) chars, sizeof(chars), "re"); + f = fmemopen_unlocked((void*) chars, sizeof(chars), "re"); assert_se(f); for (unsigned i = 0; i < sizeof(chars); i++) { @@ -727,7 +727,7 @@ static void test_read_line_one_file(FILE *f) { static void test_read_line(void) { _cleanup_fclose_ FILE *f = NULL; - f = fmemopen((void*) buffer, sizeof(buffer), "re"); + f = fmemopen_unlocked((void*) buffer, sizeof(buffer), "re"); assert_se(f); test_read_line_one_file(f); @@ -792,7 +792,7 @@ static void test_read_line4(void) { _cleanup_fclose_ FILE *f = NULL; _cleanup_free_ char *s = NULL; - assert_se(f = fmemopen((void*) eof_endings[i].string, eof_endings[i].length, "r")); + assert_se(f = fmemopen_unlocked((void*) eof_endings[i].string, eof_endings[i].length, "r")); r = read_line(f, (size_t) -1, &s); assert_se((size_t) r == eof_endings[i].length); @@ -813,7 +813,7 @@ static void test_read_nul_string(void) { _cleanup_fclose_ FILE *f = NULL; _cleanup_free_ char *s = NULL; - assert_se(f = fmemopen((void*) test, sizeof(test)-1, "r")); + assert_se(f = fmemopen_unlocked((void*) test, sizeof(test)-1, "r")); assert_se(read_nul_string(f, LONG_LINE_MAX, &s) == 13 && streq_ptr(s, "string nr. 1")); s = mfree(s); diff --git a/src/test/test-json.c b/src/test/test-json.c index 9b8a2a9422..8bd6a7221c 100644 --- a/src/test/test-json.c +++ b/src/test/test-json.c @@ -4,6 +4,7 @@ #include "alloc-util.h" #include "fd-util.h" +#include "fileio.h" #include "json-internal.h" #include "json.h" #include "string-util.h" @@ -358,7 +359,7 @@ static void test_source(void) { "%s" "--- original end ---\n", data); - assert_se(f = fmemopen((void*) data, strlen(data), "r")); + assert_se(f = fmemopen_unlocked((void*) data, strlen(data), "r")); assert_se(json_parse_file(f, "waldo", &v, NULL, NULL) >= 0);