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

[Xu Wang]

2013/7/10 Fam Zheng <address@hidden>

On Mon, 07/08 03:26, Xu Wang wrote:
> Signed-off-by: Xu Wang <address@hidden>
> ---
>  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