[Qemu-devel] [PATCH V2 0/5] Add infinite loop check for backing file chain

[Qemu-devel] [PATCH V2 0/5] Add infinite loop check for backing file chain

[Xu Wang]

Updates:
  1. Changed infinite loop check in collect_image_info_list() from filename
     checking to inode checking.
  2. Absolute or relative path is OK for filename path.
  3. Hard and soft link are works well.
  4. Added WIN32 platform support (shortcuts could be recognized correctly.)
  5. Create a file which contains loop in backing file will failed.
  6. Start a vm which boot block file contains loop in backing file chain
     will failed instead of no response and segment fault.

Xu Wang (5):
  Refine and export infinite loop checking in collect_image_info_list()
  Add WIN32 platform support for backing_file_loop_check()
  Check infinite loop in bdrv_img_create()
  Add backing file loop check in change_backing_file()
  Add infinite loop check in drive_init()

 block.c               | 211 ++++++++++++++++++++++++++++++++++++++++++++++++--
 blockdev.c            |   5 ++
 include/block/block.h |   4 +
 qemu-img.c            |  30 +++----
 4 files changed, 224 insertions(+), 26 deletions(-)

-- 
1.8.1.4

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

[Xu Wang]

Signed-off-by: Xu Wang <cngesaint@gmail.com>
---
 block.c               | 101 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/block/block.h |   4 ++
 qemu-img.c            |  30 +++++----------
 3 files changed, 114 insertions(+), 21 deletions(-)

diff --git a/block.c b/block.c
index b88ad2f..53b1a01 100644
--- a/block.c
+++ b/block.c
@@ -4431,6 +4431,107 @@ bdrv_acct_done(BlockDriverState *bs, BlockAcctCookie *cookie)
     bs->total_time_ns[cookie->type] += get_clock() - cookie->start_time_ns;
 }
 
+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, absolute or relative path 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 inode (-1 if it doesn't
+ *                exist) will insert into hash table directly. Loop check
+ *                starts from it. Absolute or relative path is OK for it.
+ * @backing_format: format of backing file
+ *
+ * If return true, stands for a backing file loop exists or some error
+ * happened. If return false, everything is ok.
+ */
+bool bdrv_backing_file_loop_check(const char *filename, const char *fmt,
+                                  bool chain, const char *backing_file,
+                                  const char *backing_format) {
+    GHashTable *inodes;
+    BlockDriverState *bs;
+    BlockDriver *drv;
+    struct stat sbuf;
+    long inode = 0;
+    int ret;
+    char fbuf[1024];
+
+    inodes = g_hash_table_new_full(g_str_hash, str_equal_func, NULL, NULL);
+
+    if (backing_file) {
+        /* Check if file exists. */
+        if (access(filename, F_OK)) {
+            inode = -1;
+        } else {
+            if (stat(filename, &sbuf) == -1) {
+                error_report("Get file %s stat failed.", filename);
+                goto err;
+            }
+            inode = (long)sbuf.st_ino;
+        }
+
+        filename = backing_file;
+        fmt = backing_format;
+        g_hash_table_insert(inodes, (gpointer)&inode, NULL);
+    }
+
+    while (filename && (filename[0] != '\0')) {
+        if (stat(filename, &sbuf) == -1) {
+                error_report("Get file %s stat failed.", filename);
+                goto err;
+        }
+        inode = (long)sbuf.st_ino;
+
+        if (g_hash_table_lookup_extended(inodes, &inode, NULL, NULL)) {
+            error_report("Backing file '%s' creates an infinite loop.",
+                         filename);
+            goto err;
+        }
+        g_hash_table_insert(inodes, (gpointer)&inode, NULL);
+
+        bs = bdrv_new("image");
+
+        if (fmt) {
+            drv = bdrv_find_format(fmt);
+            if (!drv) {
+                error_report("Unknown file format '%s'", fmt);
+                goto err;
+            }
+        } else {
+            drv = NULL;
+        }
+
+        ret = bdrv_open(bs, filename, NULL,
+                        BDRV_O_CACHE_WB | BDRV_O_NO_BACKING, drv);
+        if (ret < 0) {
+            error_report("Could not open '%s': %s", filename, strerror(-ret));
+            goto err;
+        }
+
+        filename = fmt = NULL;
+        if (chain) {
+            bdrv_get_backing_filename(bs, fbuf, sizeof(fbuf));
+            filename = fbuf;
+        }
+
+        bdrv_delete(bs);
+    }
+    g_hash_table_destroy(inodes);
+    return false;
+
+err:
+    g_hash_table_destroy(inodes);
+    return true;
+}
+
 void bdrv_img_create(const char *filename, const char *fmt,
                      const char *base_filename, const char *base_fmt,
                      char *options, uint64_t img_size, int flags,
diff --git a/include/block/block.h b/include/block/block.h
index 2307f67..6c631f2 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -332,6 +332,10 @@ int bdrv_save_vmstate(BlockDriverState *bs, const uint8_t *buf,
 int bdrv_load_vmstate(BlockDriverState *bs, uint8_t *buf,
                       int64_t pos, int size);
 
+bool bdrv_backing_file_loop_check(const char *filename, const char *fmt,
+                             bool chain, const char *backing_file,
+                             const char *backing_format);
+
 void bdrv_img_create(const char *filename, const char *fmt,
                      const char *base_filename, const char *base_fmt,
                      char *options, uint64_t img_size, int flags,
diff --git a/qemu-img.c b/qemu-img.c
index 809b4f1..48284c5 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1620,11 +1620,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 +1637,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 (bdrv_backing_file_loop_check(filename, fmt, chain, NULL, 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 +1681,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

[Qemu-devel] [PATCH V2 2/5] Add WIN32 platform support for backing_file_loop_check()

[Xu Wang]

Signed-off-by: Xu Wang <cngesaint@gmail.com>
---
 block.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 94 insertions(+)

diff --git a/block.c b/block.c
index 53b1a01..8dc6ded 100644
--- a/block.c
+++ b/block.c
@@ -4431,6 +4431,83 @@ bdrv_acct_done(BlockDriverState *bs, BlockAcctCookie *cookie)
     bs->total_time_ns[cookie->type] += get_clock() - cookie->start_time_ns;
 }
 
+#ifdef _WIN32
+static int get_lnk_target_file(const char *lnk_file, char *filepath)
+{
+    unsigned int flag, offet;
+    unsigned int sflag;
+    char uch;
+    int i = 0;
+
+    FILE *fd = fopen(lnk_file, "rb");
+    if (!fd) {
+        error_report("Open file %s failed.", lnk_file);
+        return -1;
+    }
+    fread(&flag, 4, 1, fd);
+    if (flag != 0x4c) {
+        error_report("%s is not a lnk file.", lnk_file);
+        fclose(fd);
+        return -1;
+    }
+    fseek(fd, 0x14, SEEK_SET);
+    fread(&flag, 4, 1, fd);
+    fseek(fd, 0x4c, SEEK_SET);
+
+    if (flag & 0x01) {
+        fread(&sflag, 2, 1, fd);
+        fseek(fd, sflag, SEEK_CUR);
+    }
+
+    offset = ftell(fd);
+    fseek(fd, offset + 0x10, SEEK_SET);
+    fread(&flag, 4, 1, fd);
+    fseek(fd, flag + offset, SEEK_SET);
+
+    do {
+        fread(&uch, 1, 1, fd);
+        filepath[i++] = uch;
+    } while (uch != '\0');
+
+    fclose(fd);
+    return 0;
+}
+
+static long get_win_inode(const char *filename)
+{
+    char pbuf[MAX_PATH_LEN], *p;
+    long inode;
+    struct stat sbuf;
+    char path[MAX_PATH_LEN];
+
+    /* If filename contains .lnk, it's a shortcuts. Target file
+ *      * need to be parsed.
+ *           */
+    if (strstr(filename, ".lnk")) {
+        if (get_lnk_target_file(filename, path)) {
+            error_report("Parse .lnk file %s failed.", filename);
+            return -1;
+        }
+    } else {
+        memcpy(path, filename, sizeof(filename));
+    }
+
+    if (stat(path, &sbuf) == -1) {
+        error_report("get file %s stat error.", path);
+        return -1;
+    }
+    if (GetFullPathName(path, MAX_PATH_LEN, pbuf, &p) != 0) {
+        inode = 11003;
+        for (p = pbuf; *p != '\0'; p++) {
+            inode = inode * 31 + *(unsigned char *)p;
+        }
+        return (inode * 911) & 0x7FFF;
+    }
+
+    return -1;
+}
+#endif
+
 static gboolean str_equal_func(gconstpointer a, gconstpointer b)
 {
     return strcmp(a, b) == 0;
@@ -4475,7 +4552,15 @@ bool bdrv_backing_file_loop_check(const char *filename, const char *fmt,
                 error_report("Get file %s stat failed.", filename);
                 goto err;
             }
+            #ifdef _WIN32
+            inode = get_win_inode(filename);
+            if (inode == -1) {
+                error_report("Get file %s inode failed.", filename);
+                goto err;
+            }
+            #else
             inode = (long)sbuf.st_ino;
+            #endif
         }
 
         filename = backing_file;
@@ -4488,7 +4573,16 @@ bool bdrv_backing_file_loop_check(const char *filename, const char *fmt,
                 error_report("Get file %s stat failed.", filename);
                 goto err;
         }
+
+        #ifdef _WIN32
+        inode = get_win_inode(filename);
+        if (inode == -1) {
+            error_report("Get file %s inode failed.", filename);
+            goto err;
+        }
+        #else
         inode = (long)sbuf.st_ino;
+        #endif
 
         if (g_hash_table_lookup_extended(inodes, &inode, NULL, NULL)) {
             error_report("Backing file '%s' creates an infinite loop.",
-- 
1.8.1.4

[Qemu-devel] [PATCH V2 3/5] Check infinite loop in bdrv_img_create()

[Xu Wang]

Signed-off-by: Xu Wang <cngesaint@gmail.com>
---
 block.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/block.c b/block.c
index 8dc6ded..6df25d9 100644
--- a/block.c
+++ b/block.c
@@ -4688,15 +4688,15 @@ void bdrv_img_create(const char *filename, const char *fmt,
     }
 
     backing_file = get_option_parameter(param, BLOCK_OPT_BACKING_FILE);
+    backing_fmt = get_option_parameter(param, BLOCK_OPT_BACKING_FMT);
     if (backing_file && backing_file->value.s) {
-        if (!strcmp(filename, backing_file->value.s)) {
-            error_setg(errp, "Error: Trying to create an image with the "
-                             "same filename as the backing file");
+        if (bdrv_backing_file_loop_check(filename, fmt, true,
+                                         backing_file->value.s, 
+                                         backing_fmt->value.s)) {
+            /* There is loop exists in the backing file chain */
             goto out;
         }
     }
-
-    backing_fmt = get_option_parameter(param, BLOCK_OPT_BACKING_FMT);
     if (backing_fmt && backing_fmt->value.s) {
         backing_drv = bdrv_find_format(backing_fmt->value.s);
         if (!backing_drv) {
-- 
1.8.1.4

[Qemu-devel] [PATCH V2 4/5] Add backing file loop check in change_backing_file()

[Xu Wang]

Signed-off-by: Xu Wang <cngesaint@gmail.com>
---
 block.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/block.c b/block.c
index 6df25d9..379b79b 100644
--- a/block.c
+++ b/block.c
@@ -1971,6 +1971,12 @@ int bdrv_change_backing_file(BlockDriverState *bs,
         return -EINVAL;
     }
 
+    /* Check if loop exists in backing files chain after changed */
+    if (bdrv_backing_file_loop_check(bs->filename, NULL, true, backing_file,
+                                     backing_fmt)) {
+        return -EIO;
+    }
+
     if (drv->bdrv_change_backing_file != NULL) {
         ret = drv->bdrv_change_backing_file(bs, backing_file, backing_fmt);
     } else {
-- 
1.8.1.4

[Qemu-devel] [PATCH V2 5/5] Add infinite loop check in drive_init()

[Xu Wang]

Signed-off-by: Xu Wang <cngesaint@gmail.com>
---
 blockdev.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/blockdev.c b/blockdev.c
index 5975dde..0178764 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -695,6 +695,11 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
         error_report("warning: disabling copy_on_read on readonly drive");
     }
 
+    /* Add backing file loop check */
+    if (bdrv_backing_file_loop_check(file, NULL, true, NULL, NULL)) {
+        goto err;
+    }
+
     ret = bdrv_open(dinfo->bdrv, file, bs_opts, bdrv_flags, drv);
     bs_opts = NULL;
 
-- 
1.8.1.4

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

[Fam Zheng]

On Mon, 07/08 03:26, Xu Wang wrote:
> Signed-off-by: Xu Wang <cngesaint@gmail.com>
> ---
>  block.c               | 101 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/block/block.h |   4 ++
>  qemu-img.c            |  30 +++++----------
>  3 files changed, 114 insertions(+), 21 deletions(-)
> 
> diff --git a/block.c b/block.c
> index b88ad2f..53b1a01 100644
> --- a/block.c
> +++ b/block.c
> @@ -4431,6 +4431,107 @@ bdrv_acct_done(BlockDriverState *bs, BlockAcctCookie *cookie)
>      bs->total_time_ns[cookie->type] += get_clock() - cookie->start_time_ns;
>  }
>  
> +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, absolute or relative path is OK.
> + * @fmt: topmost image format (may be NULL to autodetect)
> + * @head: head of ImageInfo list. If not need please set head to null.
This parameter is removed, you can also remove the comment.
> + * @chain: true  - enumerate entire backing file chain
> + *         false - only topmost image file
Does it make sense when chain==false for loop check?
> + * @backing_file: if this value is set, @filename inode (-1 if it doesn't
> + *                exist) will insert into hash table directly. Loop check
> + *                starts from it. Absolute or relative path is OK for it.
> + * @backing_format: format of backing file
Why these two parameters? The caller can still pass bs->backing_file to
filename and bs->backing_fmt to fmt without them, to skip the topmost.
> + *
> + * If return true, stands for a backing file loop exists or some error
> + * happened. If return false, everything is ok.
Please format like this:
  + * Returns: true for backing file loop, false for no loop.
> + */
> +bool bdrv_backing_file_loop_check(const char *filename, const char *fmt,
> +                                  bool chain, const char *backing_file,
> +                                  const char *backing_format) {
> +    GHashTable *inodes;
> +    BlockDriverState *bs;
> +    BlockDriver *drv;
> +    struct stat sbuf;
> +    long inode = 0;
> +    int ret;
> +    char fbuf[1024];
> +
> +    inodes = g_hash_table_new_full(g_str_hash, str_equal_func, NULL, NULL);
> +
> +    if (backing_file) {
> +        /* Check if file exists. */
> +        if (access(filename, F_OK)) {
> +            inode = -1;
> +        } else {
> +            if (stat(filename, &sbuf) == -1) {
> +                error_report("Get file %s stat failed.", filename);
> +                goto err;
> +            }
> +            inode = (long)sbuf.st_ino;
> +        }
> +
> +        filename = backing_file;
> +        fmt = backing_format;
> +        g_hash_table_insert(inodes, (gpointer)&inode, NULL);
> +    }
> +
> +    while (filename && (filename[0] != '\0')) {
> +        if (stat(filename, &sbuf) == -1) {
Does it mean stat() on backing_file twice if it's not NULL? As you
assigned backing_file to filename above.
> +                error_report("Get file %s stat failed.", filename);
> +                goto err;
> +        }
> +        inode = (long)sbuf.st_ino;
> +
> +        if (g_hash_table_lookup_extended(inodes, &inode, NULL, NULL)) {
> +            error_report("Backing file '%s' creates an infinite loop.",
> +                         filename);
> +            goto err;
> +        }
> +        g_hash_table_insert(inodes, (gpointer)&inode, NULL);
> +
> +        bs = bdrv_new("image");
> +
> +        if (fmt) {
> +            drv = bdrv_find_format(fmt);
> +            if (!drv) {
> +                error_report("Unknown file format '%s'", fmt);
> +                goto err;
bs is leaked.
> +            }
> +        } else {
> +            drv = NULL;
> +        }
> +
> +        ret = bdrv_open(bs, filename, NULL,
> +                        BDRV_O_CACHE_WB | BDRV_O_NO_BACKING, drv);
> +        if (ret < 0) {
bs is leaked.
> +            error_report("Could not open '%s': %s", filename, strerror(-ret));
> +            goto err;
> +        }
These lines looks like a copy & paste from bdrv_new_open(). Could you
just call it?
> +
> +        filename = fmt = NULL;
> +        if (chain) {
> +            bdrv_get_backing_filename(bs, fbuf, sizeof(fbuf));
> +            filename = fbuf;
> +        }
> +
> +        bdrv_delete(bs);
> +    }
> +    g_hash_table_destroy(inodes);
> +    return false;
> +
> +err:
> +    g_hash_table_destroy(inodes);
> +    return true;
> +}
> +
>  void bdrv_img_create(const char *filename, const char *fmt,
>                       const char *base_filename, const char *base_fmt,
>                       char *options, uint64_t img_size, int flags,
> diff --git a/include/block/block.h b/include/block/block.h
> index 2307f67..6c631f2 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -332,6 +332,10 @@ int bdrv_save_vmstate(BlockDriverState *bs, const uint8_t *buf,
>  int bdrv_load_vmstate(BlockDriverState *bs, uint8_t *buf,
>                        int64_t pos, int size);
>  
> +bool bdrv_backing_file_loop_check(const char *filename, const char *fmt,
> +                             bool chain, const char *backing_file,
> +                             const char *backing_format);
Please align to parameter beginning:
                                     bool chain, const char *backing_file,
                                     const char *backing_format);
> +
>  void bdrv_img_create(const char *filename, const char *fmt,
>                       const char *base_filename, const char *base_fmt,
>                       char *options, uint64_t img_size, int flags,
> diff --git a/qemu-img.c b/qemu-img.c
> index 809b4f1..48284c5 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1620,11 +1620,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 +1637,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);
> +        
Please avoid trailing whitespaces. You could use scripts/checkpatch.pl
to check your patch for coding style problems.
> +    /* check if loop exists and build ImageInfoList */
> +    if (bdrv_backing_file_loop_check(filename, fmt, chain, NULL, 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 +1681,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
> 
> 

-- 
Fam

Re: [Qemu-devel] [PATCH V2 2/5] Add WIN32 platform support for backing_file_loop_check()

[Fam Zheng]

On Mon, 07/08 03:26, Xu Wang wrote:
> Signed-off-by: Xu Wang <cngesaint@gmail.com>
> ---
>  block.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 94 insertions(+)
> 
> diff --git a/block.c b/block.c
> index 53b1a01..8dc6ded 100644
> --- a/block.c
> +++ b/block.c
> @@ -4431,6 +4431,83 @@ bdrv_acct_done(BlockDriverState *bs, BlockAcctCookie *cookie)
>      bs->total_time_ns[cookie->type] += get_clock() - cookie->start_time_ns;
>  }
>  
> +#ifdef _WIN32
> +static int get_lnk_target_file(const char *lnk_file, char *filepath)
> +{
> +    unsigned int flag, offet;
> +    unsigned int sflag;
> +    char uch;
> +    int i = 0;
> +
> +    FILE *fd = fopen(lnk_file, "rb");
> +    if (!fd) {
> +        error_report("Open file %s failed.", lnk_file);
> +        return -1;
> +    }
> +    fread(&flag, 4, 1, fd);
> +    if (flag != 0x4c) {
> +        error_report("%s is not a lnk file.", lnk_file);
> +        fclose(fd);
> +        return -1;
> +    }
> +    fseek(fd, 0x14, SEEK_SET);
> +    fread(&flag, 4, 1, fd);
> +    fseek(fd, 0x4c, SEEK_SET);
> +
> +    if (flag & 0x01) {
> +        fread(&sflag, 2, 1, fd);
> +        fseek(fd, sflag, SEEK_CUR);
> +    }
> +
> +    offset = ftell(fd);
> +    fseek(fd, offset + 0x10, SEEK_SET);
> +    fread(&flag, 4, 1, fd);
> +    fseek(fd, flag + offset, SEEK_SET);
> +
> +    do {
> +        fread(&uch, 1, 1, fd);
> +        filepath[i++] = uch;
> +    } while (uch != '\0');
Please limit the number of bytes to read to capacity of  filepath, avoid
the security hole.
> +
> +    fclose(fd);
> +    return 0;
> +}
> +
> +static long get_win_inode(const char *filename)
> +{
> +    char pbuf[MAX_PATH_LEN], *p;
> +    long inode;
> +    struct stat sbuf;
> +    char path[MAX_PATH_LEN];
> +
> +    /* If filename contains .lnk, it's a shortcuts. Target file
> + *      * need to be parsed.
> + *           */
> +    if (strstr(filename, ".lnk")) {
Please be more accurate with this checking, what if the filename happens
to be "a.lnked.txt"?
> +        if (get_lnk_target_file(filename, path)) {
> +            error_report("Parse .lnk file %s failed.", filename);
> +            return -1;
> +        }
> +    } else {
> +        memcpy(path, filename, sizeof(filename));
> +    }
> +
> +    if (stat(path, &sbuf) == -1) {
What's this stat() call for?
> +        error_report("get file %s stat error.", path);
> +        return -1;
> +    }
> +    if (GetFullPathName(path, MAX_PATH_LEN, pbuf, &p) != 0) {
How big is MAX_PATH_LEN?
MSDN: If the buffer is too small to contain the path, the return value
is the size, in TCHARs, of the buffer that is required to hold the path
and the terminating null character. Please try to handle this case. (And
is pbuf NULL terminated in this case?)
> +        inode = 11003;
> +        for (p = pbuf; *p != '\0'; p++) {
> +            inode = inode * 31 + *(unsigned char *)p;
> +        }
> +        return (inode * 911) & 0x7FFF;
> +    }
> +
> +    return -1;
> +}
> +#endif
> +
>  static gboolean str_equal_func(gconstpointer a, gconstpointer b)
>  {
>      return strcmp(a, b) == 0;
> @@ -4475,7 +4552,15 @@ bool bdrv_backing_file_loop_check(const char *filename, const char *fmt,
>                  error_report("Get file %s stat failed.", filename);
>                  goto err;
>              }
> +            #ifdef _WIN32
> +            inode = get_win_inode(filename);
> +            if (inode == -1) {
> +                error_report("Get file %s inode failed.", filename);
> +                goto err;
> +            }
> +            #else
Move stat() call here? (seems not necessary for windows)
>              inode = (long)sbuf.st_ino;
> +            #endif
>          }
>  
>          filename = backing_file;
> @@ -4488,7 +4573,16 @@ bool bdrv_backing_file_loop_check(const char *filename, const char *fmt,
>                  error_report("Get file %s stat failed.", filename);
>                  goto err;
>          }
> +
> +        #ifdef _WIN32
> +        inode = get_win_inode(filename);
> +        if (inode == -1) {
> +            error_report("Get file %s inode failed.", filename);
> +            goto err;
> +        }
> +        #else
>          inode = (long)sbuf.st_ino;
See above.
> +        #endif
>  
>          if (g_hash_table_lookup_extended(inodes, &inode, NULL, NULL)) {
>              error_report("Backing file '%s' creates an infinite loop.",
> -- 
> 1.8.1.4
> 
> 

-- 
Fam

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

[Fam Zheng]

On Wed, 07/10 18:25, Fam Zheng wrote:
> > +
> > +    if (backing_file) {
> > +        /* Check if file exists. */
> > +        if (access(filename, F_OK)) {
> > +            inode = -1;
> > +        } else {
> > +            if (stat(filename, &sbuf) == -1) {
> > +                error_report("Get file %s stat failed.", filename);
> > +                goto err;
> > +            }
> > +            inode = (long)sbuf.st_ino;
> > +        }
> > +
> > +        filename = backing_file;
> > +        fmt = backing_format;
> > +        g_hash_table_insert(inodes, (gpointer)&inode, NULL);
> > +    }
> > +
> > +    while (filename && (filename[0] != '\0')) {
> > +        if (stat(filename, &sbuf) == -1) {
> Does it mean stat() on backing_file twice if it's not NULL? As you
> assigned backing_file to filename above.
I was wrong, the first stat() is on filename. Please ignore this noise.

Re: [Qemu-devel] [PATCH V2 3/5] Check infinite loop in bdrv_img_create()

[Fam Zheng]

On Mon, 07/08 03:26, Xu Wang wrote:
> Signed-off-by: Xu Wang <cngesaint@gmail.com>
> ---
>  block.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 8dc6ded..6df25d9 100644
> --- a/block.c
> +++ b/block.c
> @@ -4688,15 +4688,15 @@ void bdrv_img_create(const char *filename, const char *fmt,
>      }
>  
>      backing_file = get_option_parameter(param, BLOCK_OPT_BACKING_FILE);
> +    backing_fmt = get_option_parameter(param, BLOCK_OPT_BACKING_FMT);
>      if (backing_file && backing_file->value.s) {
> -        if (!strcmp(filename, backing_file->value.s)) {
> -            error_setg(errp, "Error: Trying to create an image with the "
> -                             "same filename as the backing file");
> +        if (bdrv_backing_file_loop_check(filename, fmt, true,
> +                                         backing_file->value.s, 
Trailing whitespace.
> +                                         backing_fmt->value.s)) {
> +            /* There is loop exists in the backing file chain */
I see why you pass both filename and backing_file to in now, filename is
not created yet so bdrv_get_backing_filename is not usable, but it needs
to be checked.

-- 
Fam

Re: [Qemu-devel] [PATCH V2 4/5] Add backing file loop check in change_backing_file()

[Fam Zheng]

On Mon, 07/08 03:26, Xu Wang wrote:
> Signed-off-by: Xu Wang <cngesaint@gmail.com>
> ---
>  block.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/block.c b/block.c
> index 6df25d9..379b79b 100644
> --- a/block.c
> +++ b/block.c
> @@ -1971,6 +1971,12 @@ int bdrv_change_backing_file(BlockDriverState *bs,
>          return -EINVAL;
>      }
>  
> +    /* Check if loop exists in backing files chain after changed */
> +    if (bdrv_backing_file_loop_check(bs->filename, NULL, true, backing_file,
You can use bs->drv->format_name for fmt.
> +                                     backing_fmt)) {
> +        return -EIO;
> +    }
> +
>      if (drv->bdrv_change_backing_file != NULL) {
>          ret = drv->bdrv_change_backing_file(bs, backing_file, backing_fmt);
>      } else {
> -- 
> 1.8.1.4
> 
> 

-- 
Fam

Re: [Qemu-devel] [PATCH V2 5/5] Add infinite loop check in drive_init()

[Fam Zheng]

On Mon, 07/08 03:26, Xu Wang wrote:
> Signed-off-by: Xu Wang <cngesaint@gmail.com>
> ---
>  blockdev.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 5975dde..0178764 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -695,6 +695,11 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
>          error_report("warning: disabling copy_on_read on readonly drive");
>      }
>  
> +    /* Add backing file loop check */
> +    if (bdrv_backing_file_loop_check(file, NULL, true, NULL, NULL)) {
You can pass drv->format_name as second parameter.
> +        goto err;
> +    }
> +
>      ret = bdrv_open(dinfo->bdrv, file, bs_opts, bdrv_flags, drv);
>      bs_opts = NULL;
>  
> -- 
> 1.8.1.4
> 
> 

-- 
Fam

Re: [Qemu-devel] [PATCH V2 0/5] Add infinite loop check for backing file chain

[Fam Zheng]

On Mon, 07/08 03:26, Xu Wang wrote:
> Updates:
>   1. Changed infinite loop check in collect_image_info_list() from filename
>      checking to inode checking.
>   2. Absolute or relative path is OK for filename path.
>   3. Hard and soft link are works well.
>   4. Added WIN32 platform support (shortcuts could be recognized correctly.)
>   5. Create a file which contains loop in backing file will failed.
>   6. Start a vm which boot block file contains loop in backing file chain
>      will failed instead of no response and segment fault.
> 
> Xu Wang (5):
>   Refine and export infinite loop checking in collect_image_info_list()
>   Add WIN32 platform support for backing_file_loop_check()
>   Check infinite loop in bdrv_img_create()
>   Add backing file loop check in change_backing_file()
>   Add infinite loop check in drive_init()
> 
>  block.c               | 211 ++++++++++++++++++++++++++++++++++++++++++++++++--
>  blockdev.c            |   5 ++
>  include/block/block.h |   4 +
>  qemu-img.c            |  30 +++----
>  4 files changed, 224 insertions(+), 26 deletions(-)
> 

Thanks for the series. I made a few comments on each patch. You hashed
windows full path to get the "inode", but case sensitively, would that
work with the same files of different case?

Please prefix your commit message with "block:" or "qemu-img:",
e.g.:

    block: refactor collect_image_info_list()
    block: Add WIN32 platform support for backing_file_loop_check()
    etc..

and maybe also describe some of the long patches, explaining what is
changed.

Also please consider to write some test case on your changes, as requested
in the replies to previous version.

-- 
Fam

Re: [Qemu-devel] [PATCH V2 0/5] Add infinite loop check for backing file chain

[Kevin Wolf]

Am 10.07.2013 um 13:13 hat Fam Zheng geschrieben:
> On Mon, 07/08 03:26, Xu Wang wrote:
> > Updates:
> >   1. Changed infinite loop check in collect_image_info_list() from filename
> >      checking to inode checking.
> >   2. Absolute or relative path is OK for filename path.
> >   3. Hard and soft link are works well.
> >   4. Added WIN32 platform support (shortcuts could be recognized correctly.)
> >   5. Create a file which contains loop in backing file will failed.
> >   6. Start a vm which boot block file contains loop in backing file chain
> >      will failed instead of no response and segment fault.
> > 
> > Xu Wang (5):
> >   Refine and export infinite loop checking in collect_image_info_list()
> >   Add WIN32 platform support for backing_file_loop_check()
> >   Check infinite loop in bdrv_img_create()
> >   Add backing file loop check in change_backing_file()
> >   Add infinite loop check in drive_init()
> > 
> >  block.c               | 211 ++++++++++++++++++++++++++++++++++++++++++++++++--
> >  blockdev.c            |   5 ++
> >  include/block/block.h |   4 +
> >  qemu-img.c            |  30 +++----
> >  4 files changed, 224 insertions(+), 26 deletions(-)
> > 
> 
> Thanks for the series. I made a few comments on each patch. You hashed
> windows full path to get the "inode", but case sensitively, would that
> work with the same files of different case?

I seem to remember that Windows can be made case-sensitive. In this
case, we must not ignore case. Not detecting some loops is okay (but here
we would in fact detect it in the second iteration), rejecting valid
configurations is not.

Kevin

Re: [Qemu-devel] [PATCH V2 0/5] Add infinite loop check for backing file chain

[Eric Blake]

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

On 07/10/2013 05:42 AM, Kevin Wolf wrote:
>> Thanks for the series. I made a few comments on each patch. You hashed
>> windows full path to get the "inode", but case sensitively, would that
>> work with the same files of different case?
> 
> I seem to remember that Windows can be made case-sensitive. In this
> case, we must not ignore case. Not detecting some loops is okay (but here
> we would in fact detect it in the second iteration), rejecting valid
> configurations is not.

Indeed, Windows can be made case-sensitive.  But you still want to hash
case-insensitive, and just be careful to not report collisions until
after checking inode equality.  For that matter, stat() on windows is
notoriously lame (such as returning 0 for st_ino on FAT, regardless of
file).  While I've compiled on windows, it has generally been on cygwin
(where they go to great lengths to give a sane stat() in spite of FAT's
shortcomings); you may want to get a review from someone more intimately
acquainted with mingw (Paolo?), in case there is no way to make inode
equality work, and where you may be better off with absolute path name
comparison.

-- 
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 V2 1/5] Refine and export infinite loop checking in collect_image_info_list()

[Xu Wang]

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

2013/7/10 Fam Zheng <famz@redhat.com>

> On Mon, 07/08 03:26, Xu Wang wrote:
> > Signed-off-by: Xu Wang <cngesaint@gmail.com>
> > ---
> >  block.c               | 101
> ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  include/block/block.h |   4 ++
> >  qemu-img.c            |  30 +++++----------
> >  3 files changed, 114 insertions(+), 21 deletions(-)
> >
> > diff --git a/block.c b/block.c
> > index b88ad2f..53b1a01 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -4431,6 +4431,107 @@ bdrv_acct_done(BlockDriverState *bs,
> BlockAcctCookie *cookie)
> >      bs->total_time_ns[cookie->type] += get_clock() -
> cookie->start_time_ns;
> >  }
> >
> > +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, absolute or relative path is OK.
> > + * @fmt: topmost image format (may be NULL to autodetect)
> > + * @head: head of ImageInfo list. If not need please set head to null.
> This parameter is removed, you can also remove the comment.
>
Sorry. I'll update in new version.

> > + * @chain: true  - enumerate entire backing file chain
> > + *         false - only topmost image file
> Does it make sense when chain==false for loop check?
>
If chain==false, it means that backing file chain check will be ignored. It
just
be used to be compatible with original function variables. However, the
caller
calls bdrv_backing_file_loop_check() means that 'hey, you should check
backing
files and tell me if there is a loop in it'. So I will remove it and do
@chain logic
before call bdrv_backing_file_loop_check() in collect_image_info_list().

> > + * @backing_file: if this value is set, @filename inode (-1 if it
> doesn't
> > + *                exist) will insert into hash table directly. Loop
> check
> > + *                starts from it. Absolute or relative path is OK for
> it.
> > + * @backing_format: format of backing file
> Why these two parameters? The caller can still pass bs->backing_file to
> filename and bs->backing_fmt to fmt without them, to skip the topmost.
>
If @filename just a filename to be created, it will couldn't get the backing
file name by calling bdrv_get_backing_file(). So caller could pass them by
these
parameters to tell function 'The filename is pointed but it may just a
filename, you
couldn't get backing file from it. But I'll give it by parameter'.

> > + *
> > + * If return true, stands for a backing file loop exists or some error
> > + * happened. If return false, everything is ok.
> Please format like this:
>   + * Returns: true for backing file loop, false for no loop.
>
OK, I'll update it in new version These are my first patches list for qemu.
I will
do my best to learn rules about it as soon as possible. Thank you very much
for the advice from all of yours:-)

> > + */
> > +bool bdrv_backing_file_loop_check(const char *filename, const char *fmt,
> > +                                  bool chain, const char *backing_file,
> > +                                  const char *backing_format) {
> > +    GHashTable *inodes;
> > +    BlockDriverState *bs;
> > +    BlockDriver *drv;
> > +    struct stat sbuf;
> > +    long inode = 0;
> > +    int ret;
> > +    char fbuf[1024];
> > +
> > +    inodes = g_hash_table_new_full(g_str_hash, str_equal_func, NULL,
> NULL);
> > +
> > +    if (backing_file) {
> > +        /* Check if file exists. */
> > +        if (access(filename, F_OK)) {
> > +            inode = -1;
> > +        } else {
> > +            if (stat(filename, &sbuf) == -1) {
> > +                error_report("Get file %s stat failed.", filename);
> > +                goto err;
> > +            }
> > +            inode = (long)sbuf.st_ino;
> > +        }
> > +
> > +        filename = backing_file;
> > +        fmt = backing_format;
> > +        g_hash_table_insert(inodes, (gpointer)&inode, NULL);
> > +    }
> > +
> > +    while (filename && (filename[0] != '\0')) {
> > +        if (stat(filename, &sbuf) == -1) {
> Does it mean stat() on backing_file twice if it's not NULL? As you
> assigned backing_file to filename above.
> > +                error_report("Get file %s stat failed.", filename);
> > +                goto err;
> > +        }
> > +        inode = (long)sbuf.st_ino;
> > +
> > +        if (g_hash_table_lookup_extended(inodes, &inode, NULL, NULL)) {
> > +            error_report("Backing file '%s' creates an infinite loop.",
> > +                         filename);
> > +            goto err;
> > +        }
> > +        g_hash_table_insert(inodes, (gpointer)&inode, NULL);
> > +
> > +        bs = bdrv_new("image");
> > +
> > +        if (fmt) {
> > +            drv = bdrv_find_format(fmt);
> > +            if (!drv) {
> > +                error_report("Unknown file format '%s'", fmt);
> > +                goto err;
> bs is leaked.
> > +            }
> > +        } else {
> > +            drv = NULL;
> > +        }
> > +
> > +        ret = bdrv_open(bs, filename, NULL,
> > +                        BDRV_O_CACHE_WB | BDRV_O_NO_BACKING, drv);
> > +        if (ret < 0) {
> bs is leaked.
>
bdrv_delete will be added in err.

> > +            error_report("Could not open '%s': %s", filename,
> strerror(-ret));
> > +            goto err;
> > +        }
> These lines looks like a copy & paste from bdrv_new_open(). Could you
> just call it?
>
bdrv_new_open() is implemented as a static function in qemu-img.c and
qemu-img.c
includes block.c (). If bdrv_backing_file_loop_check() calls
bdrv_new_open(),
It's really a bit strange. If it's needed to make bdrv_new_open() moved to
block.c,
I'll do work for it.

> > +
> > +        filename = fmt = NULL;
> > +        if (chain) {
> > +            bdrv_get_backing_filename(bs, fbuf, sizeof(fbuf));
> > +            filename = fbuf;
> > +        }
> > +
> > +        bdrv_delete(bs);
> > +    }
> > +    g_hash_table_destroy(inodes);
> > +    return false;
> > +
> > +err:
> > +    g_hash_table_destroy(inodes);
> > +    return true;
> > +}
> > +
> >  void bdrv_img_create(const char *filename, const char *fmt,
> >                       const char *base_filename, const char *base_fmt,
> >                       char *options, uint64_t img_size, int flags,
> > diff --git a/include/block/block.h b/include/block/block.h
> > index 2307f67..6c631f2 100644
> > --- a/include/block/block.h
> > +++ b/include/block/block.h
> > @@ -332,6 +332,10 @@ int bdrv_save_vmstate(BlockDriverState *bs, const
> uint8_t *buf,
> >  int bdrv_load_vmstate(BlockDriverState *bs, uint8_t *buf,
> >                        int64_t pos, int size);
> >
> > +bool bdrv_backing_file_loop_check(const char *filename, const char *fmt,
> > +                             bool chain, const char *backing_file,
> > +                             const char *backing_format);
> Please align to parameter beginning:
>                                      bool chain, const char *backing_file,
>                                      const char *backing_format);
>
Sorry for that.

> > +
> >  void bdrv_img_create(const char *filename, const char *fmt,
> >                       const char *base_filename, const char *base_fmt,
> >                       char *options, uint64_t img_size, int flags,
> > diff --git a/qemu-img.c b/qemu-img.c
> > index 809b4f1..48284c5 100644
> > --- a/qemu-img.c
> > +++ b/qemu-img.c
> > @@ -1620,11 +1620,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 +1637,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);
> > +
> Please avoid trailing whitespaces. You could use scripts/checkpatch.pl
> to check your patch for coding style problems.
>
OK, I'll do that.

Xu Wang

> > +    /* check if loop exists and build ImageInfoList */
> > +    if (bdrv_backing_file_loop_check(filename, fmt, chain, NULL, 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 +1681,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
> >
> >
>
> --
> Fam
>

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

Re: [Qemu-devel] [PATCH V2 2/5] Add WIN32 platform support for backing_file_loop_check()

[Xu Wang]

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

2013/7/10 Fam Zheng <famz@redhat.com>

> On Mon, 07/08 03:26, Xu Wang wrote:
> > Signed-off-by: Xu Wang <cngesaint@gmail.com>
> > ---
> >  block.c | 94
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 94 insertions(+)
> >
> > diff --git a/block.c b/block.c
> > index 53b1a01..8dc6ded 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -4431,6 +4431,83 @@ bdrv_acct_done(BlockDriverState *bs,
> BlockAcctCookie *cookie)
> >      bs->total_time_ns[cookie->type] += get_clock() -
> cookie->start_time_ns;
> >  }
> >
> > +#ifdef _WIN32
> > +static int get_lnk_target_file(const char *lnk_file, char *filepath)
> > +{
> > +    unsigned int flag, offet;
> > +    unsigned int sflag;
> > +    char uch;
> > +    int i = 0;
> > +
> > +    FILE *fd = fopen(lnk_file, "rb");
> > +    if (!fd) {
> > +        error_report("Open file %s failed.", lnk_file);
> > +        return -1;
> > +    }
> > +    fread(&flag, 4, 1, fd);
> > +    if (flag != 0x4c) {
> > +        error_report("%s is not a lnk file.", lnk_file);
> > +        fclose(fd);
> > +        return -1;
> > +    }
> > +    fseek(fd, 0x14, SEEK_SET);
> > +    fread(&flag, 4, 1, fd);
> > +    fseek(fd, 0x4c, SEEK_SET);
> > +
> > +    if (flag & 0x01) {
> > +        fread(&sflag, 2, 1, fd);
> > +        fseek(fd, sflag, SEEK_CUR);
> > +    }
> > +
> > +    offset = ftell(fd);
> > +    fseek(fd, offset + 0x10, SEEK_SET);
> > +    fread(&flag, 4, 1, fd);
> > +    fseek(fd, flag + offset, SEEK_SET);
> > +
> > +    do {
> > +        fread(&uch, 1, 1, fd);
> > +        filepath[i++] = uch;
> > +    } while (uch != '\0');
> Please limit the number of bytes to read to capacity of  filepath, avoid
> the security hole.
>
@i was limited in [0, MAX_PATH_LEN] in the new version.

> > +
> > +    fclose(fd);
> > +    return 0;
> > +}
> > +
> > +static long get_win_inode(const char *filename)
> > +{
> > +    char pbuf[MAX_PATH_LEN], *p;
> > +    long inode;
> > +    struct stat sbuf;
> > +    char path[MAX_PATH_LEN];
> > +
> > +    /* If filename contains .lnk, it's a shortcuts. Target file
> > + *      * need to be parsed.
> > + *           */
> > +    if (strstr(filename, ".lnk")) {
> Please be more accurate with this checking, what if the filename happens
> to be "a.lnked.txt"?
>
Generally speaking filename has no more than one extenstion name on Windows
(I just found some virus could break that rule). Maybe I have not enough
experience
on WIN32 plateform, so I updated code will check whether the filename is
end with
'.lnk'.

> > +        if (get_lnk_target_file(filename, path)) {
> > +            error_report("Parse .lnk file %s failed.", filename);
> > +            return -1;
> > +        }
> > +    } else {
> > +        memcpy(path, filename, sizeof(filename));
> > +    }
> > +
> > +    if (stat(path, &sbuf) == -1) {
> What's this stat() call for?
>
I read a lot code about get inode on the WIN32 plateform. All of them have
stat calling.
The function of stat() here is checking file by calling stat() if there was
something wrong
with this file, the following caculation is useless and return -1 directly.

> > +        error_report("get file %s stat error.", path);
> > +        return -1;
> > +    }
> > +    if (GetFullPathName(path, MAX_PATH_LEN, pbuf, &p) != 0) {
> How big is MAX_PATH_LEN?
> MSDN: If the buffer is too small to contain the path, the return value
> is the size, in TCHARs, of the buffer that is required to hold the path
> and the terminating null character. Please try to handle this case. (And
> is pbuf NULL terminated in this case?)
>
This is really a hard desicion to set value of it because length of path on
Windows
could be unlimited. So here I just set an value and want to get some tips
from others.
Now it's set as 8192 but I missed it when I made this patch.

> > +        inode = 11003;
> > +        for (p = pbuf; *p != '\0'; p++) {
> > +            inode = inode * 31 + *(unsigned char *)p;
> > +        }
> > +        return (inode * 911) & 0x7FFF;
> > +    }
> > +
> > +    return -1;
> > +}
> > +#endif
> > +
> >  static gboolean str_equal_func(gconstpointer a, gconstpointer b)
> >  {
> >      return strcmp(a, b) == 0;
> > @@ -4475,7 +4552,15 @@ bool bdrv_backing_file_loop_check(const char
> *filename, const char *fmt,
> >                  error_report("Get file %s stat failed.", filename);
> >                  goto err;
> >              }
> > +            #ifdef _WIN32
> > +            inode = get_win_inode(filename);
> > +            if (inode == -1) {
> > +                error_report("Get file %s inode failed.", filename);
> > +                goto err;
> > +            }
> > +            #else
> Move stat() call here? (seems not necessary for windows)
>
I decided to move all stat() into get_inode (I changed its name from
get_win_inode()). Logic is more simpler now.

Xu Wang

> >              inode = (long)sbuf.st_ino;
> > +            #endif
> >          }
> >
> >          filename = backing_file;
> > @@ -4488,7 +4573,16 @@ bool bdrv_backing_file_loop_check(const char
> *filename, const char *fmt,
> >                  error_report("Get file %s stat failed.", filename);
> >                  goto err;
> >          }
> > +
> > +        #ifdef _WIN32
> > +        inode = get_win_inode(filename);
> > +        if (inode == -1) {
> > +            error_report("Get file %s inode failed.", filename);
> > +            goto err;
> > +        }
> > +        #else
> >          inode = (long)sbuf.st_ino;
> See above.
> > +        #endif
> >
> >          if (g_hash_table_lookup_extended(inodes, &inode, NULL, NULL)) {
> >              error_report("Backing file '%s' creates an infinite loop.",
> > --
> > 1.8.1.4
> >
> >
>
> --
> Fam
>

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

Re: [Qemu-devel] [PATCH V2 2/5] Add WIN32 platform support for backing_file_loop_check()

[Fam Zheng]

On Mon, 07/15 14:09, Xu Wang wrote:
> 2013/7/10 Fam Zheng <famz@redhat.com>
> 
> > On Mon, 07/08 03:26, Xu Wang wrote:
> > > +        error_report("get file %s stat error.", path);
> > > +        return -1;
> > > +    }
> > > +    if (GetFullPathName(path, MAX_PATH_LEN, pbuf, &p) != 0) {
> > How big is MAX_PATH_LEN?
> > MSDN: If the buffer is too small to contain the path, the return value
> > is the size, in TCHARs, of the buffer that is required to hold the path
> > and the terminating null character. Please try to handle this case. (And
> > is pbuf NULL terminated in this case?)
> >
> This is really a hard desicion to set value of it because length of path on
> Windows
> could be unlimited. So here I just set an value and want to get some tips
> from others.
> Now it's set as 8192 but I missed it when I made this patch.
> 

Yes, if you use heap allocated buffer, you can work with longer
filename. If you use fixed length buffer, ideally you should detect
whether return value is greater than your buffer size: in this case the
pbuf content is invalid (or incomplete), so your hash computation below
is inaccurate, consequentially false alarm.

-- 
Fam