Patchwork [v2,2/3] qemu-img: Detect backing file chain infinite loops

login
register
mail settings
Submitter Stefan Hajnoczi
Date Oct. 15, 2012, 12:44 p.m.
Message ID <1350305057-6287-3-git-send-email-stefanha@redhat.com>
Download mbox | patch
Permalink /patch/191556/
State New
Headers show

Comments

Stefan Hajnoczi - Oct. 15, 2012, 12:44 p.m.
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(+)
Kevin Wolf - Oct. 16, 2012, 10:11 a.m.
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
Stefan Hajnoczi - Oct. 16, 2012, 2:42 p.m.
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
Kevin Wolf - Oct. 16, 2012, 2:58 p.m.
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

Patch

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;
 }