[libvirt] [PATCH 2/3] Add virBufferEscapeShell

[Guido Günther]

On Wed, Oct 12, 2011 at 05:16:35PM -0600, Eric Blake wrote:
> 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?

Yes, it's more like cmdEcho now ;)

> 
> >---
> >  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).

Done.

> 
> >+++ 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)

Done.

> 
> >+
> >+    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.

Done.

> 
> 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++;

Since we're copying the same char in if and else I dropped else
entirely.

> 
> 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.
New version attached.
Cheers,
 -- Guido
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Add-virBufferEscapeShell.patch
Type: text/x-diff
Size: 3074 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20111013/6799e307/attachment-0001.bin>