[systemd-devel] [PATCH] core: Fix assertion with empty Exec*= paths

[Lennart Poettering]

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