[libvirt] [PATCH] vbox: Fix segfault on empty device source
<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. --- src/vbox/vbox_tmpl.c | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 1765d63..fefed1f 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 @@ -3801,7 +3802,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; -- 1.6.3.3
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. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
2010/3/22 Eric Blake <eblake@redhat.com>:
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
On Wed, Mar 24, 2010 at 01:23:37AM +0100, Matthias Bolte wrote:
2010/3/22 Eric Blake <eblake@redhat.com>:
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@googlemail.com> 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/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
2010/3/24 Daniel Veillard <veillard@redhat.com>:
On Wed, Mar 24, 2010 at 01:23:37AM +0100, Matthias Bolte wrote:
2010/3/22 Eric Blake <eblake@redhat.com>:
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@googlemail.com> 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
Thanks, pushed. Matthias
participants (3)
-
Daniel Veillard -
Eric Blake -
Matthias Bolte