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

[Peter Krempa]

On 11/13/13 12:18, Daniel P. Berrange wrote:
> 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" />

I guess it's s/type/name/, s/format/type/ in the domain definition:

Thus in the snapshot parser we have only the "type" attribute that is
being parsed using virStorageFileFormatTypeFromString().

The schema allows:
<source>
	<driver/>

and
<source>
	<driver type='qcow2'/> (or some other format)

The first of those two isn't actually any useful as the snapshot
creation will fail anyways as it's supported only with qcow2 and similar.

> 
> but forbids
> 
>   <driver format="qcow2" />

<driver type='qcow2'/> is actually allowed and quite useful. We just
don't have the "name" attribute here. Is it worth adding it without
having it to do anything?
> 
> 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

We could probably auto-assign qcow2 as the type if not provided to allow
creating of the snapshot without the need to do it explicitly.

> 
> Daniel
> 

Peter
PEter

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 901 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20131113/e5ad3ef5/attachment-0001.sig>