Martin Pitt [2015-05-13 17:01 +0200]: > I got a report [1] that you can trivially crash systemd (pid1) at boot > by creating a unit with an Exec= line with a modifier and a space: > > $ cat /tmp/foo.service > [Service] > ExecStart=- /bin/echo hello > > $ systemd-analyze verify /tmp/foo.service > Assertion 'skip < l' failed at ../src/core/load-fragment.c:607, function > config_parse_exec(). Aborting. > Aborted (core dumped) > > systemd pid 1 will crash the same way at boot, but with > systemd-analyze it's less harmful to test :-)
This patch is a minimally invasive and straighforward fix for this with the behaviour as discussed. It is appropriate for the stable branch and distro updates for stable releases. I'd like to do the rewriting to unquote_first_word() in a separate commit; it's easy to miss subtle corner cases. This is already mentioned in TODO: * code cleanup: retire FOREACH_WORD_QUOTED, port to unquote_first_word() loops instead (from the similar recent crash fixed in 470dca63c) Thanks, Martin -- Martin Pitt | http://www.piware.de Ubuntu Developer (www.ubuntu.com) | Debian Developer (www.debian.org)
From 10f6ac7c8b3dad0197ce795e33383c24ea2d53b1 Mon Sep 17 00:00:00 2001 From: Martin Pitt <martin.p...@ubuntu.com> Date: Thu, 14 May 2015 09:06:40 +0200 Subject: [PATCH] core: Fix assertion with empty Exec*= paths An Exec*= line with whitespace after modifiers, like ExecStart=- /bin/true is considered to have an empty command path. This is as specified, but causes systemd to crash with Assertion 'skip < l' failed at ../src/core/load-fragment.c:607, function config_parse_exec(). Aborting. Aborted (core dumped) Fix this by logging an error instead and ignoring the invalid line. Add corresponding test cases. Also add a test case for a completely empty value which resets the command list. https://launchpad.net/bugs/1454173 --- src/core/load-fragment.c | 6 +++++- src/test/test-unit-file.c | 21 +++++++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c index 33d9e27..3865017 100644 --- a/src/core/load-fragment.c +++ b/src/core/load-fragment.c @@ -595,7 +595,11 @@ int config_parse_exec( skip = separate_argv0 + ignore; /* skip special chars in the beginning */ - assert(skip < l); + if (l <= skip) { + log_syntax(unit, LOG_ERR, filename, line, EINVAL, "Empty path in command line, ignoring: %s", rvalue); + r = 0; + goto fail; + } } else if (strneq(word, ";", MAX(l, 1U))) /* new commandline */ diff --git a/src/test/test-unit-file.c b/src/test/test-unit-file.c index f3f6c29..03ca70a 100644 --- a/src/test/test-unit-file.c +++ b/src/test/test-unit-file.c @@ -317,6 +317,27 @@ static void test_config_parse_exec(void) { assert_se(r == 0); assert_se(c1->command_next == NULL); + log_info("/* invalid space between modifiers */"); + r = config_parse_exec(NULL, "fake", 4, "section", 1, + "LValue", 0, "- /path", + &c, NULL); + assert_se(r == 0); + assert_se(c1->command_next == NULL); + + log_info("/* only modifiers, no path */"); + r = config_parse_exec(NULL, "fake", 4, "section", 1, + "LValue", 0, "-", + &c, NULL); + assert_se(r == 0); + assert_se(c1->command_next == NULL); + + log_info("/* empty argument, reset */"); + r = config_parse_exec(NULL, "fake", 4, "section", 1, + "LValue", 0, "", + &c, NULL); + assert_se(r == 0); + assert_se(c == NULL); + exec_command_free_list(c); } -- 2.1.4
signature.asc
Description: Digital signature
_______________________________________________ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
