From: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
Date: Tue, 26 Mar 2013 23:37:14 +0000 (-0400)
Subject: Simplify the meaning of %s
X-Git-Tag: v200~18
X-Git-Url: http://www.chiark.greenend.org.uk/ucgi/~ianmdlvl/git?p=elogind.git;a=commitdiff_plain;h=3baed19327663d012c3313b72cc5b3d02a58720a;ds=sidebyside

Simplify the meaning of %s

The rules governing %s where just too complicated. First of
all, looking at $SHELL is dangerous. For systemd --system,
it usually wouldn't be set. But it could be set if the admin
first started a debug shell, let's say /sbin/sash, and then
launched systemd from it. This shouldn't influence how daemons
are started later on, so is better ignored. Similar reasoning
holds for session mode. Some shells set $SHELL, while other
set it only when it wasn't set previously (e.g. zsh). This
results in fragility that is better avoided by ignoring $SHELL
totally.

With $SHELL out of the way, simplify things by saying that
%s==/bin/sh for root, and the configured shell otherwise.
get_shell() is the only caller, so it can be inlined.

Fixes one issue seen with 'make check'.
---

diff --git a/man/systemd.unit.xml.in b/man/systemd.unit.xml.in
index e99703f7b..2196e73bb 100644
--- a/man/systemd.unit.xml.in
+++ b/man/systemd.unit.xml.in
@@ -1270,7 +1270,14 @@
                       <row>
                         <entry><literal>%s</literal></entry>
                         <entry>User shell</entry>
-                        <entry>This is the shell of the configured user of the unit, or (if none is set) the user running the systemd instance.</entry>
+                        <entry>This is the shell of the configured
+                        user of the unit, or (if none is set) the user
+                        running the systemd instance.  If the user is
+                        <literal>root</literal> (UID equal to 0), the
+                        shell configured in account database is
+                        ignored and <filename>/bin/sh</filename> is
+                        always used.
+                        </entry>
                       </row>
                       <row>
                         <entry><literal>%m</literal></entry>
diff --git a/src/core/unit-printf.c b/src/core/unit-printf.c
index 7415824cd..98274ee35 100644
--- a/src/core/unit-printf.c
+++ b/src/core/unit-printf.c
@@ -190,28 +190,37 @@ static char *specifier_user_shell(char specifier, void *data, void *userdata) {
         ExecContext *c;
         int r;
         const char *username, *shell;
+        char *ret;
 
         assert(u);
 
         c = unit_get_exec_context(u);
 
-        /* return HOME if set, otherwise from passwd */
-        if (!c || !c->user) {
-                char *sh;
+        if (c && c->user)
+                username = c->user;
+        else
+                username = "root";
 
-                r = get_shell(&sh);
-                if (r < 0)
-                        return strdup("/bin/sh");
+        /* return /bin/sh for root, otherwise the value from passwd */
+        r = get_user_creds(&username, NULL, NULL, NULL, &shell);
+        if (r < 0) {
+                log_warning_unit(u->id,
+                                 "Failed to determine shell: %s",
+                                 strerror(-r));
+                return NULL;
+        }
 
-                return sh;
+        if (!path_is_absolute(shell)) {
+                log_warning_unit(u->id,
+                                 "Shell %s is not absolute, ignoring.",
+                                 shell);
         }
 
-        username = c->user;
-        r = get_user_creds(&username, NULL, NULL, NULL, &shell);
-        if (r < 0)
-                return strdup("/bin/sh");
+        ret = strdup(shell);
+        if (!ret)
+                log_oom();
 
-        return strdup(shell);
+        return ret;
 }
 
 char *unit_name_printf(Unit *u, const char* format) {
diff --git a/src/shared/util.c b/src/shared/util.c
index 03d6f0062..0444cf445 100644
--- a/src/shared/util.c
+++ b/src/shared/util.c
@@ -5246,53 +5246,6 @@ int get_home_dir(char **_h) {
         return 0;
 }
 
-int get_shell(char **_sh) {
-        char *sh;
-        const char *e;
-        uid_t u;
-        struct passwd *p;
-
-        assert(_sh);
-
-        /* Take the user specified one */
-        e = getenv("SHELL");
-        if (e) {
-                sh = strdup(e);
-                if (!sh)
-                        return -ENOMEM;
-
-                *_sh = sh;
-                return 0;
-        }
-
-        /* Hardcode home directory for root to avoid NSS */
-        u = getuid();
-        if (u == 0) {
-                sh = strdup("/bin/sh");
-                if (!sh)
-                        return -ENOMEM;
-
-                *_sh = sh;
-                return 0;
-        }
-
-        /* Check the database... */
-        errno = 0;
-        p = getpwuid(u);
-        if (!p)
-                return errno ? -errno : -ESRCH;
-
-        if (!path_is_absolute(p->pw_shell))
-                return -EINVAL;
-
-        sh = strdup(p->pw_shell);
-        if (!sh)
-                return -ENOMEM;
-
-        *_sh = sh;
-        return 0;
-}
-
 void fclosep(FILE **f) {
         if (*f)
                 fclose(*f);
diff --git a/src/shared/util.h b/src/shared/util.h
index 7a3842100..52c33238b 100644
--- a/src/shared/util.h
+++ b/src/shared/util.h
@@ -519,7 +519,6 @@ bool in_initrd(void);
 
 void warn_melody(void);
 
-int get_shell(char **ret);
 int get_home_dir(char **ret);
 
 static inline void freep(void *p) {
diff --git a/src/test/test-unit-name.c b/src/test/test-unit-name.c
index 7bd99d31c..0b6b56394 100644
--- a/src/test/test-unit-name.c
+++ b/src/test/test-unit-name.c
@@ -164,7 +164,7 @@ static void test_unit_printf(void) {
         expect(u, "%u", root->pw_name);
         expect(u, "%U", root_uid);
         expect(u, "%h", root->pw_dir);
-        expect(u, "%s", root->pw_shell);
+        expect(u, "%s", "/bin/sh");
         expect(u, "%m", mid);
         expect(u, "%b", bid);
         expect(u, "%H", host);
@@ -184,7 +184,7 @@ static void test_unit_printf(void) {
         expect(u2, "%u", root->pw_name);
         expect(u2, "%U", root_uid);
         expect(u2, "%h", root->pw_dir);
-        expect(u2, "%s", root->pw_shell);
+        expect(u2, "%s", "/bin/sh");
         expect(u2, "%m", mid);
         expect(u2, "%b", bid);
         expect(u2, "%H", host);