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

[Maxim Nestratov]

09.06.2016 15:23, Maxim Nestratov пишет:

> 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@@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 at 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 at entry=0x7f08101d47b0, cb=cb at entry=0x7f081d078fc0 
>>> <qemuProcessAutoDestroy>) at util/virclosecallbacks.c:163
>>> 4  in qemuProcessAutoDestroyRemove 
>>> (driver=driver at entry=0x7f081018be50, vm=vm at entry=0x7f08101d47b0) at 
>>> qemu/qemu_process.c:6368
>>> 5  in qemuProcessStop (driver=driver at entry=0x7f081018be50, 
>>> vm=vm at entry=0x7f08101d47b0, 
>>> reason=reason at entry=VIR_DOMAIN_SHUTOFF_SHUTDOWN, 
>>> asyncJob=asyncJob at entry=QEMU_ASYNC_JOB_NONE, flags=flags at 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 at 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 at 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?

Disregard please. It's true only if you get a locked object from a list 
or by some getter function, otherwise a caller should guarantee validity 
of the object, which is untrue in my case and I'm trying to fix it 
(eventually incorrectly).
I'll resend another version.

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