Re: [Qemu-devel] [PATCH v5 09/12] VMDK: open/read/write for monolithicFlat image

[Fam Zheng <famcool@gmail.com>]

On Wed, Jun 29, 2011 at 11:57 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Tue, Jun 28, 2011 at 2:32 AM, Fam Zheng <famcool@gmail.com> wrote:
>> +/* find an option value out of descriptor file */
>> +static int vmdk_parse_description(const char *desc, const char *opt_name,
>> +        char *buf, int buf_size)
>> +{
>> +    char *opt_pos, *opt_end;
>> +    const char *end = desc + strlen(desc);
>> +
>> +    opt_pos = strstr(desc, opt_name);
>> +    if (!opt_pos) {
>> +        return -1;
>> +    }
>> +    /* Skip "=\"" following opt_name */
>> +    opt_pos += strlen(opt_name) + 2;
>> +    if (opt_pos >= end) {
>> +        return -1;
>> +    }
>
> This can produce false positives because strstr(desc, opt_name) will
> match anything that contains the opt_name string.  Also we don't
> verify that "=\"" follow the opt_name.  Luckily we only use this for
> "createType" which will hopefully never cause false positives.
>
>> +    opt_end = opt_pos;
>> +    while (opt_end < end && *opt_end != '"') {
>> +        opt_end++;
>> +    }
>> +    if (opt_end == end || buf_size < opt_end - opt_pos + 1) {
>> +        return -1;
>> +    }
>> +    pstrcpy(buf, opt_end - opt_pos + 1, opt_pos);
>> +    return 0;
>> +}
>> +
>> +static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
>> +        const char *desc_file_path)
>> +{
>> +    int ret = 0;
>> +    int r;
>> +    char access[11];
>> +    char type[11];
>> +    char fname[512];
>> +    const char *p = desc;
>> +    int64_t sectors = 0;
>> +    int64_t flat_offset;
>> +
>> +    while (*p) {
>> +        if (strncmp(p, "RW", strlen("RW"))) {
>> +            goto next_line;
>> +        }
>
> This check is duplicated below and can be removed.
>
>> +        /* parse extent line:
>> +         * RW [size in sectors] FLAT "file-name.vmdk" OFFSET
>> +         * or
>> +         * RW [size in sectors] SPARSE "file-name.vmdk"
>> +         */
>> +        flat_offset = -1;
>> +        sscanf(p, "%10s %lld %10s %512s",
>> +                access, &sectors, type, fname);
>
> Please check the sscanf(3) return value to ensure the format string
> matched.  The value of access[], type[], fname[], sectors will be
> unchanged at the point where sscanf(3) fails so your checks will not
> work.
>
> Declared as char fname[512] but using "%512s" format string.  The
> format string needs to be 511 to leave space for the NUL terminator.
>
>> +        if (!strcmp(type, "FLAT")) {
>> +            sscanf(p, "%10s %lld %10s %512s %lld",
>> +                access, &sectors, type, fname, &flat_offset);
>> +            if (flat_offset == -1) {
>> +                return -EINVAL;
>> +            }
>> +        }
>> +
>> +        /* trim the quotation marks around */
>> +        if (fname[0] == '"') {
>> +            memmove(fname, fname + 1, strlen(fname) + 1);
>
> This copies 1 byte too many, just strlen(fname) will do.
I meant to copy the NULL terminator too.
>
>> +            if (fname[strlen(fname) - 1] == '"') {
>> +                fname[strlen(fname) - 1] = '\0';
>> +            }
>> +        }
>
> If starts as fname[] = {'"', '\0'} then this if statement checks
> fname[-1] == '"'!
>
>> +        if (!(strlen(access) && sectors && strlen(type) && strlen(fname))) {
>> +            goto next_line;
>> +        }
>
> These can be replaced by checking the sscanf(3) return value above.
> Validating sectors is still a good idea though.
>
>> +        if (strcmp(type, "FLAT") && strcmp(type, "SPARSE")) {
>> +            goto next_line;
>> +        }
>> +        if (strcmp(access, "RW")) {
>> +            goto next_line;
>> +        }
>> +        ret++;
>
> This variable is unused.
>
>> +        /* save to extents array */
>> +        if (!strcmp(type, "FLAT")) {
>> +            /* FLAT extent */
>> +            char extent_path[PATH_MAX];
>> +            BlockDriverState *extent_file;
>> +            BlockDriver *drv;
>> +            VmdkExtent *extent;
>> +
>> +            extent_file = bdrv_new("");
>> +            drv = bdrv_find_format("file");
>> +            if (!drv) {
>
> bdrv_delete(extent_file);
>
>> +                return -EINVAL;
>> +            }
>> +            path_combine(extent_path, sizeof(extent_path),
>> +                    desc_file_path, fname);
>> +            r = bdrv_open(extent_file, extent_path,
>> +                    BDRV_O_RDWR | BDRV_O_NO_BACKING, drv);
>
> We should honor the bs->open_flags.  Otherwise
> cache=none|writeback|writethrough|unsafe is ignored on the actual
> extent files.
>
>> +            if (r) {
>
> bdrv_delete(extent_file);
>
>> +                return -EINVAL;
>> +            }
>> +            extent = vmdk_add_extent(bs, extent_file, true, sectors,
>> +                            0, 0, 0, 0, sectors);
>> +            extent->flat_start_offset = flat_offset;
>> +        } else {
>> +            /* SPARSE extent, not supported for now */
>> +            fprintf(stderr,
>> +                "VMDK: Not supported extent type \"%s\""".\n", type);
>> +            return -ENOTSUP;
>> +        }
>> +next_line:
>> +        /* move to next line */
>> +        while (*p && *p != '\n') {
>> +            p++;
>> +        }
>> +        p++;
>> +    }
>> +    return 0;
>> +}
>> +
>> +static int vmdk_open_desc_file(BlockDriverState *bs, int flags)
>> +{
>> +    int ret;
>> +    char buf[2048];
>> +    char ct[128];
>> +    BDRVVmdkState *s = bs->opaque;
>> +
>> +    ret = bdrv_pread(bs->file, 0, buf, sizeof(buf));
>> +    if (ret < 0) {
>> +        return ret;
>> +    }
>> +    buf[2047] = '\0';
>> +    if (vmdk_parse_description(buf, "createType", ct, sizeof(ct))) {
>> +        return -EINVAL;
>> +    }
>> +    if (strcmp(ct, "monolithicFlat")) {
>> +        fprintf(stderr,
>> +                "VMDK: Not supported image type \"%s\""".\n", ct);
>> +        return -ENOTSUP;
>> +    }
>> +    s->desc_offset = 0;
>> +    ret = vmdk_parse_extents(buf, bs, bs->file->filename);
>> +    if (ret) {
>> +        return ret;
>> +    }
>> +
>> +    /* try to open parent images, if exist */
>> +    if (vmdk_parent_open(bs)) {
>> +        qemu_free(s->extents);
>
> This duplicates extent freeing code from vmdk_close().  Please reuse
> that (maybe by moving it into a vmdk_free_extents() function), it also
> frees l1_table, l2_cache, and l1_backup_table.
>
> Stefan
>



-- 
Best regards!
Fam Zheng