Libguestfs - Jun 2019 - [nbdkit PATCH v2 0/2] adding nbdkit --run '$uri'

Since v1:
- new patch to add uri_quote()
- rebase on top of other recent patches needed while auditing shell_quote()
- use uri_quote() instead of shell_quote() for producing $uri

Eric Blake (2):
  common/utils: Add uri_quote and tests
  captive: Support $uri in --run

 docs/nbdkit-captive.pod    |   8 ++-
 common/utils/utils.h       |   1 +
 common/utils/test-quotes.c | 108 +++++++++++++++++++++++++++++++++++++
 common/utils/utils.c       |  27 ++++++++++
 server/captive.c           |  14 ++++-
 .gitignore                 |   1 +
 common/utils/Makefile.am   |  11 ++++
 7 files changed, 168 insertions(+), 2 deletions(-)
 create mode 100644 common/utils/test-quotes.c

-- 
2.20.1

Normally, when using captive mode --run coupled with a Unix socket,
the user is using -U -, and we get lucky that the typcial values of
$TMPDIR plus our use of mktemp produce a name that needs neither shell
quoting nor which requires %-encoding for use in a URI.  But a
determined user can use an explicit -U /odd/#path or setting of
$TMPDIR that gets correctly shell-quoted, but which would break
interpretation in a URI parser. In preparation for adding a $uri
variable under --run, it's time to ensure that we can always produce a
valid unambiguous URI by adding a new helper function.  It is
intentional that uri_quote of an empty string produces no output, as
the intended audience of uri_quote is always a smaller component
within a larger URI (different from shell_quote which does have use on
a stand-alone empty string).

It's also worth testing the existing shell_quote (including the bug
fixed in bdda3f30) as well as the new uri_quote.  The test includes an
XXX comment that matches the counterpart XXX in shell_quote(); an
audit of shell_quote() callers shows that the iso plugin might be
affected if someone ever used prog=a=b, but that is fringe enough to
put off to another day.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 common/utils/utils.h       |   1 +
 common/utils/test-quotes.c | 108 +++++++++++++++++++++++++++++++++++++
 common/utils/utils.c       |  27 ++++++++++
 .gitignore                 |   1 +
 common/utils/Makefile.am   |  11 ++++
 5 files changed, 148 insertions(+)
 create mode 100644 common/utils/test-quotes.c

diff --git a/common/utils/utils.h b/common/utils/utils.h
index 4d1abf03..3f10cdc0 100644
--- a/common/utils/utils.h
+++ b/common/utils/utils.h
@@ -34,6 +34,7 @@
 #define NBDKIT_UTILS_H

 extern void shell_quote (const char *str, FILE *fp);
+extern void uri_quote (const char *str, FILE *fp);
 extern int exit_status_to_nbd_error (int status, const char *cmd);

 #endif /* NBDKIT_UTILS_H */
diff --git a/common/utils/test-quotes.c b/common/utils/test-quotes.c
new file mode 100644
index 00000000..33b8299e
--- /dev/null
+++ b/common/utils/test-quotes.c
@@ -0,0 +1,108 @@
+/* nbdkit
+ * Copyright (C) 2019 Red Hat Inc.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are
+ * met:
+ *
+ * * Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ *
+ * * Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in the
+ * documentation and/or other materials provided with the distribution.
+ *
+ * * Neither the name of Red Hat nor the names of its contributors may be
+ * used to endorse or promote products derived from this software without
+ * specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS
IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+ * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
+ * PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
+ * USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+ * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+ * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
+ * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+
+/* Unit tests of utils quoting code. */
+
+#include <config.h>
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <assert.h>
+#include <stdbool.h>
+
+#include "utils.h"
+
+static bool
+test (const char *orig, const char *fnname, void (*fn) (const char *, FILE *),
+      const char *exp)
+{
+  char *str = NULL;
+  size_t str_len = 0;
+  FILE *fp;
+
+  fp = open_memstream (&str, &str_len);
+  assert (fp);
+  fn (orig, fp);
+  if (fclose (fp) == EOF)
+    assert (false);
+  if (str_len == 0 && !str)
+    str = strdup ("");
+  assert (str);
+
+  if (strcmp (str, exp)) {
+    fprintf (stderr, "%s failed, got '%s' expected
'%s'\n",
+             fnname, str, exp);
+    free (str);
+    return true;
+  }
+  free (str);
+  return false;
+}
+
+int
+main (void)
+{
+  struct {
+    const char *orig;
+    const char *shell;
+    const char *uri;
+  } tests[] = {
+    { "a-b_c.0", "a-b_c.0", "a-b_c.0" },
+    { "/Safe/Path", "/Safe/Path", "/Safe/Path" },
+    { "a~b", "\"a~b\"", "a~b" },
+    { "", "\"\"", "" },
+    { "a=b", "a=b", "a%3Db" }, /* XXX shell wrong
if used as argv[0] */
+    { "a;b", "\"a;b\"", "a%3Bb" },
+    { "a b", "\"a b\"", "a%20b" },
+    { "a%b", "\"a%b\"", "a%25b" },
+    { "a'b\"c$d`e\\f",
"\"a'b\\\"c\\$d\\`e\\\\f\"",
"a%27b%22c%24d%60e%5Cf" },
+  };
+  size_t i;
+  bool fail = false;
+
+  for (i = 0; i < sizeof tests / sizeof tests[0]; i++) {
+    fail |= test (tests[i].orig, "shell_quote", shell_quote,
tests[i].shell);
+    fail |= test (tests[i].orig, "uri_quote", uri_quote,
tests[i].uri);
+  }
+  return fail ? EXIT_FAILURE : EXIT_SUCCESS;
+}
+
+/* Unrelated utils code uses nbdkit_error, normally provided by the main
+ * server program.  So we have to provide it here.
+ */
+void
+nbdkit_error (const char *fs, ...)
+{
+  /* XXX split utils.c to avoid needing this linker stub? */
+  assert (false);
+}
diff --git a/common/utils/utils.c b/common/utils/utils.c
index 7a584e25..7534a13d 100644
--- a/common/utils/utils.c
+++ b/common/utils/utils.c
@@ -75,6 +75,33 @@ shell_quote (const char *str, FILE *fp)
   fputc ('"', fp);
 }

+/* Print str to fp, URI quoting if necessary.
+ * The resulting string is safe for use in a URI path or query component,
+ * and can be passed through the shell without further quoting.
+ */
+void
+uri_quote (const char *str, FILE *fp)
+{
+  /* safe_chars contains the RFC 3986 unreserved characters plus '/'.
*/
+  const char *safe_chars +   
"abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789.-_~/";
+  size_t i, len;
+
+  /* If the string consists only of safe characters, output it as-is. */
+  len = strlen (str);
+  if (len == strspn (str, safe_chars)) {
+    fputs (str, fp);
+    return;
+  }
+
+  for (i = 0; i < len; ++i) {
+    if (strchr (safe_chars, str[i]))
+      fputc (str[i], fp);
+    else
+      fprintf (fp, "%%%02X", str[i] & 0xff);
+  }
+}
+
 /* Convert exit status to nbd_error.  If the exit status was nonzero
  * or another failure then -1 is returned.
  */
diff --git a/.gitignore b/.gitignore
index 3770d3ba..46ab5dc6 100644
--- a/.gitignore
+++ b/.gitignore
@@ -35,6 +35,7 @@ Makefile.in
 /common/include/test-random
 /common/include/test-tvdiff
 /common/protocol/protostrings.c
+/common/utils/test-quotes
 /compile
 /config.guess
 /config.h
diff --git a/common/utils/Makefile.am b/common/utils/Makefile.am
index f4f17005..cc364300 100644
--- a/common/utils/Makefile.am
+++ b/common/utils/Makefile.am
@@ -43,3 +43,14 @@ libutils_la_CPPFLAGS = \
 	-I$(top_srcdir)/include
 libutils_la_CFLAGS = \
         $(WARNINGS_CFLAGS)
+
+# Unit tests.
+
+TESTS = test-quotes
+check_PROGRAMS = test-quotes
+
+test_quotes_SOURCES = test-quotes.c utils.c utils.h
+test_quotes_CPPFLAGS = \
+	-I$(top_srcdir)/common/utils
+test_quotes_CFLAGS = \
+        $(WARNINGS_CFLAGS)
-- 
2.20.1

The existing --run '$nbd' outputs an older form that differs between
libguestfs and qemu, and which is not always a valid URI.  For
historical compatibility, we probably can't change that; but we can
instead add a new '$uri' that outputs a valid URI.  Note that the
libguestfs '$nbd' TCP form is already a valid URI (at least, as long
as the user isn't exploiting $TMPDIR or -U /odd/#path), but that
libguestfs still needs to be taught about the nbd+unix:// scheme, so
for there, you are still better off using '$nbd'; but for qemu, using
'$uri' already works since qemu v1.3.0 in 2012.

Note that the NBD project has not actually yet posted a link for the
preferred URI syntax; once Rich is happy with his work on that
project, we can touch this up to link to that page.

Reported-by: Martin Kletzander <mkletzan@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 docs/nbdkit-captive.pod |  8 +++++++-
 server/captive.c        | 14 +++++++++++++-
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/docs/nbdkit-captive.pod b/docs/nbdkit-captive.pod
index 6f69cca5..59df6690 100644
--- a/docs/nbdkit-captive.pod
+++ b/docs/nbdkit-captive.pod
@@ -42,9 +42,15 @@ The following shell variables are available in the
I<--run> argument:

 =over 4

+=item C<$uri>
+
+A URI that refers to the nbdkit port or socket in the preferred form
+documented by the NBD project.
+
 =item C<$nbd>

-A URL that refers to the nbdkit port or socket.
+An older URL that refers to the nbdkit port or socket in a manner
+specific to certain tools.

 Note there is some magic here, since qemu and guestfish URLs have a
 different format, so nbdkit tries to guess which you are running.  If
diff --git a/server/captive.c b/server/captive.c
index 6971af2e..90e42050 100644
--- a/server/captive.c
+++ b/server/captive.c
@@ -66,7 +66,19 @@ run_command (void)
     exit (EXIT_FAILURE);
   }

-  /* Construct $nbd "URL".  Unfortunately guestfish and qemu take
+  /* Construct $uri. */
+  fprintf (fp, "uri=");
+  if (port) {
+    fprintf (fp, "nbd://localhost:");
+    shell_quote (port, fp);
+  }
+  else if (unixsocket) {
+    fprintf (fp, "nbd+unix://\\?socket=");
+    uri_quote (unixsocket, fp);
+  }
+  fprintf (fp, "\n");
+
+  /* Construct older $nbd "URL".  Unfortunately guestfish and qemu
take
    * different syntax, so try to guess which one we need.
    */
   fprintf (fp, "nbd=");
-- 
2.20.1

Looks good, ACK series.

Thanks,

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top