From ad5d4b1703da43acfa69503c7101cb754066914c Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 10 Jan 2018 17:11:19 +0100 Subject: [PATCH 1/6] cocci: use strempty() at more places This shortens the code by a few lines. --- coccinelle/strempty.cocci | 38 ++++++++++++++++++++++++++++++++ src/basic/fileio.c | 9 +++----- src/basic/string-util.h | 6 ++--- src/core/job.c | 3 +-- src/core/kill.c | 3 +-- src/journal/journal-send.c | 3 +-- src/shared/condition.c | 3 +-- src/udev/udev-builtin-input_id.c | 3 +-- 8 files changed, 49 insertions(+), 19 deletions(-) diff --git a/coccinelle/strempty.cocci b/coccinelle/strempty.cocci index e3bd0a1f56..13ceb338f1 100644 --- a/coccinelle/strempty.cocci +++ b/coccinelle/strempty.cocci @@ -8,3 +8,41 @@ expression s; @@ - s ? s : "" + strempty(s) +@@ +expression s; +@@ +- if (!s) +- s = ""; ++ s = strempty(s); +@@ +expression s; +@@ +- s ?: "(null)" ++ strnull(s) +@@ +expression s; +@@ +- s ? s : "(null)" ++ strnull(s) +@@ +expression s; +@@ +- if (!s) +- s = "(null)"; ++ s = strnull(s); +@@ +expression s; +@@ +- s ?: "n/a" ++ strna(s) +@@ +expression s; +@@ +- s ? s : "n/a" ++ strna(s) +@@ +expression s; +@@ +- if (!s) +- s = "n/a"; ++ s = strna(s); diff --git a/src/basic/fileio.c b/src/basic/fileio.c index 430851cc8c..26d6174664 100644 --- a/src/basic/fileio.c +++ b/src/basic/fileio.c @@ -1225,8 +1225,7 @@ int tempfn_xxxxxx(const char *p, const char *extra, char **ret) { if (!filename_is_valid(fn)) return -EINVAL; - if (!extra) - extra = ""; + extra = strempty(extra); t = new(char, strlen(p) + 2 + strlen(extra) + 6 + 1); if (!t) @@ -1259,8 +1258,7 @@ int tempfn_random(const char *p, const char *extra, char **ret) { if (!filename_is_valid(fn)) return -EINVAL; - if (!extra) - extra = ""; + extra = strempty(extra); t = new(char, strlen(p) + 2 + strlen(extra) + 16 + 1); if (!t) @@ -1300,8 +1298,7 @@ int tempfn_random_child(const char *p, const char *extra, char **ret) { return r; } - if (!extra) - extra = ""; + extra = strempty(extra); t = new(char, strlen(p) + 3 + strlen(extra) + 16 + 1); if (!t) diff --git a/src/basic/string-util.h b/src/basic/string-util.h index 09a737ad37..d8f97e697b 100644 --- a/src/basic/string-util.h +++ b/src/basic/string-util.h @@ -52,15 +52,15 @@ static inline bool streq_ptr(const char *a, const char *b) { } static inline const char* strempty(const char *s) { - return s ? s : ""; + return s ?: ""; } static inline const char* strnull(const char *s) { - return s ? s : "(null)"; + return s ?: "(null)"; } static inline const char *strna(const char *s) { - return s ? s : "n/a"; + return s ?: "n/a"; } static inline bool isempty(const char *p) { diff --git a/src/core/job.c b/src/core/job.c index b046698118..c6de8d27e4 100644 --- a/src/core/job.c +++ b/src/core/job.c @@ -306,8 +306,7 @@ void job_dump(Job *j, FILE*f, const char *prefix) { assert(j); assert(f); - if (!prefix) - prefix = ""; + prefix = strempty(prefix); fprintf(f, "%s-> Job %u:\n" diff --git a/src/core/kill.c b/src/core/kill.c index f438c4d8fa..5dfcb780fa 100644 --- a/src/core/kill.c +++ b/src/core/kill.c @@ -34,8 +34,7 @@ void kill_context_init(KillContext *c) { void kill_context_dump(KillContext *c, FILE *f, const char *prefix) { assert(c); - if (!prefix) - prefix = ""; + prefix = strempty(prefix); fprintf(f, "%sKillMode: %s\n" diff --git a/src/journal/journal-send.c b/src/journal/journal-send.c index a78aa07032..b322127a4e 100644 --- a/src/journal/journal-send.c +++ b/src/journal/journal-send.c @@ -418,8 +418,7 @@ _public_ int sd_journal_stream_fd(const char *identifier, int priority, int leve fd_inc_sndbuf(fd, SNDBUF_SIZE); - if (!identifier) - identifier = ""; + identifier = strempty(identifier); l = strlen(identifier); header = alloca(l + 1 + 1 + 2 + 2 + 2 + 2 + 2); diff --git a/src/shared/condition.c b/src/shared/condition.c index 7a825c3973..a2fd05c425 100644 --- a/src/shared/condition.c +++ b/src/shared/condition.c @@ -653,8 +653,7 @@ void condition_dump(Condition *c, FILE *f, const char *prefix, const char *(*to_ assert(c); assert(f); - if (!prefix) - prefix = ""; + prefix = strempty(prefix); fprintf(f, "%s\t%s: %s%s%s %s\n", diff --git a/src/udev/udev-builtin-input_id.c b/src/udev/udev-builtin-input_id.c index b13fdb135b..fa830213ff 100644 --- a/src/udev/udev-builtin-input_id.c +++ b/src/udev/udev-builtin-input_id.c @@ -102,8 +102,7 @@ static void get_cap_mask(struct udev_device *dev, unsigned long val; v = udev_device_get_sysattr_value(pdev, attr); - if (!v) - v = ""; + v = strempty(v); xsprintf(text, "%s", v); log_debug("%s raw kernel attribute: %s", attr, text); From dfd14786b5aa49c3c8e3866c0ecfa6d90c531eb6 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 10 Jan 2018 17:21:15 +0100 Subject: [PATCH 2/6] util-lib: save/restore errno in cleanup calls We should be careful with errno in cleanup functions, and not alter it under any circumstances. In the safe_close cleanup handlers we are already safe in that regard, but let's add similar protections on other cleanup handlers that invoke system calls. Why bother? Cleanup handlers insert code at function return in non-obvious ways. Hence, code that sets errno and returns should not be confused by us overrding the errno from a cleanup handler. This is a paranoia fix only, I am not aware where this actually mattered in real-life situations. --- src/basic/fs-util.h | 4 +++- src/basic/process-util.c | 2 ++ src/basic/rm-rf.h | 3 +++ 3 files changed, 8 insertions(+), 1 deletion(-) diff --git a/src/basic/fs-util.h b/src/basic/fs-util.h index a7ba61625d..82a5b2028d 100644 --- a/src/basic/fs-util.h +++ b/src/basic/fs-util.h @@ -29,6 +29,7 @@ #include #include "time-util.h" +#include "util.h" int unlink_noerrno(const char *path); @@ -89,13 +90,14 @@ int chase_symlinks(const char *path_with_prefix, const char *root, unsigned flag /* Useful for usage with _cleanup_(), removes a directory and frees the pointer */ static inline void rmdir_and_free(char *p) { + PROTECT_ERRNO; (void) rmdir(p); free(p); } DEFINE_TRIVIAL_CLEANUP_FUNC(char*, rmdir_and_free); static inline void unlink_and_free(char *p) { - (void) unlink(p); + (void) unlink_noerrno(p); free(p); } DEFINE_TRIVIAL_CLEANUP_FUNC(char*, unlink_and_free); diff --git a/src/basic/process-util.c b/src/basic/process-util.c index ce6c0645bb..9fc5464665 100644 --- a/src/basic/process-util.c +++ b/src/basic/process-util.c @@ -796,6 +796,8 @@ void sigkill_wait(pid_t pid) { } void sigkill_waitp(pid_t *pid) { + PROTECT_ERRNO; + if (!pid) return; if (*pid <= 1) diff --git a/src/basic/rm-rf.h b/src/basic/rm-rf.h index 1127e326b2..ad63e9be40 100644 --- a/src/basic/rm-rf.h +++ b/src/basic/rm-rf.h @@ -22,6 +22,8 @@ #include +#include "util.h" + typedef enum RemoveFlags { REMOVE_ONLY_DIRECTORIES = 1, REMOVE_ROOT = 2, @@ -34,6 +36,7 @@ int rm_rf(const char *path, RemoveFlags flags); /* Useful for usage with _cleanup_(), destroys a directory and frees the pointer */ static inline void rm_rf_physical_and_free(char *p) { + PROTECT_ERRNO; (void) rm_rf(p, REMOVE_ROOT|REMOVE_PHYSICAL); free(p); } From f1ff734fad70852e59a1a5245e0b7a75672d077e Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 10 Jan 2018 17:28:03 +0100 Subject: [PATCH 3/6] tmpfiles: modernize load_unix_sockets() a bit Let's log in case of error. Let's use read_line() instead of a fixed-size buffer Let's make use of set_free_free()'s return value. --- src/tmpfiles/tmpfiles.c | 45 +++++++++++++++++++++++++++-------------- 1 file changed, 30 insertions(+), 15 deletions(-) diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c index a7ce1a8049..d69073535c 100644 --- a/src/tmpfiles/tmpfiles.c +++ b/src/tmpfiles/tmpfiles.c @@ -375,35 +375,47 @@ static struct Item* find_glob(OrderedHashmap *h, const char *match) { static void load_unix_sockets(void) { _cleanup_fclose_ FILE *f = NULL; - char line[LINE_MAX]; + int r; if (unix_sockets) return; - /* We maintain a cache of the sockets we found in - * /proc/net/unix to speed things up a little. */ + /* We maintain a cache of the sockets we found in /proc/net/unix to speed things up a little. */ unix_sockets = set_new(&string_hash_ops); if (!unix_sockets) return; f = fopen("/proc/net/unix", "re"); - if (!f) - return; + if (!f) { + log_full_errno(errno == ENOENT ? LOG_DEBUG : LOG_WARNING, errno, + "Failed to open /proc/net/unix, ignoring: %m"); + goto fail; + } /* Skip header */ - if (!fgets(line, sizeof(line), f)) + r = read_line(f, LONG_LINE_MAX, NULL); + if (r < 0) { + log_warning_errno(r, "Failed to skip /proc/net/unix header line: %m"); goto fail; + } + if (r == 0) { + log_warning("Premature end of file reading /proc/net/unix."); + goto fail; + } for (;;) { + _cleanup_free_ char *line = NULL; char *p, *s; - int k; - if (!fgets(line, sizeof(line), f)) + r = read_line(f, LONG_LINE_MAX, &line); + if (r < 0) { + log_warning_errno(r, "Failed to read /proc/net/unix line, ignoring: %m"); + goto fail; + } + if (r == 0) /* EOF */ break; - truncate_nl(line); - p = strchr(line, ':'); if (!p) continue; @@ -420,21 +432,24 @@ static void load_unix_sockets(void) { continue; s = strdup(p); - if (!s) + if (!s) { + log_oom(); goto fail; + } path_kill_slashes(s); - k = set_consume(unix_sockets, s); - if (k < 0 && k != -EEXIST) + r = set_consume(unix_sockets, s); + if (r < 0 && r != -EEXIST) { + log_warning_errno(r, "Failed to add AF_UNIX socket to set, ignoring: %m"); goto fail; + } } return; fail: - set_free_free(unix_sockets); - unix_sockets = NULL; + unix_sockets = set_free_free(unix_sockets); } static bool unix_socket_alive(const char *fn) { From 24eccc3414a29a14b319d639531bd23c158b20e1 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 10 Jan 2018 18:26:03 +0100 Subject: [PATCH 4/6] nss-systemd,user-util: add a way how synthesizing "nobody" can be turned off This is quite ugly, but provides us with an avenue for moving distributions to define the "nobody" user properly without breaking legacy systems that us the name for other stuff. The idea is basically, that the distribution adopts the new definition of "nobody" (and thus recompiles systemd with it) and then touches /etc/systemd/dont-synthesize-nobody on legacy systems to turn off possibly conflicting synthesizing of the nobody name by systemd. --- src/basic/user-util.c | 39 +++++++++++++++++++++++++++++------ src/basic/user-util.h | 2 ++ src/nss-systemd/nss-systemd.c | 12 +++++++---- 3 files changed, 43 insertions(+), 10 deletions(-) diff --git a/src/basic/user-util.c b/src/basic/user-util.c index b99775c18f..17a9b5a8f1 100644 --- a/src/basic/user-util.c +++ b/src/basic/user-util.c @@ -137,7 +137,8 @@ int get_user_creds( return 0; } - if (STR_IN_SET(*username, NOBODY_USER_NAME, "65534")) { + if (synthesize_nobody() && + STR_IN_SET(*username, NOBODY_USER_NAME, "65534")) { *username = NOBODY_USER_NAME; if (uid) @@ -243,7 +244,8 @@ int get_group_creds(const char **groupname, gid_t *gid) { return 0; } - if (STR_IN_SET(*groupname, NOBODY_GROUP_NAME, "65534")) { + if (synthesize_nobody() && + STR_IN_SET(*groupname, NOBODY_GROUP_NAME, "65534")) { *groupname = NOBODY_GROUP_NAME; if (gid) @@ -283,7 +285,8 @@ char* uid_to_name(uid_t uid) { /* Shortcut things to avoid NSS lookups */ if (uid == 0) return strdup("root"); - if (uid == UID_NOBODY) + if (synthesize_nobody() && + uid == UID_NOBODY) return strdup(NOBODY_USER_NAME); if (uid_is_valid(uid)) { @@ -323,7 +326,8 @@ char* gid_to_name(gid_t gid) { if (gid == 0) return strdup("root"); - if (gid == GID_NOBODY) + if (synthesize_nobody() && + gid == GID_NOBODY) return strdup(NOBODY_GROUP_NAME); if (gid_is_valid(gid)) { @@ -427,7 +431,8 @@ int get_home_dir(char **_h) { *_h = h; return 0; } - if (u == UID_NOBODY) { + if (synthesize_nobody() && + u == UID_NOBODY) { h = strdup("/"); if (!h) return -ENOMEM; @@ -482,7 +487,8 @@ int get_shell(char **_s) { *_s = s; return 0; } - if (u == UID_NOBODY) { + if (synthesize_nobody() && + u == UID_NOBODY) { s = strdup("/sbin/nologin"); if (!s) return -ENOMEM; @@ -690,3 +696,24 @@ int maybe_setgroups(size_t size, const gid_t *list) { return 0; } + +bool synthesize_nobody(void) { + +#ifdef NOLEGACY + return true; +#else + /* Returns true when we shall synthesize the "nobody" user (which we do by default). This can be turned off by + * touching /etc/systemd/dont-synthesize-nobody in order to provide upgrade compatibility with legacy systems + * that used the "nobody" user name and group name for other UIDs/GIDs than 65534. + * + * Note that we do not employ any kind of synchronization on the following caching variable. If the variable is + * accessed in multi-threaded programs in the worst case it might happen that we initialize twice, but that + * shouldn't matter as each initialization should come to the same result. */ + static int cache = -1; + + if (cache < 0) + cache = access("/etc/systemd/dont-synthesize-nobody", F_OK) < 0; + + return cache; +#endif +} diff --git a/src/basic/user-util.h b/src/basic/user-util.h index 79adf91ee9..5f0391f2b8 100644 --- a/src/basic/user-util.h +++ b/src/basic/user-util.h @@ -97,3 +97,5 @@ bool valid_gecos(const char *d); bool valid_home(const char *p); int maybe_setgroups(size_t size, const gid_t *list); + +bool synthesize_nobody(void); diff --git a/src/nss-systemd/nss-systemd.c b/src/nss-systemd/nss-systemd.c index cc641e1615..f75405d2e5 100644 --- a/src/nss-systemd/nss-systemd.c +++ b/src/nss-systemd/nss-systemd.c @@ -136,7 +136,8 @@ enum nss_status _nss_systemd_getpwnam_r( *errnop = 0; return NSS_STATUS_SUCCESS; } - if (streq(name, nobody_passwd.pw_name)) { + if (synthesize_nobody() && + streq(name, nobody_passwd.pw_name)) { *pwd = nobody_passwd; *errnop = 0; return NSS_STATUS_SUCCESS; @@ -244,7 +245,8 @@ enum nss_status _nss_systemd_getpwuid_r( *errnop = 0; return NSS_STATUS_SUCCESS; } - if (uid == nobody_passwd.pw_uid) { + if (synthesize_nobody() && + uid == nobody_passwd.pw_uid) { *pwd = nobody_passwd; *errnop = 0; return NSS_STATUS_SUCCESS; @@ -351,7 +353,8 @@ enum nss_status _nss_systemd_getgrnam_r( *errnop = 0; return NSS_STATUS_SUCCESS; } - if (streq(name, nobody_group.gr_name)) { + if (synthesize_nobody() && + streq(name, nobody_group.gr_name)) { *gr = nobody_group; *errnop = 0; return NSS_STATUS_SUCCESS; @@ -456,7 +459,8 @@ enum nss_status _nss_systemd_getgrgid_r( *errnop = 0; return NSS_STATUS_SUCCESS; } - if (gid == nobody_group.gr_gid) { + if (synthesize_nobody() && + gid == nobody_group.gr_gid) { *gr = nobody_group; *errnop = 0; return NSS_STATUS_SUCCESS; From 47cf8ff206b8fd76340fe8d704e4af9eb59f11f3 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 10 Jan 2018 18:28:42 +0100 Subject: [PATCH 5/6] manager: rework manager_clean_environment() Let's rename it manager_sanitize_environment() which is a more precise name. Moreover, sort the environment implicitly inside it, as all our callers do that anyway afterwards and we can save some code this way. Also, update the list of env vars to drop, i.e. the env vars we manage ourselves and don't want user code to interfear with. Also sort this list to make it easier to update later on. --- src/core/manager.c | 30 ++++++++++++++++++------------ 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/src/core/manager.c b/src/core/manager.c index 860d58617f..a9c4330897 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -513,23 +513,31 @@ static int manager_setup_signals(Manager *m) { return 0; } -static void manager_clean_environment(Manager *m) { +static void manager_sanitize_environment(Manager *m) { assert(m); - /* Let's remove some environment variables that we - * need ourselves to communicate with our clients */ + /* Let's remove some environment variables that we need ourselves to communicate with our clients */ strv_env_unset_many( m->environment, - "NOTIFY_SOCKET", + "EXIT_CODE", + "EXIT_STATUS", + "INVOCATION_ID", + "JOURNAL_STREAM", + "LISTEN_FDNAMES", + "LISTEN_FDS", + "LISTEN_PID", "MAINPID", "MANAGERPID", - "LISTEN_PID", - "LISTEN_FDS", - "LISTEN_FDNAMES", + "NOTIFY_SOCKET", + "REMOTE_ADDR", + "REMOTE_PORT", + "SERVICE_RESULT", "WATCHDOG_PID", "WATCHDOG_USEC", - "INVOCATION_ID", NULL); + + /* Let's order the environment alphabetically, just to make it pretty */ + strv_sort(m->environment); } static int manager_default_environment(Manager *m) { @@ -556,8 +564,7 @@ static int manager_default_environment(Manager *m) { if (!m->environment) return -ENOMEM; - manager_clean_environment(m); - strv_sort(m->environment); + manager_sanitize_environment(m); return 0; } @@ -3308,8 +3315,7 @@ int manager_environment_add(Manager *m, char **minus, char **plus) { strv_free(b); m->environment = l; - manager_clean_environment(m); - strv_sort(m->environment); + manager_sanitize_environment(m); return 0; } From e557b1a655561906343918fa30959b53d16c3fdd Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 10 Jan 2018 18:37:54 +0100 Subject: [PATCH 6/6] util: minor tweaks to disable_core_dumps() First, let's rename it to disable_coredumps(), as in the rest of our codebase we spell it "coredump" rather than "core_dump", so let's stick to that. However, also log about failures to turn off core dumpling on LOG_DEBUG, because debug logging is always a good idea. --- src/basic/util.c | 12 +++++++++--- src/basic/util.h | 2 +- src/core/main.c | 2 +- src/core/shutdown.c | 3 +-- src/coredump/coredump.c | 2 +- 5 files changed, 13 insertions(+), 8 deletions(-) diff --git a/src/basic/util.c b/src/basic/util.c index b1e6f5002e..2d31d84165 100644 --- a/src/basic/util.c +++ b/src/basic/util.c @@ -618,7 +618,13 @@ int str_verscmp(const char *s1, const char *s2) { } /* Turn off core dumps but only if we're running outside of a container. */ -void disable_core_dumps(void) { - if (detect_container() <= 0) - (void) write_string_file("/proc/sys/kernel/core_pattern", "|/bin/false", 0); +void disable_coredumps(void) { + int r; + + if (detect_container() > 0) + return; + + r = write_string_file("/proc/sys/kernel/core_pattern", "|/bin/false", 0); + if (r < 0) + log_debug_errno(r, "Failed to turn off coredumps, ignoring: %m"); } diff --git a/src/basic/util.h b/src/basic/util.h index c10cfea952..9d1b10756b 100644 --- a/src/basic/util.h +++ b/src/basic/util.h @@ -192,4 +192,4 @@ int version(void); int str_verscmp(const char *s1, const char *s2); -void disable_core_dumps(void); +void disable_coredumps(void); diff --git a/src/core/main.c b/src/core/main.c index ef7d7f58dc..69a98df63c 100644 --- a/src/core/main.c +++ b/src/core/main.c @@ -1604,7 +1604,7 @@ static void initialize_coredump(bool skip_setup) { /* But at the same time, turn off the core_pattern logic by default, so that no coredumps are stored * until the systemd-coredump tool is enabled via sysctl. */ if (!skip_setup) - disable_core_dumps(); + disable_coredumps(); } static void do_reexecute( diff --git a/src/core/shutdown.c b/src/core/shutdown.c index b1f581b548..ac99211e03 100644 --- a/src/core/shutdown.c +++ b/src/core/shutdown.c @@ -323,8 +323,7 @@ int main(int argc, char *argv[]) { if (!in_container) sync_with_progress(); - /* Prevent coredumps */ - disable_core_dumps(); + disable_coredumps(); log_info("Sending SIGTERM to remaining processes..."); broadcast_signal(SIGTERM, true, true); diff --git a/src/coredump/coredump.c b/src/coredump/coredump.c index f83a951df7..fdcea22f56 100644 --- a/src/coredump/coredump.c +++ b/src/coredump/coredump.c @@ -1126,7 +1126,7 @@ static int gather_pid_metadata( /* If this is PID 1 disable coredump collection, we'll unlikely be able to process it later on. */ if (is_pid1_crash((const char**) context)) { log_notice("Due to PID 1 having crashed coredump collection will now be turned off."); - disable_core_dumps(); + disable_coredumps(); } set_iovec_field(iovec, n_iovec, "COREDUMP_UNIT=", context[CONTEXT_UNIT]);