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

[Matthias Bolte]

2010/3/24 Daniel Veillard <veillard at redhat.com>:
> On Wed, Mar 24, 2010 at 01:23:37AM +0100, Matthias Bolte wrote:
>> 2010/3/22 Eric Blake <eblake at 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 at 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