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