[libvirt] [PATCH 2/3] Add virBufferEscapeShell
Escape strings so they're safe to pass to the shell. Based on glib's g_quote_string. --- 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; virBufferStrcat; virBufferURIEncodeString; virBufferUse; diff --git a/src/util/buf.c b/src/util/buf.c index fa12855..f9d7cd7 100644 --- a/src/util/buf.c +++ 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) +{ + int len; + bool close_quote = false; + char *escaped, *out; + const char *cur; + + if ((buf == NULL) || (str == NULL)) + return; + + if (buf->error) + return; + + len = strlen(str); + if (xalloc_oversized(4, len) || + VIR_ALLOC_N(escaped, 4 * len + 3) < 0) { + virBufferSetError(buf); + return; + } + + cur = str; + out = escaped; + + /* Add outer '...' only if arg included shell metacharacters. */ + if (strpbrk(str, "\r\t\n !\"#$&'()*;<>?[\\]^`{|}~")) { + *out++ = '\''; + close_quote = true; + } + while (*cur != 0) { + if (*cur == '\'') { + /* Replace literal ' with a close ', a \', and a open ' */ + *out++ = '\''; + *out++ = '\\'; + *out++ = *cur++; + *out++ = '\''; + } else + *out++ = *cur++; + } + if (close_quote) + *out++ = '\''; + *out = 0; + virBufferAdd(buf, escaped, -1); + VIR_FREE(escaped); +} + +/** * virBufferStrcat: * @buf: the buffer to dump * @...: the variable list of strings, the last argument must be NULL diff --git a/src/util/buf.h b/src/util/buf.h index e545ed9..ad8ef86 100644 --- a/src/util/buf.h +++ b/src/util/buf.h @@ -52,6 +52,7 @@ void virBufferEscapeString(const virBufferPtr buf, const char *format, const cha void virBufferEscapeSexpr(const virBufferPtr buf, const char *format, const char *str); void virBufferEscape(const virBufferPtr buf, const char *toescape, const char *format, const char *str); void virBufferURIEncodeString (const virBufferPtr buf, const char *str); +void virBufferEscapeShell(const virBufferPtr buf, const char *str); # define virBufferAddLit(buf_, literal_string_) \ virBufferAdd (buf_, "" literal_string_ "", sizeof literal_string_ - 1) -- 1.7.6.3
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Hello, I have a De-Ja-Vu: On Thursday 13 October 2011 01:16:35 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.
We already have a escape-for-shell function: please have a look at src/qemu/qemu_monitor.c:142 qemuMonitorEscapeShell() But it works on "const char *" and returns "char *" instead of "virBuffer", so a merge with this new virBufferEscapeShell() is very much appreciated. Sincerely Philipp -- Philipp Hahn Open Source Software Engineer hahn@univention.de Univention GmbH Linux for Your Business fon: +49 421 22 232- 0 Mary-Somerville-Str.1 D-28359 Bremen fax: +49 421 22 232-99 http://www.univention.de/
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
On 10/13/2011 03:02 PM, Guido Günther wrote:
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 ;)
+++ b/src/libvirt_private.syms @@ -29,6 +29,7 @@ virBufferEscape; virBufferEscapeSexpr; virBufferEscapeString; virBufferFreeAndReset; +virBufferEscapeShell;
Sort this line (up a couple).
@@ -28,6 +28,7 @@ virBufferError; virBufferEscape; virBufferEscapeSexpr; virBufferEscapeString; +virBufferEscapeShell; virBufferFreeAndReset;
One more line up (Shell comes _before_ String :) ACK with that nit fixed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Thu, Oct 13, 2011 at 03:25:10PM -0600, Eric Blake wrote:
On 10/13/2011 03:02 PM, Guido Günther wrote:
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 ;)
+++ b/src/libvirt_private.syms @@ -29,6 +29,7 @@ virBufferEscape; virBufferEscapeSexpr; virBufferEscapeString; virBufferFreeAndReset; +virBufferEscapeShell;
Sort this line (up a couple).
@@ -28,6 +28,7 @@ virBufferError; virBufferEscape; virBufferEscapeSexpr; virBufferEscapeString; +virBufferEscapeShell; virBufferFreeAndReset;
One more line up (Shell comes _before_ String :)
Ahh...sorted alphabetically that is (as the comment says). I tried to keep the same order as in the header (as good as possible).
ACK with that nit fixed. Pushed all three patches, thanks, -- Guido
-- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (3)
-
Eric Blake -
Guido Günther -
Philipp Hahn