2010/3/22 Eric Blake <eblake@xxxxxxxxxx>:
> 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@xxxxxxxxxxxxxx>
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) {
--
1.6.3.3
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list