On Wed, Mar 24, 2010 at 01:23:37AM +0100, Matthias Bolte wrote: > 2010/3/22 Eric Blake <[email protected]>: > > On 03/22/2010 02:40 PM, Matthias Bolte wrote: > >> <source file=''/> results in def->disks[i]->src == NULL. But > >> vboxDomainDefineXML didn't check def->disks[i]->src for NULL > >> and expected it to be a valid string. > >> > >> Add checks for def->disks[i]->src != NULL to fix the segfault. > > > > ACK, but did you catch all the places? For example, > > > >> @@ -3519,7 +3519,8 @@ static virDomainPtr > >> vboxDomainDefineXML(virConnectPtr conn, const char *xml) { > >> DEBUG("disk(%d) shared: %s", i, def->disks[i]->shared > >> ? "True" : "False"); > >> > >> if (def->disks[i]->device == > >> VIR_DOMAIN_DISK_DEVICE_CDROM) { > >> - if (def->disks[i]->type == VIR_DOMAIN_DISK_TYPE_FILE) > >> { > >> + if (def->disks[i]->type == VIR_DOMAIN_DISK_TYPE_FILE > >> && > >> + def->disks[i]->src != NULL) { > >> IDVDDrive *dvdDrive = NULL; > >> /* Currently CDROM/DVD Drive is always IDE > >> * Secondary Master so neglecting the following > >> @@ -3801,7 +3802,8 @@ static virDomainPtr > >> vboxDomainDefineXML(virConnectPtr conn, const char *xml) { > > > > in between these two line ranges, I see a usage at line 3591 under > > def->disks[i]->device==VIR_DOMAIN_DISK_TYPE_DISK that seems like it > > could be vulnerable to the same problem. > > > > Yep, I didn't catch all places. Here's a patch that should catch all > unchecked usage of the src member. > > Matthias
> From 679b866bc6a62b35cdafccb6dad65e7c121ecaff Mon Sep 17 00:00:00 2001 > From: Matthias Bolte <[email protected]> > Date: Mon, 22 Mar 2010 21:01:41 +0100 > Subject: [PATCH] vbox: Fix segfault on empty device source > > <source file=''/> results in def->disks[i]->src == NULL. But > vboxDomainDefineXML and vboxDomainAttachDevice didn't check > def->disks[i]->src for NULL and expected it to be a valid string. > > Add checks for def->disks[i]->src != NULL to fix the segfault. > --- > src/vbox/vbox_tmpl.c | 18 ++++++++++++------ > 1 files changed, 12 insertions(+), 6 deletions(-) > > diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c > index 1765d63..510abae 100644 > --- a/src/vbox/vbox_tmpl.c > +++ b/src/vbox/vbox_tmpl.c > @@ -3519,7 +3519,8 @@ static virDomainPtr vboxDomainDefineXML(virConnectPtr > conn, const char *xml) { > DEBUG("disk(%d) shared: %s", i, def->disks[i]->shared ? > "True" : "False"); > > if (def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_CDROM) { > - if (def->disks[i]->type == VIR_DOMAIN_DISK_TYPE_FILE) { > + if (def->disks[i]->type == VIR_DOMAIN_DISK_TYPE_FILE && > + def->disks[i]->src != NULL) { > IDVDDrive *dvdDrive = NULL; > /* Currently CDROM/DVD Drive is always IDE > * Secondary Master so neglecting the following > @@ -3577,7 +3578,8 @@ static virDomainPtr vboxDomainDefineXML(virConnectPtr > conn, const char *xml) { > } else if (def->disks[i]->type == > VIR_DOMAIN_DISK_TYPE_BLOCK) { > } > } else if (def->disks[i]->device == > VIR_DOMAIN_DISK_DEVICE_DISK) { > - if (def->disks[i]->type == VIR_DOMAIN_DISK_TYPE_FILE) { > + if (def->disks[i]->type == VIR_DOMAIN_DISK_TYPE_FILE && > + def->disks[i]->src != NULL) { > IHardDisk *hardDisk = NULL; > PRUnichar *hddfileUtf16 = NULL; > vboxIID *hdduuid = NULL; > @@ -3674,7 +3676,8 @@ static virDomainPtr vboxDomainDefineXML(virConnectPtr > conn, const char *xml) { > } else if (def->disks[i]->type == > VIR_DOMAIN_DISK_TYPE_BLOCK) { > } > } else if (def->disks[i]->device == > VIR_DOMAIN_DISK_DEVICE_FLOPPY) { > - if (def->disks[i]->type == VIR_DOMAIN_DISK_TYPE_FILE) { > + if (def->disks[i]->type == VIR_DOMAIN_DISK_TYPE_FILE && > + def->disks[i]->src != NULL) { > IFloppyDrive *floppyDrive; > machine->vtbl->GetFloppyDrive(machine, &floppyDrive); > if (floppyDrive) { > @@ -3801,7 +3804,8 @@ static virDomainPtr vboxDomainDefineXML(virConnectPtr > conn, const char *xml) { > DEBUG("disk(%d) readonly: %s", i, def->disks[i]->readonly ? > "True" : "False"); > DEBUG("disk(%d) shared: %s", i, def->disks[i]->shared ? > "True" : "False"); > > - if (def->disks[i]->type == VIR_DOMAIN_DISK_TYPE_FILE) { > + if (def->disks[i]->type == VIR_DOMAIN_DISK_TYPE_FILE && > + def->disks[i]->src != NULL) { > IMedium *medium = NULL; > PRUnichar *mediumUUID = NULL; > PRUnichar *mediumFileUtf16 = NULL; > @@ -4696,7 +4700,8 @@ static int vboxDomainAttachDevice(virDomainPtr dom, > const char *xml) { > if (dev->type == VIR_DOMAIN_DEVICE_DISK) { > #if VBOX_API_VERSION < 3001 > if (dev->data.disk->device == > VIR_DOMAIN_DISK_DEVICE_CDROM) { > - if (dev->data.disk->type == > VIR_DOMAIN_DISK_TYPE_FILE) { > + if (dev->data.disk->type == > VIR_DOMAIN_DISK_TYPE_FILE && > + dev->data.disk->src != NULL) { > IDVDDrive *dvdDrive = NULL; > /* Currently CDROM/DVD Drive is always IDE > * Secondary Master so neglecting the following > @@ -4754,7 +4759,8 @@ static int vboxDomainAttachDevice(virDomainPtr dom, > const char *xml) { > } else if (dev->data.disk->type == > VIR_DOMAIN_DISK_TYPE_BLOCK) { > } > } else if (dev->data.disk->device == > VIR_DOMAIN_DISK_DEVICE_FLOPPY) { > - if (dev->data.disk->type == > VIR_DOMAIN_DISK_TYPE_FILE) { > + if (dev->data.disk->type == > VIR_DOMAIN_DISK_TYPE_FILE && > + dev->data.disk->src != NULL) { > IFloppyDrive *floppyDrive; > machine->vtbl->GetFloppyDrive(machine, > &floppyDrive); > if (floppyDrive) { ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ [email protected] | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/ -- libvir-list mailing list [email protected] https://www.redhat.com/mailman/listinfo/libvir-list
