[libvirt] [PATCH] virStorageTranslateDiskSourcePool: Avoid double free

[Michal Privoznik]

On 28.06.2016 15:21, John Ferlan wrote:
> 
> 
> On 06/28/2016 09:02 AM, Michal Privoznik wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1316370
>>
>> Consider the following disk for a domain:
>>
>>     <disk type='volume' device='cdrom'>
>>       <driver name='qemu' type='raw'/>
>>       <auth username='libvirt'>
>>         <secret type='iscsi' usage='libvirtiscsi'/>
>>       </auth>
>>       <source pool='iscsi-secret-pool' volume='unit:0:0:0' mode='direct' startupPolicy='optional'/>
>>       <target dev='sda' bus='scsi'/>
>>       <readonly/>
>>       <address type='drive' controller='0' bus='0' target='0' unit='0'/>
>>     </disk>
>>
>> Now, startupPolicy is currently not allowed for iscsi disks, so
>> one would expect an error message to be thrown. But what a
>> surprise is waiting for users if they try to start up such
>> domain:
>>
>> ==15724== Invalid free() / delete / delete[] / realloc()
>> ==15724==    at 0x4C2B1F0: free (vg_replace_malloc.c:473)
>> ==15724==    by 0x54B7A69: virFree (viralloc.c:582)
>> ==15724==    by 0x552DC90: virStorageAuthDefFree (virstoragefile.c:1549)
>> ==15724==    by 0x552F023: virStorageSourceClear (virstoragefile.c:2055)
>> ==15724==    by 0x552F054: virStorageSourceFree (virstoragefile.c:2067)
>> ==15724==    by 0x55556AA: virDomainDiskDefFree (domain_conf.c:1562)
>> ==15724==    by 0x5557ABE: virDomainDefFree (domain_conf.c:2547)
>> ==15724==    by 0x1B43CC42: qemuProcessStop (qemu_process.c:5918)
>> ==15724==    by 0x1B43BA2E: qemuProcessStart (qemu_process.c:5511)
>> ==15724==    by 0x1B48993E: qemuDomainObjStart (qemu_driver.c:7050)
>> ==15724==    by 0x1B489B9A: qemuDomainCreateWithFlags (qemu_driver.c:7104)
>> ==15724==    by 0x1B489C01: qemuDomainCreate (qemu_driver.c:7122)
>> ==15724==  Address 0x21cfbb90 is 0 bytes inside a block of size 48 free'd
>> ==15724==    at 0x4C2B1F0: free (vg_replace_malloc.c:473)
>> ==15724==    by 0x54B7A69: virFree (viralloc.c:582)
>> ==15724==    by 0x552DC90: virStorageAuthDefFree (virstoragefile.c:1549)
>> ==15724==    by 0x12D1C8D4: virStorageTranslateDiskSourcePool (storage_driver.c:3475)
>> ==15724==    by 0x1B4396E4: qemuProcessPrepareDomain (qemu_process.c:4896)
>> ==15724==    by 0x1B43B880: qemuProcessStart (qemu_process.c:5466)
>> ==15724==    by 0x1B48993E: qemuDomainObjStart (qemu_driver.c:7050)
>> ==15724==    by 0x1B489B9A: qemuDomainCreateWithFlags (qemu_driver.c:7104)
>> ==15724==    by 0x1B489C01: qemuDomainCreate (qemu_driver.c:7122)
>> ==15724==    by 0x561CA97: virDomainCreate (libvirt-domain.c:6787)
>> ==15724==    by 0x12B6FD: remoteDispatchDomainCreate (remote_dispatch.h:4116)
>> ==15724==    by 0x12B61A: remoteDispatchDomainCreateHelper (remote_dispatch.h:4092)
>>
>> The problem is, in virStorageTranslateDiskSourcePool disk
>> def->src->auth is freed, but the pointer is not set to NULL. So
>> later, when qemuProcessStop starts to free the domain definition,
>> virStorageAuthDefFree() tries to free the memory again, instead
>> of jumping out immediately.
>>
>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>> ---
>>  src/storage/storage_driver.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
> 
> Interesting... Looked at the bz, it wasn't clear from the description,
> but it seems that the source pool (iscsi-secret-pool) didn't have the
> '<auth>'; otherwise,  virStorageTranslateDiskSourcePoolAuth would have
> filled in the auth details.
> 
> Wonder if virStorageSourceClear should also be adjusted so that it too
> isn't bitten by the Free routines and pass by value not reference.

Well, nobody's calling just SourceClear. All the callers join the call
with VIR_FREE() of the whole definition so I'd say we are safe here.

> 
> 
> ACK for this (and I believe safe)

Thank you, pushed.

Michal