Re: [Qemu-devel] [PATCH v2 03/12] VMDK: probe for monolithicFlat images

[Stefan Hajnoczi]

On Fri, Jun 24, 2011 at 9:18 AM,  <address@hidden> wrote:
> +        const char *p = (const char *)buf;
> +        const char *end = p + buf_size;
> +        int version = 0;
> +        while (*p && p < end) {

These short-circuit comparisions need to be the other way around.  If
p >= end then p is out-of-range and we cannot dereference it.
Therefore:
while (p < end && *p) {

Although what is the point of checking for the NUL byte?  This buffer
isn't a C string and NULs don't matter.

Please fix the other loops too.

> +            if (*p == '#') {
> +                /* skip comment line */
> +                while (*p != '\n' && p < end) {
> +                    p++;
> +                }
> +                p++;
> +                continue;
> +            }
> +            if (*p == ' ') {
> +                while (*p == ' ' && p < end) {
> +                    p++;
> +                }
> +                /* only accept blank lines before 'version=' line */
> +                if (*p != '\n') {
> +                    return 0;
> +                }

What about Windows line endings (\r\n)?

> +                p++;
> +                continue;
> +            }
> +            sscanf(p, "version=%d", &version);

This function cannot be used on p because the buffer is not
NUL-terminated.  sscanf(3) may run off the end of the buffer.

How about this:

/* Match supported versions, Windows and UNIX line endings */
if (strncmp("version=1\r\n", p, end - p) == 0 ||
    strncmp("version=1\n", p, end - p) == 0 ||
    strncmp("version=2\r\n", p, end - p) == 0) ||
    strncmp("version=2\n", p, end - p) == 0) {
    return 100;
}

Stefan