core/unit: fix shell-escaping of strings

Our escaping of '$' is '$$', not '\$'. We would write unit files that
were not valid:
  $ systemd-run --user bash -c 'echo $$; sleep 1000'
  Running as unit: run-r1c7c45b5b69f487c86ae205e12100808.service
  $ systemctl cat --user run-r1c7c45b5b69f487c86ae205e12100808
  # /run/user/1000/systemd/transient/run-r1c7c45b5b69f487c86ae205e12100808.service
  ...
  ExecStart="/usr/bin/bash" "-c" "echo \$\$\; sleep 1000"

  $ systemd-analyze verify /run/user/1000/systemd/transient/run-r1c7c45b5b69f487c86ae205e12100808.service
  /run/user/1000/systemd/transient/run-r1c7c45b5b69f487c86ae205e12100808.service:7:
    Ignoring unknown escape sequences: "echo \$\$\; sleep 1000"

Similarly, ';' cannot be escaped as '\;'. Only a handful of characters
listed in "Supported escapes" is allowed.

Escaping of "'" can be done, but it's not useful because we use double quotes
around the string anyway whenever we do escaping.

unit_write_setting() is called all over the place. In a great majority of
places we write either fixed strings or something that we generate ourselves,
so no escaping or quoting is needed. (And it's not allowed, e.g.
'Type="oneshot"' would not work.)  But if we forgot to add escaping or quoting
for a free-style string, it would probably allow writing a unit file that would
be read completely wrong. I looked over various places where
unit_write_setting() is called, and I couldn't find any place where
quoting/escaping was forgotten. But trying to figure out the full
ramifications of this change is not easy.

diff --git a/src/core/unit.c b/src/core/unit.c
index 846d15b4151d064ad596b0b41ccc398e69f8e951..2d47d9de8c0684c41f37455cb9b80b32d4853302 100644 (file)
--- a/src/core/unit.c
+++ b/src/core/unit.c
@@ -4332,10 +4332,17 @@ const char* unit_escape_setting(const char *s, UnitWriteFlags flags, char **buf)
         }
 
         /* We either do C-escaping or shell-escaping, to additionally escape characters that we parse for
-         * ExecStart= and friends, i.e. '$' and ';' and quotes. */
+         * ExecStart= and friends, i.e. '$' and quotes. */
 
         if (flags & UNIT_ESCAPE_EXEC_SYNTAX) {
-                char *t2 = shell_escape(s, "$;'\"");
+                char *t2;
+
+                t2 = strreplace(s, "$", "$$");
+                if (!t2)
+                        return NULL;
+                free_and_replace(t, t2);
+
+                t2 = shell_escape(t, "\"");
                 if (!t2)
                         return NULL;
                 free_and_replace(t, t2);
@@ -4343,7 +4350,9 @@ const char* unit_escape_setting(const char *s, UnitWriteFlags flags, char **buf)
                 s = t;
 
         } else if (flags & UNIT_ESCAPE_C) {
-                char *t2 = cescape(s);
+                char *t2;
+
+                t2 = cescape(s);
                 if (!t2)
                         return NULL;
                 free_and_replace(t, t2);

diff --git a/src/test/test-core-unit.c b/src/test/test-core-unit.c
index 6593f2fb4b88c91232a67e51337ff59c0edcb852..ea514a5c8c3a4d6234ebf4ec695fb130407ab886 100644 (file)
--- a/src/test/test-core-unit.c
+++ b/src/test/test-core-unit.c
@@ -41,13 +41,13 @@ static void test_unit_escape_setting_one(
 
 TEST(unit_escape_setting) {
         test_unit_escape_setting_one("/sbin/sbash", NULL, NULL);
-        test_unit_escape_setting_one("$", "\\$", "$");
-        test_unit_escape_setting_one("$$", "\\$\\$", "$$");
-        test_unit_escape_setting_one("'", "\\'", NULL);
+        test_unit_escape_setting_one("$", "$$", "$");
+        test_unit_escape_setting_one("$$", "$$$$", "$$");
+        test_unit_escape_setting_one("'", "'", "\\'");
         test_unit_escape_setting_one("\"", "\\\"", NULL);
         test_unit_escape_setting_one("\t", "\\t", NULL);
         test_unit_escape_setting_one(" ", NULL, NULL);
-        test_unit_escape_setting_one("$;'\"\t\n", "\\$\\;\\'\\\"\\t\\n", "$;\\'\\\"\\t\\n");
+        test_unit_escape_setting_one("$;'\"\t\n", "$$;'\\\"\\t\\n", "$;\\'\\\"\\t\\n");
 }
 
 static void test_unit_concat_strv_one(
@@ -89,7 +89,7 @@ TEST(unit_concat_strv) {
                                   NULL);
         test_unit_concat_strv_one(STRV_MAKE("a", " ", "$", "$$", ""),
                                   "\"a\" \" \" \"$\" \"$$\" \"\"",
-                                  "\"a\" \" \" \"\\$\" \"\\$\\$\" \"\"",
+                                  "\"a\" \" \" \"$$\" \"$$$$\" \"\"",
                                   NULL);
         test_unit_concat_strv_one(STRV_MAKE("\n", " ", "\t"),
                                   "\"\n\" \" \" \"\t\"",