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

2015-05-15 Thread Lennart Poettering 
On Thu, 14.05.15 09:15, Martin Pitt (martin.p...@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.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
 




 ___
 systemd-devel mailing list
 systemd-devel@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/systemd-devel



Lennart

-- 
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

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

2015-05-14 Thread Zbigniew Jędrzejewski-Szmek 
On Thu, May 14, 2015 at 09:15:10AM +0200, Martin Pitt wrote:
 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 */
Looks OK, but it'd be nice to add quotes because of spaces: ...\%s\...

Zbyszek
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

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

2015-05-14 Thread Martin Pitt 
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

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

2015-05-14 Thread systemd github import bot 
Patchset imported to github.
Pull request:
https://github.com/systemd-devs/systemd/compare/master...systemd-mailing-devs:20150514071510.GC3401%40piware.de

--
Generated by https://github.com/haraldh/mail2git
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel