user-util: rework maybe_setgroups() a bit

Let's drop the caching of the setgroups /proc field for now. While there's a
strict regime in place when it changes states, let's better not cache it since
we cannot really be sure we follow that regime correctly.

More importantly however, this is not in performance sensitive code, and
there's no indication the cache is really beneficial, hence let's drop the
caching and make things a bit simpler.

Also, while we are at it, rework the error handling a bit, and always return
negative errno-style error codes, following our usual coding style. This has
the benefit that we can sensible hanld read_one_line_file() errors, without
having to updat errno explicitly.

diff --git a/src/basic/capability-util.c b/src/basic/capability-util.c
index f8db6e02123e988dd885ff017ae8ec66ec48ea0a..c3de20a0e866baa2baa0450c8f36eb29c39bad85 100644 (file)
--- a/src/basic/capability-util.c
+++ b/src/basic/capability-util.c
@@ -296,8 +296,9 @@ int drop_privileges(uid_t uid, gid_t gid, uint64_t keep_capabilities) {
         if (setresgid(gid, gid, gid) < 0)
                 return log_error_errno(errno, "Failed to change group ID: %m");
 
-        if (maybe_setgroups(0, NULL) < 0)
-                return log_error_errno(errno, "Failed to drop auxiliary groups list: %m");
+        r = maybe_setgroups(0, NULL);
+        if (r < 0)
+                return log_error_errno(r, "Failed to drop auxiliary groups list: %m");
 
         /* Ensure we keep the permitted caps across the setresuid() */
         if (prctl(PR_SET_KEEPCAPS, 1) < 0)

diff --git a/src/basic/user-util.c b/src/basic/user-util.c
index 16496fccfa4b75d51f52b91c3f2147efa29bc4b0..de6c93056eca7c140241fe834520b9621804f1dc 100644 (file)
--- a/src/basic/user-util.c
+++ b/src/basic/user-util.c
@@ -460,9 +460,11 @@ int get_shell(char **_s) {
 }
 
 int reset_uid_gid(void) {
+        int r;
 
-        if (maybe_setgroups(0, NULL) < 0)
-                return -errno;
+        r = maybe_setgroups(0, NULL);
+        if (r < 0)
+                return r;
 
         if (setresgid(0, 0, 0) < 0)
                 return -errno;
@@ -605,25 +607,30 @@ bool valid_home(const char *p) {
 }
 
 int maybe_setgroups(size_t size, const gid_t *list) {
-        static int cached_can_setgroups = -1;
-        /* check if setgroups is allowed before we try to drop all the auxiliary groups */
-        if (size == 0) {
-                if (cached_can_setgroups < 0) {
-                        _cleanup_free_ char *setgroups_content = NULL;
-                        int r = read_one_line_file("/proc/self/setgroups", &setgroups_content);
-                        if (r < 0 && errno != ENOENT)
-                                return r;
-                        if (r < 0) {
-                                /* old kernels don't have /proc/self/setgroups, so assume we can use setgroups */
-                                cached_can_setgroups = true;
-                        } else {
-                                cached_can_setgroups = streq(setgroups_content, "allow");
-                                if (!cached_can_setgroups)
-                                        log_debug("skip setgroups, /proc/self/setgroups is set to 'deny'");
-                        }
-                }
-                if (!cached_can_setgroups)
+        int r;
+
+        /* Check if setgroups is allowed before we try to drop all the auxiliary groups */
+        if (size == 0) { /* Dropping all aux groups? */
+                _cleanup_free_ char *setgroups_content = NULL;
+                bool can_setgroups;
+
+                r = read_one_line_file("/proc/self/setgroups", &setgroups_content);
+                if (r == -ENOENT)
+                        /* Old kernels don't have /proc/self/setgroups, so assume we can use setgroups */
+                        can_setgroups = true;
+                else if (r < 0)
+                        return r;
+                else
+                        can_setgroups = streq(setgroups_content, "allow");
+
+                if (!can_setgroups) {
+                        log_debug("Skipping setgroups(), /proc/self/setgroups is set to 'deny'");
                         return 0;
+                }
         }
-        return setgroups(size, list);
+
+        if (setgroups(size, list) < 0)
+                return -errno;
+
+        return 0;
 }

diff --git a/src/core/execute.c b/src/core/execute.c
index e4a23ac16955e1a1caef78aa7777dbde1de8ae47..d5c4e60796907f88ec68c078a64f9612e2514707 100644 (file)
--- a/src/core/execute.c
+++ b/src/core/execute.c
@@ -781,9 +781,10 @@ static int enforce_groups(const ExecContext *context, const char *username, gid_
                         k++;
                 }
 
-                if (maybe_setgroups(k, gids) < 0) {
+                r = maybe_setgroups(k, gids);
+                if (r < 0) {
                         free(gids);
-                        return -errno;
+                        return r;
                 }
 
                 free(gids);
@@ -950,8 +951,9 @@ static int setup_pam(
                  * If this fails, ignore the error - but expect sd-pam threads
                  * to fail to exit normally */
 
-                if (maybe_setgroups(0, NULL) < 0)
-                        log_warning_errno(errno, "Failed to setgroups() in sd-pam: %m");
+                r = maybe_setgroups(0, NULL);
+                if (r < 0)
+                        log_warning_errno(r, "Failed to setgroups() in sd-pam: %m");
                 if (setresgid(gid, gid, gid) < 0)
                         log_warning_errno(errno, "Failed to setresgid() in sd-pam: %m");
                 if (setresuid(uid, uid, uid) < 0)