On 05.09.2016 18:42, Maxim Nestratov wrote: > There is a possibility that qemu driver frees by unreferencing its > closeCallbacks pointer as it has the only reference to the object, > while in fact not all users of CloseCallbacks called thier > virCloseCallbacksUnset. > > Backtrace is the following: > Thread #1: > 0 in pthread_cond_wait@(a)GLIBC_2.3.2 () from /lib64/libpthread.so.0 > 1 in virCondWait (c=<optimized out>, m=<optimized out>) > at util/virthread.c:154 > 2 in virThreadPoolFree (pool=0x7f0810110b50) > at util/virthreadpool.c:266 > 3 in qemuStateCleanup () at qemu/qemu_driver.c:1116 > 4 in virStateCleanup () at libvirt.c:808 > 5 in main (argc=<optimized out>, argv=<optimized out>) > at libvirtd.c:1660 > > Thread #2: > 0 in virClassIsDerivedFrom (klass=0xdeadbeef, parent=0x7f0837c694d0) at util/virobject.c:169 > 1 in virObjectIsClass (anyobj=anyobj@entry=0x7f08101d4760, klass=<optimized out>) at util/virobject.c:365 > 2 in virObjectLock (anyobj=0x7f08101d4760) at util/virobject.c:317 > 3 in virCloseCallbacksUnset (closeCallbacks=0x7f08101d4760, vm=vm@entry=0x7f08101d47b0, cb=cb@entry=0x7f081d078fc0 <qemuProcessAutoDestroy>) at util/virclosecallbacks.c:163 > 4 in qemuProcessAutoDestroyRemove (driver=driver@entry=0x7f081018be50, vm=vm@entry=0x7f08101d47b0) at qemu/qemu_process.c:6368 > 5 in qemuProcessStop (driver=driver@entry=0x7f081018be50, vm=vm@entry=0x7f08101d47b0, reason=reason@entry=VIR_DOMAIN_SHUTOFF_SHUTDOWN, asyncJob=asyncJob@entry=QEMU_ASYNC_JOB_NONE, flags=flags@entry=0) at qemu/qemu_process.c:5854 > 6 in processMonitorEOFEvent (vm=0x7f08101d47b0, driver=0x7f081018be50) at qemu/qemu_driver.c:4585 > 7 qemuProcessEventHandler (data=<optimized out>, opaque=0x7f081018be50) at qemu/qemu_driver.c:4629 > 8 in virThreadPoolWorker (opaque=opaque@entry=0x7f0837c4f820) at util/virthreadpool.c:145 > 9 in virThreadHelper (data=<optimized out>) at util/virthread.c:206 > 10 in start_thread () from /lib64/libpthread.so.0 > > Let's reference CloseCallbacks object in virCloseCallbacksSet and > unreference in virCloseCallbacksUnset. > > Signed-off-by: Maxim Nestratov <mnestratov(a)virtuozzo.com&gt; > --- > src/util/virclosecallbacks.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/src/util/virclosecallbacks.c b/src/util/virclosecallbacks.c > index 82d4071..891a92b 100644 > --- a/src/util/virclosecallbacks.c > +++ b/src/util/virclosecallbacks.c > @@ -141,6 +141,7 @@ virCloseCallbacksSet(virCloseCallbacksPtr closeCallbacks, > virObjectRef(vm); > } > > + virObjectRef(closeCallbacks); > ret = 0; > cleanup: > virObjectUnlock(closeCallbacks); > @@ -180,6 +181,8 @@ virCloseCallbacksUnset(virCloseCallbacksPtr closeCallbacks, > ret = 0; > cleanup: > virObjectUnlock(closeCallbacks); > + if (!ret) > + virObjectUnref(closeCallbacks); Might as well put this a few lines above, just before 'ret = 0' line. ACK with that changed. Michal

On 07.09.2016 11:41, Maxim Nestratov wrote: > 06-Sep-16 17:42, Michal Privoznik пишет: > >> On 05.09.2016 18:42, Maxim Nestratov wrote: >>> There is a possibility that qemu driver frees by unreferencing its >>> closeCallbacks pointer as it has the only reference to the object, >>> while in fact not all users of CloseCallbacks called thier >>> virCloseCallbacksUnset. >>> >>> Backtrace is the following: >>> Thread #1: >>> 0 in pthread_cond_wait@(a)GLIBC_2.3.2 () from /lib64/libpthread.so.0 >>> 1 in virCondWait (c=<optimized out>, m=<optimized out>) >>> at util/virthread.c:154 >>> 2 in virThreadPoolFree (pool=0x7f0810110b50) >>> at util/virthreadpool.c:266 >>> 3 in qemuStateCleanup () at qemu/qemu_driver.c:1116 >>> 4 in virStateCleanup () at libvirt.c:808 >>> 5 in main (argc=<optimized out>, argv=<optimized out>) >>> at libvirtd.c:1660 >>> >>> Thread #2: >>> 0 in virClassIsDerivedFrom (klass=0xdeadbeef, parent=0x7f0837c694d0) >>> at util/virobject.c:169 >>> 1 in virObjectIsClass (anyobj=anyobj@entry=0x7f08101d4760, >>> klass=<optimized out>) at util/virobject.c:365 >>> 2 in virObjectLock (anyobj=0x7f08101d4760) at util/virobject.c:317 >>> 3 in virCloseCallbacksUnset (closeCallbacks=0x7f08101d4760, >>> vm=vm@entry=0x7f08101d47b0, cb=cb@entry=0x7f081d078fc0 >>> <qemuProcessAutoDestroy>) at util/virclosecallbacks.c:163 >>> 4 in qemuProcessAutoDestroyRemove >>> (driver=driver@entry=0x7f081018be50, vm=vm@entry=0x7f08101d47b0) at >>> qemu/qemu_process.c:6368 >>> 5 in qemuProcessStop (driver=driver@entry=0x7f081018be50, >>> vm=vm@entry=0x7f08101d47b0, >>> reason=reason@entry=VIR_DOMAIN_SHUTOFF_SHUTDOWN, >>> asyncJob=asyncJob@entry=QEMU_ASYNC_JOB_NONE, flags=flags@entry=0) at >>> qemu/qemu_process.c:5854 >>> 6 in processMonitorEOFEvent (vm=0x7f08101d47b0, >>> driver=0x7f081018be50) at qemu/qemu_driver.c:4585 >>> 7 qemuProcessEventHandler (data=<optimized out>, >>> opaque=0x7f081018be50) at qemu/qemu_driver.c:4629 >>> 8 in virThreadPoolWorker (opaque=opaque@entry=0x7f0837c4f820) at >>> util/virthreadpool.c:145 >>> 9 in virThreadHelper (data=<optimized out>) at util/virthread.c:206 >>> 10 in start_thread () from /lib64/libpthread.so.0 >>> >>> Let's reference CloseCallbacks object in virCloseCallbacksSet and >>> unreference in virCloseCallbacksUnset. >>> >>> Signed-off-by: Maxim Nestratov <mnestratov(a)virtuozzo.com&gt; >>> --- >>> src/util/virclosecallbacks.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/src/util/virclosecallbacks.c b/src/util/virclosecallbacks.c >>> index 82d4071..891a92b 100644 >>> --- a/src/util/virclosecallbacks.c >>> +++ b/src/util/virclosecallbacks.c >>> @@ -141,6 +141,7 @@ virCloseCallbacksSet(virCloseCallbacksPtr >>> closeCallbacks, >>> virObjectRef(vm); >>> } >>> + virObjectRef(closeCallbacks); >>> ret = 0; >>> cleanup: >>> virObjectUnlock(closeCallbacks); >>> @@ -180,6 +181,8 @@ virCloseCallbacksUnset(virCloseCallbacksPtr >>> closeCallbacks, >>> ret = 0; >>> cleanup: >>> virObjectUnlock(closeCallbacks); >>> + if (!ret) >>> + virObjectUnref(closeCallbacks); >> Might as well put this a few lines above, just before 'ret = 0' line. >> >> ACK with that changed. >> >> Michal > No, we can't do this, as it could be the last reference to > closeCallbacks and after derefencing it, virObjectUnlock would touch > freed memory. > Ah, good point. Stick to the original then. Michal