[libvirt] [PATCH] qemu: Fix double free when live-attaching shmem
Function qemuDomainAttachShmemDevice() steals the device data if the hotplug was successful, but the condition checked for unsuccessful execution otherwise. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 38c841420e32..a82e58b29f29 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7615,7 +7615,7 @@ qemuDomainAttachDeviceLive(virDomainObjPtr vm, case VIR_DOMAIN_DEVICE_SHMEM: ret = qemuDomainAttachShmemDevice(driver, vm, dev->data.shmem); - if (ret < 0) { + if (!ret) { alias = dev->data.shmem->info.alias; dev->data.shmem = NULL; } -- 2.10.2
On 09.11.2016 17:43, Martin Kletzander wrote:
Function qemuDomainAttachShmemDevice() steals the device data if the hotplug was successful, but the condition checked for unsuccessful execution otherwise.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 38c841420e32..a82e58b29f29 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7615,7 +7615,7 @@ qemuDomainAttachDeviceLive(virDomainObjPtr vm, case VIR_DOMAIN_DEVICE_SHMEM: ret = qemuDomainAttachShmemDevice(driver, vm, dev->data.shmem); - if (ret < 0) { + if (!ret) { alias = dev->data.shmem->info.alias; dev->data.shmem = NULL; }
ACK Michal
On Wed, Nov 09, 2016 at 05:43:49PM +0100, Martin Kletzander wrote:
Function qemuDomainAttachShmemDevice() steals the device data if the hotplug was successful, but the condition checked for unsuccessful execution otherwise.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 38c841420e32..a82e58b29f29 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7615,7 +7615,7 @@ qemuDomainAttachDeviceLive(virDomainObjPtr vm, case VIR_DOMAIN_DEVICE_SHMEM: ret = qemuDomainAttachShmemDevice(driver, vm, dev->data.shmem); - if (ret < 0) { + if (!ret) {
In this case I would probably use ret == 0. We are not consistent at all, for example in this function there is more occurrences of !ret, but in case of *int* I think it's better to check for specific value, not like in case of *pointer* or *bool* where !ret makes more sense. ACK Pavel
alias = dev->data.shmem->info.alias; dev->data.shmem = NULL; } -- 2.10.2
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Nov 09, 2016 at 05:55:34PM +0100, Pavel Hrdina wrote:
On Wed, Nov 09, 2016 at 05:43:49PM +0100, Martin Kletzander wrote:
Function qemuDomainAttachShmemDevice() steals the device data if the hotplug was successful, but the condition checked for unsuccessful execution otherwise.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 38c841420e32..a82e58b29f29 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7615,7 +7615,7 @@ qemuDomainAttachDeviceLive(virDomainObjPtr vm, case VIR_DOMAIN_DEVICE_SHMEM: ret = qemuDomainAttachShmemDevice(driver, vm, dev->data.shmem); - if (ret < 0) { + if (!ret) {
In this case I would probably use ret == 0. We are not consistent at all, for example in this function there is more occurrences of !ret, but in case of *int* I think it's better to check for specific value, not like in case of *pointer* or *bool* where !ret makes more sense.
ACK
I sent the patch without --notes which made part of the explanation disappear. But since you "asked for it" :) here goes: Notes: This is actually how the code looked like before the last review. At least now I know that I should stand up for myself. All the other relevant branches use the same syntax for this logic anyway. I could also used -W to show the whole function.
Pavel
alias = dev->data.shmem->info.alias; dev->data.shmem = NULL; } -- 2.10.2
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (3)
-
Martin Kletzander -
Michal Privoznik -
Pavel Hrdina