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

09.06.2016 15:03, Peter Krempa пишет: > On Mon, Jun 06, 2016 at 20:59:22 +0300, 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. > Do you happen to have any kind of reproducer for this crash? Unfortunately no. > >> 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 | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/src/util/virclosecallbacks.c >> b/src/util/virclosecallbacks.c >> index 82d4071..2fab56b 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); >> @@ -177,6 +178,7 @@ virCloseCallbacksUnset(virCloseCallbacksPtr >> closeCallbacks, >> goto cleanup; >> virObjectUnref(vm); >> + virObjectUnref(closeCallbacks); > If this cleared the last reference, closeCallbacks is invalid at that > point. Hm, yes, you are right. Actually I assumed that locking an object guarantees the user of the object its validity. So I really thought that locking function takes a reference and unlocking releases it. And it turned out not to be true. Effectively it's an error prone way of dealing with locking/unlocking functions. Am I the only one who finds it a bit problematic? Or is there some cases when you need to lock objects and not to reference them that I don't see?

>> ret = 0; >> cleanup: >> virObjectUnlock(closeCallbacks); > But this would dereference and use it. > > Peter -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list