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

[Filipe Brandenburger]

On Tue, Jun 2, 2015 at 3:55 AM, Daniel Mack <daniel at zonque.org> wrote:
> The problem here is that cunescape() wasn't as strict in the past as it
> should have been, and now there are unit files in the wild which contain
> escape sequences that slip through the C unescaping mechanism.
>
> So, my primary motivation was to fix the obvious regression at hand
> first, but I agree the actual problem goes deeper.

Yeah I didn't realize it was actually a regression... And looking back
at that case from the bug, the quotes systemd sees is actually double
quotes.

Not to mention that I talked about differences of single and double
quotes in shell but that doesn't really make sense because the shell
doesn't really expand \r or \n etc. (I think I was probably confusing
it with how quotes work in... well, Perl maybe? It doesn't really
matter.)

> I think that makes sense, but we need to make sure not to break existing
> setups. So for instance, both
>
>   ExecStart=-/bin/sh -c "echo foo at 0 | grep -Po '\w+@\K[\d.]+'"
>   ExecStart=-/bin/sh -c "echo foo at 0 | grep -Po '\\w+@\\K[\\d.]+'"
>
> are currently allowed and lead to the same result. The former because
> with UNESCAPE_RELAX, all sequences in the string (\w, \K, \d) are
> unrecognized and copied literal, and the latter because double
> backslashes become single ones.

Ok, I think that makes sense, we need to support those two use cases working.

So I think I'll go back to unquote_first_word and add support for
UNESCAPE_RELAX to it. I'm thinking if unquote_first_word receives
UNQUOTE_RELAX, then it should do the equivalent of UNESCAPE_RELAX to
its calls to cunescape_one. I think that would be appropriate here.

I think, as a bonus, I'll get some more use cases to work unchanged
and go back from now returning -EINVAL to returning 0 again, so
probably fewer regressions here.

> Martin already provided a test case the cases in the bug reports, and we
> should probably add more to make sure we don't cause more regressions
> with your rework.
>
> Do you have any patches in the queue for changes you proposed?

Yes, but I'll have to go back and add support to unquote_first_word to
do the equivalent of UNESCAPE_RELAX, which is where I stopped on it
yesterday... I think it's workable though.

I should have a reviewed patchset later today or tomorrow, I'll send
it as a GitHub PR.

Cheers,
Filipe