[PATCH v2 1/2] qemu: Avoid double free of VM

[Michal Privoznik <mprivozn@xxxxxxxxxx>]

One of my previous patches (c7ac2519b7f) did try to fix the issue when
domain dies too soon during migration. However, this clumsy approach was
missing removal of qemuProcessHandleMonitorDestroy resulting in double
unrefing of mon->vm and hence producing the daemon crash:

==11843== Invalid read of size 4
==11843==    at 0x50C28C5: virObjectUnref (virobject.c:255)
==11843==    by 0x1148F7DB: qemuMonitorDispose (qemu_monitor.c:258)
==11843==    by 0x50C2991: virObjectUnref (virobject.c:262)
==11843==    by 0x50C2D13: virObjectFreeCallback (virobject.c:388)
==11843==    by 0x509C37B: virEventPollCleanupHandles (vireventpoll.c:583)
==11843==    by 0x509C711: virEventPollRunOnce (vireventpoll.c:652)
==11843==    by 0x509A620: virEventRunDefaultImpl (virevent.c:274)
==11843==    by 0x520D21C: virNetServerRun (virnetserver.c:1112)
==11843==    by 0x11F368: main (libvirtd.c:1513)
==11843==  Address 0x13b88864 is 4 bytes inside a block of size 136 free'd
==11843==    at 0x4A07F5C: free (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==11843==    by 0x5079A2F: virFree (viralloc.c:580)
==11843==    by 0x50C29E3: virObjectUnref (virobject.c:270)
==11843==    by 0x114770E4: qemuProcessHandleMonitorDestroy (qemu_process.c:1103)
==11843==    by 0x1148F7CB: qemuMonitorDispose (qemu_monitor.c:257)
==11843==    by 0x50C2991: virObjectUnref (virobject.c:262)
==11843==    by 0x50C2D13: virObjectFreeCallback (virobject.c:388)
==11843==    by 0x509C37B: virEventPollCleanupHandles (vireventpoll.c:583)
==11843==    by 0x509C711: virEventPollRunOnce (vireventpoll.c:652)
==11843==    by 0x509A620: virEventRunDefaultImpl (virevent.c:274)
==11843==    by 0x520D21C: virNetServerRun (virnetserver.c:1112)
==11843==    by 0x11F368: main (libvirtd.c:1513)

Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
---
 src/qemu/qemu_process.c | 16 +++-------------
 1 file changed, 3 insertions(+), 13 deletions(-)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 7c23eb4..09d9dbd 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -1095,14 +1095,6 @@ error:
     return -1;
 }
 
-
-static void qemuProcessHandleMonitorDestroy(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
-                                            virDomainObjPtr vm,
-                                            void *opaque ATTRIBUTE_UNUSED)
-{
-    virObjectUnref(vm);
-}
-
 static int
 qemuProcessHandleTrayChange(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
                             virDomainObjPtr vm,
@@ -1366,7 +1358,6 @@ cleanup:
 
 
 static qemuMonitorCallbacks monitorCallbacks = {
-    .destroy = qemuProcessHandleMonitorDestroy,
     .eofNotify = qemuProcessHandleMonitorEOF,
     .errorNotify = qemuProcessHandleMonitorError,
     .diskSecretLookup = qemuProcessFindVolumeQcowPassphrase,
@@ -1403,7 +1394,7 @@ qemuConnectMonitor(virQEMUDriverPtr driver, virDomainObjPtr vm, int logfd)
     }
 
     /* Hold an extra reference because we can't allow 'vm' to be
-     * deleted while the monitor is active */
+     * deleted while the monitor is unlocked */
     virObjectRef(vm);
 
     ignore_value(virTimeMillisNow(&priv->monStart));
@@ -1419,11 +1410,10 @@ qemuConnectMonitor(virQEMUDriverPtr driver, virDomainObjPtr vm, int logfd)
         ignore_value(qemuMonitorSetDomainLog(mon, logfd));
 
     virObjectLock(vm);
+    virObjectUnref(vm);
     priv->monStart = 0;
 
-    if (mon == NULL) {
-        virObjectUnref(vm);
-    } else if (!virDomainObjIsActive(vm)) {
+    if (!virDomainObjIsActive(vm)) {
         qemuMonitorClose(mon);
         mon = NULL;
     }
-- 
1.8.1.5

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list