[systemd-devel] Crash with extra space after Exec prefix

[Lennart Poettering]

On Wed, 13.05.15 17:01, Martin Pitt (martin.pitt at ubuntu.com) wrote:

> Hello all,
> 
> 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)

Urks, that function isn't particular pretty anyway... I figure we
should rewrite this eventually to use unquote_first_word() instead of
FOREACH_WORD_QUOTED, which is pretty ugly...

> systemd pid 1 will crash the same way at boot, but with
> systemd-analyze it's less harmful to test :-)
> 
> So, obviously we need to fix the crash; but I was wondering what the
> desired behaviour should be? In the sense of "be liberal what you
> accept" I think the extra space(s) should just be ignored; or should
> that count as an error and the unit get rejected?

Neither.

It should be considered an error, logged about, but the line should be
ignored and we should continue. This is how we usually do it so far,
to ensure unit files stay relatively portable between version, but on
the other hand we aren't too liberal with accepting any data.

I think the "be liberal what you accept" thing really applies if you
have a true interchange format, that is read and written by multiple
different implementations. But this is not of this kind, since there's
exactly one implementation reading it that defines its semantics.

Lennart

-- 
Lennart Poettering, Red Hat