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>
> ---
> 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