2010/3/24 Daniel Veillard <veillard@xxxxxxxxxx>:
> On Wed, Mar 24, 2010 at 01:23:37AM +0100, Matthias Bolte wrote:
>> 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) {
>
> ACK,
>
> Daniel
>
Thanks, pushed.
Matthias
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list