[Qemu-devel] [PATCH 0/2] Add infinite loop checking in img_create()

[Qemu-devel] [PATCH 0/2] Add infinite loop checking in img_create()

[Xu Wang]

From: Xu Wang <cngesaint@outlook.com>

If user creates a image with loop in backing file, qemu doesn't give any
warning or error report and creation successful. If this image was opend
by qemu, no response would occure and segment fault would happend at last.
Hence these patches refine and export infinite loop checking in
collect_image_info_list() and add checking into img_create(). If a loop
would occure, an error info output and creation interrupted.

Xu Wang (2):
  Refine and export infinite loop checking in collect_image_info_list()
  Check infinite loop in img_create()

 qemu-img.c | 121 ++++++++++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 100 insertions(+), 21 deletions(-)

-- 
1.8.1.4

[Qemu-devel] [PATCH 1/2] Refine and export infinite loop checking in collect_image_info_list()

[Xu Wang]

From: Xu Wang <cngesaint@outlook.com>

Signed-off-by: Xu Wang <cngesaint@outlook.com>
---
 qemu-img.c | 110 +++++++++++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 89 insertions(+), 21 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 809b4f1..0bc265d 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -324,6 +324,86 @@ static int add_old_style_options(const char *fmt, QEMUOptionParameter *list,
     return 0;
 }
 
+static gboolean str_equal_func(gconstpointer a, gconstpointer b)
+{
+    return strcmp(a, b) == 0;
+}
+
+/**
+ * Check backing file chain if there is a loop in it and build list of
+ * ImageInfo if needed.
+ *
+ * @filename: topmost image filename
+ * @fmt: topmost image format (may be NULL to autodetect)
+ * @head: head of ImageInfo list. If not need please set head to null.
+ * @chain: true  - enumerate entire backing file chain
+ *         false - only topmost image file
+ * @backing_file: if this value is set, filename will insert into hash
+ *                table directly. open and seek backing file start from it.
+ *
+ * If return true, stands for a backing file loop exists or some error
+ * happend. If return false, everything is ok.
+ */
+static bool backing_file_loop_check(const char *filename, const char *fmt,
+                             bool chain, const char *backing_file) {
+    GHashTable *filenames;
+    BlockDriverState *bs;
+    ImageInfo *info;
+    Error *err = NULL;
+
+    filenames = g_hash_table_new_full(g_str_hash, str_equal_func, NULL, NULL);
+
+    /* If backing file exists, filename will insert into hash table and seek
+     * the whole backing file chain from @backing_file.
+     */
+    if (backing_file) {
+        g_hash_table_insert(filenames, (gpointer)filename, NULL);
+        filename = backing_file;
+    }
+
+    while (filename) {
+        if (g_hash_table_lookup_extended(filenames, filename, NULL, NULL)) {
+            error_report("Backing file '%s' creates an infinite loop.",
+                         filename);
+            goto err;
+        }
+        g_hash_table_insert(filenames, (gpointer)filename, NULL);
+
+        bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_NO_BACKING,
+                           false, false);
+        if (!bs) {
+            goto err;
+        }
+
+        bdrv_query_image_info(bs, &info, &err);
+        if (error_is_set(&err)) {
+            error_report("%s", error_get_pretty(err));
+            error_free(err);
+            goto err;
+        }
+
+        bdrv_delete(bs);
+
+        filename = fmt = NULL;
+        if (chain) {
+            if (info->has_full_backing_filename) {
+                filename = info->full_backing_filename;
+            } else if (info->has_backing_filename) {
+                filename = info->backing_filename;
+            }
+            if (info->has_backing_filename_format) {
+                fmt = info->backing_filename_format;
+            }
+        }
+    }
+    g_hash_table_destroy(filenames);
+    return false;
+
+err:
+    g_hash_table_destroy(filenames);
+    return true;
+}
+
 static int img_create(int argc, char **argv)
 {
     int c;
@@ -1620,11 +1700,6 @@ static void dump_human_image_info_list(ImageInfoList *list)
     }
 }
 
-static gboolean str_equal_func(gconstpointer a, gconstpointer b)
-{
-    return strcmp(a, b) == 0;
-}
-
 /**
  * Open an image file chain and return an ImageInfoList
  *
@@ -1642,24 +1717,18 @@ static ImageInfoList *collect_image_info_list(const char *filename,
                                               bool chain)
 {
     ImageInfoList *head = NULL;
+    BlockDriverState *bs;
+    ImageInfoList *elem;
     ImageInfoList **last = &head;
-    GHashTable *filenames;
+    ImageInfo *info;
     Error *err = NULL;
-
-    filenames = g_hash_table_new_full(g_str_hash, str_equal_func, NULL, NULL);
+        
+    /* check if loop exists and build ImageInfoList */
+    if (backing_file_loop_check(filename, fmt, chain, NULL)) {
+        goto err;
+    }
 
     while (filename) {
-        BlockDriverState *bs;
-        ImageInfo *info;
-        ImageInfoList *elem;
-
-        if (g_hash_table_lookup_extended(filenames, filename, NULL, NULL)) {
-            error_report("Backing file '%s' creates an infinite loop.",
-                         filename);
-            goto err;
-        }
-        g_hash_table_insert(filenames, (gpointer)filename, NULL);
-
         bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_NO_BACKING,
                            false, false);
         if (!bs) {
@@ -1692,12 +1761,11 @@ static ImageInfoList *collect_image_info_list(const char *filename,
             }
         }
     }
-    g_hash_table_destroy(filenames);
+
     return head;
 
 err:
     qapi_free_ImageInfoList(head);
-    g_hash_table_destroy(filenames);
     return NULL;
 }
 
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 1/2] Refine and export infinite loop checking in collect_image_info_list()

[Eric Blake]

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

On 06/27/2013 01:38 AM, Xu Wang wrote:
> From: Xu Wang <cngesaint@outlook.com>
> 
> Signed-off-by: Xu Wang <cngesaint@outlook.com>
> ---
>  qemu-img.c | 110 +++++++++++++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 89 insertions(+), 21 deletions(-)
> 

> +/**
> + * Check backing file chain if there is a loop in it and build list of
> + * ImageInfo if needed.
> + *
> + * @filename: topmost image filename

absolute? relative?

> + * @fmt: topmost image format (may be NULL to autodetect)
> + * @head: head of ImageInfo list. If not need please set head to null.
> + * @chain: true  - enumerate entire backing file chain
> + *         false - only topmost image file
> + * @backing_file: if this value is set, filename will insert into hash
> + *                table directly. open and seek backing file start from it.
> + *
> + * If return true, stands for a backing file loop exists or some error
> + * happend. If return false, everything is ok.

s/happend/happened/

> + */
> +static bool backing_file_loop_check(const char *filename, const char *fmt,
> +                             bool chain, const char *backing_file) {

Indentation is off.

> +    GHashTable *filenames;
> +    BlockDriverState *bs;
> +    ImageInfo *info;
> +    Error *err = NULL;
> +
> +    filenames = g_hash_table_new_full(g_str_hash, str_equal_func, NULL, NULL);
> +
> +    /* If backing file exists, filename will insert into hash table and seek
> +     * the whole backing file chain from @backing_file.
> +     */
> +    if (backing_file) {
> +        g_hash_table_insert(filenames, (gpointer)filename, NULL);

Does this have any false positives (perhaps mishandling due to relative
names) or false negatives (perhaps hard links allow different spellings
of the same file to create a loop, although the difference in names
won't indicate the problem)?  I'd really like to see you add a testcase
before this patch gets committed, although I agree that a patch along
these lines is worthwhile.  For example, make sure the following chain
is not rejected:

/dir1/base.img <- /dir1/wrap.img(relative backing 'base.img') <-
/dir2/base.img (absolute backing '/dir1/base.img') <-
/dir2/wrap.img(relative backing 'base.img')

whether opened in /dir2/ via relative name 'wrap.img' or absolute name
'/dir2/wrap.img'.  Likewise, make sure you can detect this loop:

create directory 'dir'
create './dir/b.img'
create './b.img' with relative backing 'dir/b.img'
remove ./dir/b.img and dir
ln -s . dir
now 'b.img' refers to itself as backing file, even though the names
./b.img and ./dir/b.img are not equal by strcmp.

-- 
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: 621 bytes --]

Re: [Qemu-devel] [PATCH 1/2] Refine and export infinite loop checking in collect_image_info_list()

[Stefan Hajnoczi]

On Fri, Jun 28, 2013 at 02:37:52PM -0600, Eric Blake wrote:
> On 06/27/2013 01:38 AM, Xu Wang wrote:
> > +    filenames = g_hash_table_new_full(g_str_hash, str_equal_func, NULL, NULL);
> > +
> > +    /* If backing file exists, filename will insert into hash table and seek
> > +     * the whole backing file chain from @backing_file.
> > +     */
> > +    if (backing_file) {
> > +        g_hash_table_insert(filenames, (gpointer)filename, NULL);
> 
> Does this have any false positives (perhaps mishandling due to relative
> names) or false negatives (perhaps hard links allow different spellings
> of the same file to create a loop, although the difference in names
> won't indicate the problem)?  I'd really like to see you add a testcase
> before this patch gets committed, although I agree that a patch along
> these lines is worthwhile.  For example, make sure the following chain
> is not rejected:
> 
> /dir1/base.img <- /dir1/wrap.img(relative backing 'base.img') <-
> /dir2/base.img (absolute backing '/dir1/base.img') <-
> /dir2/wrap.img(relative backing 'base.img')
> 
> whether opened in /dir2/ via relative name 'wrap.img' or absolute name
> '/dir2/wrap.img'.  Likewise, make sure you can detect this loop:
> 
> create directory 'dir'
> create './dir/b.img'
> create './b.img' with relative backing 'dir/b.img'
> remove ./dir/b.img and dir
> ln -s . dir
> now 'b.img' refers to itself as backing file, even though the names
> ./b.img and ./dir/b.img are not equal by strcmp.

Yes, a test case should be added in tests/qemu-iotests/.  Please see
this wiki page for documentation:

http://qemu-project.org/Documentation/QemuIoTests

Stefan

Re: [Qemu-devel] [PATCH 1/2] Refine and export infinite loop checking in collect_image_info_list()

[Xu Wang]

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

2013/6/29 Eric Blake <eblake@redhat.com>

> On 06/27/2013 01:38 AM, Xu Wang wrote:
> > From: Xu Wang <cngesaint@outlook.com>
> >
> > Signed-off-by: Xu Wang <cngesaint@outlook.com>
> > ---
> >  qemu-img.c | 110
> +++++++++++++++++++++++++++++++++++++++++++++++++------------
> >  1 file changed, 89 insertions(+), 21 deletions(-)
> >
>
> > +/**
> > + * Check backing file chain if there is a loop in it and build list of
> > + * ImageInfo if needed.
> > + *
> > + * @filename: topmost image filename
>
> absolute? relative?
>
either is OK.

>
> > + * @fmt: topmost image format (may be NULL to autodetect)
> > + * @head: head of ImageInfo list. If not need please set head to null.
> > + * @chain: true  - enumerate entire backing file chain
> > + *         false - only topmost image file
> > + * @backing_file: if this value is set, filename will insert into hash
> > + *                table directly. open and seek backing file start from
> it.
> > + *
> > + * If return true, stands for a backing file loop exists or some error
> > + * happend. If return false, everything is ok.
>
> s/happend/happened/
>
updated in V2

>
> > + */
> > +static bool backing_file_loop_check(const char *filename, const char
> *fmt,
> > +                             bool chain, const char *backing_file) {
>
> Indentation is off.
>
> > +    GHashTable *filenames;
> > +    BlockDriverState *bs;
> > +    ImageInfo *info;
> > +    Error *err = NULL;
> > +
> > +    filenames = g_hash_table_new_full(g_str_hash, str_equal_func, NULL,
> NULL);
> > +
> > +    /* If backing file exists, filename will insert into hash table and
> seek
> > +     * the whole backing file chain from @backing_file.
> > +     */
> > +    if (backing_file) {
> > +        g_hash_table_insert(filenames, (gpointer)filename, NULL);
>
> Does this have any false positives (perhaps mishandling due to relative
> names) or false negatives (perhaps hard links allow different spellings
> of the same file to create a loop, although the difference in names
> won't indicate the problem)?  I'd really like to see you add a testcase
> before this patch gets committed, although I agree that a patch along
> these lines is worthwhile.  For example, make sure the following chain
> is not rejected:
>
> /dir1/base.img <- /dir1/wrap.img(relative backing 'base.img') <-
> /dir2/base.img (absolute backing '/dir1/base.img') <-
> /dir2/wrap.img(relative backing 'base.img')
>
> whether opened in /dir2/ via relative name 'wrap.img' or absolute name
> '/dir2/wrap.img'.  Likewise, make sure you can detect this loop:
>
> create directory 'dir'
> create './dir/b.img'
> create './b.img' with relative backing 'dir/b.img'
> remove ./dir/b.img and dir
> ln -s . dir
> now 'b.img' refers to itself as backing file, even though the names
> ./b.img and ./dir/b.img are not equal by strcmp.
>
I did all test for my new version patch. It recognize the same file by
inode instead
of filename.

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

[-- Attachment #2: Type: text/html, Size: 4491 bytes --]

[Qemu-devel] [PATCH 2/2] Check infinite loop in img_create()

[Xu Wang]

From: Xu Wang <cngesaint@outlook.com>

Signed-off-by: Xu Wang <cngesaint@outlook.com>
---
 qemu-img.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/qemu-img.c b/qemu-img.c
index 0bc265d..fe11421 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -415,6 +415,7 @@ static int img_create(int argc, char **argv)
     char *options = NULL;
     Error *local_err = NULL;
     bool quiet = false;
+    char backing_file[1024];
 
     for(;;) {
         c = getopt(argc, argv, "F:b:f:he6o:q");
@@ -481,6 +482,16 @@ static int img_create(int argc, char **argv)
         return print_block_option_help(filename, fmt);
     }
 
+    /* check infinite loop in backing file chain */
+    if (options) {
+        if (get_param_value(backing_file, sizeof(backing_file),
+                            "backing_file", options)) {
+            if (backing_file_loop_check(filename, fmt, true, backing_file)) {
+                return 1;
+            }
+        }
+    }
+
     bdrv_img_create(filename, fmt, base_filename, base_fmt,
                     options, img_size, BDRV_O_FLAGS, &local_err, quiet);
     if (error_is_set(&local_err)) {
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 0/2] Add infinite loop checking in img_create()

[Andreas Färber]

Am 27.06.2013 09:38, schrieb Xu Wang:
> From: Xu Wang <cngesaint@outlook.com>
> 
> If user creates a image with loop in backing file, qemu doesn't give any
> warning or error report and creation successful. If this image was opend
> by qemu, no response would occure and segment fault would happend at last.
> Hence these patches refine and export infinite loop checking in
> collect_image_info_list() and add checking into img_create(). If a loop
> would occure, an error info output and creation interrupted.
> 
> Xu Wang (2):
>   Refine and export infinite loop checking in collect_image_info_list()
>   Check infinite loop in img_create()

In the future please use a prefix such as "qemu-img: ..." on both cover
letter and patches to indicate which part of code it concerns, and CC
the respective maintainers of that area to assure appropriate review.

Regards,
Andreas

> 
>  qemu-img.c | 121 ++++++++++++++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 100 insertions(+), 21 deletions(-)
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 0/2] Add infinite loop checking in img_create()

[Stefan Hajnoczi]

On Sat, Jun 29, 2013 at 04:32:34PM +0200, Andreas Färber wrote:
> Am 27.06.2013 09:38, schrieb Xu Wang:
> > From: Xu Wang <cngesaint@outlook.com>
> > 
> > If user creates a image with loop in backing file, qemu doesn't give any
> > warning or error report and creation successful. If this image was opend
> > by qemu, no response would occure and segment fault would happend at last.
> > Hence these patches refine and export infinite loop checking in
> > collect_image_info_list() and add checking into img_create(). If a loop
> > would occure, an error info output and creation interrupted.
> > 
> > Xu Wang (2):
> >   Refine and export infinite loop checking in collect_image_info_list()
> >   Check infinite loop in img_create()
> 
> In the future please use a prefix such as "qemu-img: ..." on both cover
> letter and patches to indicate which part of code it concerns, and CC
> the respective maintainers of that area to assure appropriate review.

To find out who to Cc on patch emails, use:

  $ scripts/get_maintainer.pl -f qemu-img.c

Stefan