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.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org