WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

Re: [Xen-devel] [PATCH] xl: Perform minimal validation of virtual disk f

To: Kamala Narasimhan <kamala.narasimhan@xxxxxxxxx>
Subject: Re: [Xen-devel] [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file
From: Gianni Tedesco <gianni.tedesco@xxxxxxxxxx>
Date: Thu, 20 Jan 2011 14:04:56 +0000
Cc: Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxx>, Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
Delivery-date: Thu, 20 Jan 2011 06:05:57 -0800
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <AANLkTin1AGxH26158mn37_Oar1PgSSJoJOnGHs+XnxsV@xxxxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <AANLkTimSUym0u+SNm0AvNp3AZQQFspetAaXmNShkPJd4@xxxxxxxxxxxxxx> <1294995912.8240.86.camel@xxxxxxxxxxxxxxxxxxxxxx> <AANLkTikfUHHc+-gVkgnRJc722wObLF3TumpK5WSfJVAE@xxxxxxxxxxxxxx> <1295024348.12018.222.camel@xxxxxxxxxxxxxxxxxxxxxx> <AANLkTikf3TVJwE_N-OyuS-UhyA8+cgzAG__9hz3AETeM@xxxxxxxxxxxxxx> <AANLkTinJ=PYsC6vbPvU8g2T8NmyohLa=4rd9zfhTMCCO@xxxxxxxxxxxxxx> <AANLkTin1AGxH26158mn37_Oar1PgSSJoJOnGHs+XnxsV@xxxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
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

<Prev in Thread] Current Thread [Next in Thread>