On Wed, Jun 29, 2011 at 11:57 PM, Stefan Hajnoczi <stefa...@gmail.com> wrote:
> On Tue, Jun 28, 2011 at 2:32 AM, Fam Zheng <famc...@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, §ors, 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, §ors, 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
