Libguestfs - Apr 2012 - [PATCH 0/4] libguestfs cannot open disk images which are symlinks to files that contain ':' (colon) character (RHBZ#812092).

Note: This is a regression in RHEL 6.3.  Please review this patch
urgently.

The description of the bug is here:

  https://bugzilla.redhat.com/show_bug.cgi?id=812092

This patch set attempts to fix the problem conservatively, because
it's a very high risk codepath and very late in the development of
RHEL 6.3.

The first patch reverts the behaviour of calling realpath(3) and
checking for duplicate filenames.

The second patch fixes 'check_path'.  Only ':' is not permitted
in
qemu paths.  (However symlinks to paths that contain ':' will work
again because the first patch was reverted.)

The third patch adds proper escaping of ',' in qemu command line
parameters.

The fourth patch adds a regression test for filenames containing
unusual characters.

Rich.

From: "Richard W.M. Jones" <rjones at redhat.com>

This reverts commit be47b66c3033105a2b880dbc10bfc2b163b7eafe.
---
 src/launch.c |   21 ++-------------------
 1 file changed, 2 insertions(+), 19 deletions(-)

diff --git a/src/launch.c b/src/launch.c
index 6388fd2..35cf49e 100644
--- a/src/launch.c
+++ b/src/launch.c
@@ -295,9 +295,7 @@ guestfs__add_drive_opts (guestfs_h *g, const char *filename,
   char *format;
   char *iface;
   char *name;
-  char *abs_path = NULL;
   int use_cache_off;
-  int check_duplicate;
 
   if (check_path(g, filename) == -1)
     return -1;
@@ -337,39 +335,24 @@ guestfs__add_drive_opts (guestfs_h *g, const char
*filename,
     }
   }
 
-  /* Make the path canonical, so we can check if the user is trying to
-   * add the same path twice.  Allow /dev/null to be added multiple
-   * times, in accordance with traditional usage.
-   */
-  abs_path = realpath (filename, NULL);
-  check_duplicate = STRNEQ (abs_path, "/dev/null");
-
   struct drive **i = &(g->drives);
-  while (*i != NULL) {
-    if (check_duplicate && STREQ((*i)->path, abs_path)) {
-      error (g, _("drive %s can't be added twice"), abs_path);
-      goto err_out;
-    }
-    i = &((*i)->next);
-  }
+  while (*i != NULL) i = &((*i)->next);
 
   *i = safe_malloc (g, sizeof (struct drive));
   (*i)->next = NULL;
-  (*i)->path = safe_strdup (g, abs_path);
+  (*i)->path = safe_strdup (g, filename);
   (*i)->readonly = readonly;
   (*i)->format = format;
   (*i)->iface = iface;
   (*i)->name = name;
   (*i)->use_cache_off = use_cache_off;
 
-  free (abs_path);
   return 0;
 
 err_out:
   free (format);
   free (iface);
   free (name);
-  free (abs_path);
   return -1;
 }
 
-- 
1.7.9.3

From: "Richard W.M. Jones" <rjones at redhat.com>

Remove the bogus check_path function and move the functionality into
the two places where it was being used.

  qemu -cdrom ,

works fine, I tested it.

Colon cannot be used in a block device filename anywhere, since the
qemu block driver interprets it as a prefix.  There is no known way to
work around this problem.  I checked this is true with kwolf.

Comma is fine in -drive options, provided it is escaped by doubling it.
---
 src/launch.c |   20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/src/launch.c b/src/launch.c
index 35cf49e..fe858b8 100644
--- a/src/launch.c
+++ b/src/launch.c
@@ -277,16 +277,6 @@ valid_format_iface (const char *str)
   return 1;
 }
 
-static int
-check_path (guestfs_h *g, const char *filename)
-{
-  if (strchr (filename, ',') != NULL) {
-    error (g, _("filename cannot contain ',' (comma)
character"));
-    return -1;
-  }
-  return 0;
-}
-
 int
 guestfs__add_drive_opts (guestfs_h *g, const char *filename,
                          const struct guestfs_add_drive_opts_argv *optargs)
@@ -297,8 +287,11 @@ guestfs__add_drive_opts (guestfs_h *g, const char
*filename,
   char *name;
   int use_cache_off;
 
-  if (check_path(g, filename) == -1)
+  if (strchr (filename, ':') != NULL) {
+    error (g, _("filename cannot contain ':' (colon) character.
"
+                "This is a limitation of qemu."));
     return -1;
+  }
 
   readonly = optargs->bitmask & GUESTFS_ADD_DRIVE_OPTS_READONLY_BITMASK
              ? optargs->readonly : 0;
@@ -406,8 +399,11 @@ guestfs__add_drive_ro_with_if (guestfs_h *g, const char
*filename,
 int
 guestfs__add_cdrom (guestfs_h *g, const char *filename)
 {
-  if (check_path(g, filename) == -1)
+  if (strchr (filename, ':') != NULL) {
+    error (g, _("filename cannot contain ':' (colon) character.
"
+                "This is a limitation of qemu."));
     return -1;
+  }
 
   if (access (filename, F_OK) == -1) {
     perrorf (g, "%s", filename);
-- 
1.7.9.3

From: "Richard W.M. Jones" <rjones at redhat.com>

---
 src/launch.c |   19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/src/launch.c b/src/launch.c
index fe858b8..b61c538 100644
--- a/src/launch.c
+++ b/src/launch.c
@@ -1370,18 +1370,31 @@ is_openable (guestfs_h *g, const char *path, int flags)
 static char *
 qemu_drive_param (guestfs_h *g, const struct drive *drv)
 {
+  size_t i;
   size_t len = 64;
+  const char *p;
   char *r;
 
-  len += strlen (drv->path);
+  len += strlen (drv->path) * 2; /* every "," could become
",," */
   len += strlen (drv->iface);
   if (drv->format)
     len += strlen (drv->format);
 
   r = safe_malloc (g, len);
 
-  snprintf (r, len, "file=%s%s%s%s%s,if=%s",
-            drv->path,
+  strcpy (r, "file=");
+  i = 5;
+
+  /* Copy the path in, escaping any "," as ",,". */
+  for (p = drv->path; *p; p++) {
+    if (*p == ',') {
+      r[i++] = ',';
+      r[i++] = ',';
+    } else
+      r[i++] = *p;
+  }
+
+  snprintf (&r[i], len-i, "%s%s%s%s,if=%s",
             drv->readonly ? ",snapshot=on" : "",
             drv->use_cache_off ? ",cache=off" : "",
             drv->format ? ",format=" : "",
-- 
1.7.9.3

From: "Richard W.M. Jones" <rjones at redhat.com>

---
 tests/regressions/Makefile.am   |    1 +
 tests/regressions/rhbz811649.sh |   49 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 50 insertions(+)
 create mode 100755 tests/regressions/rhbz811649.sh

diff --git a/tests/regressions/Makefile.am b/tests/regressions/Makefile.am
index a2a3673..3027cd8 100644
--- a/tests/regressions/Makefile.am
+++ b/tests/regressions/Makefile.am
@@ -28,6 +28,7 @@ TESTS = \
 	rhbz690819.sh \
 	rhbz789960.sh \
 	rhbz790721 \
+	rhbz811649.sh \
 	test-noexec-stack.pl
 
 tests_not_run = \
diff --git a/tests/regressions/rhbz811649.sh b/tests/regressions/rhbz811649.sh
new file mode 100755
index 0000000..1d0e56a
--- /dev/null
+++ b/tests/regressions/rhbz811649.sh
@@ -0,0 +1,49 @@
+#!/bin/bash -
+# libguestfs
+# Copyright (C) 2012 Red Hat Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+
+# https://bugzilla.redhat.com/show_bug.cgi?id=811649
+# Test filenames containing a variety of characters.
+
+set -e
+export LANG=C
+
+declare -a filenames
+filenames[0]=' '
+filenames[1]=','
+filenames[2]='='
+filenames[3]='?'
+filenames[4]='-'
+filenames[5]='-hda'
+#filenames[6]=':'     # a future version of qemu may allow colon
+#filenames[7]='http:'
+#filenames[8]='file:'
+#filenames[9]='raw:'
+
+rm -f -- test1.img "${filenames[@]}"
+
+truncate -s 10M test1.img
+
+for f in "${filenames[@]}"; do
+    ln -- test1.img "$f"
+    guestfish <<EOF
+add "$f"
+run
+EOF
+done
+
+rm -f -- test1.img "${filenames[@]}"
-- 
1.7.9.3

On 12/04/12 19:34, Richard W.M. Jones wrote:
> Note: This is a regression in RHEL 6.3.  Please review this patch
> urgently.
>
> The description of the bug is here:
>
>    https://bugzilla.redhat.com/show_bug.cgi?id=812092
>
> This patch set attempts to fix the problem conservatively, because
> it's a very high risk codepath and very late in the development of
> RHEL 6.3.
>
> The first patch reverts the behaviour of calling realpath(3) and
> checking for duplicate filenames.
>
> The second patch fixes 'check_path'.  Only ':' is not
permitted in
> qemu paths.  (However symlinks to paths that contain ':' will work
> again because the first patch was reverted.)
>
> The third patch adds proper escaping of ',' in qemu command line
> parameters.
>
> The fourth patch adds a regression test for filenames containing
> unusual characters.
>
> Rich.
>
ACK to the whole series.

Matt
-- 
Matthew Booth, RHCA, RHCSS
Red Hat Engineering, Virtualisation Team

GPG ID:  D33C3490
GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490