From 616c53544fa280a05545a88ce3a6bb0680bb4650 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 31 Jan 2023 13:47:17 +0100 Subject: [PATCH 1/8] sysusers: drop counterproductive bitfield annotations The usual story: $ diff -u <(pahole build/systemd-sysusers.0) <(pahole build/systemd-sysusers) /* size: 80, cachelines: 2, members: 15 */ - /* sum members: 68, holes: 1, sum holes: 4 */ - /* sum bitfield members: 5 bits (0 bytes) */ - /* padding: 7 */ - /* bit_padding: 3 bits */ + /* sum members: 73, holes: 1, sum holes: 4 */ + /* padding: 3 */ /* last cacheline: 16 bytes */ Effectively, because of padding, we were not saving anything. We're not putting struct Item in arrays, but when allocating on the heap, we're going to round up to normal alignment too. The code becomes shorter (and quicker): $ size build/systemd-sysusers{,.0} text data bss dec hex filename 79967 2040 264 82271 1415f build/systemd-sysusers.0 79726 2040 264 82030 1406e build/systemd-sysusers (In case you're wondering, I wrote this long commit message for a very simple change on purpose: I want to deflate the bitfield cargo cult a bit.) --- src/sysusers/sysusers.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/sysusers/sysusers.c b/src/sysusers/sysusers.c index 6e197b964d..4a695fcfab 100644 --- a/src/sysusers/sysusers.c +++ b/src/sysusers/sysusers.c @@ -76,17 +76,17 @@ typedef struct Item { gid_t gid; uid_t uid; - bool gid_set:1; + bool gid_set; /* When set the group with the specified GID must exist * and the check if a UID clashes with the GID is skipped. */ - bool id_set_strict:1; + bool id_set_strict; - bool uid_set:1; + bool uid_set; - bool todo_user:1; - bool todo_group:1; + bool todo_user; + bool todo_group; } Item; static char *arg_root = NULL; From 5afdb4629aeb2cd20f3f7ba7e4b10f5d92be6a76 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 31 Jan 2023 14:21:49 +0100 Subject: [PATCH 2/8] pid1,sysusers: drop unused SYNTHETIC_ERRNO The only function of SYNTHETIC_ERRNO is to set the return value. If we're ignoring the return value, it shouldn't be used. --- src/core/load-fragment.c | 2 +- src/sysusers/sysusers.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c index 0ed2d1b31d..ce15758901 100644 --- a/src/core/load-fragment.c +++ b/src/core/load-fragment.c @@ -4606,7 +4606,7 @@ int config_parse_exec_directories( if (r == -ENOMEM) return log_oom(); if (r <= 0) { - log_syntax(unit, LOG_WARNING, filename, line, r ?: SYNTHETIC_ERRNO(EINVAL), + log_syntax(unit, LOG_WARNING, filename, line, r, "Invalid syntax in %s=, ignoring: %s", lvalue, tuple); return 0; } diff --git a/src/sysusers/sysusers.c b/src/sysusers/sysusers.c index 4a695fcfab..4c31f96301 100644 --- a/src/sysusers/sysusers.c +++ b/src/sysusers/sysusers.c @@ -1811,7 +1811,7 @@ static int parse_line(const char *fname, unsigned line, const char *buffer) { if (r < 0) return r; if (r == 0) - log_syntax(NULL, LOG_WARNING, fname, line, SYNTHETIC_ERRNO(EUCLEAN), + log_syntax(NULL, LOG_WARNING, fname, line, 0, "Conflict with earlier configuration for %s '%s', ignoring line.", item_type_to_string(i->type), i->name); From 9a87bdd7ed9fb78fdcd404ef87746f37544e7966 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 31 Jan 2023 14:16:48 +0100 Subject: [PATCH 3/8] sysusers: add helper to create new Item --- src/sysusers/sysusers.c | 84 ++++++++++++++++++++++++----------------- 1 file changed, 50 insertions(+), 34 deletions(-) diff --git a/src/sysusers/sysusers.c b/src/sysusers/sysusers.c index 4c31f96301..c34f3991b3 100644 --- a/src/sysusers/sysusers.c +++ b/src/sysusers/sysusers.c @@ -76,6 +76,9 @@ typedef struct Item { gid_t gid; uid_t uid; + char *filename; + unsigned line; + bool gid_set; /* When set the group with the specified GID must exist @@ -1412,12 +1415,33 @@ static Item* item_free(Item *i) { free(i->description); free(i->home); free(i->shell); + free(i->filename); return mfree(i); } DEFINE_TRIVIAL_CLEANUP_FUNC(Item*, item_free); DEFINE_PRIVATE_HASH_OPS_WITH_VALUE_DESTRUCTOR(item_hash_ops, char, string_hash_func, string_compare_func, Item, item_free); +static Item* item_new(ItemType type, const char *name, const char *filename, unsigned line) { + assert(name); + assert(!!filename == (line > 0)); + + _cleanup_(item_freep) Item *new = new(Item, 1); + if (!new) + return NULL; + + *new = (Item) { + .type = type, + .line = line, + }; + + if (free_and_strdup(&new->name, name) < 0 || + free_and_strdup(&new->filename, filename) < 0) + return NULL; + + return TAKE_PTR(new); +} + static int add_implicit(void) { char *g, **l; int r; @@ -1426,17 +1450,11 @@ static int add_implicit(void) { ORDERED_HASHMAP_FOREACH_KEY(l, g, members) { STRV_FOREACH(m, l) if (!ordered_hashmap_get(users, *m)) { - _cleanup_(item_freep) Item *j = NULL; - - j = new0(Item, 1); + _cleanup_(item_freep) Item *j = + item_new(ADD_USER, *m, /* filename= */ NULL, /* line= */ 0); if (!j) return log_oom(); - j->type = ADD_USER; - j->name = strdup(*m); - if (!j->name) - return log_oom(); - r = ordered_hashmap_ensure_put(&users, &item_hash_ops, j->name, j); if (r == -ENOMEM) return log_oom(); @@ -1449,17 +1467,11 @@ static int add_implicit(void) { if (!(ordered_hashmap_get(users, g) || ordered_hashmap_get(groups, g))) { - _cleanup_(item_freep) Item *j = NULL; - - j = new0(Item, 1); + _cleanup_(item_freep) Item *j = + item_new(ADD_GROUP, g, /* filename= */ NULL, /* line= */ 0); if (!j) return log_oom(); - j->type = ADD_GROUP; - j->name = strdup(g); - if (!j->name) - return log_oom(); - r = ordered_hashmap_ensure_put(&groups, &item_hash_ops, j->name, j); if (r == -ENOMEM) return log_oom(); @@ -1720,15 +1732,14 @@ static int parse_line(const char *fname, unsigned line, const char *buffer) { if (r < 0) return log_oom(); - i = new0(Item, 1); + i = item_new(ADD_USER, resolved_name, fname, line); if (!i) return log_oom(); if (resolved_id) { - if (path_is_absolute(resolved_id)) { - i->uid_path = TAKE_PTR(resolved_id); - path_simplify(i->uid_path); - } else { + if (path_is_absolute(resolved_id)) + i->uid_path = path_simplify(TAKE_PTR(resolved_id)); + else { _cleanup_free_ char *uid = NULL, *gid = NULL; if (split_pair(resolved_id, ":", &uid, &gid) == 0) { r = parse_gid(gid, &i->gid); @@ -1776,15 +1787,14 @@ static int parse_line(const char *fname, unsigned line, const char *buffer) { if (r < 0) return log_oom(); - i = new0(Item, 1); + i = item_new(ADD_GROUP, resolved_name, fname, line); if (!i) return log_oom(); if (resolved_id) { - if (path_is_absolute(resolved_id)) { - i->gid_path = TAKE_PTR(resolved_id); - path_simplify(i->gid_path); - } else { + if (path_is_absolute(resolved_id)) + i->gid_path = path_simplify(TAKE_PTR(resolved_id)); + else { r = parse_gid(resolved_id, &i->gid); if (r < 0) return log_syntax(NULL, LOG_ERR, fname, line, r, @@ -1798,22 +1808,28 @@ static int parse_line(const char *fname, unsigned line, const char *buffer) { break; default: - return -EBADMSG; + assert_not_reached(); } - i->type = action[0]; - i->name = TAKE_PTR(resolved_name); - existing = ordered_hashmap_get(h, i->name); if (existing) { /* Two functionally-equivalent items are fine */ r = item_equivalent(existing, i); if (r < 0) return r; - if (r == 0) - log_syntax(NULL, LOG_WARNING, fname, line, 0, - "Conflict with earlier configuration for %s '%s', ignoring line.", - item_type_to_string(i->type), i->name); + if (r == 0) { + if (existing->filename) + log_syntax(NULL, LOG_WARNING, fname, line, 0, + "Conflict with earlier configuration for %s '%s' in %s:%u, ignoring line.", + item_type_to_string(i->type), + i->name, + existing->filename, existing->line); + else + log_syntax(NULL, LOG_WARNING, fname, line, 0, + "Conflict with earlier configuration for %s '%s', ignoring line.", + item_type_to_string(i->type), + i->name); + } return 0; } From 7519b880e73859156e4fb64a7db870a9bfb72b9f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 31 Jan 2023 15:10:03 +0100 Subject: [PATCH 4/8] sysusers: when comparing items, log debug the difference --- src/sysusers/sysusers.c | 71 +++++++++++++++++++++++++++++++---------- 1 file changed, 55 insertions(+), 16 deletions(-) diff --git a/src/sysusers/sysusers.c b/src/sysusers/sysusers.c index c34f3991b3..3c438a38f1 100644 --- a/src/sysusers/sysusers.c +++ b/src/sysusers/sysusers.c @@ -135,6 +135,14 @@ static int errno_is_not_exists(int code) { return IN_SET(code, 0, ENOENT, ESRCH, EBADF, EPERM); } +/* Note: the lifetime of the compound literal is the immediately surrounding block, + * see C11 §6.5.2.5, and + * https://stackoverflow.com/questions/34880638/compound-literal-lifetime-and-if-blocks */ +#define FORMAT_UID(is_set, uid) \ + ((is_set) ? snprintf_ok((char[DECIMAL_STR_MAX(uid_t)]){}, DECIMAL_STR_MAX(uid_t), UID_FMT, uid) : "(unset)") +#define FORMAT_GID(is_set, gid) \ + ((is_set) ? snprintf_ok((char[DECIMAL_STR_MAX(gid_t)]){}, DECIMAL_STR_MAX(gid_t), GID_FMT, gid) : "(unset)") + static void maybe_emit_login_defs_warning(void) { if (!login_defs_need_warning) return; @@ -1492,36 +1500,63 @@ static int item_equivalent(Item *a, Item *b) { assert(a); assert(b); - if (a->type != b->type) + if (a->type != b->type) { + log_syntax(NULL, LOG_DEBUG, a->filename, a->line, 0, + "Item not equivalent because types differ"); return false; + } - if (!streq_ptr(a->name, b->name)) + if (!streq_ptr(a->name, b->name)) { + log_syntax(NULL, LOG_DEBUG, a->filename, a->line, 0, + "Item not equivalent because names differ ('%s' vs. '%s')", + a->name, b->name); return false; + } /* Paths were simplified previously, so we can use streq. */ - if (!streq_ptr(a->uid_path, b->uid_path)) + if (!streq_ptr(a->uid_path, b->uid_path)) { + log_syntax(NULL, LOG_DEBUG, a->filename, a->line, 0, + "Item not equivalent because UID paths differ (%s vs. %s)", + a->uid_path ?: "(unset)", b->uid_path ?: "(unset)"); return false; + } - if (!streq_ptr(a->gid_path, b->gid_path)) + if (!streq_ptr(a->gid_path, b->gid_path)) { + log_syntax(NULL, LOG_DEBUG, a->filename, a->line, 0, + "Item not equivalent because GID paths differ (%s vs. %s)", + a->gid_path ?: "(unset)", b->gid_path ?: "(unset)"); return false; + } - if (!streq_ptr(a->description, b->description)) + if (!streq_ptr(a->description, b->description)) { + log_syntax(NULL, LOG_DEBUG, a->filename, a->line, 0, + "Item not equivalent because descriptions differ ('%s' vs. '%s')", + strempty(a->description), strempty(b->description)); return false; + } - if (a->uid_set != b->uid_set) + if ((a->uid_set != b->uid_set) || + (a->uid_set && a->uid != b->uid)) { + log_syntax(NULL, LOG_DEBUG, a->filename, a->line, 0, + "Item not equivalent because UIDs differ (%s vs. %s)", + FORMAT_UID(a->uid_set, a->uid), FORMAT_UID(b->uid_set, b->uid)); return false; + } - if (a->uid_set && a->uid != b->uid) + if ((a->gid_set != b->gid_set) || + (a->gid_set && a->gid != b->gid)) { + log_syntax(NULL, LOG_DEBUG, a->filename, a->line, 0, + "Item not equivalent because GIDs differ (%s vs. %s)", + FORMAT_GID(a->gid_set, a->gid), FORMAT_GID(b->gid_set, b->gid)); return false; + } - if (a->gid_set != b->gid_set) - return false; - - if (a->gid_set && a->gid != b->gid) - return false; - - if (!streq_ptr(a->home, b->home)) + if (!streq_ptr(a->home, b->home)) { + log_syntax(NULL, LOG_DEBUG, a->filename, a->line, 0, + "Item not equivalent because home directories differ ('%s' vs. '%s')", + strempty(a->description), strempty(b->description)); return false; + } /* Check if the two paths refer to the same file. * If the paths are equal (after normalization), it's obviously the same file. @@ -1552,8 +1587,12 @@ static int item_equivalent(Item *a, Item *b) { return ERRNO_IS_RESOURCE(r) ? r : false; } - if (!path_equal(pa, pb)) + if (!path_equal(pa, pb)) { + log_syntax(NULL, LOG_DEBUG, a->filename, a->line, 0, + "Item not equivalent because shells differ ('%s' vs. '%s')", + pa, pb); return false; + } } return true; @@ -1814,7 +1853,7 @@ static int parse_line(const char *fname, unsigned line, const char *buffer) { existing = ordered_hashmap_get(h, i->name); if (existing) { /* Two functionally-equivalent items are fine */ - r = item_equivalent(existing, i); + r = item_equivalent(i, existing); if (r < 0) return r; if (r == 0) { From fddb524d8a9fcfbf943c88b5c7d13eba7ac0083d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 31 Jan 2023 16:16:55 +0100 Subject: [PATCH 5/8] basic: reword some comments Without commas, the sentences can be hard to parse. --- src/basic/path-util.c | 12 ++++++------ src/basic/user-util.c | 13 +++++-------- 2 files changed, 11 insertions(+), 14 deletions(-) diff --git a/src/basic/path-util.c b/src/basic/path-util.c index b442eebb87..40a819d47d 100644 --- a/src/basic/path-util.c +++ b/src/basic/path-util.c @@ -519,17 +519,17 @@ char* path_extend_internal(char **x, ...) { va_list ap; bool slash; - /* Joins all listed strings until the sentinel and places a "/" between them unless the strings end/begin - * already with one so that it is unnecessary. Note that slashes which are already duplicate won't be - * removed. The string returned is hence always equal to or longer than the sum of the lengths of each - * individual string. + /* Joins all listed strings until the sentinel and places a "/" between them unless the strings + * end/begin already with one so that it is unnecessary. Note that slashes which are already + * duplicate won't be removed. The string returned is hence always equal to or longer than the sum of + * the lengths of the individual strings. * * The first argument may be an already allocated string that is extended via realloc() if * non-NULL. path_extend() and path_join() are macro wrappers around this function, making use of the * first parameter to distinguish the two operations. * - * Note: any listed empty string is simply skipped. This can be useful for concatenating strings of which some - * are optional. + * Note: any listed empty string is simply skipped. This can be useful for concatenating strings of + * which some are optional. * * Examples: * diff --git a/src/basic/user-util.c b/src/basic/user-util.c index e2857b3102..3daca626b8 100644 --- a/src/basic/user-util.c +++ b/src/basic/user-util.c @@ -681,15 +681,12 @@ int take_etc_passwd_lock(const char *root) { const char *path; int fd, r; - /* This is roughly the same as lckpwdf(), but not as awful. We - * don't want to use alarm() and signals, hence we implement - * our own trivial version of this. + /* This is roughly the same as lckpwdf(), but not as awful. We don't want to use alarm() and signals, + * hence we implement our own trivial version of this. * - * Note that shadow-utils also takes per-database locks in - * addition to lckpwdf(). However, we don't given that they - * are redundant as they invoke lckpwdf() first and keep - * it during everything they do. The per-database locks are - * awfully racy, and thus we just won't do them. */ + * Note that shadow-utils also takes per-database locks in addition to lckpwdf(). However, we don't, + * given that they are redundant: they invoke lckpwdf() first and keep it during everything they do. + * The per-database locks are awfully racy, and thus we just won't do them. */ if (root) path = prefix_roota(root, ETC_PASSWD_LOCK_PATH); From e5b30f7232acfc2e99bce50ac1e683deaec57140 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 31 Jan 2023 16:17:12 +0100 Subject: [PATCH 6/8] =?UTF-8?q?basic/user-util:=20convert=20prefix=5Froota?= =?UTF-8?q?=E2=86=92path=5Fjoin=20and=20use=20=5Fcleanup=5F=20more?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/basic/user-util.c | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/src/basic/user-util.c b/src/basic/user-util.c index 3daca626b8..734122c2a3 100644 --- a/src/basic/user-util.c +++ b/src/basic/user-util.c @@ -670,7 +670,6 @@ int reset_uid_gid(void) { } int take_etc_passwd_lock(const char *root) { - struct flock flock = { .l_type = F_WRLCK, .l_whence = SEEK_SET, @@ -678,9 +677,6 @@ int take_etc_passwd_lock(const char *root) { .l_len = 0, }; - const char *path; - int fd, r; - /* This is roughly the same as lckpwdf(), but not as awful. We don't want to use alarm() and signals, * hence we implement our own trivial version of this. * @@ -688,22 +684,18 @@ int take_etc_passwd_lock(const char *root) { * given that they are redundant: they invoke lckpwdf() first and keep it during everything they do. * The per-database locks are awfully racy, and thus we just won't do them. */ - if (root) - path = prefix_roota(root, ETC_PASSWD_LOCK_PATH); - else - path = ETC_PASSWD_LOCK_PATH; + _cleanup_free_ char *path = path_join(root, ETC_PASSWD_LOCK_PATH); + if (!path) + return log_oom_debug(); - fd = open(path, O_WRONLY|O_CREAT|O_CLOEXEC|O_NOCTTY|O_NOFOLLOW, 0600); + _cleanup_close_ int fd = open(path, O_WRONLY|O_CREAT|O_CLOEXEC|O_NOCTTY|O_NOFOLLOW, 0600); if (fd < 0) return log_debug_errno(errno, "Cannot open %s: %m", path); - r = fcntl(fd, F_SETLKW, &flock); - if (r < 0) { - safe_close(fd); + if (fcntl(fd, F_SETLKW, &flock) < 0) return log_debug_errno(errno, "Locking %s failed: %m", path); - } - return fd; + return TAKE_FD(fd); } bool valid_user_group_name(const char *u, ValidUserFlags flags) { From d173d5564f59426fcff234f6b8b2cf0157a6cd77 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 31 Jan 2023 16:24:50 +0100 Subject: [PATCH 7/8] basic/user-util: create /etc from take_etc_passwd_lock This allows sysusers to operate with --root that is an empty directory. It may be useful to, for example, populate the user database before installing anything else. firstboot was already doing this, so drop the duplicated call there. --- src/basic/user-util.c | 3 +++ src/firstboot/firstboot.c | 2 -- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/basic/user-util.c b/src/basic/user-util.c index 734122c2a3..6c5041230a 100644 --- a/src/basic/user-util.c +++ b/src/basic/user-util.c @@ -19,6 +19,7 @@ #include "fileio.h" #include "format-util.h" #include "macro.h" +#include "mkdir.h" #include "parse-util.h" #include "path-util.h" #include "path-util.h" @@ -688,6 +689,8 @@ int take_etc_passwd_lock(const char *root) { if (!path) return log_oom_debug(); + (void) mkdir_parents(path, 0755); + _cleanup_close_ int fd = open(path, O_WRONLY|O_CREAT|O_CLOEXEC|O_NOCTTY|O_NOFOLLOW, 0600); if (fd < 0) return log_debug_errno(errno, "Cannot open %s: %m", path); diff --git a/src/firstboot/firstboot.c b/src/firstboot/firstboot.c index 45bb386da2..9e79f84691 100644 --- a/src/firstboot/firstboot.c +++ b/src/firstboot/firstboot.c @@ -904,8 +904,6 @@ static int process_root_account(void) { return 0; } - (void) mkdir_parents(etc_passwd, 0755); - lock = take_etc_passwd_lock(arg_root); if (lock < 0) return log_error_errno(lock, "Failed to take a lock on %s: %m", etc_passwd); From 3e74e6a15b1b31538222d16136d9743cff26f62e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 31 Jan 2023 16:41:16 +0100 Subject: [PATCH 8/8] test-sysusers: check that sysusers creates /etc when missing --- test/test-sysusers.sh.in | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/test/test-sysusers.sh.in b/test/test-sysusers.sh.in index 950dc297d8..fab37960bc 100755 --- a/test/test-sysusers.sh.in +++ b/test/test-sysusers.sh.in @@ -99,6 +99,14 @@ $SYSUSERS --root=$TESTDIR \ compare $SOURCE/inline "(--inline --replace=…)" +echo "*** Testing --inline with no /etc" +rm -rf $TESTDIR/etc +$SYSUSERS --root=$TESTDIR --inline \ + "u u1 222 - - /bin/zsh" \ + "g g1 111" + +compare $SOURCE/inline "(--inline)" + rm -f $TESTDIR/etc/sysusers.d/* $TESTDIR/usr/lib/sysusers.d/* cat >$TESTDIR/etc/login.defs <