From: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
Date: Sun, 9 Jul 2023 19:11:57 +0000 (-0600)
Subject: sysusers: add comments and simplify how set with names is created
X-Git-Tag: v255-rc1~908^2~7
X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=c8e02e408fee6735abc62b72dba5a951c87f302f;p=thirdparty%2Fsystemd.git

sysusers: add comments and simplify how set with names is created

The code was correct, but rather confusing: it used two sets with strings with
trivial_hash_ops to store strings used in other hashmaps. Let's add a bunch of
comments to explain what is happening. We also don't need two sets, using just
one saves a bit of memory.

While at it, let's add some debug messages if duplicate user/group names or
uids/gids are present.
---

diff --git a/src/sysusers/sysusers.c b/src/sysusers/sysusers.c
index cfa4823df71..2d06c8d82ad 100644
--- a/src/sysusers/sysusers.c
+++ b/src/sysusers/sysusers.c
@@ -107,7 +107,9 @@ static OrderedHashmap *members = NULL;
 
 static Hashmap *database_by_uid = NULL, *database_by_username = NULL;
 static Hashmap *database_by_gid = NULL, *database_by_groupname = NULL;
-static Set *database_users = NULL, *database_groups = NULL;
+
+/* A helper set to hold names that are used by database_by_{uid,gid,username,groupname} above. */
+static Set *names = NULL;
 
 static uid_t search_uid = UID_INVALID;
 static UidRange *uid_range = NULL;
@@ -122,10 +124,9 @@ STATIC_DESTRUCTOR_REGISTER(todo_uids, ordered_hashmap_freep);
 STATIC_DESTRUCTOR_REGISTER(todo_gids, ordered_hashmap_freep);
 STATIC_DESTRUCTOR_REGISTER(database_by_uid, hashmap_freep);
 STATIC_DESTRUCTOR_REGISTER(database_by_username, hashmap_freep);
-STATIC_DESTRUCTOR_REGISTER(database_users, set_free_freep);
 STATIC_DESTRUCTOR_REGISTER(database_by_gid, hashmap_freep);
 STATIC_DESTRUCTOR_REGISTER(database_by_groupname, hashmap_freep);
-STATIC_DESTRUCTOR_REGISTER(database_groups, set_free_freep);
+STATIC_DESTRUCTOR_REGISTER(names, set_free_freep);
 STATIC_DESTRUCTOR_REGISTER(uid_range, uid_range_freep);
 STATIC_DESTRUCTOR_REGISTER(arg_root, freep);
 STATIC_DESTRUCTOR_REGISTER(arg_image, freep);
@@ -184,31 +185,35 @@ static int load_user_database(void) {
         if (r < 0)
                 return r;
 
-        r = set_ensure_allocated(&database_users, NULL);
+        /* Note that we use NULL, i.e. trivial_hash_ops here, so identical strings can exist in the set. */
+        r = set_ensure_allocated(&names, NULL);
         if (r < 0)
                 return r;
 
         while ((r = fgetpwent_sane(f, &pw)) > 0) {
-                char *n;
-                int k, q;
 
-                n = strdup(pw->pw_name);
+                char *n = strdup(pw->pw_name);
                 if (!n)
                         return -ENOMEM;
 
-                k = set_put(database_users, n);
-                if (k < 0) {
-                        free(n);
-                        return k;
-                }
+                r = set_consume(names, n);
+                if (r < 0)
+                        return r;
+                assert(r > 0);  /* The set uses pointer comparisons, so n must not be in the set. */
 
-                k = hashmap_put(database_by_username, n, UID_TO_PTR(pw->pw_uid));
-                if (k < 0 && k != -EEXIST)
-                        return k;
+                r = hashmap_put(database_by_username, n, UID_TO_PTR(pw->pw_uid));
+                if (r == -EEXIST)
+                        log_debug_errno(r, "%s: user '%s' is listed twice, ignoring duplicate uid.",
+                                        passwd_path, n);
+                else if (r < 0)
+                        return r;
 
-                q = hashmap_put(database_by_uid, UID_TO_PTR(pw->pw_uid), n);
-                if (q < 0 && q != -EEXIST)
-                        return q;
+                r = hashmap_put(database_by_uid, UID_TO_PTR(pw->pw_uid), n);
+                if (r == -EEXIST)
+                        log_debug_errno(r, "%s: uid "UID_FMT" is listed twice, ignoring duplicate name.",
+                                        passwd_path, pw->pw_uid);
+                else if (r < 0)
+                        return r;
         }
         return r;
 }
@@ -232,31 +237,35 @@ static int load_group_database(void) {
         if (r < 0)
                 return r;
 
-        r = set_ensure_allocated(&database_groups, NULL);
+        /* Note that we use NULL, i.e. trivial_hash_ops here, so identical strings can exist in the set. */
+        r = set_ensure_allocated(&names, NULL);
         if (r < 0)
                 return r;
 
         while ((r = fgetgrent_sane(f, &gr)) > 0) {
-                char *n;
-                int k, q;
 
-                n = strdup(gr->gr_name);
+                char *n = strdup(gr->gr_name);
                 if (!n)
                         return -ENOMEM;
 
-                k = set_put(database_groups, n);
-                if (k < 0) {
-                        free(n);
-                        return k;
-                }
+                r = set_consume(names, n);
+                if (r < 0)
+                        return r;
+                assert(r > 0);  /* The set uses pointer comparisons, so n must not be in the set. */
 
-                k = hashmap_put(database_by_groupname, n, GID_TO_PTR(gr->gr_gid));
-                if (k < 0 && k != -EEXIST)
-                        return k;
+                r = hashmap_put(database_by_groupname, n, GID_TO_PTR(gr->gr_gid));
+                if (r == -EEXIST)
+                        log_debug_errno(r, "%s: group '%s' is listed twice, ignoring duplicate gid.",
+                                        group_path, n);
+                else if (r < 0)
+                        return r;
 
-                q = hashmap_put(database_by_gid, GID_TO_PTR(gr->gr_gid), n);
-                if (q < 0 && q != -EEXIST)
-                        return q;
+                r = hashmap_put(database_by_gid, GID_TO_PTR(gr->gr_gid), n);
+                if (r == -EEXIST)
+                        log_debug_errno(r, "%s: gid "GID_FMT" is listed twice, ignoring duplicate name.",
+                                        group_path, gr->gr_gid);
+                else if (r < 0)
+                        return r;
         }
         return r;
 }