[systemd-devel] [systemd-commits] load-fragment: use UNESCAPE_RELAX flag to parse exec directives

[Filipe Brandenburger]

Hi,

Not sure I agree with the commit below. (In particular as I'm looking
at converting this code into using unquote_first_word.)

On Mon, Jun 1, 2015 at 9:10 AM, Daniel Mack
<zonque at kemper.freedesktop.org> wrote:
> commit 22874a348fb1540c1a2b7907748fc57c9756a7ed
> Author: Daniel Mack <daniel at zonque.org>
> Date:   Mon Jun 1 17:49:04 2015 +0200
>
>     load-fragment: use UNESCAPE_RELAX flag to parse exec directives
>
>     The cunescape() helper function used to handle unknown escaping sequences
>     gracefully by copying them over verbatim.
>
>     Commit 527b7a42 ("util: rework cunescape(), improve error handling") added
>     a flag to make that behavior optional, and changed to default to error out
>     with -EINVAL otherwise.
>
>     However, config_parse_exec(), which is used to parse the
>     Exec{Start,Stop}{Post,Pre,} directives of unit files, was not changed along
>     with that commit, which means that directives with improperly escaped
>     command line strings are no longer parsed.
>
>     Relevant bugreports include:
>
>       https://bugs.freedesktop.org/show_bug.cgi?id=90794
>       https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=787256
>
>     Fix this by passing UNESCAPE_RELAX to config_parse_exec() in order to
>     restore the original behavior.
>
> diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c
> index c95c110..df5fe6f 100644
> --- a/src/core/load-fragment.c
> +++ b/src/core/load-fragment.c
> @@ -610,7 +610,7 @@ int config_parse_exec(
>                          else
>                                  skip = strneq(word, "\\;", MAX(l, 1U));
>
> -                        r = cunescape_length(word + skip, l - skip, 0, &c);
> +                        r = cunescape_length(word + skip, l - skip, UNESCAPE_RELAX, &c);
>                          if (r < 0) {
>                                  log_syntax(unit, LOG_ERR, filename, line, r, "Failed to unescape command line, ignoring: %s", rvalue);
>                                  r = 0;

So, my problem with it is that the bug's expectation is that
backslashes inside single quotes will remain as backslashes, as the
example is a regexp '\w+@\K[\d.]+'.

But this is not true here!!! It's only fixing it for the particular
cases that are not escape sequences yet.

For instance, what if I'm doing a parameter that is a regexp that is
looking for a word boundary and I want to use '\b'? systemd with the
current patch will (still) turn this into a backspace character.

Right now the systemd quoting rules do *not* match the shell quoting
rules. (In fact, this is akin to a bug complaining that variables in
systemd do not match shell variables. That's indeed the case, but it
doesn't make it a bug. It's working as documented and as intended.)

I'd be ok with changing the rules so that backslash inside single
quotes remains a literal backslash, as I think we have the two kinds
of quotes (single quotes and double quotes) and I don't think it would
hurt to make them work a little bit closer to how the shell works...
(Though we'll keep expanding variables inside single quotes?)

In that case (of making backslashes stay literal inside single quotes)
I think the best way forward is complete the conversion to
unquote_first_word and then update unquote_first_word to introduce
those rules (essentially, just get rid of the SINGLE_QUOTE_ESCAPE rule
would do.)

Cheers,
Filipe