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
>From 452456fa556af40ac6de1cc8c91dd4bafca15d1b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Guido=20G=C3=BCnther?= <agx@xxxxxxxxxxx>
Date: Thu, 28 Jul 2011 15:25:00 +0200
Subject: [PATCH 2/4] Add virBufferEscapeShell
Escape strings so they're safe to pass to the shell. It's based on
virsh's 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..0c26ba7 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -28,6 +28,7 @@ virBufferError;
virBufferEscape;
virBufferEscapeSexpr;
virBufferEscapeString;
+virBufferEscapeShell;
virBufferFreeAndReset;
virBufferStrcat;
virBufferURIEncodeString;
diff --git a/src/util/buf.c b/src/util/buf.c
index fa12855..34347b5 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 to mean str.
+ */
+void
+virBufferEscapeShell(virBufferPtr buf, const char *str)
+{
+ int len;
+ char *escaped, *out;
+ const char *cur;
+
+ if ((buf == NULL) || (str == NULL))
+ return;
+
+ if (buf->error)
+ return;
+
+ /* Only quote if str includes shell metacharacters. */
+ if (!strpbrk(str, "\r\t\n !\"#$&'()*;<>?[\\]^`{|}~")) {
+ virBufferAdd(buf, str, -1);
+ return;
+ }
+
+ len = strlen(str);
+ if (xalloc_oversized(4, len) ||
+ VIR_ALLOC_N(escaped, 4 * len + 3) < 0) {
+ virBufferSetError(buf);
+ return;
+ }
+
+ cur = str;
+ out = escaped;
+
+ *out++ = '\'';
+ while (*cur != 0) {
+ *out++ = *cur++;
+ if (*cur == '\'') {
+ /* Replace literal ' with a close ', a \', and a open ' */
+ *out++ = '\\';
+ *out++ = '\'';
+ *out++ = '\'';
+ }
+ }
+ *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..1d0e790 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(virBufferPtr buf, const char *str);
# define virBufferAddLit(buf_, literal_string_) \
virBufferAdd (buf_, "" literal_string_ "", sizeof literal_string_ - 1)
--
1.7.6.3
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list