From 05654e712f0ccfea7bbeb3e4de89861a670f974e Mon Sep 17 00:00:00 2001 From: Filipe Brandenburger Date: Fri, 29 May 2015 23:42:47 -0700 Subject: util: Refactor common cunescape block in unquote_first_word --- src/basic/util.c | 68 ++++++-------------------------------------------------- 1 file changed, 7 insertions(+), 61 deletions(-) (limited to 'src') diff --git a/src/basic/util.c b/src/basic/util.c index e0c5069ff8..e0d1220153 100644 --- a/src/basic/util.c +++ b/src/basic/util.c @@ -5209,35 +5209,6 @@ int unquote_first_word(const char **p, char **ret, UnquoteFlags flags) { break; - case VALUE_ESCAPE: - if (c == 0) { - if (flags & UNQUOTE_RELAX) - goto finish; - return -EINVAL; - } - - if (!GREEDY_REALLOC(s, allocated, sz+7)) - return -ENOMEM; - - if (flags & UNQUOTE_CUNESCAPE) { - uint32_t u; - - r = cunescape_one(*p, (size_t) -1, &c, &u); - if (r < 0) - return -EINVAL; - - (*p) += r - 1; - - if (c != 0) - s[sz++] = c; /* normal explicit char */ - else - sz += utf8_encode_unichar(s + sz, u); /* unicode chars we'll encode as utf8 */ - } else - s[sz++] = c; - - state = VALUE; - break; - case SINGLE_QUOTE: if (c == 0) { if (flags & UNQUOTE_RELAX) @@ -5256,35 +5227,6 @@ int unquote_first_word(const char **p, char **ret, UnquoteFlags flags) { break; - case SINGLE_QUOTE_ESCAPE: - if (c == 0) { - if (flags & UNQUOTE_RELAX) - goto finish; - return -EINVAL; - } - - if (!GREEDY_REALLOC(s, allocated, sz+7)) - return -ENOMEM; - - if (flags & UNQUOTE_CUNESCAPE) { - uint32_t u; - - r = cunescape_one(*p, (size_t) -1, &c, &u); - if (r < 0) - return -EINVAL; - - (*p) += r - 1; - - if (c != 0) - s[sz++] = c; - else - sz += utf8_encode_unichar(s + sz, u); - } else - s[sz++] = c; - - state = SINGLE_QUOTE; - break; - case DOUBLE_QUOTE: if (c == 0) return -EINVAL; @@ -5301,7 +5243,9 @@ int unquote_first_word(const char **p, char **ret, UnquoteFlags flags) { break; + case SINGLE_QUOTE_ESCAPE: case DOUBLE_QUOTE_ESCAPE: + case VALUE_ESCAPE: if (c == 0) { if (flags & UNQUOTE_RELAX) goto finish; @@ -5321,13 +5265,15 @@ int unquote_first_word(const char **p, char **ret, UnquoteFlags flags) { (*p) += r - 1; if (c != 0) - s[sz++] = c; + s[sz++] = c; /* normal explicit char */ else - sz += utf8_encode_unichar(s + sz, u); + sz += utf8_encode_unichar(s + sz, u); /* unicode chars we'll encode as utf8 */ } else s[sz++] = c; - state = DOUBLE_QUOTE; + state = (state == SINGLE_QUOTE_ESCAPE) ? SINGLE_QUOTE : + (state == DOUBLE_QUOTE_ESCAPE) ? DOUBLE_QUOTE : + VALUE; break; case SPACE: -- cgit v1.2.3-54-g00ecf From d6293c070e6e4b83d8e7ec56e465b0b215d55d98 Mon Sep 17 00:00:00 2001 From: Filipe Brandenburger Date: Tue, 2 Jun 2015 21:08:24 -0700 Subject: util: New flag UNQUOTE_UNESCAPE_RELAX for unquote_first_word The new flag UNQUOTE_UNESCAPE_RELAX preserves unrecognized escape sequences verbatim in unquote_first_word, either when it's a trailing backslash (similar to UNQUOTE_RELAX, but in this case keep the extra backslash in the output) or in the middle of a sequence string. Add unit test cases to ensure the new flag works as expected and to prevent regressions from being introduced. Tested with a follow up commit converting config_parse_exec() to start using unquote_first_word, in which case this flags makes it possible to preserve unrecognized escape sequences. Relevant bug: https://bugs.freedesktop.org/show_bug.cgi?id=90794 --- src/basic/util.c | 27 ++++++++++++--- src/basic/util.h | 5 +-- src/test/test-util.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 120 insertions(+), 6 deletions(-) (limited to 'src') diff --git a/src/basic/util.c b/src/basic/util.c index e0d1220153..46a48c4d60 100644 --- a/src/basic/util.c +++ b/src/basic/util.c @@ -5246,21 +5246,39 @@ int unquote_first_word(const char **p, char **ret, UnquoteFlags flags) { case SINGLE_QUOTE_ESCAPE: case DOUBLE_QUOTE_ESCAPE: case VALUE_ESCAPE: + if (!GREEDY_REALLOC(s, allocated, sz+7)) + return -ENOMEM; + if (c == 0) { + if ((flags & UNQUOTE_CUNESCAPE_RELAX) && + (state == VALUE_ESCAPE || flags & UNQUOTE_RELAX)) { + /* If we find an unquoted trailing backslash and we're in + * UNQUOTE_CUNESCAPE_RELAX mode, keep it verbatim in the + * output. + * + * Unbalanced quotes will only be allowed in UNQUOTE_RELAX + * mode, UNQUOTE_CUNESCAP_RELAX mode does not allow them. + */ + s[sz++] = '\\'; + goto finish; + } if (flags & UNQUOTE_RELAX) goto finish; return -EINVAL; } - if (!GREEDY_REALLOC(s, allocated, sz+7)) - return -ENOMEM; - if (flags & UNQUOTE_CUNESCAPE) { uint32_t u; r = cunescape_one(*p, (size_t) -1, &c, &u); - if (r < 0) + if (r < 0) { + if (flags & UNQUOTE_CUNESCAPE_RELAX) { + s[sz++] = '\\'; + s[sz++] = c; + goto end_escape; + } return -EINVAL; + } (*p) += r - 1; @@ -5271,6 +5289,7 @@ int unquote_first_word(const char **p, char **ret, UnquoteFlags flags) { } else s[sz++] = c; +end_escape: state = (state == SINGLE_QUOTE_ESCAPE) ? SINGLE_QUOTE : (state == DOUBLE_QUOTE_ESCAPE) ? DOUBLE_QUOTE : VALUE; diff --git a/src/basic/util.h b/src/basic/util.h index 7aca46d777..748f22f1a2 100644 --- a/src/basic/util.h +++ b/src/basic/util.h @@ -839,8 +839,9 @@ int is_dir(const char *path, bool follow); int is_device_node(const char *path); typedef enum UnquoteFlags { - UNQUOTE_RELAX = 1, - UNQUOTE_CUNESCAPE = 2, + UNQUOTE_RELAX = 1, + UNQUOTE_CUNESCAPE = 2, + UNQUOTE_CUNESCAPE_RELAX = 4, } UnquoteFlags; int unquote_first_word(const char **p, char **ret, UnquoteFlags flags); diff --git a/src/test/test-util.c b/src/test/test-util.c index ed8db45115..b3e79cd6f5 100644 --- a/src/test/test-util.c +++ b/src/test/test-util.c @@ -1304,6 +1304,100 @@ static void test_unquote_first_word(void) { assert_se(streq(t, "pi\360\237\222\251le")); free(t); assert_se(p == original + 32); + + p = original = "fooo\\"; + assert_se(unquote_first_word(&p, &t, UNQUOTE_RELAX) > 0); + assert_se(streq(t, "fooo")); + free(t); + assert_se(p == original + 5); + + p = original = "fooo\\"; + assert_se(unquote_first_word(&p, &t, UNQUOTE_CUNESCAPE_RELAX) > 0); + assert_se(streq(t, "fooo\\")); + free(t); + assert_se(p == original + 5); + + p = original = "fooo\\"; + assert_se(unquote_first_word(&p, &t, UNQUOTE_CUNESCAPE_RELAX|UNQUOTE_RELAX) > 0); + assert_se(streq(t, "fooo\\")); + free(t); + assert_se(p == original + 5); + + p = original = "fooo\\"; + assert_se(unquote_first_word(&p, &t, UNQUOTE_CUNESCAPE|UNQUOTE_CUNESCAPE_RELAX) > 0); + assert_se(streq(t, "fooo\\")); + free(t); + assert_se(p == original + 5); + + p = original = "\"foo\\"; + assert_se(unquote_first_word(&p, &t, 0) == -EINVAL); + assert_se(p == original + 5); + + p = original = "\"foo\\"; + assert_se(unquote_first_word(&p, &t, UNQUOTE_RELAX) > 0); + assert_se(streq(t, "foo")); + free(t); + assert_se(p == original + 5); + + p = original = "\"foo\\"; + assert_se(unquote_first_word(&p, &t, UNQUOTE_CUNESCAPE_RELAX) == -EINVAL); + assert_se(p == original + 5); + + p = original = "\"foo\\"; + assert_se(unquote_first_word(&p, &t, UNQUOTE_CUNESCAPE_RELAX|UNQUOTE_RELAX) > 0); + assert_se(streq(t, "foo\\")); + free(t); + assert_se(p == original + 5); + + p = original = "\"foo\\"; + assert_se(unquote_first_word(&p, &t, UNQUOTE_CUNESCAPE|UNQUOTE_CUNESCAPE_RELAX|UNQUOTE_RELAX) > 0); + assert_se(streq(t, "foo\\")); + free(t); + assert_se(p == original + 5); + + p = original = "fooo\\ bar quux"; + assert_se(unquote_first_word(&p, &t, UNQUOTE_RELAX) > 0); + assert_se(streq(t, "fooo bar")); + free(t); + assert_se(p == original + 10); + + p = original = "fooo\\ bar quux"; + assert_se(unquote_first_word(&p, &t, UNQUOTE_CUNESCAPE_RELAX) > 0); + assert_se(streq(t, "fooo bar")); + free(t); + assert_se(p == original + 10); + + p = original = "fooo\\ bar quux"; + assert_se(unquote_first_word(&p, &t, UNQUOTE_CUNESCAPE_RELAX|UNQUOTE_RELAX) > 0); + assert_se(streq(t, "fooo bar")); + free(t); + assert_se(p == original + 10); + + p = original = "fooo\\ bar quux"; + assert_se(unquote_first_word(&p, &t, UNQUOTE_CUNESCAPE) == -EINVAL); + assert_se(p == original + 5); + + p = original = "fooo\\ bar quux"; + assert_se(unquote_first_word(&p, &t, UNQUOTE_CUNESCAPE|UNQUOTE_CUNESCAPE_RELAX) > 0); + assert_se(streq(t, "fooo\\ bar")); + free(t); + assert_se(p == original + 10); + + p = original = "\\w+@\\K[\\d.]+"; + assert_se(unquote_first_word(&p, &t, UNQUOTE_CUNESCAPE) == -EINVAL); + assert_se(p == original + 1); + + p = original = "\\w+@\\K[\\d.]+"; + assert_se(unquote_first_word(&p, &t, UNQUOTE_CUNESCAPE|UNQUOTE_CUNESCAPE_RELAX) > 0); + assert_se(streq(t, "\\w+@\\K[\\d.]+")); + free(t); + assert_se(p == original + 12); + + p = original = "\\w+\\b"; + assert_se(unquote_first_word(&p, &t, UNQUOTE_CUNESCAPE|UNQUOTE_CUNESCAPE_RELAX) > 0); + assert_se(streq(t, "\\w+\b")); + free(t); + assert_se(p == original + 5); } static void test_unquote_many_words(void) { -- cgit v1.2.3-54-g00ecf From b59292b296ad71a20a40d7c347b6ca71df48892d Mon Sep 17 00:00:00 2001 From: Filipe Brandenburger Date: Mon, 8 Jun 2015 21:31:43 -0700 Subject: util: Introduce unquote_first_word_and_warn It will try to unquot_first_word, but if it runs into escaping problems it will retry it adding UNQUOTE_CUNESCAPE_RELAX to the flags. If it succeeds on the second try, it will log a warning about it. If it fails both times, it will log an error. Add test cases to confirm it behaves as expected. --- src/basic/util.c | 30 +++++++++++ src/basic/util.h | 1 + src/test/test-util.c | 145 +++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 176 insertions(+) (limited to 'src') diff --git a/src/basic/util.c b/src/basic/util.c index 46a48c4d60..727be56f58 100644 --- a/src/basic/util.c +++ b/src/basic/util.c @@ -5320,6 +5320,36 @@ finish: return 1; } +int unquote_first_word_and_warn( + const char **p, + char **ret, + UnquoteFlags flags, + const char *unit, + const char *filename, + unsigned line, + const char *rvalue) { + /* Try to unquote it, if it fails, warn about it and try again but this + * time using UNQUOTE_CUNESCAPE_RELAX to keep the backslashes verbatim + * in invalid escape sequences. */ + const char *save; + int r; + + save = *p; + r = unquote_first_word(p, ret, flags); + if (r < 0 && !(flags&UNQUOTE_CUNESCAPE_RELAX)) { + /* Retry it with UNQUOTE_CUNESCAPE_RELAX. */ + *p = save; + r = unquote_first_word(p, ret, flags|UNQUOTE_CUNESCAPE_RELAX); + if (r < 0) + log_syntax(unit, LOG_ERR, filename, line, EINVAL, + "Unbalanced quoting in command line, ignoring: \"%s\"", rvalue); + else + log_syntax(unit, LOG_WARNING, filename, line, EINVAL, + "Invalid escape sequences in command line: \"%s\"", rvalue); + } + return r; +} + int unquote_many_words(const char **p, UnquoteFlags flags, ...) { va_list ap; char **l; diff --git a/src/basic/util.h b/src/basic/util.h index 748f22f1a2..a1d1dd15c3 100644 --- a/src/basic/util.h +++ b/src/basic/util.h @@ -845,6 +845,7 @@ typedef enum UnquoteFlags { } UnquoteFlags; int unquote_first_word(const char **p, char **ret, UnquoteFlags flags); +int unquote_first_word_and_warn(const char **p, char **ret, UnquoteFlags flags, const char *unit, const char *filename, unsigned line, const char *rvalue); int unquote_many_words(const char **p, UnquoteFlags flags, ...) _sentinel_; int free_and_strdup(char **p, const char *s); diff --git a/src/test/test-util.c b/src/test/test-util.c index b3e79cd6f5..ad9ea3bcce 100644 --- a/src/test/test-util.c +++ b/src/test/test-util.c @@ -1400,6 +1400,150 @@ static void test_unquote_first_word(void) { assert_se(p == original + 5); } +static void test_unquote_first_word_and_warn(void) { + const char *p, *original; + char *t; + + p = original = "foobar waldo"; + assert_se(unquote_first_word_and_warn(&p, &t, 0, NULL, "fake", 1, original) > 0); + assert_se(streq(t, "foobar")); + free(t); + assert_se(p == original + 7); + + assert_se(unquote_first_word_and_warn(&p, &t, 0, NULL, "fake", 1, original) > 0); + assert_se(streq(t, "waldo")); + free(t); + assert_se(p == original + 12); + + assert_se(unquote_first_word_and_warn(&p, &t, 0, NULL, "fake", 1, original) == 0); + assert_se(!t); + assert_se(p == original + 12); + + p = original = "\"foobar\" \'waldo\'"; + assert_se(unquote_first_word_and_warn(&p, &t, 0, NULL, "fake", 1, original) > 0); + assert_se(streq(t, "foobar")); + free(t); + assert_se(p == original + 9); + + assert_se(unquote_first_word_and_warn(&p, &t, 0, NULL, "fake", 1, original) > 0); + assert_se(streq(t, "waldo")); + free(t); + assert_se(p == original + 16); + + assert_se(unquote_first_word_and_warn(&p, &t, 0, NULL, "fake", 1, original) == 0); + assert_se(!t); + assert_se(p == original + 16); + + p = original = "\""; + assert_se(unquote_first_word_and_warn(&p, &t, 0, NULL, "fake", 1, original) == -EINVAL); + assert_se(p == original + 1); + + p = original = "\'"; + assert_se(unquote_first_word_and_warn(&p, &t, 0, NULL, "fake", 1, original) == -EINVAL); + assert_se(p == original + 1); + + p = original = "\'fooo"; + assert_se(unquote_first_word_and_warn(&p, &t, 0, NULL, "fake", 1, original) == -EINVAL); + assert_se(p == original + 5); + + p = original = "\'fooo"; + assert_se(unquote_first_word_and_warn(&p, &t, UNQUOTE_RELAX, NULL, "fake", 1, original) > 0); + assert_se(streq(t, "fooo")); + free(t); + assert_se(p == original + 5); + + p = original = " foo\\ba\\x6ar "; + assert_se(unquote_first_word_and_warn(&p, &t, UNQUOTE_CUNESCAPE, NULL, "fake", 1, original) > 0); + assert_se(streq(t, "foo\ba\x6ar")); + free(t); + assert_se(p == original + 13); + + p = original = " foo\\ba\\x6ar "; + assert_se(unquote_first_word_and_warn(&p, &t, 0, NULL, "fake", 1, original) > 0); + assert_se(streq(t, "foobax6ar")); + free(t); + assert_se(p == original + 13); + + p = original = " f\\u00f6o \"pi\\U0001F4A9le\" "; + assert_se(unquote_first_word_and_warn(&p, &t, UNQUOTE_CUNESCAPE, NULL, "fake", 1, original) > 0); + assert_se(streq(t, "föo")); + free(t); + assert_se(p == original + 13); + + assert_se(unquote_first_word_and_warn(&p, &t, UNQUOTE_CUNESCAPE, NULL, "fake", 1, original) > 0); + assert_se(streq(t, "pi\360\237\222\251le")); + free(t); + assert_se(p == original + 32); + + p = original = "fooo\\"; + assert_se(unquote_first_word_and_warn(&p, &t, UNQUOTE_RELAX, NULL, "fake", 1, original) > 0); + assert_se(streq(t, "fooo")); + free(t); + assert_se(p == original + 5); + + p = original = "fooo\\"; + assert_se(unquote_first_word_and_warn(&p, &t, 0, NULL, "fake", 1, original) > 0); + assert_se(streq(t, "fooo\\")); + free(t); + assert_se(p == original + 5); + + p = original = "fooo\\"; + assert_se(unquote_first_word_and_warn(&p, &t, UNQUOTE_CUNESCAPE, NULL, "fake", 1, original) > 0); + assert_se(streq(t, "fooo\\")); + free(t); + assert_se(p == original + 5); + + p = original = "\"foo\\"; + assert_se(unquote_first_word_and_warn(&p, &t, 0, NULL, "fake", 1, original) == -EINVAL); + assert_se(p == original + 5); + + p = original = "\"foo\\"; + assert_se(unquote_first_word_and_warn(&p, &t, UNQUOTE_RELAX, NULL, "fake", 1, original) > 0); + assert_se(streq(t, "foo")); + free(t); + assert_se(p == original + 5); + + p = original = "\"foo\\"; + assert_se(unquote_first_word_and_warn(&p, &t, UNQUOTE_CUNESCAPE, NULL, "fake", 1, original) == -EINVAL); + assert_se(p == original + 5); + + p = original = "\"foo\\"; + assert_se(unquote_first_word_and_warn(&p, &t, UNQUOTE_CUNESCAPE|UNQUOTE_RELAX, NULL, "fake", 1, original) > 0); + assert_se(streq(t, "foo")); + free(t); + assert_se(p == original + 5); + + p = original = "fooo\\ bar quux"; + assert_se(unquote_first_word_and_warn(&p, &t, UNQUOTE_RELAX, NULL, "fake", 1, original) > 0); + assert_se(streq(t, "fooo bar")); + free(t); + assert_se(p == original + 10); + + p = original = "fooo\\ bar quux"; + assert_se(unquote_first_word_and_warn(&p, &t, 0, NULL, "fake", 1, original) > 0); + assert_se(streq(t, "fooo bar")); + free(t); + assert_se(p == original + 10); + + p = original = "fooo\\ bar quux"; + assert_se(unquote_first_word_and_warn(&p, &t, UNQUOTE_CUNESCAPE, NULL, "fake", 1, original) > 0); + assert_se(streq(t, "fooo\\ bar")); + free(t); + assert_se(p == original + 10); + + p = original = "\\w+@\\K[\\d.]+"; + assert_se(unquote_first_word_and_warn(&p, &t, UNQUOTE_CUNESCAPE, NULL, "fake", 1, original) > 0); + assert_se(streq(t, "\\w+@\\K[\\d.]+")); + free(t); + assert_se(p == original + 12); + + p = original = "\\w+\\b"; + assert_se(unquote_first_word_and_warn(&p, &t, UNQUOTE_CUNESCAPE, NULL, "fake", 1, original) > 0); + assert_se(streq(t, "\\w+\b")); + free(t); + assert_se(p == original + 5); +} + static void test_unquote_many_words(void) { const char *p, *original; char *a, *b, *c; @@ -1704,6 +1848,7 @@ int main(int argc, char *argv[]) { test_glob_exists(); test_execute_directory(); test_unquote_first_word(); + test_unquote_first_word_and_warn(); test_unquote_many_words(); test_parse_proc_cmdline(); test_raw_clone(); -- cgit v1.2.3-54-g00ecf From 0e9800d5d938acb350b2a3a29b938bc1deed0313 Mon Sep 17 00:00:00 2001 From: Filipe Brandenburger Date: Mon, 8 Jun 2015 22:01:52 -0700 Subject: tests: additional cases in test-unit-file These tests will be useful to check the cases regarding quoted and escaped semicolon when we switch to using unquote_first_word. Additionally, convert some of the tests that have semicolons so that the argument after the semicolon looks like a path (starts with /) so that we can see the change of behavior when making config_parse_exec more strict about what it accepts as a command separator. --- src/test/test-unit-file.c | 70 +++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 68 insertions(+), 2 deletions(-) (limited to 'src') diff --git a/src/test/test-unit-file.c b/src/test/test-unit-file.c index a8025c825b..892b65d322 100644 --- a/src/test/test-unit-file.c +++ b/src/test/test-unit-file.c @@ -195,6 +195,19 @@ static void test_config_parse_exec(void) { c1 = c1->command_next; check_execcommand(c1, "/goo/goo", NULL, "boo", NULL, false); + log_info("/* two semicolons in a row */"); + r = config_parse_exec(NULL, "fake", 5, "section", 1, + "LValue", 0, + "-@/RValue argv0 r1 ; ; " + "/goo/goo boo", + &c, NULL); + assert_se(r >= 0); + c1 = c1->command_next; + check_execcommand(c1, "/RValue", "argv0", "r1", NULL, true); + + /* second command fails because the executable name is ";" */ + assert_se(c1->command_next == NULL); + log_info("/* trailing semicolon */"); r = config_parse_exec(NULL, "fake", 5, "section", 1, "LValue", 0, @@ -206,6 +219,26 @@ static void test_config_parse_exec(void) { assert_se(c1->command_next == NULL); + log_info("/* trailing semicolon, no whitespace */"); + r = config_parse_exec(NULL, "fake", 5, "section", 1, + "LValue", 0, + "-@/RValue argv0 r1 ;", + &c, NULL); + assert_se(r >= 0); + c1 = c1->command_next; + check_execcommand(c1, "/RValue", "argv0", "r1", NULL, true); + + assert_se(c1->command_next == NULL); + + log_info("/* trailing semicolon in single quotes */"); + r = config_parse_exec(NULL, "fake", 5, "section", 1, + "LValue", 0, + "-@/RValue argv0 r1 ';'", + &c, NULL); + assert_se(r >= 0); + c1 = c1->command_next; + check_execcommand(c1, "/RValue", "argv0", "r1", NULL, true); + log_info("/* escaped semicolon */"); r = config_parse_exec(NULL, "fake", 5, "section", 1, "LValue", 0, @@ -218,12 +251,22 @@ static void test_config_parse_exec(void) { log_info("/* escaped semicolon with following arg */"); r = config_parse_exec(NULL, "fake", 5, "section", 1, "LValue", 0, - "/sbin/find \\; x", + "/sbin/find \\; /x", + &c, NULL); + assert_se(r >= 0); + c1 = c1->command_next; + check_execcommand(c1, + "/sbin/find", NULL, ";", "/x", false); + + log_info("/* escaped semicolon as part of an expression */"); + r = config_parse_exec(NULL, "fake", 5, "section", 1, + "LValue", 0, + "/sbin/find \\;x", &c, NULL); assert_se(r >= 0); c1 = c1->command_next; check_execcommand(c1, - "/sbin/find", NULL, ";", "x", false); + "/sbin/find", NULL, "\\;x", NULL, false); log_info("/* encoded semicolon */"); r = config_parse_exec(NULL, "fake", 5, "section", 1, @@ -234,6 +277,29 @@ static void test_config_parse_exec(void) { c1 = c1->command_next; check_execcommand(c1, "/bin/find", NULL, ";", NULL, false); + log_info("/* quoted semicolon */"); + r = config_parse_exec(NULL, "fake", 5, "section", 1, + "LValue", 0, + "/bin/find \";\"", + &c, NULL); + assert_se(r >= 0); + c1 = c1->command_next; + check_execcommand(c1, "/bin/find", NULL, NULL, NULL, false); + + log_info("/* quoted semicolon with following arg */"); + r = config_parse_exec(NULL, "fake", 5, "section", 1, + "LValue", 0, + "/sbin/find \";\" /x", + &c, NULL); + 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); + log_info("/* spaces in the filename */"); r = config_parse_exec(NULL, "fake", 5, "section", 1, "LValue", 0, -- cgit v1.2.3-54-g00ecf 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 +++++++++++++++++++++++----------------------- src/test/test-unit-file.c | 10 +- 2 files changed, 117 insertions(+), 121 deletions(-) (limited to 'src') 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"); diff --git a/src/test/test-unit-file.c b/src/test/test-unit-file.c index 892b65d322..c7e8353b3b 100644 --- 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, -- 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') 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