Re: [libvirt] [PATCH] util: fix crash in virClassIsDerivedFrom for CloseCallbacks objects

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.