Re: [Xen-devel] [PATCH v5 6/8] libxl: introduce libxl__alloc_vdev

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Stefano Stabellini writes ("[PATCH v5 6/8] libxl: introduce libxl__alloc_vdev"):
> Introduce libxl__alloc_vdev: find a spare virtual block device in the
> domain passed as argument.
...
> +    do {
> +        vdev = GCSPRINTF("d%dp0", disk);
> +        devid = libxl__device_disk_dev_number(vdev, NULL, NULL);
> +        if (libxl__xs_read(gc, t,
> +                    libxl__sprintf(gc, "%s/device/vbd/%d/backend",
> +                        dompath, devid)) == NULL) {
> +            if (errno == ENOENT)
> +                return libxl__devid_to_localdev(gc, devid);
> +            else
> +                return NULL;
> +        }
> +        disk++;
> +    } while (disk <= (1 << 20));

This should be "<", as disk==(1<<20) is too big.
And the magic number 20 should not be repeated.

But it turns out that you don't need to do this check here at all
because libxl__device_disk_dev_number will check this, correctly.

All you need to do is actually pay attention to its error return.

> +    return NULL;

All the NULL error exits should log an error surely.

> +/* same as in Linux */
> +static char *encode_disk_name(char *ptr, unsigned int n)
> +{
> +    if (n >= 26)
> +        ptr = encode_disk_name(ptr, n / 26 - 1);
> +    *ptr = 'a' + n % 26;
> +    return ptr + 1;
> +}

This still makes it difficult to see that the buffer cannot be
overrun.  I mentioned this in response to a previous posting of this
code and IIRC you were going to do something about it, if only a
comment explaining what the maximum buffer length is and why it's
safe.

> +    strcpy(ret, "xvd");
> +    ptr = encode_disk_name(ret + 3, offset);
> +    if (minor % nr_parts == 0)
> +        *ptr = 0;
> +    else
> +        snprintf(ptr, ret + 32 - ptr,
> +                "%d", minor & (nr_parts - 1));

You do not handle snprintf buffer truncation sensibly here.  As it
happens I think this overflow cannot happen but this deserves a
comment at the very least, to explain the lack of error handling.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel