Re: [libvirt] [PATCH] snapshot: conf: Fix NULL dereference when <driver> element is empty

On Wed, Nov 13, 2013 at 12:07:43PM +0100, Peter Krempa wrote: > Consider the following valid snapshot XML as the <driver> element is > allowed to be empty in the domainsnapshot.rng schema: > > $ cat snap.xml > <domainsnapshot> > <disks> > <disk name='vda' snapshot='external'> > <source file='/tmp/foo'/> > <driver/> > </disk> > </disks> > </domainsnapshot> > > produces the following error: > > $ virsh snapshot-create domain snap.xml > error: internal error: unknown disk snapshot driver '(null)' > > The driver type is parsed as NULL from the XML as the attribute is not > present and then directly used to produce the error message. > > With this patch the attempt to parse the driver type is skipped if not > present to avoid changing the schema to forbid the empty driver element. > --- > src/conf/snapshot_conf.c | 16 +++++++++------- > 1 file changed, 9 insertions(+), 7 deletions(-) > > diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c > index d8910c9..418987b 100644 > --- a/src/conf/snapshot_conf.c > +++ b/src/conf/snapshot_conf.c > @@ -175,15 +175,17 @@ virDomainSnapshotDiskDefParseXML(xmlNodePtr node, > } else if (!def->format && > xmlStrEqual(cur->name, BAD_CAST "driver")) { > char *driver = virXMLPropString(cur, "type"); > - def->format = virStorageFileFormatTypeFromString(driver); > - if (def->format <= 0) { > - virReportError(VIR_ERR_INTERNAL_ERROR, > - _("unknown disk snapshot driver '%s'"), > - driver); > + if (driver) { > + def->format = virStorageFileFormatTypeFromString(driver); > + if (def->format <= 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("unknown disk snapshot driver '%s'"), > + driver); > + VIR_FREE(driver); > + goto cleanup; > + } > VIR_FREE(driver); > - goto cleanup; > } > - VIR_FREE(driver); > } > } So IIUC, this allows <driver type="qemu"/> <driver type="qemu" format="qcow2" />

but forbids <driver format="qcow2" />

The domain XML parser, however, allows all 3 and 'type' will be auto determined if omitted. IMHO it would be desirable to be consistent between the parsers

Daniel