From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Jackson Subject: Re: [PATCH] xl: Perform minimal validation of virtual disk file while parsing config file Date: Thu, 20 Jan 2011 15:49:20 +0000 Message-ID: <19768.22912.878633.622270@mariner.uk.xensource.com> References: <1294995912.8240.86.camel@zakaz.uk.xensource.com> <1295024348.12018.222.camel@qabil.uk.xensource.com> <1295532296.12018.337.camel@qabil.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1295532296.12018.337.camel@qabil.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Gianni Tedesco Cc: Ian Campbell , Kamala Narasimhan , "xen-devel@lists.xensource.com" , Stefano Stabellini List-Id: xen-devel@lists.xenproject.org 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.