[Qemu-devel] [PATCH v2 0/3] qemu-img: Add --backing-chain option to info command

[Qemu-devel] [PATCH v2 0/3] qemu-img: Add --backing-chain option to info command

[Stefan Hajnoczi]

This series adds the --backing-chain option for enumerating the backing file
chain.  Given the topmost image it will print qemu-img info information for
each image file in the chain.

Special care needs to be taken when image files form an infinite loop.  This is
very unusual, most like due to malicious image files.  Nevertheless, qemu-img
must be robust against invalid inputs so we explicit check for this.

Stefan Hajnoczi (3):
  qemu-img: Add --backing-chain option to info command
  qemu-img: Detect backing file chain infinite loops
  qemu-iotests: Add 041 backing file chain infinite loop test

 qemu-img.c                   | 115 ++++++++++++++++++++++++++++++++++---------
 tests/qemu-iotests/041       |  90 +++++++++++++++++++++++++++++++++
 tests/qemu-iotests/041.out   |  81 ++++++++++++++++++++++++++++++
 tests/qemu-iotests/common.rc |   9 ++++
 tests/qemu-iotests/group     |   1 +
 5 files changed, 274 insertions(+), 22 deletions(-)
 create mode 100755 tests/qemu-iotests/041
 create mode 100644 tests/qemu-iotests/041.out

-- 
1.7.11.7

[Qemu-devel] [PATCH v2 1/3] qemu-img: Add --backing-chain option to info command

[Stefan Hajnoczi]

The qemu-img info --backing-chain option enumerates the backing file
chain.  For example, for base.qcow2 <- snap1.qcow2 <- snap2.qcow2 the
output becomes:

  $ qemu-img info --backing-chain snap2.qcow2
  image: snap2.qcow2
  file format: qcow2
  virtual size: 100M (104857600 bytes)
  disk size: 196K
  cluster_size: 65536
  backing file: snap1.qcow2
  backing file format: qcow2

  image: snap1.qcow2
  file format: qcow2
  virtual size: 100M (104857600 bytes)
  disk size: 196K
  cluster_size: 65536
  backing file: base.qcow2
  backing file format: qcow2

  image: base.qcow2
  file format: qcow2
  virtual size: 100M (104857600 bytes)
  disk size: 136K
  cluster_size: 65536

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 qemu-img.c | 98 ++++++++++++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 76 insertions(+), 22 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index f17f187..c717f3e 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1249,7 +1249,10 @@ static void dump_human_image_info(ImageInfo *info)
     }
 }
 
-enum {OPTION_OUTPUT = 256};
+enum {
+    OPTION_OUTPUT = 256,
+    OPTION_BACKING_CHAIN = 257,
+};
 
 typedef enum OutputFormat {
     OFORMAT_JSON,
@@ -1260,7 +1263,9 @@ static int img_info(int argc, char **argv)
 {
     int c;
     OutputFormat output_format = OFORMAT_HUMAN;
-    const char *filename, *fmt, *output;
+    bool chain = false;
+    const char *output;
+    char *filename, *fmt;
     BlockDriverState *bs;
     ImageInfo *info;
 
@@ -1272,6 +1277,7 @@ static int img_info(int argc, char **argv)
             {"help", no_argument, 0, 'h'},
             {"format", required_argument, 0, 'f'},
             {"output", required_argument, 0, OPTION_OUTPUT},
+            {"backing-chain", no_argument, 0, OPTION_BACKING_CHAIN},
             {0, 0, 0, 0}
         };
         c = getopt_long(argc, argv, "f:h",
@@ -1285,17 +1291,20 @@ static int img_info(int argc, char **argv)
             help();
             break;
         case 'f':
-            fmt = optarg;
+            fmt = g_strdup(optarg);
             break;
         case OPTION_OUTPUT:
             output = optarg;
             break;
+        case OPTION_BACKING_CHAIN:
+            chain = true;
+            break;
         }
     }
     if (optind >= argc) {
         help();
     }
-    filename = argv[optind++];
+    filename = g_strdup(argv[optind++]);
 
     if (output && !strcmp(output, "json")) {
         output_format = OFORMAT_JSON;
@@ -1303,31 +1312,76 @@ static int img_info(int argc, char **argv)
         output_format = OFORMAT_HUMAN;
     } else if (output) {
         error_report("--output must be used with human or json as argument.");
-        return 1;
+        goto err;
     }
 
-    bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_NO_BACKING, false);
-    if (!bs) {
-        return 1;
+    if (chain && output_format == OFORMAT_JSON) {
+        printf("[\n");
     }
 
-    info = g_new0(ImageInfo, 1);
-    collect_image_info(bs, info, filename, fmt);
+    do {
+        bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_NO_BACKING,
+                           false);
+        if (!bs) {
+            goto err;
+        }
 
-    switch (output_format) {
-    case OFORMAT_HUMAN:
-        dump_human_image_info(info);
-        dump_snapshots(bs);
-        break;
-    case OFORMAT_JSON:
-        collect_snapshots(bs, info);
-        dump_json_image_info(info);
-        break;
-    }
+        info = g_new0(ImageInfo, 1);
+        collect_image_info(bs, info, filename, fmt);
 
-    qapi_free_ImageInfo(info);
-    bdrv_delete(bs);
+        switch (output_format) {
+        case OFORMAT_HUMAN:
+            dump_human_image_info(info);
+            dump_snapshots(bs);
+            break;
+        case OFORMAT_JSON:
+            collect_snapshots(bs, info);
+            dump_json_image_info(info);
+            break;
+        }
+
+        g_free(filename);
+        g_free(fmt);
+        filename = NULL;
+        fmt = NULL;
+
+        if (chain) {
+            if (info->has_full_backing_filename) {
+                filename = g_strdup(info->full_backing_filename);
+            } else if (info->has_backing_filename) {
+                filename = g_strdup(info->backing_filename);
+            }
+
+            if (filename && info->has_backing_filename_format) {
+                fmt = g_strdup(info->backing_filename_format);
+            }
+
+            /* Print delimiters between items */
+            if (filename) {
+                switch (output_format) {
+                case OFORMAT_HUMAN:
+                    printf("\n");
+                    break;
+                case OFORMAT_JSON:
+                    printf(",\n");
+                    break;
+                }
+            }
+        }
+
+        qapi_free_ImageInfo(info);
+        bdrv_delete(bs);
+    } while (filename);
+
+    if (chain && output_format == OFORMAT_JSON) {
+        printf("]\n");
+    }
     return 0;
+
+err:
+    g_free(filename);
+    g_free(fmt);
+    return 1;
 }
 
 #define SNAPSHOT_LIST   1
-- 
1.7.11.7

[Qemu-devel] [PATCH v2 2/3] qemu-img: Detect backing file chain infinite loops

[Stefan Hajnoczi]

A malicious or corruption image can contain an infinite loop of backing
files.  The qemu-img info --backing-chain command must not hang when
such files are encountered.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 qemu-img.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/qemu-img.c b/qemu-img.c
index c717f3e..60a5cc8 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1259,6 +1259,11 @@ typedef enum OutputFormat {
     OFORMAT_HUMAN,
 } OutputFormat;
 
+static gboolean str_equal_func(gconstpointer a, gconstpointer b)
+{
+    return strcmp(a, b) == 0;
+}
+
 static int img_info(int argc, char **argv)
 {
     int c;
@@ -1268,6 +1273,7 @@ static int img_info(int argc, char **argv)
     char *filename, *fmt;
     BlockDriverState *bs;
     ImageInfo *info;
+    GHashTable *filenames;
 
     fmt = NULL;
     output = NULL;
@@ -1306,6 +1312,9 @@ static int img_info(int argc, char **argv)
     }
     filename = g_strdup(argv[optind++]);
 
+    filenames = g_hash_table_new_full(g_str_hash, str_equal_func,
+                                      g_free, NULL);
+
     if (output && !strcmp(output, "json")) {
         output_format = OFORMAT_JSON;
     } else if (output && !strcmp(output, "human")) {
@@ -1320,6 +1329,12 @@ static int img_info(int argc, char **argv)
     }
 
     do {
+        if (g_hash_table_lookup_extended(filenames, filename, NULL, NULL)) {
+            error_report("Aborting due to backing file chain infinite loop.");
+            goto err;
+        }
+        g_hash_table_insert(filenames, g_strdup(filename), NULL);
+
         bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_NO_BACKING,
                            false);
         if (!bs) {
@@ -1376,11 +1391,13 @@ static int img_info(int argc, char **argv)
     if (chain && output_format == OFORMAT_JSON) {
         printf("]\n");
     }
+    g_hash_table_destroy(filenames);
     return 0;
 
 err:
     g_free(filename);
     g_free(fmt);
+    g_hash_table_destroy(filenames);
     return 1;
 }
 
-- 
1.7.11.7

[Qemu-devel] [PATCH v2 3/3] qemu-iotests: Add 041 backing file chain infinite loop test

[Stefan Hajnoczi]

This new test verifies that qemu-img info --backing-chain safely aborts
when an image file has a backing file infinite loop.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/qemu-iotests/041       | 90 ++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/041.out   | 81 +++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/common.rc |  9 +++++
 tests/qemu-iotests/group     |  1 +
 4 files changed, 181 insertions(+)
 create mode 100755 tests/qemu-iotests/041
 create mode 100644 tests/qemu-iotests/041.out

diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
new file mode 100755
index 0000000..839255f
--- /dev/null
+++ b/tests/qemu-iotests/041
@@ -0,0 +1,90 @@
+#!/bin/bash
+#
+# Test that qemu-img info --backing-chain detects infinite loops
+#
+# 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, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=stefanha@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+
+_cleanup()
+{
+	_cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+# Any format supporting backing files
+_supported_fmt qcow qcow2 vmdk qed
+_supported_proto generic
+_supported_os Linux
+
+
+size=128M
+_make_test_img $size
+$QEMU_IMG rebase -u -b $TEST_IMG $TEST_IMG
+
+echo
+echo "== backing file references self =="
+_img_info --backing-chain
+
+_make_test_img $size
+mv $TEST_IMG $TEST_IMG.base
+_make_test_img -b $TEST_IMG.base $size
+$QEMU_IMG rebase -u -b $TEST_IMG $TEST_IMG.base
+
+echo
+echo "== parent references self =="
+_img_info --backing-chain
+
+_make_test_img $size
+mv $TEST_IMG $TEST_IMG.1.base
+_make_test_img -b $TEST_IMG.1.base $size
+mv $TEST_IMG $TEST_IMG.2.base
+_make_test_img -b $TEST_IMG.2.base $size
+mv $TEST_IMG $TEST_IMG.3.base
+_make_test_img -b $TEST_IMG.3.base $size
+$QEMU_IMG rebase -u -b $TEST_IMG.2.base $TEST_IMG.1.base
+
+echo
+echo "== ancestor references another ancestor =="
+_img_info --backing-chain
+
+_make_test_img $size
+mv $TEST_IMG $TEST_IMG.1.base
+_make_test_img -b $TEST_IMG.1.base $size
+mv $TEST_IMG $TEST_IMG.2.base
+_make_test_img -b $TEST_IMG.2.base $size
+
+echo
+echo "== finite chain of length 3 =="
+_img_info --backing-chain
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/041.out b/tests/qemu-iotests/041.out
new file mode 100644
index 0000000..0c871b0
--- /dev/null
+++ b/tests/qemu-iotests/041.out
@@ -0,0 +1,81 @@
+QA output created by 041
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 
+
+== backing file references self ==
+qemu-img: Aborting due to backing file chain infinite loop.
+image: TEST_DIR/t.IMGFMT
+file format: IMGFMT
+virtual size: 128M (134217728 bytes)
+cluster_size: 65536
+backing file: TEST_DIR/t.IMGFMT
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 backing_file='TEST_DIR/t.IMGFMT.base' 
+
+== parent references self ==
+qemu-img: Aborting due to backing file chain infinite loop.
+image: TEST_DIR/t.IMGFMT
+file format: IMGFMT
+virtual size: 128M (134217728 bytes)
+cluster_size: 65536
+backing file: TEST_DIR/t.IMGFMT.base
+
+image: TEST_DIR/t.IMGFMT.base
+file format: IMGFMT
+virtual size: 128M (134217728 bytes)
+cluster_size: 65536
+backing file: TEST_DIR/t.IMGFMT
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 backing_file='TEST_DIR/t.IMGFMT.1.base' 
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 backing_file='TEST_DIR/t.IMGFMT.2.base' 
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 backing_file='TEST_DIR/t.IMGFMT.3.base' 
+
+== ancestor references another ancestor ==
+qemu-img: Aborting due to backing file chain infinite loop.
+image: TEST_DIR/t.IMGFMT
+file format: IMGFMT
+virtual size: 128M (134217728 bytes)
+cluster_size: 65536
+backing file: TEST_DIR/t.IMGFMT.3.base
+
+image: TEST_DIR/t.IMGFMT.3.base
+file format: IMGFMT
+virtual size: 128M (134217728 bytes)
+cluster_size: 65536
+backing file: TEST_DIR/t.IMGFMT.2.base
+
+image: TEST_DIR/t.IMGFMT.2.base
+file format: IMGFMT
+virtual size: 128M (134217728 bytes)
+cluster_size: 65536
+backing file: TEST_DIR/t.IMGFMT.1.base
+
+image: TEST_DIR/t.IMGFMT.1.base
+file format: IMGFMT
+virtual size: 128M (134217728 bytes)
+cluster_size: 65536
+backing file: TEST_DIR/t.IMGFMT.2.base
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 backing_file='TEST_DIR/t.IMGFMT.1.base' 
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 backing_file='TEST_DIR/t.IMGFMT.2.base' 
+
+== finite chain of length 3 ==
+image: TEST_DIR/t.IMGFMT
+file format: IMGFMT
+virtual size: 128M (134217728 bytes)
+cluster_size: 65536
+backing file: TEST_DIR/t.IMGFMT.2.base
+
+image: TEST_DIR/t.IMGFMT.2.base
+file format: IMGFMT
+virtual size: 128M (134217728 bytes)
+cluster_size: 65536
+backing file: TEST_DIR/t.IMGFMT.1.base
+
+image: TEST_DIR/t.IMGFMT.1.base
+file format: IMGFMT
+virtual size: 128M (134217728 bytes)
+cluster_size: 65536
+*** done
diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index d534e94..aa80b5f 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -145,6 +145,15 @@ _check_test_img()
     	sed -e 's/qemu-img\: This image format does not support checks/No errors were found on the image./'
 }
 
+_img_info()
+{
+    $QEMU_IMG info "$@" $TEST_IMG | \
+        sed -e "s#$IMGPROTO:$TEST_DIR#TEST_DIR#g" \
+            -e "s#$TEST_DIR#TEST_DIR#g" \
+            -e "s#$IMGFMT#IMGFMT#g" \
+            -e "/^disk size:/ D" # ignore on-disk size, it can vary
+}
+
 _get_pids_by_name()
 {
     if [ $# -ne 1 ]
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 66d2ba9..ee73b8d 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -47,3 +47,4 @@
 038 rw auto backing
 039 rw auto
 040 rw auto
+041 rw auto backing
-- 
1.7.11.7

Re: [Qemu-devel] [PATCH v2 3/3] qemu-iotests: Add 041 backing file chain infinite loop test

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 678 bytes --]

On 10/15/2012 06:44 AM, Stefan Hajnoczi wrote:
> This new test verifies that qemu-img info --backing-chain safely aborts
> when an image file has a backing file infinite loop.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  tests/qemu-iotests/041       | 90 ++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/041.out   | 81 +++++++++++++++++++++++++++++++++++++++

Didn't Paolo already request test id 41, and 42 is already claimed,
leaving you at 43?
https://lists.gnu.org/archive/html/qemu-devel/2012-10/msg02481.html

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 617 bytes --]

Re: [Qemu-devel] [PATCH v2 0/3] qemu-img: Add --backing-chain option to info command

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1370 bytes --]

On 10/15/2012 06:44 AM, Stefan Hajnoczi wrote:
> This series adds the --backing-chain option for enumerating the backing file
> chain.  Given the topmost image it will print qemu-img info information for
> each image file in the chain.
> 
> Special care needs to be taken when image files form an infinite loop.  This is
> very unusual, most like due to malicious image files.  Nevertheless, qemu-img
> must be robust against invalid inputs so we explicit check for this.
> 
> Stefan Hajnoczi (3):
>   qemu-img: Add --backing-chain option to info command
>   qemu-img: Detect backing file chain infinite loops
>   qemu-iotests: Add 041 backing file chain infinite loop test
> 
>  qemu-img.c                   | 115 ++++++++++++++++++++++++++++++++++---------
>  tests/qemu-iotests/041       |  90 +++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/041.out   |  81 ++++++++++++++++++++++++++++++
>  tests/qemu-iotests/common.rc |   9 ++++
>  tests/qemu-iotests/group     |   1 +
>  5 files changed, 274 insertions(+), 22 deletions(-)
>  create mode 100755 tests/qemu-iotests/041
>  create mode 100644 tests/qemu-iotests/041.out

Series: Reviewed-by: Eric Blake <eblake@redhat.com>
although you may need a file rename in patch 3/3

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 617 bytes --]

Re: [Qemu-devel] [PATCH v2 3/3] qemu-iotests: Add 041 backing file chain infinite loop test

[Stefan Hajnoczi]

On Mon, Oct 15, 2012 at 11:47:04AM -0600, Eric Blake wrote:
> On 10/15/2012 06:44 AM, Stefan Hajnoczi wrote:
> > This new test verifies that qemu-img info --backing-chain safely aborts
> > when an image file has a backing file infinite loop.
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  tests/qemu-iotests/041       | 90 ++++++++++++++++++++++++++++++++++++++++++++
> >  tests/qemu-iotests/041.out   | 81 +++++++++++++++++++++++++++++++++++++++
> 
> Didn't Paolo already request test id 41, and 42 is already claimed,
> leaving you at 43?
> https://lists.gnu.org/archive/html/qemu-devel/2012-10/msg02481.html

Okay, thanks.  I didn't see that before sending.

Kevin, Paolo: let me know after you've reviewed these patches and I can
bump to 043.

Stefan

Re: [Qemu-devel] [PATCH v2 1/3] qemu-img: Add --backing-chain option to info command

[Kevin Wolf]

Am 15.10.2012 14:44, schrieb Stefan Hajnoczi:
> The qemu-img info --backing-chain option enumerates the backing file
> chain.  For example, for base.qcow2 <- snap1.qcow2 <- snap2.qcow2 the
> output becomes:
> 
>   $ qemu-img info --backing-chain snap2.qcow2
>   image: snap2.qcow2
>   file format: qcow2
>   virtual size: 100M (104857600 bytes)
>   disk size: 196K
>   cluster_size: 65536
>   backing file: snap1.qcow2
>   backing file format: qcow2
> 
>   image: snap1.qcow2
>   file format: qcow2
>   virtual size: 100M (104857600 bytes)
>   disk size: 196K
>   cluster_size: 65536
>   backing file: base.qcow2
>   backing file format: qcow2
> 
>   image: base.qcow2
>   file format: qcow2
>   virtual size: 100M (104857600 bytes)
>   disk size: 136K
>   cluster_size: 65536
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  qemu-img.c | 98 ++++++++++++++++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 76 insertions(+), 22 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index f17f187..c717f3e 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1249,7 +1249,10 @@ static void dump_human_image_info(ImageInfo *info)
>      }
>  }
>  
> -enum {OPTION_OUTPUT = 256};
> +enum {
> +    OPTION_OUTPUT = 256,
> +    OPTION_BACKING_CHAIN = 257,
> +};
>  
>  typedef enum OutputFormat {
>      OFORMAT_JSON,
> @@ -1260,7 +1263,9 @@ static int img_info(int argc, char **argv)
>  {
>      int c;
>      OutputFormat output_format = OFORMAT_HUMAN;
> -    const char *filename, *fmt, *output;
> +    bool chain = false;
> +    const char *output;
> +    char *filename, *fmt;

Maybe I'm missing something, but where are the strings pointed at by
filename and fmt ever modified? If they aren't, strdup() isn't necessary
either.

Kevin

Re: [Qemu-devel] [PATCH v2 2/3] qemu-img: Detect backing file chain infinite loops

[Kevin Wolf]

Am 15.10.2012 14:44, schrieb Stefan Hajnoczi:
> A malicious or corruption image can contain an infinite loop of backing
> files.  The qemu-img info --backing-chain command must not hang when
> such files are encountered.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

This seems to do what is intended, but I think rather than fixing the
'qemu-img info' special case I'd have fixed bdrv_open() to detect the
situation.

Do we need to properly communicate what the broken image is with JSON
output? This patch will only produce broken JSON in the failure case.

Kevin

Re: [Qemu-devel] [PATCH v2 3/3] qemu-iotests: Add 041 backing file chain infinite loop test

[Kevin Wolf]

Am 15.10.2012 14:44, schrieb Stefan Hajnoczi:
> This new test verifies that qemu-img info --backing-chain safely aborts
> when an image file has a backing file infinite loop.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  tests/qemu-iotests/041       | 90 ++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/041.out   | 81 +++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/common.rc |  9 +++++
>  tests/qemu-iotests/group     |  1 +
>  4 files changed, 181 insertions(+)
>  create mode 100755 tests/qemu-iotests/041
>  create mode 100644 tests/qemu-iotests/041.out
> 
> diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
> new file mode 100755
> index 0000000..839255f
> --- /dev/null
> +++ b/tests/qemu-iotests/041
> @@ -0,0 +1,90 @@
> +#!/bin/bash
> +#
> +# Test that qemu-img info --backing-chain detects infinite loops
> +#
> +# 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, see <http://www.gnu.org/licenses/>.
> +#
> +
> +# creator
> +owner=stefanha@redhat.com
> +
> +seq=`basename $0`
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +tmp=/tmp/$$
> +status=1	# failure is the default!
> +
> +_cleanup()
> +{
> +	_cleanup_test_img

This forgets to remove $TEST_IMG.[123].base.

> diff --git a/tests/qemu-iotests/041.out b/tests/qemu-iotests/041.out
> new file mode 100644
> index 0000000..0c871b0
> --- /dev/null
> +++ b/tests/qemu-iotests/041.out
> @@ -0,0 +1,81 @@
> +QA output created by 041
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 
> +
> +== backing file references self ==
> +qemu-img: Aborting due to backing file chain infinite loop.
> +image: TEST_DIR/t.IMGFMT
> +file format: IMGFMT
> +virtual size: 128M (134217728 bytes)
> +cluster_size: 65536
> +backing file: TEST_DIR/t.IMGFMT

I'd expect the error message to be at the end. Should we disable stdout
buffering completely in qemu-img or add an fflush(stdout)?

Also I think adding a test for JSON mode would make sense.

Kevin

Re: [Qemu-devel] [PATCH v2 2/3] qemu-img: Detect backing file chain infinite loops

[Stefan Hajnoczi]

On Tue, Oct 16, 2012 at 12:11:44PM +0200, Kevin Wolf wrote:
> Am 15.10.2012 14:44, schrieb Stefan Hajnoczi:
> > A malicious or corruption image can contain an infinite loop of backing
> > files.  The qemu-img info --backing-chain command must not hang when
> > such files are encountered.
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> 
> This seems to do what is intended, but I think rather than fixing the
> 'qemu-img info' special case I'd have fixed bdrv_open() to detect the
> situation.

When bdrv_open() on the chain fails we'll still need to iterate each
image (with infinite loop detection) in qemu-img info.  This allows
qemu-img info to print out details of the chain and where the chain goes
wrong.

> Do we need to properly communicate what the broken image is with JSON
> output? This patch will only produce broken JSON in the failure case.

Ah, you're right.  We must close the JSON array and add a test case for
this :(.

Will send v3.

Stefan

Re: [Qemu-devel] [PATCH v2 1/3] qemu-img: Add --backing-chain option to info command

[Stefan Hajnoczi]

On Tue, Oct 16, 2012 at 12:04:24PM +0200, Kevin Wolf wrote:
> Am 15.10.2012 14:44, schrieb Stefan Hajnoczi:
> > The qemu-img info --backing-chain option enumerates the backing file
> > chain.  For example, for base.qcow2 <- snap1.qcow2 <- snap2.qcow2 the
> > output becomes:
> > 
> >   $ qemu-img info --backing-chain snap2.qcow2
> >   image: snap2.qcow2
> >   file format: qcow2
> >   virtual size: 100M (104857600 bytes)
> >   disk size: 196K
> >   cluster_size: 65536
> >   backing file: snap1.qcow2
> >   backing file format: qcow2
> > 
> >   image: snap1.qcow2
> >   file format: qcow2
> >   virtual size: 100M (104857600 bytes)
> >   disk size: 196K
> >   cluster_size: 65536
> >   backing file: base.qcow2
> >   backing file format: qcow2
> > 
> >   image: base.qcow2
> >   file format: qcow2
> >   virtual size: 100M (104857600 bytes)
> >   disk size: 136K
> >   cluster_size: 65536
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  qemu-img.c | 98 ++++++++++++++++++++++++++++++++++++++++++++++++--------------
> >  1 file changed, 76 insertions(+), 22 deletions(-)
> > 
> > diff --git a/qemu-img.c b/qemu-img.c
> > index f17f187..c717f3e 100644
> > --- a/qemu-img.c
> > +++ b/qemu-img.c
> > @@ -1249,7 +1249,10 @@ static void dump_human_image_info(ImageInfo *info)
> >      }
> >  }
> >  
> > -enum {OPTION_OUTPUT = 256};
> > +enum {
> > +    OPTION_OUTPUT = 256,
> > +    OPTION_BACKING_CHAIN = 257,
> > +};
> >  
> >  typedef enum OutputFormat {
> >      OFORMAT_JSON,
> > @@ -1260,7 +1263,9 @@ static int img_info(int argc, char **argv)
> >  {
> >      int c;
> >      OutputFormat output_format = OFORMAT_HUMAN;
> > -    const char *filename, *fmt, *output;
> > +    bool chain = false;
> > +    const char *output;
> > +    char *filename, *fmt;
> 
> Maybe I'm missing something, but where are the strings pointed at by
> filename and fmt ever modified? If they aren't, strdup() isn't necessary
> either.

It's necessary because the loop strdups the current image's backing
file, then frees the current image info (including the original backing
file string).  To avoid special cases for the topmost image filename/fmt
we just strdup them.

Stefan

Re: [Qemu-devel] [PATCH v2 3/3] qemu-iotests: Add 041 backing file chain infinite loop test

[Stefan Hajnoczi]

On Tue, Oct 16, 2012 at 12:22:38PM +0200, Kevin Wolf wrote:
> Am 15.10.2012 14:44, schrieb Stefan Hajnoczi:
> > This new test verifies that qemu-img info --backing-chain safely aborts
> > when an image file has a backing file infinite loop.
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  tests/qemu-iotests/041       | 90 ++++++++++++++++++++++++++++++++++++++++++++
> >  tests/qemu-iotests/041.out   | 81 +++++++++++++++++++++++++++++++++++++++
> >  tests/qemu-iotests/common.rc |  9 +++++
> >  tests/qemu-iotests/group     |  1 +
> >  4 files changed, 181 insertions(+)
> >  create mode 100755 tests/qemu-iotests/041
> >  create mode 100644 tests/qemu-iotests/041.out
> > 
> > diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
> > new file mode 100755
> > index 0000000..839255f
> > --- /dev/null
> > +++ b/tests/qemu-iotests/041
> > @@ -0,0 +1,90 @@
> > +#!/bin/bash
> > +#
> > +# Test that qemu-img info --backing-chain detects infinite loops
> > +#
> > +# 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, see <http://www.gnu.org/licenses/>.
> > +#
> > +
> > +# creator
> > +owner=stefanha@redhat.com
> > +
> > +seq=`basename $0`
> > +echo "QA output created by $seq"
> > +
> > +here=`pwd`
> > +tmp=/tmp/$$
> > +status=1	# failure is the default!
> > +
> > +_cleanup()
> > +{
> > +	_cleanup_test_img
> 
> This forgets to remove $TEST_IMG.[123].base.

Thanks for pointing this out.

> > diff --git a/tests/qemu-iotests/041.out b/tests/qemu-iotests/041.out
> > new file mode 100644
> > index 0000000..0c871b0
> > --- /dev/null
> > +++ b/tests/qemu-iotests/041.out
> > @@ -0,0 +1,81 @@
> > +QA output created by 041
> > +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 
> > +
> > +== backing file references self ==
> > +qemu-img: Aborting due to backing file chain infinite loop.
> > +image: TEST_DIR/t.IMGFMT
> > +file format: IMGFMT
> > +virtual size: 128M (134217728 bytes)
> > +cluster_size: 65536
> > +backing file: TEST_DIR/t.IMGFMT
> 
> I'd expect the error message to be at the end. Should we disable stdout
> buffering completely in qemu-img or add an fflush(stdout)?

This is a special case - we'll also need to carefully terminate the JSON
array.  So a fflush(stdout) with a comment works well.  I will add it.

> Also I think adding a test for JSON mode would make sense.

I'll do that.

Stefan

Re: [Qemu-devel] [PATCH v2 2/3] qemu-img: Detect backing file chain infinite loops

[Kevin Wolf]

Am 16.10.2012 16:42, schrieb Stefan Hajnoczi:
> On Tue, Oct 16, 2012 at 12:11:44PM +0200, Kevin Wolf wrote:
>> Am 15.10.2012 14:44, schrieb Stefan Hajnoczi:
>>> A malicious or corruption image can contain an infinite loop of backing
>>> files.  The qemu-img info --backing-chain command must not hang when
>>> such files are encountered.
>>>
>>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>>
>> This seems to do what is intended, but I think rather than fixing the
>> 'qemu-img info' special case I'd have fixed bdrv_open() to detect the
>> situation.
> 
> When bdrv_open() on the chain fails we'll still need to iterate each
> image (with infinite loop detection) in qemu-img info.  This allows
> qemu-img info to print out details of the chain and where the chain goes
> wrong.

Depends on the expected output. If you don't print the start of chain
but only an error message with the file name of the image that creates
the loop, you could generate the error in bdrv_open().

But yes, we can have both. It's just that having nothing in bdrv_open()
isn't really satisfying.

Kevin