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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Gianni Tedesco writes ("Re: [Xen-devel] [PATCH] xl: Perform minimal validation 
of virtual disk file while parsing config file"):
> On Wed, 2011-01-19 at 18:26 +0000, Kamala Narasimhan wrote:
> > +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...

Yes, quite.  I don't think this check should be here at all.

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

Yes, that code is a bit too close to Daily WTF for my liking.

Also, convention in libxl is for functions to return error values, not
booleans.  I think that validate_virtual_disk should return 0 or
ERROR_INVAL, and libxl_device_disk add should simply pass on the
return code.

> >  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 think so.

> 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?

I agree with this sentiment but currently the error handling is that
qemu-dm leaves a message in an obscure logfile.  We need to do
something better for 4.1 and writing new arrangments for plumbing
errors through is too late now.

For 4.2 we should revisit this and probably revert this patch and
replace it with something much better.

Ian.

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