From 30c81ce2cef97b05e143c7adf4cd1b1c5fb59932 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= <zbyszek@in.waw.pl>
Date: Thu, 22 Feb 2018 11:30:59 +0100
Subject: [PATCH] pid1: when creating service directories, don't chown existing
 files (#8181)

This partially reverts 3536f49e8fa281539798a7bc5004d73302f39673 and
3536f49e8fa281539798a7bc5004d73302f39673.

When the user is dynamic, and we are setting up state, cache, or logs dirs,
behaviour is unchanged, we always do a recursive chown. This is necessary
because the user number might change between invocations.

But when setting up a directory for non-dynamic user, or a runtime directory
for a dynamic user, do any ownership or mode changes only when the directory
is initially created. Nothing says that the files under those directories have
to be all recursively owned by our user. This restores behaviour before
3536f49e8fa281539798a7bc5004d73302f39673, so modifications to the state of
the runtime directory persist between ExecStartPre's and ExecStart's, and even
longer in case the directory is persistent.

I think it _would_ be a nice property if setting a user would automatically
propagate to ownership of any Runtime/Logs/Cache directories. But this is
incompatible with another nice property, namely preserving changes to those
directories made by an admin, and with allowing change of ownership of files
in those directories by the service (e.g. to allow other users to access them).
Of the two, I think the second property is more important. Also, it's backwards
compatible.

https://bugzilla.redhat.com/show_bug.cgi?id=1508495

There is no need to chmod a directory we just created, so move that step
up into a branch. After that, 'effective' is only used once, so get rid of
it too.
---
 src/core/execute.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/src/core/execute.c b/src/core/execute.c
index 4b041edc15..3c8d47948f 100644
--- a/src/core/execute.c
+++ b/src/core/execute.c
@@ -2012,7 +2012,6 @@ static int setup_exec_directory(
 
         STRV_FOREACH(rt, context->directories[type].paths) {
                 _cleanup_free_ char *p = NULL, *pp = NULL;
-                const char *effective;
 
                 p = strjoin(params->prefix[type], "/", *rt);
                 if (!p) {
@@ -2106,20 +2105,17 @@ static int setup_exec_directory(
                         if (r < 0)
                                 goto fail;
 
-                        effective = pp;
-
+                        /* Lock down the access mode */
+                        if (chmod(pp, context->directories[type].mode) < 0) {
+                                r = -errno;
+                                goto fail;
+                        }
                 } else {
                         r = mkdir_label(p, context->directories[type].mode);
-                        if (r < 0 && r != -EEXIST)
+                        if (r == -EEXIST)
+                                continue;
+                        if (r < 0)
                                 goto fail;
-
-                        effective = p;
-                }
-
-                /* First lock down the access mode */
-                if (chmod(effective, context->directories[type].mode) < 0) {
-                        r = -errno;
-                        goto fail;
                 }
 
                 /* Don't change the owner of the configuration directory, as in the common case it is not written to by
@@ -2128,7 +2124,7 @@ static int setup_exec_directory(
                         continue;
 
                 /* Then, change the ownership of the whole tree, if necessary */
-                r = path_chown_recursive(effective, uid, gid);
+                r = path_chown_recursive(pp ?: p, uid, gid);
                 if (r < 0)
                         goto fail;
         }