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

[Stefan Hajnoczi]

On Mon, Jun 27, 2011 at 8:00 AM, Fam Zheng <address@hidden> wrote:
> On Mon, Jun 27, 2011 at 12:54 PM, Stefan Hajnoczi <address@hidden> wrote:
>> On Mon, Jun 27, 2011 at 4:48 AM, Fam Zheng <address@hidden> wrote:
>>> Parse vmdk decriptor file and open mono flat image.
>>> @@ -598,6 +600,154 @@ static int vmdk_open_vmdk4(BlockDriverState *bs, int 
>>> flags)
>>>     return ret;
>>>  }
>>>
>>> +/* 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 = strstr(desc, opt_name);
>>> +    int r;
>>> +    const char *end = desc + strlen(desc);
>>> +
>>> +    if (!opt_pos) {
>>> +        return -1;
>>> +    }
>>> +    opt_pos += strlen(opt_name) + 2;
>>> +    if (opt_pos >= end) {
>>> +        return -1;
>>> +    }
>>> +    r = sscanf(opt_pos, "%[^\"]s", buf);
>>> +    return r <= 0;
>>> +}
>>
>> This is still unsafe.  Please see my comments on the previous version
>> of this patch.
> How about this:
>
> 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);

Already game over here because desc is not NUL-terminated.  Either
make desc NUL-terminated or add a desc_size argument.

>
>    opt_pos = strstr(desc, opt_name);

And again here.

>    if (!opt_pos) {
>        return -1;
>    }
>    /* Skip "=\"" following opt_name */
>    opt_pos += strlen(opt_name) + 2;
>    if (opt_pos >= end) {
>        return -1;
>    }
>    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;
>    }
>    strncpy(buf, opt_pos, opt_end - opt_pos);
>    buf[opt_end - opt_pos] = '\0';

cutils.c:pstrcpy() is easier to use than strncpy(), no need for the
explicit NUL-termination.

Stefan