Re: [libvirt] [PATCH] vbox: Fix segfault on empty device source

2010/3/22 Eric Blake <eblake(a)redhat.com&gt;: > 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 <matthias.bolte(a)googlemail.com&gt; 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) {