load-fragment: use unquote_first_word in config_parse_exec

Convert config_parse_exec() from using FOREACH_WORD_QUOTED into a loop
of unquote_first_word.

Loop through the arguments only once (the FOREACH_WORD_QUOTED
implementation did it twice, once to count them and another time to
process and store them.)

Use newly introduced flag UNQUOTE_UNESCAPE_RELAX to preserve
unrecognized escape sequences such as regexps matches such as "\w",
"\d", etc. (Valid escape sequences such as "\s" or "\b" still need an
extra backslash if literals are desired for regexps.)

Differences in behavior:

- Handle ; (command separator) in special, so that only ; on its own is
  valid for that purpose, an quoted semicolon ";" or ';' will now behave
  as a literal semicolon.  This is probably what was initially intended.

- Handle \; (to introduce a literal semicolon) in special, so that only \;
  is turned into a semicolon but not \\; or "\\;" or "\;" which are kept
  as a literal \; in the output.  This is probably what was initially
  intended.

Known issues:

- Using an empty string (for example, ExecStartPre=<empty>) will empty
  the list and remove the existing commands, but using whitespace only
  (for example, ExecStartPre=<spaces>) will not.  This is a pre-existing
  issue and will be dealt with in a follow up commit.

Tested:

- Unit tests passing.  Also `make distcheck` still works as expected.

- Installed it on a local machine and booted with it, checked console
  output, systemctl and journalctl output, did not notice any issues
  running the patched systemd binaries.

Relevant bug: https://bugs.freedesktop.org/show_bug.cgi?id=90794

diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c
index df5fe6fb326367082add9dda2c7beee7ee2e9442..41ba4c7eb75c860c62308b340ad7694ea5fe61a7 100644 (file)
--- a/src/core/load-fragment.c
+++ b/src/core/load-fragment.c
@@ -520,9 +520,9 @@ int config_parse_exec(
                 void *data,
                 void *userdata) {
 
-        ExecCommand **e = data, *nce;
-        char *path, **n;
-        unsigned k;
+        ExecCommand **e = data;
+        const char *p;
+        bool semicolon;
         int r;
 
         assert(filename);
@@ -532,156 +532,156 @@ int config_parse_exec(
 
         e += ltype;
 
+        /* FIXME: ExecStart=<empty> clears the list, but ExecStart=<whitespace>
+         * doesn't, they should behave the same.  */
         if (isempty(rvalue)) {
                 /* An empty assignment resets the list */
                 *e = exec_command_free_list(*e);
                 return 0;
         }
 
-        /* We accept an absolute path as first argument, or
-         * alternatively an absolute prefixed with @ to allow
-         * overriding of argv[0]. */
-        for (;;) {
+        rvalue += strspn(rvalue, WHITESPACE);
+        p = rvalue;
+
+        do {
                 int i;
-                const char *word, *state, *reason;
-                size_t l;
+                _cleanup_strv_free_ char **n = NULL;
+                size_t nlen = 0, nbufsize = 0;
+                _cleanup_free_ ExecCommand *nce = NULL;
+                _cleanup_free_ char *path = NULL, *firstword = NULL;
+                char *f;
                 bool separate_argv0 = false, ignore = false;
 
-                path = NULL;
-                nce = NULL;
-                n = NULL;
+                semicolon = false;
 
-                rvalue += strspn(rvalue, WHITESPACE);
+                r = unquote_first_word_and_warn(&p, &firstword, UNQUOTE_CUNESCAPE, unit, filename, line, rvalue);
+                if (r <= 0)
+                        return 0;
 
-                if (rvalue[0] == 0)
-                        break;
+                f = firstword;
+                for (i = 0; i < 2; i++) {
+                        /* We accept an absolute path as first argument, or
+                         * alternatively an absolute prefixed with @ to allow
+                         * overriding of argv[0]. */
+                        if (*f == '-' && !ignore)
+                                ignore = true;
+                        else if (*f == '@' && !separate_argv0)
+                                separate_argv0 = true;
+                        else
+                                break;
+                        f ++;
+                }
 
-                k = 0;
-                FOREACH_WORD_QUOTED(word, l, rvalue, state) {
-                        if (k == 0) {
-                                for (i = 0; i < 2; i++) {
-                                        if (*word == '-' && !ignore) {
-                                                ignore = true;
-                                                word ++;
-                                        }
-
-                                        if (*word == '@' && !separate_argv0) {
-                                                separate_argv0 = true;
-                                                word ++;
-                                        }
-                                }
-                        } else if (strneq(word, ";", MAX(l, 1U)))
-                                goto found;
+                if (isempty(f)) {
+                        /* First word is either "-" or "@" with no command. */
+                        log_syntax(unit, LOG_ERR, filename, line, EINVAL,
+                                   "Empty path in command line, ignoring: \"%s\"", rvalue);
+                        return 0;
+                }
 
-                        k++;
+                if (!string_is_safe(f)) {
+                        log_syntax(unit, LOG_ERR, filename, line, EINVAL,
+                                        "Executable path contains special characters, ignoring: %s", rvalue);
+                        return 0;
                 }
-                if (!isempty(state)) {
-                        log_syntax(unit, LOG_ERR, filename, line, EINVAL, "Trailing garbage, ignoring.");
+                if (!path_is_absolute(f)) {
+                        log_syntax(unit, LOG_ERR, filename, line, EINVAL,
+                                        "Executable path is not absolute, ignoring: %s", rvalue);
+                        return 0;
+                }
+                if (endswith(f, "/")) {
+                        log_syntax(unit, LOG_ERR, filename, line, EINVAL,
+                                        "Executable path specifies a directory, ignoring: %s", rvalue);
                         return 0;
                 }
 
-        found:
-                /* If separate_argv0, we'll move first element to path variable */
-                n = new(char*, MAX(k + !separate_argv0, 1u));
-                if (!n)
-                        return log_oom();
+                if (f == firstword) {
+                        path = firstword;
+                        firstword = NULL;
+                } else {
+                        path = strdup(f);
+                        if (!path)
+                                return log_oom();
+                }
 
-                k = 0;
-                FOREACH_WORD_QUOTED(word, l, rvalue, state) {
-                        char *c;
-                        unsigned skip;
-
-                        if (separate_argv0 ? path == NULL : k == 0) {
-                                /* first word, very special */
-                                skip = separate_argv0 + ignore;
-
-                                /* skip special chars in the beginning */
-                                if (l <= skip) {
-                                        log_syntax(unit, LOG_ERR, filename, line, EINVAL,
-                                                   "Empty path in command line, ignoring: \"%s\"", rvalue);
-                                        r = 0;
-                                        goto fail;
-                                }
+                if (!separate_argv0) {
+                        if (!GREEDY_REALLOC(n, nbufsize, nlen + 2))
+                                return log_oom();
+                        f = strdup(path);
+                        if (!f)
+                                return log_oom();
+                        n[nlen++] = f;
+                        n[nlen] = NULL;
+                }
 
-                        } else if (strneq(word, ";", MAX(l, 1U)))
-                                /* new commandline */
-                                break;
+                path_kill_slashes(path);
 
-                        else
-                                skip = strneq(word, "\\;", MAX(l, 1U));
+                for (;;) {
+                        _cleanup_free_ char *word = NULL;
 
-                        r = cunescape_length(word + skip, l - skip, UNESCAPE_RELAX, &c);
-                        if (r < 0) {
-                                log_syntax(unit, LOG_ERR, filename, line, r, "Failed to unescape command line, ignoring: %s", rvalue);
-                                r = 0;
-                                goto fail;
+                        /* Check explicitly for an unquoted semicolon as
+                         * command separator token.  */
+                        if (p[0] == ';' && (!p[1] || strchr(WHITESPACE, p[1]))) {
+                                p ++;
+                                p += strspn(p, WHITESPACE);
+                                semicolon = true;
+                                break;
                         }
 
-                        if (!utf8_is_valid(c)) {
-                                log_invalid_utf8(unit, LOG_ERR, filename, line, EINVAL, rvalue);
-                                r = 0;
-                                goto fail;
+                        /* Check for \; explicitly, to not confuse it with \\;
+                         * or "\;" or "\\;" etc.  unquote_first_word would
+                         * return the same for all of those.  */
+                        if (p[0] == '\\' && p[1] == ';' && (!p[2] || strchr(WHITESPACE, p[2]))) {
+                                p += 2;
+                                p += strspn(p, WHITESPACE);
+                                if (!GREEDY_REALLOC(n, nbufsize, nlen + 2))
+                                        return log_oom();
+                                f = strdup(";");
+                                if (!f)
+                                        return log_oom();
+                                n[nlen++] = f;
+                                n[nlen] = NULL;
+                                continue;
                         }
 
-                        /* where to stuff this? */
-                        if (separate_argv0 && path == NULL)
-                                path = c;
-                        else
-                                n[k++] = c;
-                }
+                        r = unquote_first_word_and_warn(&p, &word, UNQUOTE_CUNESCAPE, unit, filename, line, rvalue);
+                        if (r == 0)
+                                break;
+                        else if (r < 0)
+                                return 0;
 
-                n[k] = NULL;
+                        if (!GREEDY_REALLOC(n, nbufsize, nlen + 2))
+                                return log_oom();
+                        n[nlen++] = word;
+                        n[nlen] = NULL;
+                        word = NULL;
+                }
 
-                if (!n[0])
-                        reason = "Empty executable name or zeroeth argument";
-                else if (!string_is_safe(path ?: n[0]))
-                        reason = "Executable path contains special characters";
-                else if (!path_is_absolute(path ?: n[0]))
-                        reason = "Executable path is not absolute";
-                else if (endswith(path ?: n[0], "/"))
-                        reason = "Executable path specifies a directory";
-                else
-                        goto ok;
-
-                log_syntax(unit, LOG_ERR, filename, line, EINVAL, "%s, ignoring: %s", reason, rvalue);
-                r = 0;
-                goto fail;
-
-ok:
-                if (!path) {
-                        path = strdup(n[0]);
-                        if (!path) {
-                                r = log_oom();
-                                goto fail;
-                        }
+                if (!n || !n[0]) {
+                        log_syntax(unit, LOG_ERR, filename, line, EINVAL,
+                                        "Empty executable name or zeroeth argument, ignoring: %s", rvalue);
+                        return 0;
                 }
 
                 nce = new0(ExecCommand, 1);
-                if (!nce) {
-                        r = log_oom();
-                        goto fail;
-                }
+                if (!nce)
+                        return log_oom();
 
                 nce->argv = n;
                 nce->path = path;
                 nce->ignore = ignore;
 
-                path_kill_slashes(nce->path);
-
                 exec_command_append_list(e, nce);
 
-                rvalue = state;
-        }
-
-        return 0;
+                /* Do not _cleanup_free_ these. */
+                n = NULL;
+                path = NULL;
+                nce = NULL;
 
-fail:
-        n[k] = NULL;
-        strv_free(n);
-        free(path);
-        free(nce);
+                rvalue = p;
+        } while (semicolon);
 
-        return r;
+        return 0;
 }
 
 DEFINE_CONFIG_PARSE_ENUM(config_parse_service_type, service_type, ServiceType, "Failed to parse service type");

diff --git a/src/test/test-unit-file.c b/src/test/test-unit-file.c
index 892b65d322debd0af1261df60f12ac17cde33d20..c7e8353b3b99c295db9d14cf6bcf594d7d6be7e3 100644 (file)
--- a/src/test/test-unit-file.c
+++ b/src/test/test-unit-file.c
@@ -237,7 +237,7 @@ static void test_config_parse_exec(void) {
                               &c, NULL);
         assert_se(r >= 0);
         c1 = c1->command_next;
-        check_execcommand(c1, "/RValue", "argv0", "r1", NULL, true);
+        check_execcommand(c1, "/RValue", "argv0", "r1", ";", true);
 
         log_info("/* escaped semicolon */");
         r = config_parse_exec(NULL, "fake", 5, "section", 1,
@@ -284,7 +284,7 @@ static void test_config_parse_exec(void) {
                               &c, NULL);
         assert_se(r >= 0);
         c1 = c1->command_next;
-        check_execcommand(c1, "/bin/find", NULL, NULL, NULL, false);
+        check_execcommand(c1, "/bin/find", NULL, ";", NULL, false);
 
         log_info("/* quoted semicolon with following arg */");
         r = config_parse_exec(NULL, "fake", 5, "section", 1,
@@ -294,11 +294,7 @@ static void test_config_parse_exec(void) {
         assert_se(r >= 0);
         c1 = c1->command_next;
         check_execcommand(c1,
-                          "/sbin/find", NULL, NULL, NULL, false);
-
-        c1 = c1->command_next;
-        check_execcommand(c1,
-                          "/x", NULL, NULL, NULL, false);
+                          "/sbin/find", NULL, ";", "/x", false);
 
         log_info("/* spaces in the filename */");
         r = config_parse_exec(NULL, "fake", 5, "section", 1,