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.

On Wed, Mar 24, 2010 at 01:23:37AM +0100, Matthias Bolte wrote: > 2010/3/22 Eric Blake <eblake(a)redhat.com&gt;: > > 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(a)googlemail.com&gt; > 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