Re: [Xen-devel] [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file

[Gianni Tedesco <gianni.tedesco@xxxxxxxxxx>]

On Wed, 2011-01-19 at 18:26 +0000, Kamala Narasimhan wrote:
> Apologies.  I inadvertently neglected Gianni's suggestion to switch to
> logging from fprintf.
> 
> Signed-off-by: Kamala Narasimhan <kamala.narasimhan@xxxxxxxxxx>
> 
> Kamala
> 
> diff -r fe8a177ae9cb tools/libxl/libxl.c
> --- a/tools/libxl/libxl.c       Wed Jan 19 15:29:04 2011 +0000
> +++ b/tools/libxl/libxl.c       Wed Jan 19 13:23:16 2011 -0500
> @@ -826,6 +826,41 @@ skip_autopass:
> 
>  
> /******************************************************************************/
> 
> +static int validate_virtual_disk(libxl_ctx *ctx, char *file_name,
> libxl_disk_phystype disk_type)
> +{
> +    struct stat stat_buf;
> +
> +    if ( file_name == NULL ) {
> +        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Virtual disk file name is 
> NULL!\n");
> +        return 0;
> +    }

I prefer assert() for things caused by programmer error. But in this
case we could just let the dereference below catch it...

> +    /* Return without further validation for empty cdrom drive.
> +       Note: Post 4.1 we need to change the interface to handle empty
> +       cdrom rather than go with the below assumption.
> +     */

So this handles CD-ROM images too? See below...

> +    if ( (strncmp(file_name, "", sizeof("")) == 0) && (disk_type == 
> PHYSTYPE_PHY) )
> +        return 1;

Hrm, strlen(file_name) == 0 ? :)

> +
> +    if ( stat(file_name, &stat_buf) != 0 ) {
> +        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Stat on virtual disk %s returned 
> error - \"%s\".\n",
> +            file_name, strerror(errno));
> +        return 0;
> +    }
> +    if ( disk_type == PHYSTYPE_PHY ) {
> +        if ( !(S_ISBLK(stat_buf.st_mode)) ) {
> +            LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Virtual disk %s is not a 
> block device!\n",
> +                file_name);
> +            return 0;
> +        }
> +    } else if ( stat_buf.st_size == 0 ) {
> +        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Virtual disk %s size is 0!\n", 
> file_name);
> +        return 0;
> +    }
> +
> +    return 1;
> +}
> +
>  int libxl_device_disk_add(libxl_ctx *ctx, uint32_t domid, libxl_device_disk 
> *disk)
>  {
>      libxl__gc gc = LIBXL_INIT_GC(ctx);
> @@ -835,6 +870,9 @@ int libxl_device_disk_add(libxl_ctx *ctx
>      int devid;
>      libxl__device device;
>      int major, minor, rc;
> +
> +    if ( validate_virtual_disk(ctx, disk->physpath, disk->phystype) == 0 )
> +        return ERROR_FAIL;
> 
>      front = flexarray_make(16, 1);
>      if (!front) {

In which case libxl_cdrom_insert() needs the same addition?

I also wonder about the motivation of the patch. To be honest, usually,
the best way to validate things like this is to go ahead and try to use
them and bail with an error at that time. Where do things bail out for
you and what problems does that cause? I also think that these checks
are maximal as well as minimal in that if we checked much further we
would be re-implementing code?

Gianni


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel