[systemd-devel] [PATCH] core: Fix assertion with empty Exec*= paths
Lennart Poettering
lennart at poettering.net
Fri May 15 11:03:23 PDT 2015
On Thu, 14.05.15 09:15, Martin Pitt (martin.pitt at ubuntu.com) wrote:
Thanks! APplied!
> 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.pitt at 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
>
> _______________________________________________
> systemd-devel mailing list
> systemd-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Lennart
--
Lennart Poettering, Red Hat
More information about the systemd-devel
mailing list