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

[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.
> +
> +    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