[systemd-devel] [PATCH 7/9] nspawn: escape paths in overlay mount options

[Richard Maw]

On Thu, Jun 18, 2015 at 08:39:10PM +0200, Lennart Poettering wrote:
> On Thu, 28.05.15 13:02, Richard Maw (richard.maw at codethink.co.uk) wrote:
> 
> > Overlayfs uses , as an option separator and : as a list separator. These
> > characters are both valid in file paths, so overlayfs allows file paths
> > which contain these characters to backslash escape these values.
> > ---
> >  src/nspawn/nspawn.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++------
> >  1 file changed, 56 insertions(+), 7 deletions(-)
> > 
> > diff --git a/src/nspawn/nspawn.c b/src/nspawn/nspawn.c
> > index c40d50f..f7580f9 100644
> > --- a/src/nspawn/nspawn.c
> > +++ b/src/nspawn/nspawn.c
> > @@ -1237,6 +1237,42 @@ static int mount_tmpfs(const char *dest, CustomMount *m) {
> >          return 0;
> >  }
> >  
> > +static char *escaped_overlay_path(const char *path) {
> > +        _cleanup_free_ char *colon_escaped = NULL;
> > +        char *comma_escaped = NULL;
> > +
> > +        colon_escaped = strreplace(path, ":", "\\:");
> > +        if (!colon_escaped)
> > +                return NULL;
> > +
> > +        comma_escaped = strreplace(colon_escaped, ",", "\\,");
> > +
> > +        return comma_escaped;
> 
> This looks incomplete. What happens with "\\" itself?

Ah, yes. --overlay='\\a' would become '\a' after command-line parsing,
and overlayfs' unescaping would turn that into 'a'. I'll sort it out.

> Also, it's really inefficient, since strreplace() goes through the
> string each time from the beginning.

I was looking to do something simple rather than fast, as this isn't a
particularly latency sensitive bit of code, and I wasn't confident in
the need of a generic solution, but if you think it'll have wider use,
that's fine by me.

> I think it would make sense to add a generic 
> 
>   char *shell_escape(const char *s, const char *bad);
> 
> that works like xescape() but applies shell-style escaping instead of
> C-style escaping.

This isn't exactly shell escaping as I understand it, as that would also
require putting quotation marks aroud the string in certain circumstances,
and shell_maybe_quote() handles that already.

I'd call it backslash_escape() and possibly use it inside
shell_maybe_quote().

> > +}
> > +
> > +static char *joined_and_escaped_lower_dirs(char * const *lower) {
> > +        _cleanup_free_ char *s = NULL;
> > +        char *ret = NULL;
> > +        char * const *path;
> > +        bool first = true;
> > +
> > +        STRV_FOREACH_BACKWARDS(path, lower) {
> > +                _cleanup_free_ char *escaped_path = NULL;
> > +                escaped_path = escaped_overlay_path(*path);
> > +                if (first) {
> > +                        if (!strextend(&s, escaped_path, NULL))
> > +                                return NULL;
> > +                        first = false;
> > +                } else
> > +                        if (!strextend(&s, ":", escaped_path, NULL))
> > +                                return NULL;
> > +        }
> > +
> > +        ret = s;
> > +        s = NULL;
> > +        return ret;
> > +}
> 
> I'd prefer if we could have a routine:
> 
>     char **shell_escape_strv(char **l, const char *bad);
> 
> that goes through the strv list "l", and replaces all items in-place
> with shell_escape() and returns that.
> 
> If we have that, then we can nicely invoke strv_join() after
> shell_escape_strv(), and all would be good and simple.

Ok.