[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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/


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]