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@xxxxxxxxxx> >> --- >> 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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list