From 46a0d98ac0fb5b507a6423e60058a2483830b432 Mon Sep 17 00:00:00 2001 From: Filipe Brandenburger Date: Sat, 30 May 2015 22:48:52 -0700 Subject: 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=) will empty the list and remove the existing commands, but using whitespace only (for example, ExecStartPre=) 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 --- src/core/load-fragment.c | 228 +++++++++++++++++++++++------------------------ 1 file changed, 114 insertions(+), 114 deletions(-) (limited to 'src/core/load-fragment.c') diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c index df5fe6fb32..41ba4c7eb7 100644 --- 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= clears the list, but ExecStart= + * 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"); -- cgit v1.2.3-54-g00ecf From c83f1f30b80253de8a1a6034e7b016fa55d24523 Mon Sep 17 00:00:00 2001 From: Filipe Brandenburger Date: Fri, 5 Jun 2015 23:12:25 -0700 Subject: load-fragment: reset the list on an ExecStart= containing only whitespace This is consistent with how an empty string works in an ExecStart= statement. We should not differentiate between an empty string and whitespace only (since they look the same.) Update the test case with whitespace only to reflect that the list is reset. Tested that `test-unit-file` passes and other test cases are not affected. Installed the patched systemd binaries on a machine, booted it, looked for out of the usual behavior but did not find any. --- src/core/load-fragment.c | 8 +++----- src/test/test-unit-file.c | 6 +++--- 2 files changed, 6 insertions(+), 8 deletions(-) (limited to 'src/core/load-fragment.c') diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c index 41ba4c7eb7..a48cb4029a 100644 --- a/src/core/load-fragment.c +++ b/src/core/load-fragment.c @@ -532,17 +532,15 @@ int config_parse_exec( e += ltype; - /* FIXME: ExecStart= clears the list, but ExecStart= - * doesn't, they should behave the same. */ + rvalue += strspn(rvalue, WHITESPACE); + p = rvalue; + if (isempty(rvalue)) { /* An empty assignment resets the list */ *e = exec_command_free_list(*e); return 0; } - rvalue += strspn(rvalue, WHITESPACE); - p = rvalue; - do { int i; _cleanup_strv_free_ char **n = NULL; diff --git a/src/test/test-unit-file.c b/src/test/test-unit-file.c index c7e8353b3b..8358789e6f 100644 --- a/src/test/test-unit-file.c +++ b/src/test/test-unit-file.c @@ -145,19 +145,19 @@ static void test_config_parse_exec(void) { assert_se(r == 0); assert_se(c1->command_next == NULL); - log_info("/* no command, check for bad memory access */"); + log_info("/* no command, whitespace only, reset */"); r = config_parse_exec(NULL, "fake", 3, "section", 1, "LValue", 0, " ", &c, NULL); assert_se(r == 0); - assert_se(c1->command_next == NULL); + assert_se(c == NULL); log_info("/* ignore && honour_argv0 */"); r = config_parse_exec(NULL, "fake", 4, "section", 1, "LValue", 0, "-@/RValue///slashes3 argv0a r1", &c, NULL); assert_se(r >= 0); - c1 = c1->command_next; + c1 = c; check_execcommand(c1, "/RValue/slashes3", "argv0a", "r1", NULL, true); log_info("/* ignore && honour_argv0 */"); -- cgit v1.2.3-54-g00ecf