[libvirt] [PATCH 2/3] Add virBufferEscapeShell

[Eric Blake]

On 10/12/2011 04:39 PM, Guido Günther wrote:
> Escape strings so they're safe to pass to the shell. Based on glib's
> g_quote_string.

Is this still true, or does it now resemble more what I did 
(independently from g_quote_string) in tools/virsh.c cmdEcho?

> ---
>   src/libvirt_private.syms |    1 +
>   src/util/buf.c           |   54 ++++++++++++++++++++++++++++++++++++++++++++++
>   src/util/buf.h           |    1 +
>   3 files changed, 56 insertions(+), 0 deletions(-)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 4f96518..862f3ac 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -29,6 +29,7 @@ virBufferEscape;
>   virBufferEscapeSexpr;
>   virBufferEscapeString;
>   virBufferFreeAndReset;
> +virBufferEscapeShell;

Sort this line (up a couple).

> +++ b/src/util/buf.c
> @@ -486,6 +486,60 @@ virBufferURIEncodeString (virBufferPtr buf, const char *str)
>   }
>
>   /**
> + * virBufferEscapeShell:
> + * @buf:  the buffer to append to
> + * @str:  an unquoted string
> + *
> + * Quotes a string so that the shell (/bin/sh) will interpret the
> + * quoted string as unquoted_string.
> + */
> +void
> +virBufferEscapeShell(const virBufferPtr buf, const char *str)

We are modifying buf, so this should be "(virBufferPtr buf", not "(const 
virBufferPtr buf" (I have another pending patch that tried to do this on 
existing functions:
https://www.redhat.com/archives/libvir-list/2011-September/msg01255.html)

> +
> +    len = strlen(str);
> +    if (xalloc_oversized(4, len) ||
> +        VIR_ALLOC_N(escaped, 4 * len + 3)<  0) {
> +        virBufferSetError(buf);

Yay - conflict resolution fun for whichever of our two series gets 
pushed second.

> +        return;
> +    }
> +
> +    cur = str;
> +    out = escaped;
> +
> +    /* Add outer '...' only if arg included shell metacharacters. */
> +    if (strpbrk(str, "\r\t\n !\"#$&'()*;<>?[\\]^`{|}~")) {

Hmm, I'm wondering if we should include '=' in this list, possibly 
conditionally?  That is, in shell, 'a=b' is much different from a=b (the 
former runs the command literally named "a=b", the latter affects the 
shell variable named "a").  At the same time, I want to someday make 
virCommand use this method to output env-var settings, in a nicely 
quoted manner (that is, as "a='b c'" rather than "'a=b c'").  But I'll 
worry about that later; for now, this matches the string in tools/virsh.c.

> +        *out++ = '\'';
> +        close_quote = true;
> +    }

Here, you have already proven there is nothing to quote; you could just 
call virBufferAdd and return early here.  Then you wouldn't even have to 
track close_quote - if you get past the short-circuit, then it is 
obvious that quoting is needed.

Do we need to worry about the special case of the empty string?  That 
is, if len == 0, should we append '' to the buffer rather than being a 
no-op?

> +    while (*cur != 0) {
> +        if (*cur == '\'') {
> +            /* Replace literal ' with a close ', a \', and a open ' */
> +            *out++ = '\'';
> +            *out++ = '\\';
> +            *out++ = *cur++;
> +            *out++ = '\'';
> +        } else
> +            *out++ = *cur++;

Style.  If one branch of if-else needs braces, then both should have it:

if {
    ...
} else {
     *out++ = *cur++;
}

Since it has been so long since this was first reported; I'm tempted to 
conditionally ACK this and leave it up to you if you post a v3 or just 
push things with my nits fixed.

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org