Re: [libvirt] [PATCH 2/3] Add virBufferEscapeShell

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.