[libvirt] [PATCH] virStorageTranslateDiskSourcePool: Avoid double free
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@redhat.com> --- src/storage/storage_driver.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 831e6b0..cb9d578 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -3472,7 +3472,10 @@ virStorageTranslateDiskSourcePool(virConnectPtr conn, VIR_FREE(def->src->path); virStorageNetHostDefFree(def->src->nhosts, def->src->hosts); + def->src->nhosts = 0; + def->src->hosts = NULL; virStorageAuthDefFree(def->src->auth); + def->src->auth = NULL; switch ((virStoragePoolType) pooldef->type) { case VIR_STORAGE_POOL_DIR: -- 2.8.4
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@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. ACK for this (and I believe safe) John
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 831e6b0..cb9d578 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -3472,7 +3472,10 @@ virStorageTranslateDiskSourcePool(virConnectPtr conn,
VIR_FREE(def->src->path); virStorageNetHostDefFree(def->src->nhosts, def->src->hosts); + def->src->nhosts = 0; + def->src->hosts = NULL; virStorageAuthDefFree(def->src->auth); + def->src->auth = NULL;
switch ((virStoragePoolType) pooldef->type) { case VIR_STORAGE_POOL_DIR:
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@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
participants (2)
-
John Ferlan -
Michal Privoznik